TCP MD5 and socket accept

TCP MD5 and socket accept

[Stephen Hemminger]

It looks like the child socket on accept doesn't inherit the MD5 mappings
from the listening socket.  This leads to the situation where the data
after the initial SYN, ACK gets a MD5 mismatch until the child socket
is updated with setsockopt.

My question was this an intentional part of the initial design?

What will break if tcp_create_openreq_child was fixed to copy md5_info if
present?

This all comes about because right now using Quagga a Linux to Linux
works with TCP MD5. But a Linux to Cisco connection fails if using
TCP MD5.

Re: TCP MD5 and socket accept

[Adam Langley]

On Wed, Jun 25, 2008 at 10:56 PM, Stephen Hemminger
<shemminger@vyatta.com> wrote:
> What will break if tcp_create_openreq_child was fixed to copy md5_info if
> present?
>
> This all comes about because right now using Quagga a Linux to Linux
> works with TCP MD5. But a Linux to Cisco connection fails if using
> TCP MD5.

I'll have a look at this later today but, as you say, Linux to Linux
works, and getting the key wrong certainly breaks it (and without
setsockopt on the child, I believe). So some MD5 information is
getting copied from listening sockets to children.

Also note the MD5 on Linux is pretty badly broken in the face of
packet loss at the moment. I have patches floating around to fix it,
but not in any trees yet.


AGL

-- 
Adam Langley agl@imperialviolet.org http://www.imperialviolet.org

Re: TCP MD5 and socket accept

[Adam Langley]

On Thu, Jun 26, 2008 at 7:46 AM, Adam Langley <agl@imperialviolet.org> wrote:
> I'll have a look at this later today

Setup: Linux net-2.6 (almost) <-> Linux net-2.6 (almost)). Userspace
is only setting TCP_MD5SIG just before connect on the client side and
just before bind on the server side. Packet dumps show that all
packets are signed correctly.

>From the code, it appears that we might, in fact, be coping the key
information twice:

tcp_minisocks.c:tcp_check_req is calling
inet_csk(sk)->icsk_af_ops->syn_recv_sock, which becomes
tcp_ipv4.c:tcp_v4_syn_recv_sock which appears to copy the MD5 keys
over. Additionally, right after that call in tcp_check_req, it appears
that the keys are copied again.

Can you provide packet dumps of Linux screwing up in the face of a
connection to a Cisco?

Cheers,

-- 
Adam Langley agl@imperialviolet.org http://www.imperialviolet.org

Re: TCP MD5 and socket accept

[Stephen Hemminger]

On Thu, 26 Jun 2008 07:46:59 -0700
"Adam Langley" <agl@imperialviolet.org> wrote:

> On Wed, Jun 25, 2008 at 10:56 PM, Stephen Hemminger
> <shemminger@vyatta.com> wrote:
> > What will break if tcp_create_openreq_child was fixed to copy md5_info if
> > present?
> >
> > This all comes about because right now using Quagga a Linux to Linux
> > works with TCP MD5. But a Linux to Cisco connection fails if using
> > TCP MD5.
> 
> I'll have a look at this later today but, as you say, Linux to Linux
> works, and getting the key wrong certainly breaks it (and without
> setsockopt on the child, I believe). So some MD5 information is
> getting copied from listening sockets to children.
> 
> Also note the MD5 on Linux is pretty badly broken in the face of
> packet loss at the moment. I have patches floating around to fix it,
> but not in any trees yet.
> 
> 
> AGL
> 

The problem is that md5 calculation assumes that the data buffer is
linear! It doesn't handle any kind of scatter-gather in skb!

