Patchwork [natty,natty/ti-omap4,CVE,1/1] ipv6: make fragment identifications less predictable

login
register
mail settings
Submitter Andy Whitcroft
Date Aug. 23, 2011, 2:58 p.m.
Message ID <1314111512-24177-6-git-send-email-apw@canonical.com>
Download mbox | patch
Permalink /patch/111126/
State New
Headers show

Comments

Andy Whitcroft - Aug. 23, 2011, 2:58 p.m.
[ Backport of upstream commit 87c48fa3b4630905f98268dde838ee43626a060c ]

Fernando Gont reported current IPv6 fragment identification generation
was not secure, because using a very predictable system-wide generator,
allowing various attacks.

IPv4 uses inetpeer cache to address this problem and to get good
performance. We'll use this mechanism when IPv6 inetpeer is stable
enough in linux-3.1

For the time being, we use jhash on destination address to provide less
predictable identifications. Also remove a spinlock and use cmpxchg() to
get better SMP performance.

Reported-by: Fernando Gont <fernando@gont.com.ar>
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

CVE-2011-2699
BugLink: http://bugs.launchpad.net/bugs/827685
Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 include/net/ipv6.h      |   12 +-----------
 include/net/transp_v6.h |    2 ++
 net/ipv6/af_inet6.c     |    2 ++
 net/ipv6/ip6_output.c   |   40 +++++++++++++++++++++++++++++++++++-----
 net/ipv6/udp.c          |    2 +-
 5 files changed, 41 insertions(+), 17 deletions(-)
Tim Gardner - Aug. 23, 2011, 6:31 p.m.

