TCP sequence number inference attack on Linux

On Fri, 2012-12-21 at 14:49 -0500, Zhiyun Qian wrote:

> If I am not mistaken, line 6142 in kernel v3.7.1 corresponds to
> tcp_rcv_state_process(). According to the comments, "This function
> implements the receiving procedure of RFC 793 for all states except
> ESTABLISHED and TIME_WAIT." Are you referring to a different kernel
> version?

You are not mistaken, it seems code is too permissive.

We should reject a frame without ACK flag while in ESTABLISHED state.

Thats explicitly stated in RFC 973.

Then we should make all possible safety checks before even sending a
frame or changing socket variables.

(For instance the tests done in tcp_ack() should be done before calling
tcp_validate_incoming())

John Dykstra in commit 96e0bf4b5193d0 (tcp: Discard segments that ack
data not yet sent) did a step into right direction, but missed this.

Current code assumes the incoming frame is mostly legitimate.



--
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

On Fri, Dec 21, 2012 at 5:45 PM, Eric Dumazet <erdnetdev@gmail.com> wrote:
> On Fri, 2012-12-21 at 14:49 -0500, Zhiyun Qian wrote:
>
>> If I am not mistaken, line 6142 in kernel v3.7.1 corresponds to
>> tcp_rcv_state_process(). According to the comments, "This function
>> implements the receiving procedure of RFC 793 for all states except
>> ESTABLISHED and TIME_WAIT." Are you referring to a different kernel
>> version?
>
> You are not mistaken, it seems code is too permissive.
>
> We should reject a frame without ACK flag while in ESTABLISHED state.
>
> Thats explicitly stated in RFC 973.
>
> Then we should make all possible safety checks before even sending a
> frame or changing socket variables.

I completely agree!

>
> (For instance the tests done in tcp_ack() should be done before calling
> tcp_validate_incoming())
>

It seems that it is not straightforward to simply move tcp_ack()
before tcp_validate_incoming() as tcp_ack() currently assumes the tcp
sequence number is already validated and it may adjust certain states
purely depending on the ACK number. I guess the solution is to extract
all safety checks out and put at the very beginning. The rest of the
code in tcp_validate_incoming() and tcp_ack() may still need to
perform the redundant checks since if some state changes are dependent
on the sequence number or ACK number (e.g., window update).

I'm willing to help on this. Perhaps I can draft an initial patch and
you can help take a look before I submit it?

> John Dykstra in commit 96e0bf4b5193d0 (tcp: Discard segments that ack
> data not yet sent) did a step into right direction, but missed this.
>
> Current code assumes the incoming frame is mostly legitimate.
>
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index a136925..2ea4937 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -5551,7 +5551,7 @@ slow_path:
>                 return 0;
>
>  step5:
> -       if (th->ack && tcp_ack(sk, skb, FLAG_SLOWPATH) < 0)
> +       if (!th->ack || tcp_ack(sk, skb, FLAG_SLOWPATH) < 0)
>                 goto discard;
>
>         /* ts_recent update must be made after we are sure that the packet
>
>

Neat change. This should enforce the ACK flag and ACK number check for
every packet received in established state.
--
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

On Fri, 2012-12-21 at 18:52 -0500, Zhiyun Qian wrote:

> It seems that it is not straightforward to simply move tcp_ack()
> before tcp_validate_incoming() as tcp_ack() currently assumes the tcp
> sequence number is already validated and it may adjust certain states
> purely depending on the ACK number. I guess the solution is to extract
> all safety checks out and put at the very beginning. The rest of the
> code in tcp_validate_incoming() and tcp_ack() may still need to
> perform the redundant checks since if some state changes are dependent
> on the sequence number or ACK number (e.g., window update).
> 
> I'm willing to help on this. Perhaps I can draft an initial patch and
> you can help take a look before I submit it?

I began coding a serie of patches.

Part of the problem comes from a 'fast path' idea that was nice 20 years
ago but no longer a real killer these days.

So some tests are duplicated, others are not correctly done.

For example, the fast path misses the more complete tests done in
tcp_ack()

Its a big mess.



--
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

On Fri, 2012-12-21 at 16:01 -0800, Eric Dumazet wrote:

> I began coding a serie of patches.

My goal is to make very small patches, to ease code review, and
eventually come to a state where we have more factorized code.



--
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

I see. If you need any help, just let me know! E.g., I can definitely
help review the patches.

-Zhiyun

On Fri, Dec 21, 2012 at 7:04 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Fri, 2012-12-21 at 16:01 -0800, Eric Dumazet wrote:
>
>> I began coding a serie of patches.
>
> My goal is to make very small patches, to ease code review, and
> eventually come to a state where we have more factorized code.
>
>
>
--
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