netfilter: Wrong icmp6 checksum for ICMPV6_TIME_EXCEED in reverse SNATv6 path

Submitted by Dave Johnson on April 20, 2017, 12:44 p.m.

Details

Message ID 22776.44325.616928.335465@gargle.gargle.HOWL
State Changes Requested
Delegated to: Pablo Neira
Headers show

Commit Message

Dave Johnson April 20, 2017, 12:44 p.m.
When recalculating the outer ICMPv6 checksum for a reverse path NATv6
such as ICMPV6_TIME_EXCEED nf_nat_icmpv6_reply_translation() was
accessing data beyond the headlen of the skb for non-linear skb.  This
resulted in incorrect ICMPv6 checksum as garbage data was used.

Signed-off-by: Dave Johnson <dave-kernel@centerclick.org>
---
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Pablo Neira April 24, 2017, 8:43 a.m.
On Thu, Apr 20, 2017 at 08:44:21AM -0400, Dave Johnson wrote:
> 
> When recalculating the outer ICMPv6 checksum for a reverse path NATv6
> such as ICMPV6_TIME_EXCEED nf_nat_icmpv6_reply_translation() was
> accessing data beyond the headlen of the skb for non-linear skb.  This
> resulted in incorrect ICMPv6 checksum as garbage data was used.
> 
> Signed-off-by: Dave Johnson <dave-kernel@centerclick.org>
> ---
> diff -rup linux-4.9.23.orig/net/ipv6/netfilter/nf_nat_l3proto_ipv6.c linux-4.9.23/net/ipv6/netfilter/nf_nat_l3proto_ipv6.c
> --- linux-4.9.23.orig/net/ipv6/netfilter/nf_nat_l3proto_ipv6.c	2017-04-18 01:12:30.000000000 -0400
> +++ linux-4.9.23/net/ipv6/netfilter/nf_nat_l3proto_ipv6.c	2017-04-20 08:13:41.070493666 -0400
> @@ -229,7 +229,12 @@ int nf_nat_icmpv6_reply_translation(stru
>  		return 0;
>  
>  	if (skb->ip_summed != CHECKSUM_PARTIAL) {
> -		struct ipv6hdr *ipv6h = ipv6_hdr(skb);
> +		struct ipv6hdr *ipv6h;
> +
> +		if (!skb_make_writable(skb, skb->len))

can we just make sure what we need is linear? I mean, just the ipv6
header that is what we need, instead of the entire skbuff.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dave Johnson April 24, 2017, 1:10 p.m.
Pablo Neira Ayuso writes:
> On Thu, Apr 20, 2017 at 08:44:21AM -0400, Dave Johnson wrote:
> > 
> > When recalculating the outer ICMPv6 checksum for a reverse path NATv6
> > such as ICMPV6_TIME_EXCEED nf_nat_icmpv6_reply_translation() was
> > accessing data beyond the headlen of the skb for non-linear skb.  This
> > resulted in incorrect ICMPv6 checksum as garbage data was used.
> > 
> > Signed-off-by: Dave Johnson <dave-kernel@centerclick.org>
> > ---
> > diff -rup linux-4.9.23.orig/net/ipv6/netfilter/nf_nat_l3proto_ipv6.c linux-4.9.23/net/ipv6/netfilter/nf_nat_l3proto_ipv6.c
> > --- linux-4.9.23.orig/net/ipv6/netfilter/nf_nat_l3proto_ipv6.c	2017-04-18 01:12:30.000000000 -0400
> > +++ linux-4.9.23/net/ipv6/netfilter/nf_nat_l3proto_ipv6.c	2017-04-20 08:13:41.070493666 -0400
> > @@ -229,7 +229,12 @@ int nf_nat_icmpv6_reply_translation(stru
> >  		return 0;
> >  
> >  	if (skb->ip_summed != CHECKSUM_PARTIAL) {
> > -		struct ipv6hdr *ipv6h = ipv6_hdr(skb);
> > +		struct ipv6hdr *ipv6h;
> > +
> > +		if (!skb_make_writable(skb, skb->len))
> 
> can we just make sure what we need is linear? I mean, just the ipv6
> header that is what we need, instead of the entire skbuff.

the checksum below those lines is across the entire skb as unknown
updates were done in the l4 manip call just prior to this.

nf_nat_icmp_reply_translation() for ipv4 uses skb_checksum() to walk
non-linear skbs.  gave that a try and it works, will send an updated
patch in a bit.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pablo Neira April 25, 2017, 9:15 a.m.
On Thu, Apr 20, 2017 at 08:44:21AM -0400, Dave Johnson wrote:
> 
> When recalculating the outer ICMPv6 checksum for a reverse path NATv6
> such as ICMPV6_TIME_EXCEED nf_nat_icmpv6_reply_translation() was
> accessing data beyond the headlen of the skb for non-linear skb.  This
> resulted in incorrect ICMPv6 checksum as garbage data was used.

Applied to nf, thanks.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch hide | download patch | download mbox

diff -rup linux-4.9.23.orig/net/ipv6/netfilter/nf_nat_l3proto_ipv6.c linux-4.9.23/net/ipv6/netfilter/nf_nat_l3proto_ipv6.c
--- linux-4.9.23.orig/net/ipv6/netfilter/nf_nat_l3proto_ipv6.c	2017-04-18 01:12:30.000000000 -0400
+++ linux-4.9.23/net/ipv6/netfilter/nf_nat_l3proto_ipv6.c	2017-04-20 08:13:41.070493666 -0400
@@ -229,7 +229,12 @@  int nf_nat_icmpv6_reply_translation(stru
 		return 0;
 
 	if (skb->ip_summed != CHECKSUM_PARTIAL) {
-		struct ipv6hdr *ipv6h = ipv6_hdr(skb);
+		struct ipv6hdr *ipv6h;
+
+		if (!skb_make_writable(skb, skb->len))
+			return 0;
+
+		ipv6h = ipv6_hdr(skb);
 		inside = (void *)skb->data + hdrlen;
 		inside->icmp6.icmp6_cksum = 0;
 		inside->icmp6.icmp6_cksum =