Stefan Bader - Aug. 24, 2011, 9:53 a.m.
On 23.08.2011 16:58, Andy Whitcroft wrote:
> [ Backport of upstream commit 87c48fa3b4630905f98268dde838ee43626a060c ]
> 
> Fernando Gont reported current IPv6 fragment identification generation
> was not secure, because using a very predictable system-wide generator,
> allowing various attacks.
> 
> IPv4 uses inetpeer cache to address this problem and to get good
> performance. We'll use this mechanism when IPv6 inetpeer is stable
> enough in linux-3.1
> 
> For the time being, we use jhash on destination address to provide less
> predictable identifications. Also remove a spinlock and use cmpxchg() to
> get better SMP performance.
> 
> Reported-by: Fernando Gont <fernando@gont.com.ar>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> Signed-off-by: David S. Miller <davem@davemloft.net>
> Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
> 
> CVE-2011-2699
> BugLink: http://bugs.launchpad.net/bugs/827685
> Signed-off-by: Andy Whitcroft <apw@canonical.com>
> ---
>  include/net/ipv6.h      |   12 +-----------
>  include/net/transp_v6.h |    2 ++
>  net/ipv6/af_inet6.c     |    2 ++
>  net/ipv6/ip6_output.c   |   40 +++++++++++++++++++++++++++++++++++-----
>  net/ipv6/udp.c          |    2 +-
>  5 files changed, 41 insertions(+), 17 deletions(-)
> 
> diff --git a/include/net/ipv6.h b/include/net/ipv6.h
> index 96e50e0..24175cf 100644
> --- a/include/net/ipv6.h
> +++ b/include/net/ipv6.h
> @@ -460,17 +460,7 @@ static inline int ipv6_addr_diff(const struct in6_addr *a1, const struct in6_add
>  	return __ipv6_addr_diff(a1, a2, sizeof(struct in6_addr));
>  }
>  
> -static __inline__ void ipv6_select_ident(struct frag_hdr *fhdr)
> -{
> -	static u32 ipv6_fragmentation_id = 1;
> -	static DEFINE_SPINLOCK(ip6_id_lock);
> -
> -	spin_lock_bh(&ip6_id_lock);
> -	fhdr->identification = htonl(ipv6_fragmentation_id);
> -	if (++ipv6_fragmentation_id == 0)
> -		ipv6_fragmentation_id = 1;
> -	spin_unlock_bh(&ip6_id_lock);
> -}
> +extern void ipv6_select_ident(struct frag_hdr *fhdr, struct rt6_info *rt);
>  
>  /*
>   *	Prototypes exported by ipv6
> diff --git a/include/net/transp_v6.h b/include/net/transp_v6.h
> index 42a0eb6..70eef79 100644
> --- a/include/net/transp_v6.h
> +++ b/include/net/transp_v6.h
> @@ -16,6 +16,8 @@ extern struct proto tcpv6_prot;
>  
>  struct flowi;
>  
> +extern void initialize_hashidentrnd(void);
> +
>  /* extention headers */
>  extern int				ipv6_exthdrs_init(void);
>  extern void				ipv6_exthdrs_exit(void);
> diff --git a/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c
> index 978e80e..ca171b7 100644
> --- a/net/ipv6/af_inet6.c
> +++ b/net/ipv6/af_inet6.c
> @@ -1081,6 +1081,8 @@ static int __init inet6_init(void)
>  		goto out;
>  	}
>  
> +	initialize_hashidentrnd();
> +
>  	err = proto_register(&tcpv6_prot, 1);
>  	if (err)
>  		goto out;
> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
> index 5f8d242..ed0c1a7 100644
> --- a/net/ipv6/ip6_output.c
> +++ b/net/ipv6/ip6_output.c
> @@ -596,6 +596,35 @@ int ip6_find_1stfragopt(struct sk_buff *skb, u8 **nexthdr)
>  	return offset;
>  }
>  
> +static u32 hashidentrnd __read_mostly;
> +#define FID_HASH_SZ 16
> +static u32 ipv6_fragmentation_id[FID_HASH_SZ];
> +
> +void __init initialize_hashidentrnd(void)
> +{
> +	get_random_bytes(&hashidentrnd, sizeof(hashidentrnd));
> +}
> +
> +static u32 __ipv6_select_ident(const struct in6_addr *addr)
> +{
> +	u32 newid, oldid, hash = jhash2((u32 *)addr, 4, hashidentrnd);
> +	u32 *pid = &ipv6_fragmentation_id[hash % FID_HASH_SZ];
> +
> +	do {
> +		oldid = *pid;
> +		newid = oldid + 1;
> +		if (!(hash + newid))
> +			newid++;

I am trying to understand what hash + newid is supposed to tell. Looking at the
old code there was a test to prevent the id from being 0 when it wrapped. If I
understand this patch correctly there is now multiple ids selected based on the
addresses hash... So why is the above not just

if (!newid)
  newid = 1;

?

> +	} while (cmpxchg(pid, oldid, newid) != oldid);
> +
> +	return hash + newid;
> +}
> +
> +void ipv6_select_ident(struct frag_hdr *fhdr, struct rt6_info *rt)
> +{
> +	fhdr->identification = htonl(__ipv6_select_ident(&rt->rt6i_dst.addr));
> +}
> +
>  int ip6_fragment(struct sk_buff *skb, int (*output)(struct sk_buff *))
>  {
>  	struct sk_buff *frag;
> @@ -680,7 +709,7 @@ int ip6_fragment(struct sk_buff *skb, int (*output)(struct sk_buff *))
>  		skb_reset_network_header(skb);
>  		memcpy(skb_network_header(skb), tmp_hdr, hlen);
>  
> -		ipv6_select_ident(fh);
> +		ipv6_select_ident(fh, rt);
>  		fh->nexthdr = nexthdr;
>  		fh->reserved = 0;
>  		fh->frag_off = htons(IP6_MF);
> @@ -826,7 +855,7 @@ slow_path:
>  		fh->nexthdr = nexthdr;
>  		fh->reserved = 0;
>  		if (!frag_id) {
> -			ipv6_select_ident(fh);
> +			ipv6_select_ident(fh, rt);
>  			frag_id = fh->identification;
>  		} else
>  			fh->identification = frag_id;
> @@ -1030,7 +1059,8 @@ static inline int ip6_ufo_append_data(struct sock *sk,
>  			int getfrag(void *from, char *to, int offset, int len,
>  			int odd, struct sk_buff *skb),
>  			void *from, int length, int hh_len, int fragheaderlen,
> -			int transhdrlen, int mtu,unsigned int flags)
> +			int transhdrlen, int mtu,unsigned int flags,
> +			struct rt6_info *rt)
>  
>  {
>  	struct sk_buff *skb;
> @@ -1075,7 +1105,7 @@ static inline int ip6_ufo_append_data(struct sock *sk,
>  		skb_shinfo(skb)->gso_size = (mtu - fragheaderlen -
>  					     sizeof(struct frag_hdr)) & ~7;
>  		skb_shinfo(skb)->gso_type = SKB_GSO_UDP;
> -		ipv6_select_ident(&fhdr);
> +		ipv6_select_ident(&fhdr, rt);
>  		skb_shinfo(skb)->ip6_frag_id = fhdr.identification;
>  		__skb_queue_tail(&sk->sk_write_queue, skb);
>  
> @@ -1231,7 +1261,7 @@ int ip6_append_data(struct sock *sk, int getfrag(void *from, char *to,
>  
>  			err = ip6_ufo_append_data(sk, getfrag, from, length,
>  						  hh_len, fragheaderlen,
> -						  transhdrlen, mtu, flags);
> +						  transhdrlen, mtu, flags, rt);
>  			if (err)
>  				goto error;
>  			return 0;
> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
> index 119edb6..b6de206 100644
> --- a/net/ipv6/udp.c
> +++ b/net/ipv6/udp.c
> @@ -1360,7 +1360,7 @@ static struct sk_buff *udp6_ufo_fragment(struct sk_buff *skb, int features)
>  	fptr = (struct frag_hdr *)(skb_network_header(skb) + unfrag_ip6hlen);
>  	fptr->nexthdr = nexthdr;
>  	fptr->reserved = 0;
> -	ipv6_select_ident(fptr);
> +	ipv6_select_ident(fptr, (struct rt6_info *)skb_dst(skb));
>  
>  	/* Fragment the skb. ipv6 header and the remaining fields of the
>  	 * fragment header are updated in ipv6_gso_segment()
Andy Whitcroft - Aug. 24, 2011, 10:30 a.m.
On Wed, Aug 24, 2011 at 11:53:30AM +0200, Stefan Bader wrote:

> > -static __inline__ void ipv6_select_ident(struct frag_hdr *fhdr)
> > -{
> > -	static u32 ipv6_fragmentation_id = 1;
> > -	static DEFINE_SPINLOCK(ip6_id_lock);
> > -
> > -	spin_lock_bh(&ip6_id_lock);
> > -	fhdr->identification = htonl(ipv6_fragmentation_id);
> > -	if (++ipv6_fragmentation_id == 0)
> > -		ipv6_fragmentation_id = 1;
> > -	spin_unlock_bh(&ip6_id_lock);
> > -}

This _old_ code ensures that fragmentation_id is never == 0, and that
value is used directly without modification.  An id of 0 is invalid.

> > +static u32 hashidentrnd __read_mostly;
> > +#define FID_HASH_SZ 16
> > +static u32 ipv6_fragmentation_id[FID_HASH_SZ];
> > +
> > +void __init initialize_hashidentrnd(void)
> > +{
> > +	get_random_bytes(&hashidentrnd, sizeof(hashidentrnd));
> > +}
> > +
> > +static u32 __ipv6_select_ident(const struct in6_addr *addr)
> > +{
> > +	u32 newid, oldid, hash = jhash2((u32 *)addr, 4, hashidentrnd);
> > +	u32 *pid = &ipv6_fragmentation_id[hash % FID_HASH_SZ];
> > +
> > +	do {
> > +		oldid = *pid;
> > +		newid = oldid + 1;
> > +		if (!(hash + newid))
> > +			newid++;
> 
> I am trying to understand what hash + newid is supposed to tell. Looking at the
> old code there was a test to prevent the id from being 0 when it wrapped. If I
> understand this patch correctly there is now multiple ids selected based on the
> addresses hash... So why is the above not just
> 
> if (!newid)
>   newid = 1;
> 
> ?
> 
> > +	} while (cmpxchg(pid, oldid, newid) != oldid);
> > +
> > +	return hash + newid;
> > +}

The key here is that the value which will be used is this last one at
the end.  It will return hash + newid.  It is that value which must not
be 0 as it is that which is used as the id.  Therefore the check is
hash + newid not being 0 to match the return.

-apw
Stefan Bader - Aug. 24, 2011, 10:42 a.m.
On 24.08.2011 12:30, Andy Whitcroft wrote:
> On Wed, Aug 24, 2011 at 11:53:30AM +0200, Stefan Bader wrote:
> 
>>> -static __inline__ void ipv6_select_ident(struct frag_hdr *fhdr)
>>> -{
>>> -	static u32 ipv6_fragmentation_id = 1;
>>> -	static DEFINE_SPINLOCK(ip6_id_lock);
>>> -
>>> -	spin_lock_bh(&ip6_id_lock);
>>> -	fhdr->identification = htonl(ipv6_fragmentation_id);
>>> -	if (++ipv6_fragmentation_id == 0)
>>> -		ipv6_fragmentation_id = 1;
>>> -	spin_unlock_bh(&ip6_id_lock);
>>> -}
> 
> This _old_ code ensures that fragmentation_id is never == 0, and that
> value is used directly without modification.  An id of 0 is invalid.
> 
>>> +static u32 hashidentrnd __read_mostly;
>>> +#define FID_HASH_SZ 16
>>> +static u32 ipv6_fragmentation_id[FID_HASH_SZ];
>>> +
>>> +void __init initialize_hashidentrnd(void)
>>> +{
>>> +	get_random_bytes(&hashidentrnd, sizeof(hashidentrnd));
>>> +}
>>> +
>>> +static u32 __ipv6_select_ident(const struct in6_addr *addr)
>>> +{
>>> +	u32 newid, oldid, hash = jhash2((u32 *)addr, 4, hashidentrnd);
>>> +	u32 *pid = &ipv6_fragmentation_id[hash % FID_HASH_SZ];
>>> +
>>> +	do {
>>> +		oldid = *pid;
>>> +		newid = oldid + 1;
>>> +		if (!(hash + newid))
>>> +			newid++;
>>
>> I am trying to understand what hash + newid is supposed to tell. Looking at the
>> old code there was a test to prevent the id from being 0 when it wrapped. If I
>> understand this patch correctly there is now multiple ids selected based on the
>> addresses hash... So why is the above not just
>>
>> if (!newid)
>>   newid = 1;
>>
>> ?
>>
>>> +	} while (cmpxchg(pid, oldid, newid) != oldid);
>>> +
>>> +	return hash + newid;
>>> +}
> 
> The key here is that the value which will be used is this last one at
> the end.  It will return hash + newid.  It is that value which must not
> be 0 as it is that which is used as the id.  Therefore the check is
> hash + newid not being 0 to match the return.
> 
> -apw

I see. Somehow missed that the whole hash+newid is returned for assignment and
only the newid part being stored in a bucket...

Patch

diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index 96e50e0..24175cf 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -460,17 +460,7 @@  static inline int ipv6_addr_diff(const struct in6_addr *a1, const struct in6_add
 	return __ipv6_addr_diff(a1, a2, sizeof(struct in6_addr));
 }
 
-static __inline__ void ipv6_select_ident(struct frag_hdr *fhdr)
-{
-	static u32 ipv6_fragmentation_id = 1;
-	static DEFINE_SPINLOCK(ip6_id_lock);
-
-	spin_lock_bh(&ip6_id_lock);
-	fhdr->identification = htonl(ipv6_fragmentation_id);
-	if (++ipv6_fragmentation_id == 0)
-		ipv6_fragmentation_id = 1;
-	spin_unlock_bh(&ip6_id_lock);
-}
+extern void ipv6_select_ident(struct frag_hdr *fhdr, struct rt6_info *rt);
 
 /*
  *	Prototypes exported by ipv6
diff --git a/include/net/transp_v6.h b/include/net/transp_v6.h
index 42a0eb6..70eef79 100644
--- a/include/net/transp_v6.h
+++ b/include/net/transp_v6.h
@@ -16,6 +16,8 @@  extern struct proto tcpv6_prot;
 
 struct flowi;
 
+extern void initialize_hashidentrnd(void);
+
 /* extention headers */
 extern int				ipv6_exthdrs_init(void);
 extern void				ipv6_exthdrs_exit(void);
diff --git a/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c
index 978e80e..ca171b7 100644
--- a/net/ipv6/af_inet6.c
+++ b/net/ipv6/af_inet6.c
@@ -1081,6 +1081,8 @@  static int __init inet6_init(void)
 		goto out;
 	}
 
+	initialize_hashidentrnd();
+
 	err = proto_register(&tcpv6_prot, 1);
 	if (err)
 		goto out;
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 5f8d242..ed0c1a7 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -596,6 +596,35 @@  int ip6_find_1stfragopt(struct sk_buff *skb, u8 **nexthdr)
 	return offset;
 }
 
+static u32 hashidentrnd __read_mostly;
+#define FID_HASH_SZ 16
+static u32 ipv6_fragmentation_id[FID_HASH_SZ];
+
+void __init initialize_hashidentrnd(void)
+{
+	get_random_bytes(&hashidentrnd, sizeof(hashidentrnd));
+}
+
+static u32 __ipv6_select_ident(const struct in6_addr *addr)
+{
+	u32 newid, oldid, hash = jhash2((u32 *)addr, 4, hashidentrnd);
+	u32 *pid = &ipv6_fragmentation_id[hash % FID_HASH_SZ];
+
+	do {
+		oldid = *pid;
+		newid = oldid + 1;
+		if (!(hash + newid))
+			newid++;
+	} while (cmpxchg(pid, oldid, newid) != oldid);
+
+	return hash + newid;
+}
+
+void ipv6_select_ident(struct frag_hdr *fhdr, struct rt6_info *rt)
+{
+	fhdr->identification = htonl(__ipv6_select_ident(&rt->rt6i_dst.addr));
+}
+
 int ip6_fragment(struct sk_buff *skb, int (*output)(struct sk_buff *))
 {
 	struct sk_buff *frag;
@@ -680,7 +709,7 @@  int ip6_fragment(struct sk_buff *skb, int (*output)(struct sk_buff *))
 		skb_reset_network_header(skb);
 		memcpy(skb_network_header(skb), tmp_hdr, hlen);
 
-		ipv6_select_ident(fh);
+		ipv6_select_ident(fh, rt);
 		fh->nexthdr = nexthdr;
 		fh->reserved = 0;
 		fh->frag_off = htons(IP6_MF);
@@ -826,7 +855,7 @@  slow_path:
 		fh->nexthdr = nexthdr;
 		fh->reserved = 0;
 		if (!frag_id) {
-			ipv6_select_ident(fh);
+			ipv6_select_ident(fh, rt);
 			frag_id = fh->identification;
 		} else
 			fh->identification = frag_id;
@@ -1030,7 +1059,8 @@  static inline int ip6_ufo_append_data(struct sock *sk,
 			int getfrag(void *from, char *to, int offset, int len,
 			int odd, struct sk_buff *skb),
 			void *from, int length, int hh_len, int fragheaderlen,
-			int transhdrlen, int mtu,unsigned int flags)
+			int transhdrlen, int mtu,unsigned int flags,
+			struct rt6_info *rt)
 
 {
 	struct sk_buff *skb;
@@ -1075,7 +1105,7 @@  static inline int ip6_ufo_append_data(struct sock *sk,
 		skb_shinfo(skb)->gso_size = (mtu - fragheaderlen -
 					     sizeof(struct frag_hdr)) & ~7;
 		skb_shinfo(skb)->gso_type = SKB_GSO_UDP;
-		ipv6_select_ident(&fhdr);
+		ipv6_select_ident(&fhdr, rt);
 		skb_shinfo(skb)->ip6_frag_id = fhdr.identification;
 		__skb_queue_tail(&sk->sk_write_queue, skb);
 
@@ -1231,7 +1261,7 @@  int ip6_append_data(struct sock *sk, int getfrag(void *from, char *to,
 
 			err = ip6_ufo_append_data(sk, getfrag, from, length,
 						  hh_len, fragheaderlen,
-						  transhdrlen, mtu, flags);
+						  transhdrlen, mtu, flags, rt);
 			if (err)
 				goto error;
 			return 0;
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index 119edb6..b6de206 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -1360,7 +1360,7 @@  static struct sk_buff *udp6_ufo_fragment(struct sk_buff *skb, int features)
 	fptr = (struct frag_hdr *)(skb_network_header(skb) + unfrag_ip6hlen);
 	fptr->nexthdr = nexthdr;
 	fptr->reserved = 0;
-	ipv6_select_ident(fptr);
+	ipv6_select_ident(fptr, (struct rt6_info *)skb_dst(skb));
 
 	/* Fragment the skb. ipv6 header and the remaining fields of the
 	 * fragment header are updated in ipv6_gso_segment()