Skip to contentSkip to conversation

TCP MD5 and socket accept

PATCH New Stephen Hemminger 3 participants 8 messages Jun 26, 2008 – Jun 27, 2008

Subsystems (3)
Networking Network drivers TCP
lore
Share & tools
lore

Stephen Hemminger

author
#0 lore raw

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.

#1 lore raw
Quoted 6 lines · Stephen Hemminger

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

Signature · 1 line
Adam Langley [email protected] http://www.imperialviolet.org
#2 lore raw
Quoted 1 line · Adam Langley

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.

Quoted 1 line

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,

Signature · 1 line
Adam Langley [email protected] http://www.imperialviolet.org
author
#3 lore raw
In reply to Adam Langley
Quoted 14 lines · On Thu, 26 Jun 2008 07:46:59 -0700 "Adam Langley" <[email protected]>…
Quoted 6 lines · Stephen Hemminger

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.

Quoted 7 lines

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.

author
#4 lore raw

PATCH 1/1 TCP MD5 needs to disable Scatter/Gather

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 <[email protected]>
net/ipv4/tcp_ipv4.c +11 -2
@@ -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
net/ipv4/tcp_output.c +13 -1
@@ -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,
net/ipv6/tcp_ipv6.c +3 -2
@@ -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
#5 lore raw

PATCH 1/1 TCP MD5 and TSO/SG breakage

In reply to Adam Langley

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 <[email protected]>
net/ipv4/tcp_ipv4.c +11 -2
@@ -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
net/ipv4/tcp_output.c +13 -1
@@ -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,
net/ipv6/tcp_ipv6.c +3 -2
@@ -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
#6 lore raw

PATCH 1/1 TCP MD5 and TSO/SG breakage

In reply to Adam Langley

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 <[email protected]>

---
Resent because it didn't show up on netdev

net/ipv4/tcp_ipv4.c +11 -2
@@ -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
net/ipv4/tcp_output.c +13 -1
@@ -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,
net/ipv6/tcp_ipv6.c +3 -2
@@ -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
Acked-by on v1 1/1
#7 lore raw

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

Quoted 5 lines · 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.

Acked-by Adam Langley <[email protected]>

AGL

Signature · 1 line
Adam Langley [email protected] http://www.imperialviolet.org