* [PATCH] tcp: Restore RFC5961-compliant behavior for SYN packets
@ 2014-11-20 23:09 Calvin Owens
2014-11-20 23:42 ` Eric Dumazet
2014-11-21 18:55 ` Neal Cardwell
0 siblings, 2 replies; 6+ messages in thread
From: Calvin Owens @ 2014-11-20 23:09 UTC (permalink / raw)
To: David S. Miller, Alexey Kuznetsov, James Morris, Eric Dumazet
Cc: kernel-team, netdev, linux-kernel, Calvin Owens
Commit c3ae62af8e755 ("tcp: should drop incoming frames without ACK
flag set") was created to mitigate a security vulnerability in which a
local attacker is able to inject data into locally-opened sockets by
using TCP protocol statistics in procfs to quickly find the correct
sequence number.
This broke the RFC5961 requirement to send a challenge ACK in response
to spurious RST packets, which was subsequently fixed by commit
7b514a886ba50 ("tcp: accept RST without ACK flag").
Unfortunately, the RFC5961 requirement that spurious SYN packets be
handled in a similar manner remains broken.
RFC5961 section 4 states that:
... the handling of the SYN in the synchronized state SHOULD be
performed as follows:
1) If the SYN bit is set, irrespective of the sequence number, TCP
MUST send an ACK (also referred to as challenge ACK) to the remote
peer:
<SEQ=SND.NXT><ACK=RCV.NXT><CTL=ACK>
After sending the acknowledgment, TCP MUST drop the unacceptable
segment and stop processing further.
By sending an ACK, the remote peer is challenged to confirm the loss
of the previous connection and the request to start a new connection.
A legitimate peer, after restart, would not have a TCB in the
synchronized state. Thus, when the ACK arrives, the peer should send
a RST segment back with the sequence number derived from the ACK
field that caused the RST.
This RST will confirm that the remote peer has indeed closed the
previous connection. Upon receipt of a valid RST, the local TCP
endpoint MUST terminate its connection. The local TCP endpoint
should then rely on SYN retransmission from the remote end to
re-establish the connection.
This patch lets SYN packets through the discard added in c3ae62af8e755,
so that spurious SYN packets are properly dealt with as per the RFC.
The challenge ACK is sent unconditionally and is rate-limited, so the
original vulnerability is not reintroduced by this patch.
Signed-off-by: Calvin Owens <calvinowens@fb.com>
---
net/ipv4/tcp_input.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 88fa2d1..d107ee2 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -5231,7 +5231,7 @@ slow_path:
if (len < (th->doff << 2) || tcp_checksum_complete_user(sk, skb))
goto csum_error;
- if (!th->ack && !th->rst)
+ if (!th->ack && !th->rst && !th->syn)
goto discard;
/*
@@ -5650,7 +5650,7 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb,
goto discard;
}
- if (!th->ack && !th->rst)
+ if (!th->ack && !th->rst && !th->syn)
goto discard;
if (!tcp_validate_incoming(sk, skb, th, 0))
--
2.1.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] tcp: Restore RFC5961-compliant behavior for SYN packets
2014-11-20 23:09 [PATCH] tcp: Restore RFC5961-compliant behavior for SYN packets Calvin Owens
@ 2014-11-20 23:42 ` Eric Dumazet
2014-11-21 1:47 ` Calvin Owens
2014-11-21 18:55 ` Neal Cardwell
1 sibling, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2014-11-20 23:42 UTC (permalink / raw)
To: Calvin Owens
Cc: David S. Miller, Alexey Kuznetsov, James Morris, Eric Dumazet,
kernel-team, netdev, linux-kernel
On Thu, 2014-11-20 at 15:09 -0800, Calvin Owens wrote:
> Commit c3ae62af8e755 ("tcp: should drop incoming frames without ACK
> flag set") was created to mitigate a security vulnerability in which a
> local attacker is able to inject data into locally-opened sockets by
> using TCP protocol statistics in procfs to quickly find the correct
> sequence number.
>
> This broke the RFC5961 requirement to send a challenge ACK in response
> to spurious RST packets, which was subsequently fixed by commit
> 7b514a886ba50 ("tcp: accept RST without ACK flag").
>
> Unfortunately, the RFC5961 requirement that spurious SYN packets be
> handled in a similar manner remains broken.
>
> RFC5961 section 4 states that:
>
> ... the handling of the SYN in the synchronized state SHOULD be
> performed as follows:
>
> 1) If the SYN bit is set, irrespective of the sequence number, TCP
> MUST send an ACK (also referred to as challenge ACK) to the remote
> peer:
>
> <SEQ=SND.NXT><ACK=RCV.NXT><CTL=ACK>
>
> After sending the acknowledgment, TCP MUST drop the unacceptable
> segment and stop processing further.
>
> By sending an ACK, the remote peer is challenged to confirm the loss
> of the previous connection and the request to start a new connection.
> A legitimate peer, after restart, would not have a TCB in the
> synchronized state. Thus, when the ACK arrives, the peer should send
> a RST segment back with the sequence number derived from the ACK
> field that caused the RST.
>
> This RST will confirm that the remote peer has indeed closed the
> previous connection. Upon receipt of a valid RST, the local TCP
> endpoint MUST terminate its connection. The local TCP endpoint
> should then rely on SYN retransmission from the remote end to
> re-establish the connection.
>
> This patch lets SYN packets through the discard added in c3ae62af8e755,
> so that spurious SYN packets are properly dealt with as per the RFC.
>
> The challenge ACK is sent unconditionally and is rate-limited, so the
> original vulnerability is not reintroduced by this patch.
I think this patch makes sense. But I wonder if the rate limiting wont
hurt anyway, as I presume you need that after some server being
rebooted, and if many connections are attempted in a small amount of
time, some of them wont get any answer ?
Thanks !
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tcp: Restore RFC5961-compliant behavior for SYN packets
2014-11-20 23:42 ` Eric Dumazet
@ 2014-11-21 1:47 ` Calvin Owens
2014-11-21 2:22 ` Eric Dumazet
0 siblings, 1 reply; 6+ messages in thread
From: Calvin Owens @ 2014-11-21 1:47 UTC (permalink / raw)
To: Eric Dumazet
Cc: David S. Miller, Alexey Kuznetsov, James Morris, Eric Dumazet,
kernel-team, netdev, linux-kernel
On Thursday 11/20 at 15:42 -0800, Eric Dumazet wrote:
> On Thu, 2014-11-20 at 15:09 -0800, Calvin Owens wrote:
> > Commit c3ae62af8e755 ("tcp: should drop incoming frames without ACK
> > flag set") was created to mitigate a security vulnerability in which a
> > local attacker is able to inject data into locally-opened sockets by
> > using TCP protocol statistics in procfs to quickly find the correct
> > sequence number.
> >
> > This broke the RFC5961 requirement to send a challenge ACK in response
> > to spurious RST packets, which was subsequently fixed by commit
> > 7b514a886ba50 ("tcp: accept RST without ACK flag").
> >
> > Unfortunately, the RFC5961 requirement that spurious SYN packets be
> > handled in a similar manner remains broken.
> >
> > RFC5961 section 4 states that:
> >
> > ... the handling of the SYN in the synchronized state SHOULD be
> > performed as follows:
> >
> > 1) If the SYN bit is set, irrespective of the sequence number, TCP
> > MUST send an ACK (also referred to as challenge ACK) to the remote
> > peer:
> >
> > <SEQ=SND.NXT><ACK=RCV.NXT><CTL=ACK>
> >
> > After sending the acknowledgment, TCP MUST drop the unacceptable
> > segment and stop processing further.
> >
> > By sending an ACK, the remote peer is challenged to confirm the loss
> > of the previous connection and the request to start a new connection.
> > A legitimate peer, after restart, would not have a TCB in the
> > synchronized state. Thus, when the ACK arrives, the peer should send
> > a RST segment back with the sequence number derived from the ACK
> > field that caused the RST.
> >
> > This RST will confirm that the remote peer has indeed closed the
> > previous connection. Upon receipt of a valid RST, the local TCP
> > endpoint MUST terminate its connection. The local TCP endpoint
> > should then rely on SYN retransmission from the remote end to
> > re-establish the connection.
> >
> > This patch lets SYN packets through the discard added in c3ae62af8e755,
> > so that spurious SYN packets are properly dealt with as per the RFC.
> >
> > The challenge ACK is sent unconditionally and is rate-limited, so the
> > original vulnerability is not reintroduced by this patch.
>
>
> I think this patch makes sense. But I wonder if the rate limiting wont
> hurt anyway, as I presume you need that after some server being
> rebooted, and if many connections are attempted in a small amount of
> time, some of them wont get any answer ?
That's actually not what led to finding this, but it's a good point. :)
What if the challenge-ACK counter were decremented in tcp_validate_incoming()
when a valid RST packet is seen? That would allow legitimate remote
hosts to reestablish connections without being ratelimited, and still
prevent a malicious host from guessing sequence numbers.
There would need to be a way to tell if a challenge ACK had in fact been
sent and only decrement in that case, since otherwise a local attacker
could establish and immediately reset lots of connections to keep the
counter below the ratelimit threshold and guess sequence numbers.
Simply adding a flag to struct tcp_sock would work: just set the flag
whenever a challenge ACK is sent, and clear it and decrement the counter
only if it is set when a valid RST packet is seen.
I suppose that should be a seperate patch?
Thanks,
Calvin
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tcp: Restore RFC5961-compliant behavior for SYN packets
2014-11-21 1:47 ` Calvin Owens
@ 2014-11-21 2:22 ` Eric Dumazet
2014-11-21 20:34 ` David Miller
0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2014-11-21 2:22 UTC (permalink / raw)
To: Calvin Owens
Cc: David S. Miller, Alexey Kuznetsov, James Morris, Eric Dumazet,
kernel-team, netdev, linux-kernel
On Thu, 2014-11-20 at 17:47 -0800, Calvin Owens wrote:
> That's actually not what led to finding this, but it's a good point. :)
>
> What if the challenge-ACK counter were decremented in tcp_validate_incoming()
> when a valid RST packet is seen? That would allow legitimate remote
> hosts to reestablish connections without being ratelimited, and still
> prevent a malicious host from guessing sequence numbers.
>
> There would need to be a way to tell if a challenge ACK had in fact been
> sent and only decrement in that case, since otherwise a local attacker
> could establish and immediately reset lots of connections to keep the
> counter below the ratelimit threshold and guess sequence numbers.
>
> Simply adding a flag to struct tcp_sock would work: just set the flag
> whenever a challenge ACK is sent, and clear it and decrement the counter
> only if it is set when a valid RST packet is seen.
Seems tricky, a Challenge ACK do not necessarily gives an RST.
Anyway this certainly can wait, as we already have a sysctl to
eventually work around the issue.
Acked-by: Eric Dumazet <edumazet@google.com>
Thanks !
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tcp: Restore RFC5961-compliant behavior for SYN packets
2014-11-20 23:09 [PATCH] tcp: Restore RFC5961-compliant behavior for SYN packets Calvin Owens
2014-11-20 23:42 ` Eric Dumazet
@ 2014-11-21 18:55 ` Neal Cardwell
1 sibling, 0 replies; 6+ messages in thread
From: Neal Cardwell @ 2014-11-21 18:55 UTC (permalink / raw)
To: Calvin Owens
Cc: David S. Miller, Alexey Kuznetsov, James Morris, Eric Dumazet,
kernel-team, Netdev, LKML
On Thu, Nov 20, 2014 at 6:09 PM, Calvin Owens <calvinowens@fb.com> wrote:
> Commit c3ae62af8e755 ("tcp: should drop incoming frames without ACK
> flag set") was created to mitigate a security vulnerability in which a
> local attacker is able to inject data into locally-opened sockets by
> using TCP protocol statistics in procfs to quickly find the correct
> sequence number.
>
> This broke the RFC5961 requirement to send a challenge ACK in response
> to spurious RST packets, which was subsequently fixed by commit
> 7b514a886ba50 ("tcp: accept RST without ACK flag").
>
> Unfortunately, the RFC5961 requirement that spurious SYN packets be
> handled in a similar manner remains broken.
>
> RFC5961 section 4 states that:
>
> ... the handling of the SYN in the synchronized state SHOULD be
> performed as follows:
>
> 1) If the SYN bit is set, irrespective of the sequence number, TCP
> MUST send an ACK (also referred to as challenge ACK) to the remote
> peer:
>
> <SEQ=SND.NXT><ACK=RCV.NXT><CTL=ACK>
>
> After sending the acknowledgment, TCP MUST drop the unacceptable
> segment and stop processing further.
>
> By sending an ACK, the remote peer is challenged to confirm the loss
> of the previous connection and the request to start a new connection.
> A legitimate peer, after restart, would not have a TCB in the
> synchronized state. Thus, when the ACK arrives, the peer should send
> a RST segment back with the sequence number derived from the ACK
> field that caused the RST.
>
> This RST will confirm that the remote peer has indeed closed the
> previous connection. Upon receipt of a valid RST, the local TCP
> endpoint MUST terminate its connection. The local TCP endpoint
> should then rely on SYN retransmission from the remote end to
> re-establish the connection.
>
> This patch lets SYN packets through the discard added in c3ae62af8e755,
> so that spurious SYN packets are properly dealt with as per the RFC.
>
> The challenge ACK is sent unconditionally and is rate-limited, so the
> original vulnerability is not reintroduced by this patch.
>
> Signed-off-by: Calvin Owens <calvinowens@fb.com>
Acked-by: Neal Cardwell <ncardwell@google.com>
neal
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tcp: Restore RFC5961-compliant behavior for SYN packets
2014-11-21 2:22 ` Eric Dumazet
@ 2014-11-21 20:34 ` David Miller
0 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2014-11-21 20:34 UTC (permalink / raw)
To: eric.dumazet
Cc: calvinowens, kuznet, jmorris, edumazet, kernel-team, netdev,
linux-kernel
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 20 Nov 2014 18:22:07 -0800
> On Thu, 2014-11-20 at 17:47 -0800, Calvin Owens wrote:
>
>> That's actually not what led to finding this, but it's a good point. :)
>>
>> What if the challenge-ACK counter were decremented in tcp_validate_incoming()
>> when a valid RST packet is seen? That would allow legitimate remote
>> hosts to reestablish connections without being ratelimited, and still
>> prevent a malicious host from guessing sequence numbers.
>>
>> There would need to be a way to tell if a challenge ACK had in fact been
>> sent and only decrement in that case, since otherwise a local attacker
>> could establish and immediately reset lots of connections to keep the
>> counter below the ratelimit threshold and guess sequence numbers.
>>
>> Simply adding a flag to struct tcp_sock would work: just set the flag
>> whenever a challenge ACK is sent, and clear it and decrement the counter
>> only if it is set when a valid RST packet is seen.
>
> Seems tricky, a Challenge ACK do not necessarily gives an RST.
>
> Anyway this certainly can wait, as we already have a sysctl to
> eventually work around the issue.
>
> Acked-by: Eric Dumazet <edumazet@google.com>
Applied, thanks everyone.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-11-21 20:34 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-11-20 23:09 [PATCH] tcp: Restore RFC5961-compliant behavior for SYN packets Calvin Owens
2014-11-20 23:42 ` Eric Dumazet
2014-11-21 1:47 ` Calvin Owens
2014-11-21 2:22 ` Eric Dumazet
2014-11-21 20:34 ` David Miller
2014-11-21 18:55 ` Neal Cardwell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).