Re: PATCH: tcp rfc 2385 security/bugfix for sparc64

Re: PATCH: tcp rfc 2385 security/bugfix for sparc64

[Matthias Dellweg]

Hi David,
while reviewing the tcp_md5-related code further i came across with another
two of these casts which you probably have missed. I don't actually think
that they impose a problem by now, but as you said we should remove them.
Matthias


--- linux-2.6.23.1.orig/net/ipv4/tcp_ipv4.c     2007-10-16 17:25:05.000000000 +0200
+++ linux-2.6.23.1/net/ipv4/tcp_ipv4.c  2007-10-16 17:50:05.000000000 +0200
@@ -857,16 +857,16 @@ int tcp_v4_md5_do_add(struct sock *sk, _
                      u8 *newkey, u8 newkeylen)
 {
        /* Add Key to the list */
-       struct tcp4_md5sig_key *key;
+       struct tcp_md5sig_key *key;
        struct tcp_sock *tp = tcp_sk(sk);
        struct tcp4_md5sig_key *keys;

-       key = (struct tcp4_md5sig_key *)tcp_v4_md5_do_lookup(sk, addr);
+       key = tcp_v4_md5_do_lookup(sk, addr);
        if (key) {
                /* Pre-existing entry - just update that one. */
-               kfree(key->base.key);
-               key->base.key = newkey;
-               key->base.keylen = newkeylen;
+               kfree(key->key);
+               key->key = newkey;
+               key->keylen = newkeylen;
        } else {
                struct tcp_md5sig_info *md5sig;

--- linux-2.6.23.1.orig/net/ipv6/tcp_ipv6.c     2007-10-16 17:47:57.000000000 +0200
+++ linux-2.6.23.1/net/ipv6/tcp_ipv6.c  2007-10-16 17:49:25.000000000 +0200
@@ -560,16 +560,16 @@ static int tcp_v6_md5_do_add(struct sock
                             char *newkey, u8 newkeylen)
 {
        /* Add key to the list */
-       struct tcp6_md5sig_key *key;
+       struct tcp_md5sig_key *key;
        struct tcp_sock *tp = tcp_sk(sk);
        struct tcp6_md5sig_key *keys;

-       key = (struct tcp6_md5sig_key*) tcp_v6_md5_do_lookup(sk, peer);
+       key = tcp_v6_md5_do_lookup(sk, peer);
        if (key) {
                /* modify existing entry - just update that one */
-               kfree(key->base.key);
-               key->base.key = newkey;
-               key->base.keylen = newkeylen;
+               kfree(key->key);
+               key->key = newkey;
+               key->keylen = newkeylen;
        } else {
                /* reallocate new list if current one is full. */
                if (!tp->md5sig_info) {


Signed-off-by: Matthias M. Dellweg <2500@gmx.de>

Re: PATCH: tcp rfc 2385 security/bugfix for sparc64

[David Miller]

From: Matthias Dellweg <2500@gmx.de>
Date: Tue, 16 Oct 2007 18:47:56 +0200

> while reviewing the tcp_md5-related code further i came across with another
> two of these casts which you probably have missed. I don't actually think
> that they impose a problem by now, but as you said we should remove them.

I'll apply this patch, thanks Matthias.

PATCH: tcp rfc 2385 security/bugfix for sparc64

[Peter Lieven]

TCP MD5 signatures on sparc64 (big-endian) completely fail on current
kernel releases in interoperability with Cisco/Foundry or other
little-endian linux systems.

The root cause is a cast in the return statement of tcp_v4_md5_do_lookup,
where a tcp4_md5sig_key is casted onto tcp_md5sig_key without proper
conversion. On little-endian systems the upper 8 bits are cut of which
yields the expected behaviour. However, on big-endian systems (like
sparc64) only the most significant 8 bits are preserved. Since
TCP_MD5SIG_MAXKEYLEN is 80, this always yields 0.

In the calculation of the md5 signature afterwards the key is therefore
not appended to the tcp segment which could result in a security problem
since only the presence of a md5 signature is checked, and the key itself
doesn't matter.

--- linux.old/include/net/tcp.h 2007-09-28 21:43:26.000000000 +0200 +++
linux/include/net/tcp.h     2007-09-28 21:45:35.000000000 +0200 @@ -1055,6
+1055,7 @@ static inline void clear_all_retrans_hin
 struct crypto_hash;

 /* - key database */
+/* this should be compatible with the head of the following two structs */
 struct tcp_md5sig_key {
        u8                      *key;
        u8                      keylen;
@@ -1062,13 +1063,13 @@ struct tcp_md5sig_key {

 struct tcp4_md5sig_key {
        u8                      *key;
-       u16                     keylen;
+       u8                      keylen;
        __be32                  addr;
 };

 struct tcp6_md5sig_key {
        u8                      *key;
-       u16                     keylen;
+       u8                      keylen;
 #if 0
        u32                     scope_id;       /* XXX */
 #endif


Signed-off-by: Peter Lieven <pl@dlh.net>
Signed-off-by: Matthias M. Dellweg <2500@gmx.de>

Re: PATCH: tcp rfc 2385 security/bugfix for sparc64

[David Miller]

From: "Peter Lieven" <pl@dlh.net>
Date: Fri, 28 Sep 2007 22:42:25 +0200 (CEST)

> TCP MD5 signatures on sparc64 (big-endian) completely fail on current
> kernel releases in interoperability with Cisco/Foundry or other
> little-endian linux systems.
> 
> The root cause is a cast in the return statement of tcp_v4_md5_do_lookup,
> where a tcp4_md5sig_key is casted onto tcp_md5sig_key without proper
> conversion. On little-endian systems the upper 8 bits are cut of which
> yields the expected behaviour. However, on big-endian systems (like
> sparc64) only the most significant 8 bits are preserved. Since
> TCP_MD5SIG_MAXKEYLEN is 80, this always yields 0.
> 
> In the calculation of the md5 signature afterwards the key is therefore
> not appended to the tcp segment which could result in a security problem
> since only the presence of a md5 signature is checked, and the key itself
> doesn't matter.

Thanks for finding this bug.

> --- linux.old/include/net/tcp.h 2007-09-28 21:43:26.000000000 +0200 +++
> linux/include/net/tcp.h     2007-09-28 21:45:35.000000000 +0200 @@ -1055,6
> +1055,7 @@ static inline void clear_all_retrans_hin

I'll have to apply this patch by hand because your email client
completely corrupted the patch.

> Signed-off-by: Peter Lieven <pl@dlh.net>
> Signed-off-by: Matthias M. Dellweg <2500@gmx.de>

Thanks again.

Re: PATCH: tcp rfc 2385 security/bugfix for sparc64

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Fri, 28 Sep 2007 14:20:15 -0700 (PDT)

> Thanks for finding this bug.
> 
> > --- linux.old/include/net/tcp.h 2007-09-28 21:43:26.000000000 +0200 +++
> > linux/include/net/tcp.h     2007-09-28 21:45:35.000000000 +0200 @@ -1055,6
> > +1055,7 @@ static inline void clear_all_retrans_hin
> 
> I'll have to apply this patch by hand because your email client
> completely corrupted the patch.

Actually, I think I'm going to put in a slightly different
fix.

If tcp4_md5sig_key and tcp6_md5sig_key have to begin with exacytly
tcp_md5sig_key's only two members, we should fully tell this
explicitly to the compiler and remove those ugly casts.

The casts are the real bug.

Here is the patch I will use after some testing:

diff --git a/include/net/tcp.h b/include/net/tcp.h
index 185c7ec..54053de 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -1059,14 +1059,12 @@ struct tcp_md5sig_key {
 };
 
 struct tcp4_md5sig_key {
-	u8			*key;
-	u16			keylen;
+	struct tcp_md5sig_key	base;
 	__be32			addr;
 };
 
 struct tcp6_md5sig_key {
-	u8			*key;
-	u16			keylen;
+	struct tcp_md5sig_key	base;
 #if 0
 	u32			scope_id;	/* XXX */
 #endif
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 9c94627..e089a97 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -833,8 +833,7 @@ static struct tcp_md5sig_key *
 		return NULL;
 	for (i = 0; i < tp->md5sig_info->entries4; i++) {
 		if (tp->md5sig_info->keys4[i].addr == addr)
-			return (struct tcp_md5sig_key *)
-						&tp->md5sig_info->keys4[i];
+			return &tp->md5sig_info->keys4[i].base;
 	}
 	return NULL;
 }
@@ -865,9 +864,9 @@ int tcp_v4_md5_do_add(struct sock *sk, __be32 addr,
 	key = (struct tcp4_md5sig_key *)tcp_v4_md5_do_lookup(sk, addr);
 	if (key) {
 		/* Pre-existing entry - just update that one. */
-		kfree(key->key);
-		key->key = newkey;
-		key->keylen = newkeylen;
+		kfree(key->base.key);
+		key->base.key = newkey;
+		key->base.keylen = newkeylen;
 	} else {
 		struct tcp_md5sig_info *md5sig;
 
@@ -906,9 +905,9 @@ int tcp_v4_md5_do_add(struct sock *sk, __be32 addr,
 			md5sig->alloced4++;
 		}
 		md5sig->entries4++;
-		md5sig->keys4[md5sig->entries4 - 1].addr   = addr;
-		md5sig->keys4[md5sig->entries4 - 1].key    = newkey;
-		md5sig->keys4[md5sig->entries4 - 1].keylen = newkeylen;
+		md5sig->keys4[md5sig->entries4 - 1].addr        = addr;
+		md5sig->keys4[md5sig->entries4 - 1].base.key    = newkey;
+		md5sig->keys4[md5sig->entries4 - 1].base.keylen = newkeylen;
 	}
 	return 0;
 }
@@ -930,7 +929,7 @@ int tcp_v4_md5_do_del(struct sock *sk, __be32 addr)
 	for (i = 0; i < tp->md5sig_info->entries4; i++) {
 		if (tp->md5sig_info->keys4[i].addr == addr) {
 			/* Free the key */
-			kfree(tp->md5sig_info->keys4[i].key);
+			kfree(tp->md5sig_info->keys4[i].base.key);
 			tp->md5sig_info->entries4--;
 
 			if (tp->md5sig_info->entries4 == 0) {
@@ -964,7 +963,7 @@ static void tcp_v4_clear_md5_list(struct sock *sk)
 	if (tp->md5sig_info->entries4) {
 		int i;
 		for (i = 0; i < tp->md5sig_info->entries4; i++)
-			kfree(tp->md5sig_info->keys4[i].key);
+			kfree(tp->md5sig_info->keys4[i].base.key);
 		tp->md5sig_info->entries4 = 0;
 		tcp_free_md5sig_pool();
 	}
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 0f7defb..3e06799 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -539,7 +539,7 @@ static struct tcp_md5sig_key *tcp_v6_md5_do_lookup(struct sock *sk,
 
 	for (i = 0; i < tp->md5sig_info->entries6; i++) {
 		if (ipv6_addr_cmp(&tp->md5sig_info->keys6[i].addr, addr) == 0)
-			return (struct tcp_md5sig_key *)&tp->md5sig_info->keys6[i];
+			return &tp->md5sig_info->keys6[i].base;
 	}
 	return NULL;
 }
@@ -567,9 +567,9 @@ static int tcp_v6_md5_do_add(struct sock *sk, struct in6_addr *peer,
 	key = (struct tcp6_md5sig_key*) tcp_v6_md5_do_lookup(sk, peer);
 	if (key) {
 		/* modify existing entry - just update that one */
-		kfree(key->key);
-		key->key = newkey;
-		key->keylen = newkeylen;
+		kfree(key->base.key);
+		key->base.key = newkey;
+		key->base.keylen = newkeylen;
 	} else {
 		/* reallocate new list if current one is full. */
 		if (!tp->md5sig_info) {
@@ -603,8 +603,8 @@ static int tcp_v6_md5_do_add(struct sock *sk, struct in6_addr *peer,
 
 		ipv6_addr_copy(&tp->md5sig_info->keys6[tp->md5sig_info->entries6].addr,
 			       peer);
-		tp->md5sig_info->keys6[tp->md5sig_info->entries6].key = newkey;
-		tp->md5sig_info->keys6[tp->md5sig_info->entries6].keylen = newkeylen;
+		tp->md5sig_info->keys6[tp->md5sig_info->entries6].base.key = newkey;
+		tp->md5sig_info->keys6[tp->md5sig_info->entries6].base.keylen = newkeylen;
 
 		tp->md5sig_info->entries6++;
 	}
@@ -626,7 +626,7 @@ static int tcp_v6_md5_do_del(struct sock *sk, struct in6_addr *peer)
 	for (i = 0; i < tp->md5sig_info->entries6; i++) {
 		if (ipv6_addr_cmp(&tp->md5sig_info->keys6[i].addr, peer) == 0) {
 			/* Free the key */
-			kfree(tp->md5sig_info->keys6[i].key);
+			kfree(tp->md5sig_info->keys6[i].base.key);
 			tp->md5sig_info->entries6--;
 
 			if (tp->md5sig_info->entries6 == 0) {
@@ -657,7 +657,7 @@ static void tcp_v6_clear_md5_list (struct sock *sk)
 
 	if (tp->md5sig_info->entries6) {
 		for (i = 0; i < tp->md5sig_info->entries6; i++)
-			kfree(tp->md5sig_info->keys6[i].key);
+			kfree(tp->md5sig_info->keys6[i].base.key);
 		tp->md5sig_info->entries6 = 0;
 		tcp_free_md5sig_pool();
 	}
@@ -668,7 +668,7 @@ static void tcp_v6_clear_md5_list (struct sock *sk)
 
 	if (tp->md5sig_info->entries4) {
 		for (i = 0; i < tp->md5sig_info->entries4; i++)
-			kfree(tp->md5sig_info->keys4[i].key);
+			kfree(tp->md5sig_info->keys4[i].base.key);
 		tp->md5sig_info->entries4 = 0;
 		tcp_free_md5sig_pool();
 	}