int tcp_calc_md5_hash(char *md5_hash, struct tcp_md5sig_key *key,
		      int bplen,
		      struct tcphdr *th, unsigned int tcplen,
		      struct tcp_md5sig_pool *hp)
{
...
	/* 3. The TCP segment data (if any) */
	data_len = tcplen - (th->doff << 2);
	if (data_len > 0) {
		u8 *data = (u8 *)th + (th->doff << 2);
		sg_set_buf(&sg[block++], data, data_len);
		nbytes += data_len;
	}

This is wrong, it needs to handle fragmented skb's.  I'll work out a patch
but it means passing skb to calc_md5_hash or just turn off using scatter/gather
on MD5 connections.

[PATCH] TCP MD5 and TSO/SG breakage

[Stephen Hemminger]

The TCP MD5 support is broken on any device that does scatter gather.
The MD5 calculation code doesn't support scatter/gather, the md5_calc
API assumes the data follows the TCP header. It is too late to rework this
code for 2.6.26 (and backport to stable). So the sane thing to do is block
use of SG on TCP sockets using MD5 option.

The sk->sk_route_caps are used at the top level to determine if TSO
or scatter gather can be used. Since the route_caps are set in several
places, and can get changed by ip rerouting, they need to be wacked
in several places.

There is also a small problem in old code where a retransmit gets a new route and
therefore inherits new route_caps. This could cause TSO segments to be
generated with MD5!

Patch against 2.6.26-rc8 (latest), this patch won't work for net-next-2.6
since the MD5 stuff got rearranged there. For 2.6.27 let's work out
a version MD5 calculation that can do SG.

Should also be considered for stable, because a user can turn on MD5
and send the kernel off doing MD5 calculation on random bits of memory.
Not a security hole, unless your massively paranoid but certainly a potential
for kernel crasher.

Tested by using a modified version of netcat with MD5 support.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/net/ipv4/tcp_ipv4.c	2008-06-26 22:05:13.000000000 -0700
+++ b/net/ipv4/tcp_ipv4.c	2008-06-26 22:10:15.000000000 -0700
@@ -861,7 +861,7 @@ int tcp_v4_md5_do_add(struct sock *sk, _
 				kfree(newkey);
 				return -ENOMEM;
 			}
-			sk->sk_route_caps &= ~NETIF_F_GSO_MASK;
+			sk->sk_route_caps &= ~(NETIF_F_GSO_MASK|NETIF_F_SG);
 		}
 		if (tcp_alloc_md5sig_pool() == NULL) {
 			kfree(newkey);
@@ -990,7 +990,7 @@ static int tcp_v4_parse_md5_keys(struct 
 			return -EINVAL;
 
 		tp->md5sig_info = p;
-		sk->sk_route_caps &= ~NETIF_F_GSO_MASK;
+		sk->sk_route_caps &= ~(NETIF_F_GSO_MASK|NETIF_F_SG);
 	}
 
 	newkey = kmemdup(cmd.tcpm_key, cmd.tcpm_keylen, GFP_KERNEL);
@@ -1196,6 +1196,14 @@ done_opts:
 		return 1;
 	}
 
+	/* md5 calculation needs linear skb, so if can't fix it. assume mismatch */
+	if (skb_is_nonlinear(skb)) {
+		if (__skb_linearize(skb))
+			return 1;	/* out of memory, assume failed (ie drop) */
+		th = tcp_hdr(skb);
+		iph = ip_hdr(skb);
+	}
+
 	/* Okay, so this is hash_expected and hash_location -
 	 * so we need to calculate the checksum.
 	 */
@@ -1452,6 +1460,7 @@ struct sock *tcp_v4_syn_recv_sock(struct
 		if (newkey != NULL)
 			tcp_v4_md5_do_add(newsk, inet_sk(sk)->daddr,
 					  newkey, key->keylen);
+		newsk->sk_route_caps &= ~(NETIF_F_GSO_MASK|NETIF_F_SG);
 	}
 #endif
 
