[PATCH] Fix corrupt TCP packets when options space overflows with MD5SIG enabled

[PATCH] Fix corrupt TCP packets when options space overflows with MD5SIG enabled

[Adam Langley]

When MD5 signatures are turned on we can end up with syntactically invalid
packets with a header length < 20 bytes. This is because tcp_header_size
overflows with 12 bytes of timestamp, 20 bytes of signature and > 8 bytes of
SACK option.

Since we can't fit any SACK blocks in the final 8 bytes of options space, and
the MD5 signature is more important, we disable including SACK, or even
advertising it, when MD5 is enabled.

Signed-off-by: Adam Langley <agl@imperialviolet.org>

---

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index e399bde..70392a6 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -504,6 +504,16 @@ static int tcp_transmit_skb(struct sock *sk,
struct sk_buff *skb, int clone_it,
        tcb = TCP_SKB_CB(skb);
        tcp_header_size = tp->tcp_header_len;

+#ifdef CONFIG_TCP_MD5SIG
+       /*
+        * Are we doing MD5 on this segment? If so - make
+        * room for it.
+        */
+       md5 = tp->af_specific->md5_lookup(sk, sk);
+       if (md5)
+               tcp_header_size += TCPOLEN_MD5SIG_ALIGNED;
+#endif
+
 #define SYSCTL_FLAG_TSTAMPS    0x1
 #define SYSCTL_FLAG_WSCALE     0x2
 #define SYSCTL_FLAG_SACK       0x4
@@ -519,12 +529,23 @@ static int tcp_transmit_skb(struct sock *sk,
struct sk_buff *skb, int clone_it,
                        tcp_header_size += TCPOLEN_WSCALE_ALIGNED;
                        sysctl_flags |= SYSCTL_FLAG_WSCALE;
                }
-               if (sysctl_tcp_sack) {
+               if (sysctl_tcp_sack
+#ifdef CONFIG_TCP_MD5SIG
+                   /* We don't include SACK options if we are going to
+                    * include an MD5 signature because they can't fit
+                    * in the options space */
+                   && !md5
+#endif
+                   ) {
                        sysctl_flags |= SYSCTL_FLAG_SACK;
                        if (!(sysctl_flags & SYSCTL_FLAG_TSTAMPS))
                                tcp_header_size += TCPOLEN_SACKPERM_ALIGNED;
                }
-       } else if (unlikely(tp->rx_opt.eff_sacks)) {
+       } else if (unlikely(tp->rx_opt.eff_sacks
+#ifdef CONFIG_TCP_MD5SIG
+                           && !md5
+#endif
+                           )) {
                /* A SACK is 2 pad bytes, a 2 byte header, plus
                 * 2 32-bit sequence numbers for each SACK block.
                 */
@@ -536,16 +557,6 @@ static int tcp_transmit_skb(struct sock *sk,
struct sk_buff *skb, int clone_it,
        if (tcp_packets_in_flight(tp) == 0)
                tcp_ca_event(sk, CA_EVENT_TX_START);

-#ifdef CONFIG_TCP_MD5SIG
-       /*
-        * Are we doing MD5 on this segment? If so - make
-        * room for it.
-        */
-       md5 = tp->af_specific->md5_lookup(sk, sk);
-       if (md5)
-               tcp_header_size += TCPOLEN_MD5SIG_ALIGNED;
-#endif
-
        skb_push(skb, tcp_header_size);
        skb_reset_transport_header(skb);
        skb_set_owner_w(skb, sk);

Re: [PATCH] Fix corrupt TCP packets when options space overflows with MD5SIG enabled

[Ben Hutchings]

Adam Langley wrote:
> When MD5 signatures are turned on we can end up with syntactically invalid
> packets with a header length < 20 bytes. This is because tcp_header_size
> overflows with 12 bytes of timestamp, 20 bytes of signature and > 8 bytes of
> SACK option.
[...]
> @@ -519,12 +529,23 @@ static int tcp_transmit_skb(struct sock *sk,
> struct sk_buff *skb, int clone_it,
>                         tcp_header_size += TCPOLEN_WSCALE_ALIGNED;
>                         sysctl_flags |= SYSCTL_FLAG_WSCALE;
>                 }
> -               if (sysctl_tcp_sack) {
> +               if (sysctl_tcp_sack
> +#ifdef CONFIG_TCP_MD5SIG
> +                   /* We don't include SACK options if we are going to
> +                    * include an MD5 signature because they can't fit
> +                    * in the options space */
> +                   && !md5
> +#endif

Putting an #ifdef..#endif in the middle of an if () is a bit nasty.  Why not
define md5 and set it to 0 if CONFIG_TCP_MD5SIG is not set?  The compiler
should be able to optimise it away if it's always initialised to 0.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.

Re: [PATCH] Fix corrupt TCP packets when options space overflows with MD5SIG enabled

[Adam Langley]

On Fri, May 30, 2008 at 12:02 PM, Ben Hutchings
<bhutchings@solarflare.com> wrote:
> Putting an #ifdef..#endif in the middle of an if () is a bit nasty.  Why not
> define md5 and set it to 0 if CONFIG_TCP_MD5SIG is not set?  The compiler
> should be able to optimise it away if it's always initialised to 0.

That's a good point, thanks. Followup patch in a second...



AGL

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

Re: [PATCH] Fix corrupt TCP packets when options space overflows with MD5SIG enabled

[Adam Langley]

When MD5 signatures are turned on we can end up with syntactically invalid
packets with a header length < 20 bytes. This is because tcp_header_size
overflows with 12 bytes of timestamp, 20 bytes of signature and > 8 bytes of
SACK option.

Since we can't fit any SACK blocks in the final 8 bytes of options space, and
the MD5 signature is more important, we disable including SACK, or even
advertising it, when MD5 is enabled.

Signed-off-by: Adam Langley <agl@imperialviolet.org>

---

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index e399bde..c98a4c9 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -477,6 +477,8 @@ static int tcp_transmit_skb(struct sock *sk,
struct sk_buff *skb, int clone_it,
 #ifdef CONFIG_TCP_MD5SIG
        struct tcp_md5sig_key *md5;
        __u8 *md5_hash_location;
+#else
+       void *const md5 = NULL;
 #endif
        struct tcphdr *th;
        int sysctl_flags;
@@ -504,6 +506,16 @@ static int tcp_transmit_skb(struct sock *sk,
struct sk_buff *skb, int clone_it,
        tcb = TCP_SKB_CB(skb);
        tcp_header_size = tp->tcp_header_len;

+#ifdef CONFIG_TCP_MD5SIG
+       /*
+        * Are we doing MD5 on this segment? If so - make
+        * room for it.
+        */
+       md5 = tp->af_specific->md5_lookup(sk, sk);
+       if (md5)
+               tcp_header_size += TCPOLEN_MD5SIG_ALIGNED;
+#endif
+
 #define SYSCTL_FLAG_TSTAMPS    0x1
 #define SYSCTL_FLAG_WSCALE     0x2
 #define SYSCTL_FLAG_SACK       0x4
@@ -519,12 +531,15 @@ static int tcp_transmit_skb(struct sock *sk,
struct sk_buff *skb, int clone_it,
                        tcp_header_size += TCPOLEN_WSCALE_ALIGNED;
                        sysctl_flags |= SYSCTL_FLAG_WSCALE;
                }
-               if (sysctl_tcp_sack) {
+               /* We don't include SACK options if we are going to
+                * include an MD5 signature because they can't fit
+                * in the options space */
+               if (sysctl_tcp_sack && !md5) {
                        sysctl_flags |= SYSCTL_FLAG_SACK;
                        if (!(sysctl_flags & SYSCTL_FLAG_TSTAMPS))
                                tcp_header_size += TCPOLEN_SACKPERM_ALIGNED;
                }
-       } else if (unlikely(tp->rx_opt.eff_sacks)) {
+       } else if (unlikely(tp->rx_opt.eff_sacks && !md5)) {
                /* A SACK is 2 pad bytes, a 2 byte header, plus
                 * 2 32-bit sequence numbers for each SACK block.
                 */
@@ -536,16 +551,6 @@ static int tcp_transmit_skb(struct sock *sk,
struct sk_buff *skb, int clone_it,
        if (tcp_packets_in_flight(tp) == 0)
                tcp_ca_event(sk, CA_EVENT_TX_START);

-#ifdef CONFIG_TCP_MD5SIG
-       /*
-        * Are we doing MD5 on this segment? If so - make
-        * room for it.
-        */
-       md5 = tp->af_specific->md5_lookup(sk, sk);
-       if (md5)
-               tcp_header_size += TCPOLEN_MD5SIG_ALIGNED;
-#endif
-
        skb_push(skb, tcp_header_size);
        skb_reset_transport_header(skb);
        skb_set_owner_w(skb, sk);

Re: [PATCH] Fix corrupt TCP packets when options space overflows with MD5SIG enabled

[Stephen Hemminger]

On Fri, 30 May 2008 12:17:12 -0700
"Adam Langley" <agl@imperialviolet.org> wrote:

> When MD5 signatures are turned on we can end up with syntactically invalid
> packets with a header length < 20 bytes. This is because tcp_header_size
> overflows with 12 bytes of timestamp, 20 bytes of signature and > 8 bytes of
> SACK option.
> 
> Since we can't fit any SACK blocks in the final 8 bytes of options space, and
> the MD5 signature is more important, we disable including SACK, or even
> advertising it, when MD5 is enabled.
> 
> Signed-off-by: Adam Langley <agl@imperialviolet.org>
> 
> ---
> 
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index e399bde..c98a4c9 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -477,6 +477,8 @@ static int tcp_transmit_skb(struct sock *sk,
> struct sk_buff *skb, int clone_it,

Your mailer is wrapping lines so patch is munged.

>  #ifdef CONFIG_TCP_MD5SIG
>         struct tcp_md5sig_key *md5;
>         __u8 *md5_hash_location;
> +#else
> +       void *const md5 = NULL;
>  #endif

You can just do:

+	struct tcp_md5sig_key *md5 = NULL;
 #ifdef CONFIG_TCP_MD5SIG
-	struct tcp_md5sig_key *md5;
 	__u8 *md5_hash_location;
 #endif

GCC does the right thing.

>         struct tcphdr *th;
>         int sysctl_flags;
> @@ -504,6 +506,16 @@ static int tcp_transmit_skb(struct sock *sk,
> struct sk_buff *skb, int clone_it,
>         tcb = TCP_SKB_CB(skb);
>         tcp_header_size = tp->tcp_header_len;
> 
> +#ifdef CONFIG_TCP_MD5SIG
> +       /*
> +        * Are we doing MD5 on this segment? If so - make
> +        * room for it.
> +        */
> +       md5 = tp->af_specific->md5_lookup(sk, sk);
> +       if (md5)
> +               tcp_header_size += TCPOLEN_MD5SIG_ALIGNED;
> +#endif
> +
>  #define SYSCTL_FLAG_TSTAMPS    0x1
>  #define SYSCTL_FLAG_WSCALE     0x2
>  #define SYSCTL_FLAG_SACK       0x4
> @@ -519,12 +531,15 @@ static int tcp_transmit_skb(struct sock *sk,
> struct sk_buff *skb, int clone_it,
>                         tcp_header_size += TCPOLEN_WSCALE_ALIGNED;
>                         sysctl_flags |= SYSCTL_FLAG_WSCALE;
>                 }
> -               if (sysctl_tcp_sack) {
> +               /* We don't include SACK options if we are going to
> +                * include an MD5 signature because they can't fit
> +                * in the options space */
> +               if (sysctl_tcp_sack && !md5) {
>                         sysctl_flags |= SYSCTL_FLAG_SACK;
>                         if (!(sysctl_flags & SYSCTL_FLAG_TSTAMPS))
>                                 tcp_header_size += TCPOLEN_SACKPERM_ALIGNED;
>                 }
> -       } else if (unlikely(tp->rx_opt.eff_sacks)) {
> +       } else if (unlikely(tp->rx_opt.eff_sacks && !md5)) {
>                 /* A SACK is 2 pad bytes, a 2 byte header, plus
>                  * 2 32-bit sequence numbers for each SACK block.
>                  */
> @@ -536,16 +551,6 @@ static int tcp_transmit_skb(struct sock *sk,
> struct sk_buff *skb, int clone_it,
>         if (tcp_packets_in_flight(tp) == 0)
>                 tcp_ca_event(sk, CA_EVENT_TX_START);
> 
> -#ifdef CONFIG_TCP_MD5SIG
> -       /*
> -        * Are we doing MD5 on this segment? If so - make
> -        * room for it.
> -        */
> -       md5 = tp->af_specific->md5_lookup(sk, sk);
> -       if (md5)
> -               tcp_header_size += TCPOLEN_MD5SIG_ALIGNED;
> -#endif
> -
>         skb_push(skb, tcp_header_size);
>         skb_reset_transport_header(skb);
>         skb_set_owner_w(skb, sk);
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Fix corrupt TCP packets when options space overflows with MD5SIG enabled

[Adam Langley]

When MD5 signatures are turned on we can end up with syntactically invalid
packets with a header length < 20 bytes. This is because tcp_header_size
overflows with 12 bytes of timestamp, 20 bytes of signature and > 8 bytes of
SACK option.

Since we can't fit any SACK blocks in the final 8 bytes of options space, and
the MD5 signature is more important, we disable including SACK, or even
advertising it, when MD5 is enabled.

Signed-off-by: Adam Langley <agl@imperialviolet.org>

---
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index e399bde..e4fb5d9 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -474,8 +474,9 @@ static int tcp_transmit_skb(struct sock *sk, struct sk_buff *skb, int clone_it,
 	struct tcp_sock *tp;
 	struct tcp_skb_cb *tcb;
 	int tcp_header_size;
+
+	struct tcp_md5sig_key *md5 = NULL;
 #ifdef CONFIG_TCP_MD5SIG
-	struct tcp_md5sig_key *md5;
 	__u8 *md5_hash_location;
 #endif
 	struct tcphdr *th;
@@ -504,6 +505,16 @@ static int tcp_transmit_skb(struct sock *sk, struct sk_buff *skb, int clone_it,
 	tcb = TCP_SKB_CB(skb);
 	tcp_header_size = tp->tcp_header_len;
 
+#ifdef CONFIG_TCP_MD5SIG
+	/*
+	 * Are we doing MD5 on this segment? If so - make
+	 * room for it.
+	 */
+	md5 = tp->af_specific->md5_lookup(sk, sk);
+	if (md5)
+		tcp_header_size += TCPOLEN_MD5SIG_ALIGNED;
+#endif
+
 #define SYSCTL_FLAG_TSTAMPS	0x1
 #define SYSCTL_FLAG_WSCALE	0x2
 #define SYSCTL_FLAG_SACK	0x4
@@ -519,12 +530,15 @@ static int tcp_transmit_skb(struct sock *sk, struct sk_buff *skb, int clone_it,
 			tcp_header_size += TCPOLEN_WSCALE_ALIGNED;
 			sysctl_flags |= SYSCTL_FLAG_WSCALE;
 		}
-		if (sysctl_tcp_sack) {
+		/* We don't include SACK options if we are going to
+		 * include an MD5 signature because they can't fit
+		 * in the options space */
+		if (sysctl_tcp_sack && !md5) {
 			sysctl_flags |= SYSCTL_FLAG_SACK;
 			if (!(sysctl_flags & SYSCTL_FLAG_TSTAMPS))
 				tcp_header_size += TCPOLEN_SACKPERM_ALIGNED;
 		}
-	} else if (unlikely(tp->rx_opt.eff_sacks)) {
+	} else if (unlikely(tp->rx_opt.eff_sacks && !md5)) {
 		/* A SACK is 2 pad bytes, a 2 byte header, plus
 		 * 2 32-bit sequence numbers for each SACK block.
 		 */
@@ -536,16 +550,6 @@ static int tcp_transmit_skb(struct sock *sk, struct sk_buff *skb, int clone_it,
 	if (tcp_packets_in_flight(tp) == 0)
 		tcp_ca_event(sk, CA_EVENT_TX_START);
 
-#ifdef CONFIG_TCP_MD5SIG
-	/*
-	 * Are we doing MD5 on this segment? If so - make
-	 * room for it.
-	 */
-	md5 = tp->af_specific->md5_lookup(sk, sk);
-	if (md5)
-		tcp_header_size += TCPOLEN_MD5SIG_ALIGNED;
-#endif
-
 	skb_push(skb, tcp_header_size);
 	skb_reset_transport_header(skb);
 	skb_set_owner_w(skb, sk);

Re: [PATCH] Fix corrupt TCP packets when options space overflows with MD5SIG enabled

[Adam Langley]

On Fri, May 30, 2008 at 1:32 PM, Stephen Hemminger
<shemminger@vyatta.com> wrote:
> Your mailer is wrapping lines so patch is munged.

Will use a better MUA in future, thanks.

> You can just do:
>
> +       struct tcp_md5sig_key *md5 = NULL;
>  #ifdef CONFIG_TCP_MD5SIG
> -       struct tcp_md5sig_key *md5;
>        __u8 *md5_hash_location;
>  #endif

Will do; hopefully final version of the patch coming in a second.. thanks


AGL

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

Re: [PATCH] Fix corrupt TCP packets when options space overflows with MD5SIG enabled

[James Morris]

On Sat, 31 May 2008, Adam Langley wrote:

> When MD5 signatures are turned on we can end up with syntactically invalid
> packets with a header length < 20 bytes. This is because tcp_header_size
> overflows with 12 bytes of timestamp, 20 bytes of signature and > 8 bytes of
> SACK option.
> 
> Since we can't fit any SACK blocks in the final 8 bytes of options space, and
> the MD5 signature is more important, we disable including SACK, or even
> advertising it, when MD5 is enabled.
> 
> Signed-off-by: Adam Langley <agl@imperialviolet.org>

Reviewed-by: James Morris <jmorris@namei.org>


(FYI, the upcoming replacement for this, TCP-AO, is designed to be SACK 
compatible)


-- 
James Morris
<jmorris@namei.org>

Re: [PATCH] Fix corrupt TCP packets when options space overflows with MD5SIG enabled

[David Miller]

From: James Morris <jmorris@namei.org>
Date: Mon, 2 Jun 2008 09:40:15 +1000 (EST)

> On Sat, 31 May 2008, Adam Langley wrote:
> 
> > When MD5 signatures are turned on we can end up with syntactically invalid
> > packets with a header length < 20 bytes. This is because tcp_header_size
> > overflows with 12 bytes of timestamp, 20 bytes of signature and > 8 bytes of
> > SACK option.
> > 
> > Since we can't fit any SACK blocks in the final 8 bytes of options space, and
> > the MD5 signature is more important, we disable including SACK, or even
> > advertising it, when MD5 is enabled.
> > 
> > Signed-off-by: Adam Langley <agl@imperialviolet.org>
> 
> Reviewed-by: James Morris <jmorris@namei.org>

After making some minor coding style fixups, I applied this
fix, thanks everyone.

Re: [PATCH] Fix corrupt TCP packets when options space overflows with MD5SIG enabled

[Adam Langley]

On Sun, Jun 1, 2008 at 4:40 PM, James Morris <jmorris@namei.org> wrote:
> Reviewed-by: James Morris <jmorris@namei.org>


Looking at this code some more, I fear that I've fucked up that patch.
The logic for TCP options seems to be duplicated three times in the
code: the size calculation (patched), the building of the actual
options (tcp_build_and_update_options / tcp_syn_build_options) and the
MSS calculations (tcp_current_mss).

It looks, on second glance, that this code in
tcp_build_and_update_options will include the options even though we
calculated the size without:

if (tp->rx_opt.eff_sacks) {
	struct tcp_sack_block *sp = tp->rx_opt.dsack ? tp->duplicate_sack :
tp->selective_acks;
	int this_sack;

	*ptr++ = htonl((TCPOPT_NOP  << 24) |
		       (TCPOPT_NOP  << 16) |
		       (TCPOPT_SACK <<  8) |
		       (TCPOLEN_SACK_BASE + (tp->rx_opt.eff_sacks *
			     TCPOLEN_SACK_PERBLOCK)));
...

Unless I'm missing something, that patch was incomplete and we're
still sending invalid packets on in the MD5SIG + SACK case.

If so, I'll try and get the other two cases with another patch.

Additionally, it would appear that it would be useful to pull this
logic into a single place: maybe a function which runs multiple times
(to calculate the MSS / header size and a second time to actually
perform the options writes). But that'll be a different patch.

Cheers,

AGL

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

Re: [PATCH] Fix corrupt TCP packets when options space overflows with MD5SIG enabled

[David Miller]

From: "Adam Langley" <agl@imperialviolet.org>
Date: Tue, 3 Jun 2008 09:31:02 -0700

> It looks, on second glance, that this code in
> tcp_build_and_update_options will include the options even though we
> calculated the size without:
> 
> if (tp->rx_opt.eff_sacks) {
> 	struct tcp_sack_block *sp = tp->rx_opt.dsack ? tp->duplicate_sack :
> tp->selective_acks;
> 	int this_sack;
> 
> 	*ptr++ = htonl((TCPOPT_NOP  << 24) |
> 		       (TCPOPT_NOP  << 16) |
> 		       (TCPOPT_SACK <<  8) |
> 		       (TCPOLEN_SACK_BASE + (tp->rx_opt.eff_sacks *
> 			     TCPOLEN_SACK_PERBLOCK)));
> ...
> 
> Unless I'm missing something, that patch was incomplete and we're
> still sending invalid packets on in the MD5SIG + SACK case.

That's right, the code assumes there is always enough space for
the SACK blocks.