[PATCH] tcp: should drop incoming frames without ACK flag set

[PATCH] tcp: should drop incoming frames without ACK flag set

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

In commit 96e0bf4b5193d (tcp: Discard segments that ack data not yet
sent) John Dykstra enforced a check against ack sequences.

In commit 354e4aa391ed5 (tcp: RFC 5961 5.2 Blind Data Injection Attack
Mitigation) I added more safety tests.

But we missed fact that these tests are not performed if ACK bit is
not set.

RFC 793 3.9 mandates TCP should drop a frame without ACK flag set.

" fifth check the ACK field,
      if the ACK bit is off drop the segment and return"

Not doing so permits an attacker to only guess an acceptable sequence
number, evading stronger checks.

Many thanks to Zhiyun Qian for bringing this issue to our attention.

See : http://web.eecs.umich.edu/~zhiyunq/pub/ccs12_TCP_sequence_number_inference.pdf

Reported-by: Zhiyun Qian <zhiyunq@umich.edu>
Signed-off-by: Eric Dumazet <edumazet@google.com>
Acked-by: Nandita Dukkipati <nanditad@google.com>
Cc: Neal Cardwell <ncardwell@google.com>
Cc: John Dykstra <john.dykstra1@gmail.com>
---
Notes
 - I left a "if (true)" block that I'll remove in a cleanup patch in
   linux-3.9, to permit this patch being easily back-ported to stable
   branches.
 - A followup patch will be sent (in net-next) to have stronger checks
   before sending dupack

 net/ipv4/tcp_input.c |   14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index a136925..903d0ef 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -5540,6 +5540,9 @@ no_ack:
 	}
 
 slow_path:
+	if (!th->ack)
+			goto discard;
+
 	if (len < (th->doff << 2) || tcp_checksum_complete_user(sk, skb))
 		goto csum_error;
 
@@ -5551,7 +5554,7 @@ slow_path:
 		return 0;
 
 step5:
-	if (th->ack && tcp_ack(sk, skb, FLAG_SLOWPATH) < 0)
+	if (tcp_ack(sk, skb, FLAG_SLOWPATH) < 0)
 		goto discard;
 
 	/* ts_recent update must be made after we are sure that the packet
@@ -5984,11 +5987,15 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb,
 		if (tcp_check_req(sk, skb, req, NULL, true) == NULL)
 			goto discard;
 	}
+
+	if (!th->ack)
+		goto discard;
+
 	if (!tcp_validate_incoming(sk, skb, th, 0))
 		return 0;
 
 	/* step 5: check the ACK field */