--- a/net/ipv4/tcp_output.c	2008-06-26 22:05:13.000000000 -0700
+++ b/net/ipv4/tcp_output.c	2008-06-26 22:26:58.000000000 -0700
@@ -542,8 +542,10 @@ static int tcp_transmit_skb(struct sock 
 	 * room for it.
 	 */
 	md5 = tp->af_specific->md5_lookup(sk, sk);
-	if (md5)
+	if (md5) {
 		tcp_header_size += TCPOLEN_MD5SIG_ALIGNED;
+		sk->sk_route_caps &= ~(NETIF_F_GSO_MASK|NETIF_F_SG);
+	}
 #endif
 
 	skb_push(skb, tcp_header_size);
@@ -603,6 +605,16 @@ static int tcp_transmit_skb(struct sock 
 #ifdef CONFIG_TCP_MD5SIG
 	/* Calculate the MD5 hash, as we have all we need now */
 	if (md5) {
+		/* This shouldn't happen, but it is possible that a retransmit
+		 * causes a reroute onto a different interface and we get TSO/SG
+		 * skb that is dropped here, and route_caps has already been
+		 * reset so the next retransmit will be okay.
+		 */
+		if (unlikely(skb_is_nonlinear(skb))) {
+			kfree_skb(skb);
+			return NET_XMIT_DROP;
+		}
+
 		tp->af_specific->calc_md5_hash(md5_hash_location,
 					       md5,
 					       sk, NULL, NULL,
--- a/net/ipv6/tcp_ipv6.c	2008-06-26 22:05:13.000000000 -0700
+++ b/net/ipv6/tcp_ipv6.c	2008-06-26 22:05:15.000000000 -0700
@@ -583,7 +583,7 @@ static int tcp_v6_md5_do_add(struct sock
 				kfree(newkey);
 				return -ENOMEM;
 			}
-			sk->sk_route_caps &= ~NETIF_F_GSO_MASK;
+			sk->sk_route_caps &= ~(NETIF_F_GSO_MASK|NETIF_F_SG);
 		}
 		if (tcp_alloc_md5sig_pool() == NULL) {
 			kfree(newkey);
@@ -720,7 +720,7 @@ static int tcp_v6_parse_md5_keys (struct
 			return -ENOMEM;
 
 		tp->md5sig_info = p;
-		sk->sk_route_caps &= ~NETIF_F_GSO_MASK;
+		sk->sk_route_caps &= ~(NETIF_F_GSO_MASK|NETIF_F_SG);
 	}
 
 	newkey = kmemdup(cmd.tcpm_key, cmd.tcpm_keylen, GFP_KERNEL);
@@ -1529,6 +1529,7 @@ static struct sock * tcp_v6_syn_recv_soc
 		if (newkey != NULL)
 			tcp_v6_md5_do_add(newsk, &inet6_sk(sk)->daddr,
 					  newkey, key->keylen);
+		newsk->sk_route_caps &= ~(NETIF_F_GSO_MASK|NETIF_F_SG);
 	}
 #endif
 

[PATCH] TCP MD5 and TSO/SG breakage

[Stephen Hemminger]

The TCP MD5 support is broken on any device that does scatter gather.
The MD5 calculation code doesn't support scatter/gather, the md5_calc
API assumes the data follows the TCP header. It is too late to rework this
code for 2.6.26 (and backport to stable). So the sane thing to do is block
use of SG on TCP sockets using MD5 option.

The sk->sk_route_caps are used at the top level to determine if TSO
or scatter gather can be used. Since the route_caps are set in several
places, and can get changed by ip rerouting, they need to be wacked
in several places.

There is also a small problem in old code where a retransmit gets a new route and
therefore inherits new route_caps. This could cause TSO segments to be
generated with MD5!

Patch against 2.6.26-rc8 (latest), this patch won't work for net-next-2.6
since the MD5 stuff got rearranged there. For 2.6.27 let's work out
a version MD5 calculation that can do SG.

Should also be considered for stable, because a user can turn on MD5
and send the kernel off doing MD5 calculation on random bits of memory.
Not a security hole, unless your massively paranoid but certainly a potential
for kernel crasher.

Tested by using a modified version of netcat with MD5 support.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
---
Resent because it didn't show up on netdev

--- a/net/ipv4/tcp_ipv4.c	2008-06-26 22:05:13.000000000 -0700
+++ b/net/ipv4/tcp_ipv4.c	2008-06-26 22:10:15.000000000 -0700
@@ -861,7 +861,7 @@ int tcp_v4_md5_do_add(struct sock *sk, _
 				kfree(newkey);
 				return -ENOMEM;
 			}
-			sk->sk_route_caps &= ~NETIF_F_GSO_MASK;
+			sk->sk_route_caps &= ~(NETIF_F_GSO_MASK|NETIF_F_SG);
 		}
 		if (tcp_alloc_md5sig_pool() == NULL) {
 			kfree(newkey);
@@ -990,7 +990,7 @@ static int tcp_v4_parse_md5_keys(struct 
 			return -EINVAL;
 
 		tp->md5sig_info = p;
-		sk->sk_route_caps &= ~NETIF_F_GSO_MASK;
+		sk->sk_route_caps &= ~(NETIF_F_GSO_MASK|NETIF_F_SG);
 	}
 
 	newkey = kmemdup(cmd.tcpm_key, cmd.tcpm_keylen, GFP_KERNEL);
@@ -1196,6 +1196,14 @@ done_opts:
 		return 1;
 	}
 
+	/* md5 calculation needs linear skb, so if can't fix it. assume mismatch */
+	if (skb_is_nonlinear(skb)) {
+		if (__skb_linearize(skb))
+			return 1;	/* out of memory, assume failed (ie drop) */
+		th = tcp_hdr(skb);
+		iph = ip_hdr(skb);
+	}
+
 	/* Okay, so this is hash_expected and hash_location -
 	 * so we need to calculate the checksum.
 	 */
@@ -1452,6 +1460,7 @@ struct sock *tcp_v4_syn_recv_sock(struct
 		if (newkey != NULL)
 			tcp_v4_md5_do_add(newsk, inet_sk(sk)->daddr,
 					  newkey, key->keylen);
+		newsk->sk_route_caps &= ~(NETIF_F_GSO_MASK|NETIF_F_SG);
 	}
 #endif
 
--- a/net/ipv4/tcp_output.c	2008-06-26 22:05:13.000000000 -0700
+++ b/net/ipv4/tcp_output.c	2008-06-26 22:26:58.000000000 -0700
@@ -542,8 +542,10 @@ static int tcp_transmit_skb(struct sock 
 	 * room for it.
 	 */
 	md5 = tp->af_specific->md5_lookup(sk, sk);
-	if (md5)
+	if (md5) {
 		tcp_header_size += TCPOLEN_MD5SIG_ALIGNED;
+		sk->sk_route_caps &= ~(NETIF_F_GSO_MASK|NETIF_F_SG);
+	}
 #endif
 
 	skb_push(skb, tcp_header_size);
@@ -603,6 +605,16 @@ static int tcp_transmit_skb(struct sock 
 #ifdef CONFIG_TCP_MD5SIG
 	/* Calculate the MD5 hash, as we have all we need now */
 	if (md5) {
+		/* This shouldn't happen, but it is possible that a retransmit
+		 * causes a reroute onto a different interface and we get TSO/SG
+		 * skb that is dropped here, and route_caps has already been
+		 * reset so the next retransmit will be okay.
+		 */
+		if (unlikely(skb_is_nonlinear(skb))) {
+			kfree_skb(skb);
+			return NET_XMIT_DROP;
+		}
+
 		tp->af_specific->calc_md5_hash(md5_hash_location,
 					       md5,
 					       sk, NULL, NULL,
--- a/net/ipv6/tcp_ipv6.c	2008-06-26 22:05:13.000000000 -0700
+++ b/net/ipv6/tcp_ipv6.c	2008-06-26 22:05:15.000000000 -0700
@@ -583,7 +583,7 @@ static int tcp_v6_md5_do_add(struct sock
 				kfree(newkey);
 				return -ENOMEM;
 			}
-			sk->sk_route_caps &= ~NETIF_F_GSO_MASK;
+			sk->sk_route_caps &= ~(NETIF_F_GSO_MASK|NETIF_F_SG);
 		}
 		if (tcp_alloc_md5sig_pool() == NULL) {
 			kfree(newkey);
@@ -720,7 +720,7 @@ static int tcp_v6_parse_md5_keys (struct
 			return -ENOMEM;
 
 		tp->md5sig_info = p;
-		sk->sk_route_caps &= ~NETIF_F_GSO_MASK;
+		sk->sk_route_caps &= ~(NETIF_F_GSO_MASK|NETIF_F_SG);
 	}
 
 	newkey = kmemdup(cmd.tcpm_key, cmd.tcpm_keylen, GFP_KERNEL);
@@ -1529,6 +1529,7 @@ static struct sock * tcp_v6_syn_recv_soc
 		if (newkey != NULL)
 			tcp_v6_md5_do_add(newsk, &inet6_sk(sk)->daddr,
 					  newkey, key->keylen);
+		newsk->sk_route_caps &= ~(NETIF_F_GSO_MASK|NETIF_F_SG);
 	}
 #endif
 

Re: [PATCH] TCP MD5 and TSO/SG breakage

[Adam Langley]

On Fri, Jun 27, 2008 at 11:21 AM, Stephen Hemminger
<stephen.hemminger@vyatta.com> wrote:
> The TCP MD5 support is broken on any device that does scatter gather.
> The MD5 calculation code doesn't support scatter/gather, the md5_calc
> API assumes the data follows the TCP header. It is too late to rework this
> code for 2.6.26 (and backport to stable). So the sane thing to do is block
> use of SG on TCP sockets using MD5 option.

Acked-By: Adam Langley <agl@imperialviolet.org>


AGL

-- 
Adam Langley agl@imperialviolet.org http://www.imperialviolet.org

[PATCH] TCP MD5 needs to disable Scatter/Gather

[Stephen Hemminger]

The TCP MD5 support is broken on any device that does scatter gather.
The MD5 calculation code doesn't support scatter/gather, the md5_calc
API assumes the data follows the TCP header. Since MD5 is only used
for non-performance centric applications and the data has to all be
read anyway, the loss of SG support is not a big issue.

Patch against net-next-2.6.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/net/ipv4/tcp_ipv4.c	2008-06-27 10:49:42.000000000 -0700
+++ b/net/ipv4/tcp_ipv4.c	2008-06-27 10:50:34.000000000 -0700
@@ -846,7 +846,7 @@ int tcp_v4_md5_do_add(struct sock *sk, _
 				kfree(newkey);
 				return -ENOMEM;
 			}
-			sk->sk_route_caps &= ~NETIF_F_GSO_MASK;
+			sk->sk_route_caps &= ~(NETIF_F_GSO_MASK|NETIF_F_SG);
 		}
 		if (tcp_alloc_md5sig_pool() == NULL) {
 			kfree(newkey);
@@ -975,7 +975,7 @@ static int tcp_v4_parse_md5_keys(struct 
 			return -EINVAL;
 
 		tp->md5sig_info = p;
-		sk->sk_route_caps &= ~NETIF_F_GSO_MASK;
+		sk->sk_route_caps &= ~(NETIF_F_GSO_MASK|NETIF_F_SG);
 	}
 
 	newkey = kmemdup(cmd.tcpm_key, cmd.tcpm_keylen, GFP_KERNEL);
@@ -1097,6 +1097,14 @@ static int tcp_v4_inbound_md5_hash(struc
 		return 1;
 	}
 
+	/* md5 calculation needs linear skb */
+	if (skb_is_nonlinear(skb)) {
+		if (__skb_linearize(skb))
+			return 1;	/* out of memory, assume failed (ie drop) */
+		th = tcp_hdr(skb);
+		iph = ip_hdr(skb);
+	}
+
 	/* Okay, so this is hash_expected and hash_location -
 	 * so we need to calculate the checksum.
 	 */
@@ -1352,6 +1360,7 @@ struct sock *tcp_v4_syn_recv_sock(struct
 		if (newkey != NULL)
 			tcp_v4_md5_do_add(newsk, inet_sk(sk)->daddr,
 					  newkey, key->keylen);
+		newsk->sk_route_caps &= ~(NETIF_F_GSO_MASK|NETIF_F_SG);
 	}
 #endif
 
--- a/net/ipv4/tcp_output.c	2008-06-18 10:33:15.000000000 -0700
+++ b/net/ipv4/tcp_output.c	2008-06-27 10:50:34.000000000 -0700
@@ -540,8 +540,10 @@ static int tcp_transmit_skb(struct sock 
 	 * room for it.
 	 */
 	md5 = tp->af_specific->md5_lookup(sk, sk);
-	if (md5)
+	if (md5) {
 		tcp_header_size += TCPOLEN_MD5SIG_ALIGNED;
+		sk->sk_route_caps &= ~(NETIF_F_GSO_MASK|NETIF_F_SG);
+	}
 #endif
 
 	skb_push(skb, tcp_header_size);
@@ -601,6 +603,16 @@ static int tcp_transmit_skb(struct sock 
 #ifdef CONFIG_TCP_MD5SIG
 	/* Calculate the MD5 hash, as we have all we need now */
 	if (md5) {
+		/* This shouldn't happen, but it is possible that a retransmit
+		 * causes a reroute onto a different interface and we get TSO/SG
+		 * skb that is dropped here, and route_caps has already been
+		 * reset so the next retransmit will be okay.
+		 */
+		if (unlikely(skb_is_nonlinear(skb))) {
+			kfree_skb(skb);
+			return NET_XMIT_DROP;
+		}
+
 		tp->af_specific->calc_md5_hash(md5_hash_location,
 					       md5,
 					       sk, NULL, NULL,
--- a/net/ipv6/tcp_ipv6.c	2008-06-27 09:19:13.000000000 -0700
+++ b/net/ipv6/tcp_ipv6.c	2008-06-27 10:50:34.000000000 -0700
@@ -585,7 +585,7 @@ static int tcp_v6_md5_do_add(struct sock
 				kfree(newkey);
 				return -ENOMEM;
 			}
-			sk->sk_route_caps &= ~NETIF_F_GSO_MASK;
+			sk->sk_route_caps &= ~(NETIF_F_GSO_MASK|NETIF_F_SG);
 		}
 		if (tcp_alloc_md5sig_pool() == NULL) {
 			kfree(newkey);
@@ -722,7 +722,7 @@ static int tcp_v6_parse_md5_keys (struct
 			return -ENOMEM;
 
 		tp->md5sig_info = p;
-		sk->sk_route_caps &= ~NETIF_F_GSO_MASK;
+		sk->sk_route_caps &= ~(NETIF_F_GSO_MASK|NETIF_F_SG);
 	}
 
 	newkey = kmemdup(cmd.tcpm_key, cmd.tcpm_keylen, GFP_KERNEL);
@@ -1439,6 +1439,7 @@ static struct sock * tcp_v6_syn_recv_soc
 		if (newkey != NULL)
 			tcp_v6_md5_do_add(newsk, &inet6_sk(sk)->daddr,
 					  newkey, key->keylen);
+		newsk->sk_route_caps &= ~(NETIF_F_GSO_MASK|NETIF_F_SG);
 	}
 #endif