-	if (th->ack) {
+	if (true) {
 		int acceptable = tcp_ack(sk, skb, FLAG_SLOWPATH) > 0;
 
 		switch (sk->sk_state) {
@@ -6138,8 +6145,7 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb,
 			}
 			break;
 		}
-	} else
-		goto discard;
+	}
 
 	/* ts_recent update must be made after we are sure that the packet
 	 * is in window.

Re: [PATCH] tcp: should drop incoming frames without ACK flag set

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 26 Dec 2012 09:10:01 -0800

> @@ -5540,6 +5540,9 @@ no_ack:
>  	}
>  
>  slow_path:
> +	if (!th->ack)
> +			goto discard;

One too many tabs there on that last line :-)

> +
>  	if (len < (th->doff << 2) || tcp_checksum_complete_user(sk, skb))
>  		goto csum_error;
>  
> @@ -5551,7 +5554,7 @@ slow_path:


Also, I would say that this checksum test should come first, because
that takes priority since you could be testing the ACK bit of a
corrupted packet.

Better to get the statistic bump on the bad checksum then a silent
drop on the ACK being cleared.

> @@ -5984,11 +5987,15 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb,
>  		if (tcp_check_req(sk, skb, req, NULL, true) == NULL)
>  			goto discard;
>  	}
> +
> +	if (!th->ack)
> +		goto discard;
> +

And that is effectively what is going to happen in this case since
the caller has already done the checksum checks.

Thanks.

Re: [PATCH] tcp: should drop incoming frames without ACK flag set

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

On Wed, 2012-12-26 at 14:11 -0800, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Wed, 26 Dec 2012 09:10:01 -0800
> 
> > @@ -5540,6 +5540,9 @@ no_ack:
> >  	}
> >  
> >  slow_path:
> > +	if (!th->ack)
> > +			goto discard;
> 
> One too many tabs there on that last line :-)

Hmm, yes :(

> 
> > +
> >  	if (len < (th->doff << 2) || tcp_checksum_complete_user(sk, skb))
> >  		goto csum_error;
> >  
> > @@ -5551,7 +5554,7 @@ slow_path:
> 
> 
> Also, I would say that this checksum test should come first, because
> that takes priority since you could be testing the ACK bit of a
> corrupted packet.
> 
> Better to get the statistic bump on the bad checksum then a silent
> drop on the ACK being cleared.

Indeed, good catch.

> 
> > @@ -5984,11 +5987,15 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb,
> >  		if (tcp_check_req(sk, skb, req, NULL, true) == NULL)
> >  			goto discard;
> >  	}
> > +
> > +	if (!th->ack)
> > +		goto discard;
> > +
> 
> And that is effectively what is going to happen in this case since
> the caller has already done the checksum checks.
> 

That makes perfect sense.

Thanks !

[PATCH v2] tcp: should drop incoming frames without ACK flag set

In commit 96e0bf4b5193d (tcp: Discard segments that ack data not yet
sent) John Dykstra enforced a check against ack sequences.

In commit 354e4aa391ed5 (tcp: RFC 5961 5.2 Blind Data Injection Attack
Mitigation) I added more safety tests.

But we missed fact that these tests are not performed if ACK bit is
not set.

RFC 793 3.9 mandates TCP should drop a frame without ACK flag set.

" fifth check the ACK field,
      if the ACK bit is off drop the segment and return"

Not doing so permits an attacker to only guess an acceptable sequence
number, evading stronger checks.

Many thanks to Zhiyun Qian for bringing this issue to our attention.

See :
http://web.eecs.umich.edu/~zhiyunq/pub/ccs12_TCP_sequence_number_inference.pdf

Reported-by: Zhiyun Qian <zhiyunq@umich.edu>
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Nandita Dukkipati <nanditad@google.com>
Cc: Neal Cardwell <ncardwell@google.com>
Cc: John Dykstra <john.dykstra1@gmail.com>
---
V2: moves the th->ack check after checksum one
 
 net/ipv4/tcp_input.c |   14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index a136925..a28e4db 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -5543,6 +5543,9 @@ slow_path:
 	if (len < (th->doff << 2) || tcp_checksum_complete_user(sk, skb))
 		goto csum_error;
 
+	if (!th->ack)
+		goto discard;
+
 	/*
 	 *	Standard slow path.
 	 */
@@ -5551,7 +5554,7 @@ slow_path:
 		return 0;
 
 step5:
-	if (th->ack && tcp_ack(sk, skb, FLAG_SLOWPATH) < 0)
+	if (tcp_ack(sk, skb, FLAG_SLOWPATH) < 0)
 		goto discard;
 
 	/* ts_recent update must be made after we are sure that the packet
@@ -5984,11 +5987,15 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb,
 		if (tcp_check_req(sk, skb, req, NULL, true) == NULL)
 			goto discard;
 	}
+
+	if (!th->ack)
+		goto discard;
+
 	if (!tcp_validate_incoming(sk, skb, th, 0))
 		return 0;
 
 	/* step 5: check the ACK field */
-	if (th->ack) {
+	if (true) {
 		int acceptable = tcp_ack(sk, skb, FLAG_SLOWPATH) > 0;
 
 		switch (sk->sk_state) {
@@ -6138,8 +6145,7 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb,
 			}
 			break;
 		}
-	} else
-		goto discard;
+	}
 
 	/* ts_recent update must be made after we are sure that the packet
 	 * is in window.

Re: [PATCH] tcp: should drop incoming frames without ACK flag set

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 26 Dec 2012 14:44:34 -0800

> [PATCH v2] tcp: should drop incoming frames without ACK flag set
> 
> In commit 96e0bf4b5193d (tcp: Discard segments that ack data not yet
> sent) John Dykstra enforced a check against ack sequences.
> 
> In commit 354e4aa391ed5 (tcp: RFC 5961 5.2 Blind Data Injection Attack
> Mitigation) I added more safety tests.
> 
> But we missed fact that these tests are not performed if ACK bit is
> not set.
> 
> RFC 793 3.9 mandates TCP should drop a frame without ACK flag set.
> 
> " fifth check the ACK field,
>       if the ACK bit is off drop the segment and return"
> 
> Not doing so permits an attacker to only guess an acceptable sequence
> number, evading stronger checks.
> 
> Many thanks to Zhiyun Qian for bringing this issue to our attention.
> 
> See :
> http://web.eecs.umich.edu/~zhiyunq/pub/ccs12_TCP_sequence_number_inference.pdf
> 
> Reported-by: Zhiyun Qian <zhiyunq@umich.edu>
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Applied, thanks Eric.