[PATCH net-next] tcp: accept RST if SEQ matches right edge of SACK block

[PATCH net-next] tcp: accept RST if SEQ matches right edge of SACK block

[Pau Espin Pedrol]

RFC 5961 advises to only accept RST packets containing a seq number
matching the next expected seq number instead of the whole receive
window in order to avoid spoofing attacks.

However, this situation is not optimal in the case SACK is in use at the
time the RST is sent. I recently run into a scenario in which packet
losses were high while uploading data to a server, and userspace was
willing to frequently terminate connections by sending a RST. In
this case, the ACK sent on the receiver side is frozen waiting for a lost
packet retransmission and a SACK block is used to let the client
continue uploading data. At some point later on, the client sends the
RST, which matches the next expected seq number of the SACK block on the
receiver side which is going forward receiving data.

In this scenario, as RFC 5961 defines, the SEQ doesn't match the frozen
main ACK at receiver side and thus gets dropped and a challenge ACK is
sent, which gets usually lost due to network conditions. The main
consequence is that the connection stays alive for a while even if it
made sense to accept the RST. This can get really bad if lots of
connections like this one are created in few seconds, allocating all the
resources of the server easily.

>From security point of view: the maximum number of SACK blocks for a TCP
connection is limited to 4 due to options field maximum length, and that
means we match at maximum against 5 seq numbers, which should make it
still difficult for attackers to inject a valid RST message.

This patch was tested in a 3.18 kernel and probed to improve the
situation in the scenario described above.

Signed-off-by: Pau Espin Pedrol <pau.espin@tessares.net>
---
 net/ipv4/tcp_input.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index d6c8f4cd0..4727dc8 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -5159,6 +5159,7 @@ static bool tcp_validate_incoming(struct sock *sk, struct sk_buff *skb,
 				  const struct tcphdr *th, int syn_inerr)
 {
 	struct tcp_sock *tp = tcp_sk(sk);
+	bool rst_seq_match = false;
 
 	/* RFC1323: H1. Apply PAWS check first. */
 	if (tcp_fast_parse_options(skb, th, tp) && tp->rx_opt.saw_tstamp &&
@@ -5195,13 +5196,28 @@ static bool tcp_validate_incoming(struct sock *sk, struct sk_buff *skb,
 
 	/* Step 2: check RST bit */
 	if (th->rst) {
-		/* RFC 5961 3.2 :
+		/* RFC 5961 3.2 (extended to match against SACK too if available):
 		 * If sequence number exactly matches RCV.NXT, then
 		 *     RESET the connection
 		 * else
 		 *     Send a challenge ACK
 		 */
 		if (TCP_SKB_CB(skb)->seq == tp->rcv_nxt)
+			rst_seq_match = true;
+		else if (tcp_is_sack(tp)) {
+			int this_sack;
+			struct tcp_sack_block *sp = tp->rx_opt.dsack ?
+					tp->duplicate_sack : tp->selective_acks;
+
+			for (this_sack = 0; this_sack < tp->rx_opt.num_sacks; ++this_sack) {
+				if (TCP_SKB_CB(skb)->seq == sp[this_sack].end_seq) {
+					rst_seq_match = true;
+					break;
+				}
+			}
+		}
+
+		if (rst_seq_match)
 			tcp_reset(sk);
 		else
 			tcp_send_challenge_ack(sk, skb);
-- 
2.5.0


-- 

------------------------------
DISCLAIMER.
This email and any files transmitted with it are confidential and intended 
solely for the use of the individual or entity to whom they are addressed. 
If you have received this email in error please notify the system manager. 
This message contains confidential information and is intended only for the 
individual named. If you are not the named addressee you should not 
disseminate, distribute or copy this e-mail. Please notify the sender 
immediately by e-mail if you have received this e-mail by mistake and 
delete this e-mail from your system. If you are not the intended recipient 
you are notified that disclosing, copying, distributing or taking any 
action in reliance on the contents of this information is strictly 
prohibited.

Re: [PATCH net-next] tcp: accept RST if SEQ matches right edge of SACK block

[Eric Dumazet]

On Tue, 2016-05-31 at 13:38 +0200, Pau Espin Pedrol wrote:
> RFC 5961 advises to only accept RST packets containing a seq number
> matching the next expected seq number instead of the whole receive
> window in order to avoid spoofing attacks.
> 
> However, this situation is not optimal in the case SACK is in use at the
> time the RST is sent. I recently run into a scenario in which packet
> losses were high while uploading data to a server, and userspace was
> willing to frequently terminate connections by sending a RST. In
> this case, the ACK sent on the receiver side is frozen waiting for a lost
> packet retransmission and a SACK block is used to let the client
> continue uploading data. At some point later on, the client sends the
> RST, which matches the next expected seq number of the SACK block on the
> receiver side which is going forward receiving data.
> 
> In this scenario, as RFC 5961 defines, the SEQ doesn't match the frozen
> main ACK at receiver side and thus gets dropped and a challenge ACK is
> sent, which gets usually lost due to network conditions. The main
> consequence is that the connection stays alive for a while even if it
> made sense to accept the RST. This can get really bad if lots of
> connections like this one are created in few seconds, allocating all the
> resources of the server easily.
> 
> From security point of view: the maximum number of SACK blocks for a TCP
> connection is limited to 4 due to options field maximum length, and that
> means we match at maximum against 5 seq numbers, which should make it
> still difficult for attackers to inject a valid RST message.
> 
> This patch was tested in a 3.18 kernel and probed to improve the
> situation in the scenario described above.
> 
> Signed-off-by: Pau Espin Pedrol <pau.espin@tessares.net>
> ---
>  net/ipv4/tcp_input.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index d6c8f4cd0..4727dc8 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -5159,6 +5159,7 @@ static bool tcp_validate_incoming(struct sock *sk, struct sk_buff *skb,
>  				  const struct tcphdr *th, int syn_inerr)
>  {
>  	struct tcp_sock *tp = tcp_sk(sk);
> +	bool rst_seq_match = false;
>  
>  	/* RFC1323: H1. Apply PAWS check first. */
>  	if (tcp_fast_parse_options(skb, th, tp) && tp->rx_opt.saw_tstamp &&
> @@ -5195,13 +5196,28 @@ static bool tcp_validate_incoming(struct sock *sk, struct sk_buff *skb,
>  
>  	/* Step 2: check RST bit */
>  	if (th->rst) {
> -		/* RFC 5961 3.2 :
> +		/* RFC 5961 3.2 (extended to match against SACK too if available):
>  		 * If sequence number exactly matches RCV.NXT, then
>  		 *     RESET the connection
>  		 * else
>  		 *     Send a challenge ACK
>  		 */
>  		if (TCP_SKB_CB(skb)->seq == tp->rcv_nxt)
> +			rst_seq_match = true;
> +		else if (tcp_is_sack(tp)) {
> +			int this_sack;
> +			struct tcp_sack_block *sp = tp->rx_opt.dsack ?
> +					tp->duplicate_sack : tp->selective_acks;
> +
> +			for (this_sack = 0; this_sack < tp->rx_opt.num_sacks; ++this_sack) {
> +				if (TCP_SKB_CB(skb)->seq == sp[this_sack].end_seq) {
> +					rst_seq_match = true;
> +					break;
> +				}
> +			}
> +		}
> +
> +		if (rst_seq_match)
>  			tcp_reset(sk);
>  		else
>  			tcp_send_challenge_ack(sk, skb);
> -- 
> 2.5.0

It looks like you want to seriously relax RFC 5961 ...

Could you have a problem because of the host-wide RFC 5961 rate limit ?

Have you contacted RFC authors ?

If the peer sends the RST, presumably it should answer to the challenge
ACK right away, since it does not care of the SACK blocks anymore.

A packetdrill test demonstrating the issue would be nice.

Thanks.

Re: [PATCH net-next] tcp: accept RST if SEQ matches right edge of SACK block

[Pau Espin]

Hi, sorry re-sending because I didn't have plain text mode enabled and
message didn't arrive to the mailing list. Also dropping some mail
addresses from RFC authors which probed to be unavailable anymore.

The problem is not caused by the host-wide rate limit.
I analyzed the scenario with tcpdump and I see the challenge ACKs are
being sent, but due to network conditions sometimes they can be lost.
On top of that, the RST being sent from the client to the server after
receiving the challenge ACK can also sometimes be lost. I even found
the client/router had a problem with iptables setup and was actually
not sending back RST for incoming TCP packets without an existing
connection established (should be the one already closed before with
the first RST sent by the client and which originated the challenge
ACK).

In my test scenario I'm using MultiPathTCP and I'm recreating (destroy
with RST, then create with SYN)  the subflow on one of the interfaces
every aprox 5 seconds while uploading a big file. Due to network
conditions and/or router stopping the RSTs, after less than 5 min, I
have more than 32 subflow connections created for that MPTCP
connection, which is the hardcoded maximum, and from that point I'm
unable to create/use new subflows until some of them are closed, which
can take quite a lot of time. After this patch is applied, the number
of subflows is kind of stable at 4-5 subflows during the whole upload.
It's still not 2 (one permanent in iface1 and the recreated one in
iface2) because sometimes due to network problems the packet before
the RST is lost and thus the RST which arrives is not equal to next
expected seq number from the right edge of the SACK block. But still,
it makes the situation quite better, specially from user point of
view.

Here's an example of the issue without my patch (challenge ACK is sent
although the RST is in current place). It shows server acknowledging
data with SACK in first line. Then, on 2nd line, client decides to
terminate the connexion and uses his next SEQ number available. On 3rd
line, server answers with the challenge ACK. Then no answer comes from
that challenge ack and the TCP conn is left opened.

14202    73.086360    10.0.4.2    443    10.67.15.100    53755    TCP
  94    [TCP Dup ACK 14192#4] 443→53755 [ACK] Seq=3551806992
Ack=1240653972 Win=1573120 Len=0 TSval=4022818 TSecr=1106789
SLE=1240836636 SRE=1240942836 SLE=1240770084 SRE=1240835220
14203    73.086363    10.67.15.100    53755    10.0.4.2    443    TCP
  74    53755→443 [RST, ACK] Seq=1240942836 Ack=3551806992 Win=90240
Len=0 TSval=1106847 TSecr=4022536
14204    73.086368    10.0.4.2    443    10.67.15.100    53755    TCP
  94    [TCP Dup ACK 14192#5] 443→53755 [ACK] Seq=3551806992
Ack=1240653972 Win=1573120 Len=0 TSval=4022818 TSecr=1106789
SLE=1240836636 SRE=1240942836 SLE=1240770084 SRE=1240835220

So, the main point in here is trying to improve the situation to close
the connections and free resources in some specific cases without
actually going pre RFC5961 state. That would mean when a RST is
received, up to 4-5 SEQs are checked to match instead of 1.

I didn't contact the authors of the RFC. I CC them in this e-mail. I
hope that's the right thing to do in this case and that they don't
mind it in case they want to follow the topic.

I will have a look at packetdrill to try to reproduce it somehow there.

On Tue, May 31, 2016 at 5:12 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Tue, 2016-05-31 at 13:38 +0200, Pau Espin Pedrol wrote:
>> RFC 5961 advises to only accept RST packets containing a seq number
>> matching the next expected seq number instead of the whole receive
>> window in order to avoid spoofing attacks.
>>
>> However, this situation is not optimal in the case SACK is in use at the
>> time the RST is sent. I recently run into a scenario in which packet
>> losses were high while uploading data to a server, and userspace was
>> willing to frequently terminate connections by sending a RST. In
>> this case, the ACK sent on the receiver side is frozen waiting for a lost
>> packet retransmission and a SACK block is used to let the client
>> continue uploading data. At some point later on, the client sends the
>> RST, which matches the next expected seq number of the SACK block on the
>> receiver side which is going forward receiving data.
>>
>> In this scenario, as RFC 5961 defines, the SEQ doesn't match the frozen
>> main ACK at receiver side and thus gets dropped and a challenge ACK is
>> sent, which gets usually lost due to network conditions. The main
>> consequence is that the connection stays alive for a while even if it
>> made sense to accept the RST. This can get really bad if lots of
>> connections like this one are created in few seconds, allocating all the
>> resources of the server easily.
>>
>> From security point of view: the maximum number of SACK blocks for a TCP
>> connection is limited to 4 due to options field maximum length, and that
>> means we match at maximum against 5 seq numbers, which should make it
>> still difficult for attackers to inject a valid RST message.
>>
>> This patch was tested in a 3.18 kernel and probed to improve the
>> situation in the scenario described above.
>>
>> Signed-off-by: Pau Espin Pedrol <pau.espin@tessares.net>
>> ---
>>  net/ipv4/tcp_input.c | 18 +++++++++++++++++-
>>  1 file changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
>> index d6c8f4cd0..4727dc8 100644
>> --- a/net/ipv4/tcp_input.c
>> +++ b/net/ipv4/tcp_input.c
>> @@ -5159,6 +5159,7 @@ static bool tcp_validate_incoming(struct sock *sk, struct sk_buff *skb,
>>                                 const struct tcphdr *th, int syn_inerr)
>>  {
>>       struct tcp_sock *tp = tcp_sk(sk);
>> +     bool rst_seq_match = false;
>>
>>       /* RFC1323: H1. Apply PAWS check first. */
>>       if (tcp_fast_parse_options(skb, th, tp) && tp->rx_opt.saw_tstamp &&
>> @@ -5195,13 +5196,28 @@ static bool tcp_validate_incoming(struct sock *sk, struct sk_buff *skb,
>>
>>       /* Step 2: check RST bit */
>>       if (th->rst) {
>> -             /* RFC 5961 3.2 :
>> +             /* RFC 5961 3.2 (extended to match against SACK too if available):
>>                * If sequence number exactly matches RCV.NXT, then
>>                *     RESET the connection
>>                * else
>>                *     Send a challenge ACK
>>                */
>>               if (TCP_SKB_CB(skb)->seq == tp->rcv_nxt)
>> +                     rst_seq_match = true;
>> +             else if (tcp_is_sack(tp)) {
>> +                     int this_sack;
>> +                     struct tcp_sack_block *sp = tp->rx_opt.dsack ?
>> +                                     tp->duplicate_sack : tp->selective_acks;
>> +
>> +                     for (this_sack = 0; this_sack < tp->rx_opt.num_sacks; ++this_sack) {
>> +                             if (TCP_SKB_CB(skb)->seq == sp[this_sack].end_seq) {
>> +                                     rst_seq_match = true;
>> +                                     break;
>> +                             }
>> +                     }
>> +             }
>> +
>> +             if (rst_seq_match)
>>                       tcp_reset(sk);
>>               else
>>                       tcp_send_challenge_ack(sk, skb);
>> --
>> 2.5.0
>
> It looks like you want to seriously relax RFC 5961 ...
>
> Could you have a problem because of the host-wide RFC 5961 rate limit ?
>
> Have you contacted RFC authors ?
>
> If the peer sends the RST, presumably it should answer to the challenge
> ACK right away, since it does not care of the SACK blocks anymore.
>
> A packetdrill test demonstrating the issue would be nice.
>
> Thanks.
>
>
>



-- 
Pau Espin Pedrol | R&D Engineer - External
pau.espin@tessares.net | +32 487 43 36 50
Tessares SA | Hybrid Access Solutions
www.tessares.net
6 Rue Louis de Geer, 1348 Louvain-la-Neuve, Belgium

-- 

------------------------------
DISCLAIMER.
This email and any files transmitted with it are confidential and intended 
solely for the use of the individual or entity to whom they are addressed. 
If you have received this email in error please notify the system manager. 
This message contains confidential information and is intended only for the 
individual named. If you are not the named addressee you should not 
disseminate, distribute or copy this e-mail. Please notify the sender 
immediately by e-mail if you have received this e-mail by mistake and 
delete this e-mail from your system. If you are not the intended recipient 
you are notified that disclosing, copying, distributing or taking any 
action in reliance on the contents of this information is strictly 
prohibited.

Re: [PATCH net-next] tcp: accept RST if SEQ matches right edge of SACK block

[Yuchung Cheng]

On Tue, May 31, 2016 at 9:50 AM, Pau Espin <pau.espin@tessares.net> wrote:
>
> Hi, sorry re-sending because I didn't have plain text mode enabled and
> message didn't arrive to the mailing list. Also dropping some mail
> addresses from RFC authors which probed to be unavailable anymore.
>
> The problem is not caused by the host-wide rate limit.
> I analyzed the scenario with tcpdump and I see the challenge ACKs are
> being sent, but due to network conditions sometimes they can be lost.
> On top of that, the RST being sent from the client to the server after
> receiving the challenge ACK can also sometimes be lost. I even found
> the client/router had a problem with iptables setup and was actually
> not sending back RST for incoming TCP packets without an existing
> connection established (should be the one already closed before with
> the first RST sent by the client and which originated the challenge
> ACK).
>
> In my test scenario I'm using MultiPathTCP and I'm recreating (destroy
> with RST, then create with SYN)  the subflow on one of the interfaces
> every aprox 5 seconds while uploading a big file. Due to network
> conditions and/or router stopping the RSTs, after less than 5 min, I
> have more than 32 subflow connections created for that MPTCP
> connection, which is the hardcoded maximum, and from that point I'm
> unable to create/use new subflows until some of them are closed, which
> can take quite a lot of time. After this patch is applied, the number
> of subflows is kind of stable at 4-5 subflows during the whole upload.
> It's still not 2 (one permanent in iface1 and the recreated one in
> iface2) because sometimes due to network problems the packet before
> the RST is lost and thus the RST which arrives is not equal to next
> expected seq number from the right edge of the SACK block. But still,
> it makes the situation quite better, specially from user point of
> view.
>
> Here's an example of the issue without my patch (challenge ACK is sent
> although the RST is in current place). It shows server acknowledging
> data with SACK in first line. Then, on 2nd line, client decides to
> terminate the connexion and uses his next SEQ number available. On 3rd
> line, server answers with the challenge ACK. Then no answer comes from
> that challenge ack and the TCP conn is left opened.
>
> 14202    73.086360    10.0.4.2    443    10.67.15.100    53755    TCP
>   94    [TCP Dup ACK 14192#4] 443→53755 [ACK] Seq=3551806992
> Ack=1240653972 Win=1573120 Len=0 TSval=4022818 TSecr=1106789
> SLE=1240836636 SRE=1240942836 SLE=1240770084 SRE=1240835220
> 14203    73.086363    10.67.15.100    53755    10.0.4.2    443    TCP
>   74    53755→443 [RST, ACK] Seq=1240942836 Ack=3551806992 Win=90240
> Len=0 TSval=1106847 TSecr=4022536
> 14204    73.086368    10.0.4.2    443    10.67.15.100    53755    TCP
>   94    [TCP Dup ACK 14192#5] 443→53755 [ACK] Seq=3551806992
> Ack=1240653972 Win=1573120 Len=0 TSval=4022818 TSecr=1106789
> SLE=1240836636 SRE=1240942836 SLE=1240770084 SRE=1240835220
>
> So, the main point in here is trying to improve the situation to close
> the connections and free resources in some specific cases without
> actually going pre RFC5961 state. That would mean when a RST is
> received, up to 4-5 SEQs are checked to match instead of 1.
>
> I didn't contact the authors of the RFC. I CC them in this e-mail. I
> hope that's the right thing to do in this case and that they don't
> mind it in case they want to follow the topic.
>
> I will have a look at packetdrill to try to reproduce it somehow there.
>
> On Tue, May 31, 2016 at 5:12 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > On Tue, 2016-05-31 at 13:38 +0200, Pau Espin Pedrol wrote:
> >> RFC 5961 advises to only accept RST packets containing a seq number
> >> matching the next expected seq number instead of the whole receive
> >> window in order to avoid spoofing attacks.
> >>
> >> However, this situation is not optimal in the case SACK is in use at the
> >> time the RST is sent. I recently run into a scenario in which packet
> >> losses were high while uploading data to a server, and userspace was
> >> willing to frequently terminate connections by sending a RST. In
> >> this case, the ACK sent on the receiver side is frozen waiting for a lost
> >> packet retransmission and a SACK block is used to let the client
> >> continue uploading data. At some point later on, the client sends the
> >> RST, which matches the next expected seq number of the SACK block on the
> >> receiver side which is going forward receiving data.
> >>
> >> In this scenario, as RFC 5961 defines, the SEQ doesn't match the frozen
> >> main ACK at receiver side and thus gets dropped and a challenge ACK is
> >> sent, which gets usually lost due to network conditions. The main
> >> consequence is that the connection stays alive for a while even if it
> >> made sense to accept the RST. This can get really bad if lots of
> >> connections like this one are created in few seconds, allocating all the
> >> resources of the server easily.
> >>
> >> From security point of view: the maximum number of SACK blocks for a TCP
> >> connection is limited to 4 due to options field maximum length, and that
This is not true. The maximum number of SACK blocks for a TCP "packet"
is limited to 4. But a TCP connection can keep an arbitrary amount of
SACK blocks.

> >> means we match at maximum against 5 seq numbers, which should make it
> >> still difficult for attackers to inject a valid RST message.
> >>
> >> This patch was tested in a 3.18 kernel and probed to improve the
> >> situation in the scenario described above.
> >>
> >> Signed-off-by: Pau Espin Pedrol <pau.espin@tessares.net>
> >> ---
> >>  net/ipv4/tcp_input.c | 18 +++++++++++++++++-
> >>  1 file changed, 17 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> >> index d6c8f4cd0..4727dc8 100644
> >> --- a/net/ipv4/tcp_input.c
> >> +++ b/net/ipv4/tcp_input.c
> >> @@ -5159,6 +5159,7 @@ static bool tcp_validate_incoming(struct sock *sk, struct sk_buff *skb,
> >>                                 const struct tcphdr *th, int syn_inerr)
> >>  {
> >>       struct tcp_sock *tp = tcp_sk(sk);
> >> +     bool rst_seq_match = false;
> >>
> >>       /* RFC1323: H1. Apply PAWS check first. */
> >>       if (tcp_fast_parse_options(skb, th, tp) && tp->rx_opt.saw_tstamp &&
> >> @@ -5195,13 +5196,28 @@ static bool tcp_validate_incoming(struct sock *sk, struct sk_buff *skb,
> >>
> >>       /* Step 2: check RST bit */
> >>       if (th->rst) {
> >> -             /* RFC 5961 3.2 :
> >> +             /* RFC 5961 3.2 (extended to match against SACK too if available):
> >>                * If sequence number exactly matches RCV.NXT, then
> >>                *     RESET the connection
> >>                * else
> >>                *     Send a challenge ACK
> >>                */
> >>               if (TCP_SKB_CB(skb)->seq == tp->rcv_nxt)
> >> +                     rst_seq_match = true;
> >> +             else if (tcp_is_sack(tp)) {
> >> +                     int this_sack;
> >> +                     struct tcp_sack_block *sp = tp->rx_opt.dsack ?
> >> +                                     tp->duplicate_sack : tp->selective_acks;
> >> +
> >> +                     for (this_sack = 0; this_sack < tp->rx_opt.num_sacks; ++this_sack) {
> >> +                             if (TCP_SKB_CB(skb)->seq == sp[this_sack].end_seq) {
> >> +                                     rst_seq_match = true;
> >> +                                     break;
> >> +                             }
> >> +                     }
> >> +             }
> >> +
> >> +             if (rst_seq_match)
> >>                       tcp_reset(sk);
> >>               else
> >>                       tcp_send_challenge_ack(sk, skb);
> >> --
> >> 2.5.0
> >
> > It looks like you want to seriously relax RFC 5961 ...
> >
> > Could you have a problem because of the host-wide RFC 5961 rate limit ?
> >
> > Have you contacted RFC authors ?
> >
> > If the peer sends the RST, presumably it should answer to the challenge
> > ACK right away, since it does not care of the SACK blocks anymore.
> >
> > A packetdrill test demonstrating the issue would be nice.
> >
> > Thanks.
> >
> >
> >
>
>
>
> --
> Pau Espin Pedrol | R&D Engineer - External
> pau.espin@tessares.net | +32 487 43 36 50
> Tessares SA | Hybrid Access Solutions
> www.tessares.net
> 6 Rue Louis de Geer, 1348 Louvain-la-Neuve, Belgium
>
> --
>
> ------------------------------
> DISCLAIMER.
> This email and any files transmitted with it are confidential and intended
> solely for the use of the individual or entity to whom they are addressed.
> If you have received this email in error please notify the system manager.
> This message contains confidential information and is intended only for the
> individual named. If you are not the named addressee you should not
> disseminate, distribute or copy this e-mail. Please notify the sender
> immediately by e-mail if you have received this e-mail by mistake and
> delete this e-mail from your system. If you are not the intended recipient
> you are notified that disclosing, copying, distributing or taking any
> action in reliance on the contents of this information is strictly
> prohibited.

Re: [PATCH net-next] tcp: accept RST if SEQ matches right edge of SACK block

[Pau Espin]

Hi, first of all, here you can find the packetdrill test I created to
show up the scenario in which SACK is used and the RST is answered
with a challenge_ack. You will find below too some answers to some
previous comments.

0  socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
+0 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
+0 bind(3, {sa_family = AF_INET, sin_port = htons(13000), sin_addr =
inet_addr("192.168.0.1")}, ...) = 0
+0 listen(3, 1) = 0

+0 < S 0:0(0) win 500 <mss 1460,sackOK,nop,nop,nop,wscale 7> sock(3)
+0 > S. 0:0(0) ack 1 win 29200 <mss 1460,nop,nop,sackOK,nop,wscale 7> sock(3)
+0 < . 1:1(0) ack 1 win 500 sock(3)
+0 accept(3, ..., ...) = 4

// First roundtrip of data client->server (upload)
+0 < P. 1:1001(1000) win 500 sock(4)
+0 > . 1:1(0) ack 1001 <...> sock(4)

//In here we would have 2nd packet but it's dropped by net...
// +0 < P. 1001:2001(1000) win 500 sock(4)

// 3nd packet arrives without prev packet being seen. SACK will be used.
+0 < P. 2001:3001(1000) win 500 sock(4)
+0 > . 1:1(0) ack 1001 <nop,nop,sack 2001:3001>

// RST arrives from client, for instance caused by setsockopt(fd,
SOL_SOCKET, SO_LINGER, ...)
//Challenge ACK is sent back
+0 < R. 3001:3001(0) win 500 sock(4)
+0 > . 1:1(0) ack 1001 <nop,nop,sack 2001:3001>

// To clearly differentiate closing connections around the same time
due to packetdrill finishing
+0 `sleep 20`


On Wed, Jun 1, 2016 at 12:32 PM, Randall Stewart <rrs@netflix.com> wrote:
> Yuchung:
>
> Thanks for adding me in here.. Not sure if Mitesh is still at Cisco or not..
> most of the
> rest of us are *not*. I think Anantha was at NEC last I heard but I have not
> talked to him in a while :-)
>
> Now there is one thing that Pau is missing here is that while in theory
> each ACK can only have 4 Sack blocks, that does not mean the
> SACK sender does not have *many* more SACK blocks on his score
> board. The scoreboard is kept until a time-out occurs and you only
> are sending the latest four. So I think the risk is a bit more than
> only 5 sequence numbers depending of course how one is to
> implement this :)
>
> You can implement it with a limit of 4 blocks, but one could
> also implement it based on the total number of sack blocks
> the receiver has..
>
> So in the scenario tcpdumped below, the client is sending
> RST(1240942836) to the server. Presumably the client’s snd_nxt
> was set to this after say retransmitting the missing piece. The client
> therefore (if its anything like BSD) will use the snd_nxt to send in the
> reset.. which
> of course would generate a challenge ack which as you say is dropped.
>
> One could take the approach below, and allow N blocks but in this
> scenario you are just luckily that the snd_nxt of the client happens to
> be an edge of one of your SACK blocks.
>
Indeed, as Yuchung noticed too, I was wrong from conceptual point of
view thinking the SACKs in the packet were all the SACK blocks which
could be stored. But still, the code patch changing the behavior
(which improved the situation in my scenario) was checking only
against selective_acks (struct tcp_sack_block selective_acks[4];), so
there's no need to check for all the possible SACK blocks in the score
board to improve the situation imho.

I did a quick check  on some of the related code and as far as I
understood, when  a new out of order packet is received, it is checked
(tcp_sack_new_ofo_skb) if it can be added to either rcv_next or any of
the 4 selective_acks blocks of the connection. If that's not possible,
a new one is built and added to the front of the list, shifting
previous ones.

The consequence of that, as far as I understand it, is that most
recently received out of order SEQ numbers are usually stored in the
selective_acks structure, which would act then as a cache for recently
seen SEQ numbers (and next expected seq numbers to receive). As
Randall pointed out, up to now RST are probably sent using snd_nxt, so
my guess is that probability of having a match with the selective_acks
blocks is quite high, as it probed in my test scenario which got a big
improvement.

> Possibly another alternative is to change the client where when sending a
> RST
> with a TCB instead of using snd_nxt you could use snd_una. Of course that
> could
> also result in a challenge ACK if the receiver has not yet received a
> ACK that is in flight (that was the whole purpose of the challenge ack). I
> think overall
> you will always have this problem i.e. the sender of the RST may not know
> precisely
> the state of the receiver.

Indeed, I guess there will still be same problem in other scenarios.
On top of that, it seems a bit weird to me to send a RST packet using
a SEQ number which was already used to send a different packet (that's
what would happen in this case right?).

>
> Your fix happens to work since the receiver happens to have the SACK blocks
> in question.. this is fine and if you don’t mind *weakening the security* of
> the
> RST you could do that. I think for stack I am working on for FreeBSD I will
> change
> the stack I am working on to recognize the RST going out and use snd_una.
>

You mean here you are always going to use snd_una or that you are
going to try to figure out some heuristics to use either snd_next or
snd_una depending on the scenario?

> I don’t like the idea of weakening the security, I know as you propose below
> its
> just 5 in 2^^32 instead of 1 in 2^^32 but depending on how you implement it
> you could stretch that to be the number of sack blocks the receiver holds.
>

As stated above, I think it would probably be enough to check for
those 4-5 seq numbers and not for all of them, which would on top not
hit on security that much (but still maybe too much for somebody).

>
> R
>
>
> On Jun 1, 2016, at 1:41 AM, Yuchung Cheng <ycheng@google.com> wrote:
>
> On Tue, May 31, 2016 at 9:50 AM, Pau Espin <pau.espin@tessares.net> wrote:
>
>
> Hi, sorry re-sending because I didn't have plain text mode enabled and
> message didn't arrive to the mailing list. Also dropping some mail
> addresses from RFC authors which probed to be unavailable anymore.
>
> The problem is not caused by the host-wide rate limit.
> I analyzed the scenario with tcpdump and I see the challenge ACKs are
> being sent, but due to network conditions sometimes they can be lost.
> On top of that, the RST being sent from the client to the server after
> receiving the challenge ACK can also sometimes be lost. I even found
> the client/router had a problem with iptables setup and was actually
> not sending back RST for incoming TCP packets without an existing
> connection established (should be the one already closed before with
> the first RST sent by the client and which originated the challenge
> ACK).
>
> In my test scenario I'm using MultiPathTCP and I'm recreating (destroy
> with RST, then create with SYN)  the subflow on one of the interfaces
> every aprox 5 seconds while uploading a big file. Due to network
> conditions and/or router stopping the RSTs, after less than 5 min, I
> have more than 32 subflow connections created for that MPTCP
> connection, which is the hardcoded maximum, and from that point I'm
> unable to create/use new subflows until some of them are closed, which
> can take quite a lot of time. After this patch is applied, the number
> of subflows is kind of stable at 4-5 subflows during the whole upload.
> It's still not 2 (one permanent in iface1 and the recreated one in
> iface2) because sometimes due to network problems the packet before
> the RST is lost and thus the RST which arrives is not equal to next
> expected seq number from the right edge of the SACK block. But still,
> it makes the situation quite better, specially from user point of
> view.
>
> Here's an example of the issue without my patch (challenge ACK is sent
> although the RST is in current place). It shows server acknowledging
> data with SACK in first line. Then, on 2nd line, client decides to
> terminate the connexion and uses his next SEQ number available. On 3rd
> line, server answers with the challenge ACK. Then no answer comes from
> that challenge ack and the TCP conn is left opened.
>
> 14202    73.086360    10.0.4.2    443    10.67.15.100    53755    TCP
>  94    [TCP Dup ACK 14192#4] 443→53755 [ACK] Seq=3551806992
> Ack=1240653972 Win=1573120 Len=0 TSval=4022818 TSecr=1106789
> SLE=1240836636 SRE=1240942836 SLE=1240770084 SRE=1240835220
> 14203    73.086363    10.67.15.100    53755    10.0.4.2    443    TCP
>  74    53755→443 [RST, ACK] Seq=1240942836 Ack=3551806992 Win=90240
> Len=0 TSval=1106847 TSecr=4022536
> 14204    73.086368    10.0.4.2    443    10.67.15.100    53755    TCP
>  94    [TCP Dup ACK 14192#5] 443→53755 [ACK] Seq=3551806992
> Ack=1240653972 Win=1573120 Len=0 TSval=4022818 TSecr=1106789
> SLE=1240836636 SRE=1240942836 SLE=1240770084 SRE=1240835220
>
> So, the main point in here is trying to improve the situation to close
> the connections and free resources in some specific cases without
> actually going pre RFC5961 state. That would mean when a RST is
> received, up to 4-5 SEQs are checked to match instead of 1.
>
> I didn't contact the authors of the RFC. I CC them in this e-mail. I
> hope that's the right thing to do in this case and that they don't
> mind it in case they want to follow the topic.
>
> I will have a look at packetdrill to try to reproduce it somehow there.
>
> On Tue, May 31, 2016 at 5:12 PM, Eric Dumazet <eric.dumazet@gmail.com>
> wrote:
>
> On Tue, 2016-05-31 at 13:38 +0200, Pau Espin Pedrol wrote:
>
> RFC 5961 advises to only accept RST packets containing a seq number
> matching the next expected seq number instead of the whole receive
> window in order to avoid spoofing attacks.
>
> However, this situation is not optimal in the case SACK is in use at the
> time the RST is sent. I recently run into a scenario in which packet
> losses were high while uploading data to a server, and userspace was
> willing to frequently terminate connections by sending a RST. In
> this case, the ACK sent on the receiver side is frozen waiting for a lost
> packet retransmission and a SACK block is used to let the client
> continue uploading data. At some point later on, the client sends the
> RST, which matches the next expected seq number of the SACK block on the
> receiver side which is going forward receiving data.
>
> In this scenario, as RFC 5961 defines, the SEQ doesn't match the frozen
> main ACK at receiver side and thus gets dropped and a challenge ACK is
> sent, which gets usually lost due to network conditions. The main
> consequence is that the connection stays alive for a while even if it
> made sense to accept the RST. This can get really bad if lots of
> connections like this one are created in few seconds, allocating all the
> resources of the server easily.
>
> From security point of view: the maximum number of SACK blocks for a TCP
> connection is limited to 4 due to options field maximum length, and that
>
> This is not true. The maximum number of SACK blocks for a TCP "packet"
> is limited to 4. But a TCP connection can keep an arbitrary amount of
> SACK blocks.
>
> means we match at maximum against 5 seq numbers, which should make it
> still difficult for attackers to inject a valid RST message.
>
> This patch was tested in a 3.18 kernel and probed to improve the
> situation in the scenario described above.
>
> Signed-off-by: Pau Espin Pedrol <pau.espin@tessares.net>
> ---
> net/ipv4/tcp_input.c | 18 +++++++++++++++++-
> 1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index d6c8f4cd0..4727dc8 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -5159,6 +5159,7 @@ static bool tcp_validate_incoming(struct sock *sk,
> struct sk_buff *skb,
>                                const struct tcphdr *th, int syn_inerr)
> {
>      struct tcp_sock *tp = tcp_sk(sk);
> +     bool rst_seq_match = false;
>
>      /* RFC1323: H1. Apply PAWS check first. */
>      if (tcp_fast_parse_options(skb, th, tp) && tp->rx_opt.saw_tstamp &&
> @@ -5195,13 +5196,28 @@ static bool tcp_validate_incoming(struct sock *sk,
> struct sk_buff *skb,
>
>      /* Step 2: check RST bit */
>      if (th->rst) {
> -             /* RFC 5961 3.2 :
> +             /* RFC 5961 3.2 (extended to match against SACK too if
> available):
>               * If sequence number exactly matches RCV.NXT, then
>               *     RESET the connection
>               * else
>               *     Send a challenge ACK
>               */
>              if (TCP_SKB_CB(skb)->seq == tp->rcv_nxt)
> +                     rst_seq_match = true;
> +             else if (tcp_is_sack(tp)) {
> +                     int this_sack;
> +                     struct tcp_sack_block *sp = tp->rx_opt.dsack ?
> +                                     tp->duplicate_sack :
> tp->selective_acks;
> +
> +                     for (this_sack = 0; this_sack < tp->rx_opt.num_sacks;
> ++this_sack) {
> +                             if (TCP_SKB_CB(skb)->seq ==
> sp[this_sack].end_seq) {
> +                                     rst_seq_match = true;
> +                                     break;
> +                             }
> +                     }
> +             }
> +
> +             if (rst_seq_match)
>                      tcp_reset(sk);
>              else
>                      tcp_send_challenge_ack(sk, skb);
> --
> 2.5.0
>
>
> It looks like you want to seriously relax RFC 5961 ...
>
> Could you have a problem because of the host-wide RFC 5961 rate limit ?
>
> Have you contacted RFC authors ?
>
> If the peer sends the RST, presumably it should answer to the challenge
> ACK right away, since it does not care of the SACK blocks anymore.
>
> A packetdrill test demonstrating the issue would be nice.
>
> Thanks.
>
>
>
>
>
>
> --
> Pau Espin Pedrol | R&D Engineer - External
> pau.espin@tessares.net | +32 487 43 36 50
> Tessares SA | Hybrid Access Solutions
> www.tessares.net
> 6 Rue Louis de Geer, 1348 Louvain-la-Neuve, Belgium
>
> --
>
> ------------------------------
> DISCLAIMER.
> This email and any files transmitted with it are confidential and intended
> solely for the use of the individual or entity to whom they are addressed.
> If you have received this email in error please notify the system manager.
> This message contains confidential information and is intended only for the
> individual named. If you are not the named addressee you should not
> disseminate, distribute or copy this e-mail. Please notify the sender
> immediately by e-mail if you have received this e-mail by mistake and
> delete this e-mail from your system. If you are not the intended recipient
> you are notified that disclosing, copying, distributing or taking any
> action in reliance on the contents of this information is strictly
> prohibited.
>
>
> --------
> Randall Stewart
> rrs@netflix.com
> 803-317-4952
>
>
>
>
>



-- 
Pau Espin Pedrol | R&D Engineer - External
pau.espin@tessares.net | +32 487 43 36 50
Tessares SA | Hybrid Access Solutions
www.tessares.net
6 Rue Louis de Geer, 1348 Louvain-la-Neuve, Belgium

-- 

------------------------------
DISCLAIMER.
This email and any files transmitted with it are confidential and intended 
solely for the use of the individual or entity to whom they are addressed. 
If you have received this email in error please notify the system manager. 
This message contains confidential information and is intended only for the 
individual named. If you are not the named addressee you should not 
disseminate, distribute or copy this e-mail. Please notify the sender 
immediately by e-mail if you have received this e-mail by mistake and 
delete this e-mail from your system. If you are not the intended recipient 
you are notified that disclosing, copying, distributing or taking any 
action in reliance on the contents of this information is strictly 
prohibited.

Re: [PATCH net-next] tcp: accept RST if SEQ matches right edge of SACK block

[Pau Espin]

Hi Randall, I think the last mail you sent contained HTML and was
rejected by the ML. I answer to it in here so it should be available
for everybody now too. Find my comments below.

On Wed, Jun 1, 2016 at 5:44 PM, Randall Stewart <rrs@netflix.com> wrote:
> A few comments in-line...
> On Jun 1, 2016, at 11:19 AM, Pau Espin <pau.espin@tessares.net> wrote:
>
> Hi, first of all, here you can find the packetdrill test I created to
> show up the scenario in which SACK is used and the RST is answered
> with a challenge_ack. You will find below too some answers to some
> previous comments.
>
> 0  socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
> +0 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
> +0 bind(3, {sa_family = AF_INET, sin_port = htons(13000), sin_addr =
> inet_addr("192.168.0.1")}, ...) = 0
> +0 listen(3, 1) = 0
>
> +0 < S 0:0(0) win 500 <mss 1460,sackOK,nop,nop,nop,wscale 7> sock(3)
> +0 > S. 0:0(0) ack 1 win 29200 <mss 1460,nop,nop,sackOK,nop,wscale 7>
> sock(3)
> +0 < . 1:1(0) ack 1 win 500 sock(3)
> +0 accept(3, ..., ...) = 4
>
> // First roundtrip of data client->server (upload)
> +0 < P. 1:1001(1000) win 500 sock(4)
> +0 > . 1:1(0) ack 1001 <...> sock(4)
>
> //In here we would have 2nd packet but it's dropped by net...
> // +0 < P. 1001:2001(1000) win 500 sock(4)
>
> // 3nd packet arrives without prev packet being seen. SACK will be used.
> +0 < P. 2001:3001(1000) win 500 sock(4)
> +0 > . 1:1(0) ack 1001 <nop,nop,sack 2001:3001>
>
> // RST arrives from client, for instance caused by setsockopt(fd,
> SOL_SOCKET, SO_LINGER, ...)
> //Challenge ACK is sent back
> +0 < R. 3001:3001(0) win 500 sock(4)
> +0 > . 1:1(0) ack 1001 <nop,nop,sack 2001:3001>
>
> // To clearly differentiate closing connections around the same time
> due to packetdrill finishing
> +0 `sleep 20`
>
>
> On Wed, Jun 1, 2016 at 12:32 PM, Randall Stewart <rrs@netflix.com> wrote:
>
> Yuchung:
>
> Thanks for adding me in here.. Not sure if Mitesh is still at Cisco or not..
> most of the
> rest of us are *not*. I think Anantha was at NEC last I heard but I have not
> talked to him in a while :-)
>
> Now there is one thing that Pau is missing here is that while in theory
> each ACK can only have 4 Sack blocks, that does not mean the
> SACK sender does not have *many* more SACK blocks on his score
> board. The scoreboard is kept until a time-out occurs and you only
> are sending the latest four. So I think the risk is a bit more than
> only 5 sequence numbers depending of course how one is to
> implement this :)
>
> You can implement it with a limit of 4 blocks, but one could
> also implement it based on the total number of sack blocks
> the receiver has..
>
> So in the scenario tcpdumped below, the client is sending
> RST(1240942836) to the server. Presumably the client’s snd_nxt
> was set to this after say retransmitting the missing piece. The client
> therefore (if its anything like BSD) will use the snd_nxt to send in the
> reset.. which
> of course would generate a challenge ack which as you say is dropped.
>
> One could take the approach below, and allow N blocks but in this
> scenario you are just luckily that the snd_nxt of the client happens to
> be an edge of one of your SACK blocks.
>
> Indeed, as Yuchung noticed too, I was wrong from conceptual point of
> view thinking the SACKs in the packet were all the SACK blocks which
> could be stored. But still, the code patch changing the behavior
> (which improved the situation in my scenario) was checking only
> against selective_acks (struct tcp_sack_block selective_acks[4];), so
> there's no need to check for all the possible SACK blocks in the score
> board to improve the situation imho.
>
> I did a quick check  on some of the related code and as far as I
> understood, when  a new out of order packet is received, it is checked
> (tcp_sack_new_ofo_skb) if it can be added to either rcv_next or any of
> the 4 selective_acks blocks of the connection. If that's not possible,
> a new one is built and added to the front of the list, shifting
> previous ones.
>
> The consequence of that, as far as I understand it, is that most
> recently received out of order SEQ numbers are usually stored in the
> selective_acks structure, which would act then as a cache for recently
> seen SEQ numbers (and next expected seq numbers to receive). As
> Randall pointed out, up to now RST are probably sent using snd_nxt, so
> my guess is that probability of having a match with the selective_acks
> blocks is quite high, as it probed in my test scenario which got a big
> improvement.
>
>
> Well yes the probability is increased but definitely not assured :-)
> Your scenario is specific to a very high loss path. Which is why
> the challenge ack is lost...
>

Correct. But still an improvement in this particular situation with
the only drawback of checking against 5 (4+1) SEQ numbers instead of
1.


>
>
> Possibly another alternative is to change the client where when sending a
> RST
> with a TCB instead of using snd_nxt you could use snd_una. Of course that
> could
> also result in a challenge ACK if the receiver has not yet received a
> ACK that is in flight (that was the whole purpose of the challenge ack). I
> think overall
> you will always have this problem i.e. the sender of the RST may not know
> precisely
> the state of the receiver.
>
>
> Indeed, I guess there will still be same problem in other scenarios.
> On top of that, it seems a bit weird to me to send a RST packet using
> a SEQ number which was already used to send a different packet (that's
> what would happen in this case right?).
>
>
> Why the trick here is you want to RST the connection. You need to use
> a seq number that is valid.. the seq numbers once a RST is being sent
> mean nothing the app will get the same thing.
>
> In fact snd_nxt in the scenario above is *also* a re-used sequence number.
> You
> retransmitted from snd_una for one segment, snd_nxt got left 1 segment up,
> so
> when you sent the RST it would be snd_nxt which was previously a data
> segment being marked with RST.
>
> If you wanted to assure that no other segment had been sent with that
> sequence you would have to put snd_max as the value, but of course
> that *would* return a challenge ack for sure.
>
> The trick here is you are trying to “guess” where the peer is. The only
> thing you know for sure is snd_una. Anything else in flight won’t  reset
> the peer. In fact in your scenario if you had sent snd_una instead of
> snd_nxt it would have worked. If you changed the sender to use
> snd_una then it would be interesting to see if that also gave you
> similar results...
>
>
>
>
> Your fix happens to work since the receiver happens to have the SACK blocks
> in question.. this is fine and if you don’t mind *weakening the security* of
> the
> RST you could do that. I think for stack I am working on for FreeBSD I will
> change
> the stack I am working on to recognize the RST going out and use snd_una.
>
>
> You mean here you are always going to use snd_una or that you are
> going to try to figure out some heuristics to use either snd_next or
> snd_una depending on the scenario?
>
>
> For this scenario I will always use snd_una I am not sure you can reliably
> develop any heuristics to tell you to use snd_una/snd_nxt or some other
> block that as been sent :-)
>
> snd_una is actually the most actuate as to what you know at the time not
> snd_nxt. I am sure using snd_nxt is a hold over from before the RFC got
> implemented.
>

As, as far as I understand now, it could be useful to improve the
situation in the sender too by checking if we recently received SACK
blocks from the receiver and in that case sending the RST using
snd_una instead of snd_nxt because in that case we will almost surely
receive a challenge_ack if we use snd_nxt as SEQ for the RST.

On the other hand, using  always snd_una as SEQ for the RST would
cause other (even more usual) cases to be discarded or answered with a
challenge ACK which are accepted right now. I'm thinking for instance
any case in which you send packets (so in flight packets
sender->receiver) just before the RST is sent (with the snd_una).
Packets are received by the receiver and RCV_NXT is updated and then
you receive the RST which is < than RCV_NXT just updated. Am I missing
something? Please correct me if I'm wrong.

Even in my case (SACK on), I guess it could even happen that due to
fast retransmit the first unacked packet (snd_una) is sent and then
the sender also sends the RST, and the receiver received the data
packet, updates RCV_NXT and the RST packet is discarded.

So, as a summary I think we should still keep the patch in the
receiver to improve the situation in any case, and also do further
work to improve the situation in the sender.

All that being said, it's OK for me to add a sysctl to configure it.
More opinions on whether it's needed or not for the patch are welcome.

> I don’t like the idea of weakening the security, I know as you propose below
> its
> just 5 in 2^^32 instead of 1 in 2^^32 but depending on how you implement it
> you could stretch that to be the number of sack blocks the receiver holds.
>
>
> As stated above, I think it would probably be enough to check for
> those 4-5 seq numbers and not for all of them, which would on top not
> hit on security that much (but still maybe too much for somebody).
>
>
> I would think if you implement it that way I would have an option of sysctl
> to turn
> it off. But I still think snd_una is a better alternative ;-)
>
> R
>
>
>
> R
>
>
> On Jun 1, 2016, at 1:41 AM, Yuchung Cheng <ycheng@google.com> wrote:
>
> On Tue, May 31, 2016 at 9:50 AM, Pau Espin <pau.espin@tessares.net> wrote:
>
>
> Hi, sorry re-sending because I didn't have plain text mode enabled and
> message didn't arrive to the mailing list. Also dropping some mail
> addresses from RFC authors which probed to be unavailable anymore.
>
> The problem is not caused by the host-wide rate limit.
> I analyzed the scenario with tcpdump and I see the challenge ACKs are
> being sent, but due to network conditions sometimes they can be lost.
> On top of that, the RST being sent from the client to the server after
> receiving the challenge ACK can also sometimes be lost. I even found
> the client/router had a problem with iptables setup and was actually
> not sending back RST for incoming TCP packets without an existing
> connection established (should be the one already closed before with
> the first RST sent by the client and which originated the challenge
> ACK).
>
> In my test scenario I'm using MultiPathTCP and I'm recreating (destroy
> with RST, then create with SYN)  the subflow on one of the interfaces
> every aprox 5 seconds while uploading a big file. Due to network
> conditions and/or router stopping the RSTs, after less than 5 min, I
> have more than 32 subflow connections created for that MPTCP
> connection, which is the hardcoded maximum, and from that point I'm
> unable to create/use new subflows until some of them are closed, which
> can take quite a lot of time. After this patch is applied, the number
> of subflows is kind of stable at 4-5 subflows during the whole upload.
> It's still not 2 (one permanent in iface1 and the recreated one in
> iface2) because sometimes due to network problems the packet before
> the RST is lost and thus the RST which arrives is not equal to next
> expected seq number from the right edge of the SACK block. But still,
> it makes the situation quite better, specially from user point of
> view.
>
> Here's an example of the issue without my patch (challenge ACK is sent
> although the RST is in current place). It shows server acknowledging
> data with SACK in first line. Then, on 2nd line, client decides to
> terminate the connexion and uses his next SEQ number available. On 3rd
> line, server answers with the challenge ACK. Then no answer comes from
> that challenge ack and the TCP conn is left opened.
>
> 14202    73.086360    10.0.4.2    443    10.67.15.100    53755    TCP
> 94    [TCP Dup ACK 14192#4] 443→53755 [ACK] Seq=3551806992
> Ack=1240653972 Win=1573120 Len=0 TSval=4022818 TSecr=1106789
> SLE=1240836636 SRE=1240942836 SLE=1240770084 SRE=1240835220
> 14203    73.086363    10.67.15.100    53755    10.0.4.2    443    TCP
> 74    53755→443 [RST, ACK] Seq=1240942836 Ack=3551806992 Win=90240
> Len=0 TSval=1106847 TSecr=4022536
> 14204    73.086368    10.0.4.2    443    10.67.15.100    53755    TCP
> 94    [TCP Dup ACK 14192#5] 443→53755 [ACK] Seq=3551806992
> Ack=1240653972 Win=1573120 Len=0 TSval=4022818 TSecr=1106789
> SLE=1240836636 SRE=1240942836 SLE=1240770084 SRE=1240835220
>
> So, the main point in here is trying to improve the situation to close
> the connections and free resources in some specific cases without
> actually going pre RFC5961 state. That would mean when a RST is
> received, up to 4-5 SEQs are checked to match instead of 1.
>
> I didn't contact the authors of the RFC. I CC them in this e-mail. I
> hope that's the right thing to do in this case and that they don't
> mind it in case they want to follow the topic.
>
> I will have a look at packetdrill to try to reproduce it somehow there.
>
> On Tue, May 31, 2016 at 5:12 PM, Eric Dumazet <eric.dumazet@gmail.com>
> wrote:
>
> On Tue, 2016-05-31 at 13:38 +0200, Pau Espin Pedrol wrote:
>
> RFC 5961 advises to only accept RST packets containing a seq number
> matching the next expected seq number instead of the whole receive
> window in order to avoid spoofing attacks.
>
> However, this situation is not optimal in the case SACK is in use at the
> time the RST is sent. I recently run into a scenario in which packet
> losses were high while uploading data to a server, and userspace was
> willing to frequently terminate connections by sending a RST. In
> this case, the ACK sent on the receiver side is frozen waiting for a lost
> packet retransmission and a SACK block is used to let the client
> continue uploading data. At some point later on, the client sends the
> RST, which matches the next expected seq number of the SACK block on the
> receiver side which is going forward receiving data.
>
> In this scenario, as RFC 5961 defines, the SEQ doesn't match the frozen
> main ACK at receiver side and thus gets dropped and a challenge ACK is
> sent, which gets usually lost due to network conditions. The main
> consequence is that the connection stays alive for a while even if it
> made sense to accept the RST. This can get really bad if lots of
> connections like this one are created in few seconds, allocating all the
> resources of the server easily.
>
> From security point of view: the maximum number of SACK blocks for a TCP
> connection is limited to 4 due to options field maximum length, and that
>
> This is not true. The maximum number of SACK blocks for a TCP "packet"
> is limited to 4. But a TCP connection can keep an arbitrary amount of
> SACK blocks.
>
> means we match at maximum against 5 seq numbers, which should make it
> still difficult for attackers to inject a valid RST message.
>
> This patch was tested in a 3.18 kernel and probed to improve the
> situation in the scenario described above.
>
> Signed-off-by: Pau Espin Pedrol <pau.espin@tessares.net>
> ---
> net/ipv4/tcp_input.c | 18 +++++++++++++++++-
> 1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index d6c8f4cd0..4727dc8 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -5159,6 +5159,7 @@ static bool tcp_validate_incoming(struct sock *sk,
> struct sk_buff *skb,
>                               const struct tcphdr *th, int syn_inerr)
> {
>     struct tcp_sock *tp = tcp_sk(sk);
> +     bool rst_seq_match = false;
>
>     /* RFC1323: H1. Apply PAWS check first. */
>     if (tcp_fast_parse_options(skb, th, tp) && tp->rx_opt.saw_tstamp &&
> @@ -5195,13 +5196,28 @@ static bool tcp_validate_incoming(struct sock *sk,
> struct sk_buff *skb,
>
>     /* Step 2: check RST bit */
>     if (th->rst) {
> -             /* RFC 5961 3.2 :
> +             /* RFC 5961 3.2 (extended to match against SACK too if
> available):
>              * If sequence number exactly matches RCV.NXT, then
>              *     RESET the connection
>              * else
>              *     Send a challenge ACK
>              */
>             if (TCP_SKB_CB(skb)->seq == tp->rcv_nxt)
> +                     rst_seq_match = true;
> +             else if (tcp_is_sack(tp)) {
> +                     int this_sack;
> +                     struct tcp_sack_block *sp = tp->rx_opt.dsack ?
> +                                     tp->duplicate_sack :
> tp->selective_acks;
> +
> +                     for (this_sack = 0; this_sack < tp->rx_opt.num_sacks;
> ++this_sack) {
> +                             if (TCP_SKB_CB(skb)->seq ==
> sp[this_sack].end_seq) {
> +                                     rst_seq_match = true;
> +                                     break;
> +                             }
> +                     }
> +             }
> +
> +             if (rst_seq_match)
>                     tcp_reset(sk);
>             else
>                     tcp_send_challenge_ack(sk, skb);
> --
> 2.5.0
>
>
> It looks like you want to seriously relax RFC 5961 ...
>
> Could you have a problem because of the host-wide RFC 5961 rate limit ?
>
> Have you contacted RFC authors ?
>
> If the peer sends the RST, presumably it should answer to the challenge
> ACK right away, since it does not care of the SACK blocks anymore.
>
> A packetdrill test demonstrating the issue would be nice.
>
> Thanks.
>
>
>
>
>
>
> --
> Pau Espin Pedrol | R&D Engineer - External
> pau.espin@tessares.net | +32 487 43 36 50
> Tessares SA | Hybrid Access Solutions
> www.tessares.net
> 6 Rue Louis de Geer, 1348 Louvain-la-Neuve, Belgium
>
> --
>
> ------------------------------
> DISCLAIMER.
> This email and any files transmitted with it are confidential and intended
> solely for the use of the individual or entity to whom they are addressed.
> If you have received this email in error please notify the system manager.
> This message contains confidential information and is intended only for the
> individual named. If you are not the named addressee you should not
> disseminate, distribute or copy this e-mail. Please notify the sender
> immediately by e-mail if you have received this e-mail by mistake and
> delete this e-mail from your system. If you are not the intended recipient
> you are notified that disclosing, copying, distributing or taking any
> action in reliance on the contents of this information is strictly
> prohibited.
>
>
> --------
> Randall Stewart
> rrs@netflix.com
> 803-317-4952
>
>
>
>
>
>
>
>
> --
> Pau Espin Pedrol | R&D Engineer - External
> pau.espin@tessares.net | +32 487 43 36 50
> Tessares SA | Hybrid Access Solutions
> www.tessares.net
> 6 Rue Louis de Geer, 1348 Louvain-la-Neuve, Belgium
>
> --
>
> ------------------------------
> DISCLAIMER.
> This email and any files transmitted with it are confidential and intended
> solely for the use of the individual or entity to whom they are addressed.
> If you have received this email in error please notify the system manager.
> This message contains confidential information and is intended only for the
> individual named. If you are not the named addressee you should not
> disseminate, distribute or copy this e-mail. Please notify the sender
> immediately by e-mail if you have received this e-mail by mistake and
> delete this e-mail from your system. If you are not the intended recipient
> you are notified that disclosing, copying, distributing or taking any
> action in reliance on the contents of this information is strictly
> prohibited.
>
>
> --------
> Randall Stewart
> rrs@netflix.com
> 803-317-4952
>
>
>
>
>



-- 
Pau Espin Pedrol | R&D Engineer - External
pau.espin@tessares.net | +32 487 43 36 50
Tessares SA | Hybrid Access Solutions
www.tessares.net
6 Rue Louis de Geer, 1348 Louvain-la-Neuve, Belgium

-- 

------------------------------
DISCLAIMER.
This email and any files transmitted with it are confidential and intended 
solely for the use of the individual or entity to whom they are addressed. 
If you have received this email in error please notify the system manager. 
This message contains confidential information and is intended only for the 
individual named. If you are not the named addressee you should not 
disseminate, distribute or copy this e-mail. Please notify the sender 
immediately by e-mail if you have received this e-mail by mistake and 
delete this e-mail from your system. If you are not the intended recipient 
you are notified that disclosing, copying, distributing or taking any 
action in reliance on the contents of this information is strictly 
prohibited.

Re: [PATCH net-next] tcp: accept RST if SEQ matches right edge of SACK block

[Randall Stewart]

Pau:

Hopefully me setting the “plain text” in my Mac-Mail preferences will make this
plain text :-)


>> 
>> 
>> Well yes the probability is increased but definitely not assured :-)
>> Your scenario is specific to a very high loss path. Which is why
>> the challenge ack is lost...
>> 
> 
> Correct. But still an improvement in this particular situation with
> the only drawback of checking against 5 (4+1) SEQ numbers instead of
> 1.
> 
> 
>> 
>> 
>> Possibly another alternative is to change the client where when sending a
>> RST
>> with a TCB instead of using snd_nxt you could use snd_una. Of course that
>> could
>> also result in a challenge ACK if the receiver has not yet received a
>> ACK that is in flight (that was the whole purpose of the challenge ack). I
>> think overall
>> you will always have this problem i.e. the sender of the RST may not know
>> precisely
>> the state of the receiver.
>> 
>> 
>> Indeed, I guess there will still be same problem in other scenarios.
>> On top of that, it seems a bit weird to me to send a RST packet using
>> a SEQ number which was already used to send a different packet (that's
>> what would happen in this case right?).
>> 
>> 
>> Why the trick here is you want to RST the connection. You need to use
>> a seq number that is valid.. the seq numbers once a RST is being sent
>> mean nothing the app will get the same thing.
>> 
>> In fact snd_nxt in the scenario above is *also* a re-used sequence number.
>> You
>> retransmitted from snd_una for one segment, snd_nxt got left 1 segment up,
>> so
>> when you sent the RST it would be snd_nxt which was previously a data
>> segment being marked with RST.
>> 
>> If you wanted to assure that no other segment had been sent with that
>> sequence you would have to put snd_max as the value, but of course
>> that *would* return a challenge ack for sure.
>> 
>> The trick here is you are trying to “guess” where the peer is. The only
>> thing you know for sure is snd_una. Anything else in flight won’t  reset
>> the peer. In fact in your scenario if you had sent snd_una instead of
>> snd_nxt it would have worked. If you changed the sender to use
>> snd_una then it would be interesting to see if that also gave you
>> similar results...
>> 
>> 
>> 
>> 
>> Your fix happens to work since the receiver happens to have the SACK blocks
>> in question.. this is fine and if you don’t mind *weakening the security* of
>> the
>> RST you could do that. I think for stack I am working on for FreeBSD I will
>> change
>> the stack I am working on to recognize the RST going out and use snd_una.
>> 
>> 
>> You mean here you are always going to use snd_una or that you are
>> going to try to figure out some heuristics to use either snd_next or
>> snd_una depending on the scenario?
>> 
>> 
>> For this scenario I will always use snd_una I am not sure you can reliably
>> develop any heuristics to tell you to use snd_una/snd_nxt or some other
>> block that as been sent :-)
>> 
>> snd_una is actually the most actuate as to what you know at the time not
>> snd_nxt. I am sure using snd_nxt is a hold over from before the RFC got
>> implemented.
>> 
> 
> As, as far as I understand now, it could be useful to improve the
> situation in the sender too by checking if we recently received SACK
> blocks from the receiver and in that case sending the RST using
> snd_una instead of snd_nxt because in that case we will almost surely
> receive a challenge_ack if we use snd_nxt as SEQ for the RST.
> 

Checking the scoreboard and using snd_una instead of snd_nxt
might help things. 

> On the other hand, using  always snd_una as SEQ for the RST would
> cause other (even more usual) cases to be discarded or answered with a
> challenge ACK which are accepted right now. I'm thinking for instance
> any case in which you send packets (so in flight packets
> sender->receiver) just before the RST is sent (with the snd_una).
> Packets are received by the receiver and RCV_NXT is updated and then
> you receive the RST which is < than RCV_NXT just updated. Am I missing
> something? Please correct me if I'm wrong.
> 

I think if packets are in flight either way you are taking a gamble on what
you are sending, snd_nxt/snd_una and snd_max. 

If you are idle and send a RST then those should all be the same and
you will win :)

snd_nxt will be questionable if you are in the middle of retransmitting (your case)
since you really don’t know where it is. 

I do like your idea of using the scoreboard to tell if you need to use
snd_una or snd_nxt. In theory if the scoreboard is empty using
snd_nxt should be the equivalent to using snd_max.. but if they
are not equal then you are doing a retransmit and it becomes a crap
shoot what you should use here.

All of this IMO is only relevant if you are having high packet loss. If that
is not the case having to go through a challenge ack to remove a TCB
does not hurt anything.

> Even in my case (SACK on), I guess it could even happen that due to
> fast retransmit the first unacked packet (snd_una) is sent and then
> the sender also sends the RST, and the receiver received the data
> packet, updates RCV_NXT and the RST packet is discarded.

And the challenge ack is sent back.

> 
> So, as a summary I think we should still keep the patch in the
> receiver to improve the situation in any case, and also do further
> work to improve the situation in the sender.
> 
> All that being said, it's OK for me to add a sysctl to configure it.
> More opinions on whether it's needed or not for the patch are welcome.

I can’t say whats best for linux.. since I am really not involved in that
community :-) But I do like the ideas of sysctl’s to control behavior. That
way the sysadmin can cater the system to his/or/her environment :-D

R

> 
>> I don’t like the idea of weakening the security, I know as you propose below
>> its
>> just 5 in 2^^32 instead of 1 in 2^^32 but depending on how you implement it
>> you could stretch that to be the number of sack blocks the receiver holds.
>> 
>> 
>> As stated above, I think it would probably be enough to check for
>> those 4-5 seq numbers and not for all of them, which would on top not
>> hit on security that much (but still maybe too much for somebody).
>> 
>> 
>> I would think if you implement it that way I would have an option of sysctl
>> to turn
>> it off. But I still think snd_una is a better alternative ;-)
>> 
>> R
>> 
>> 
>> 
>> R
>> 
>> 
>> On Jun 1, 2016, at 1:41 AM, Yuchung Cheng <ycheng@google.com> wrote:
>> 
>> On Tue, May 31, 2016 at 9:50 AM, Pau Espin <pau.espin@tessares.net> wrote:
>> 
>> 
>> Hi, sorry re-sending because I didn't have plain text mode enabled and
>> message didn't arrive to the mailing list. Also dropping some mail
>> addresses from RFC authors which probed to be unavailable anymore.
>> 
>> The problem is not caused by the host-wide rate limit.
>> I analyzed the scenario with tcpdump and I see the challenge ACKs are
>> being sent, but due to network conditions sometimes they can be lost.
>> On top of that, the RST being sent from the client to the server after
>> receiving the challenge ACK can also sometimes be lost. I even found
>> the client/router had a problem with iptables setup and was actually
>> not sending back RST for incoming TCP packets without an existing
>> connection established (should be the one already closed before with
>> the first RST sent by the client and which originated the challenge
>> ACK).
>> 
>> In my test scenario I'm using MultiPathTCP and I'm recreating (destroy
>> with RST, then create with SYN)  the subflow on one of the interfaces
>> every aprox 5 seconds while uploading a big file. Due to network
>> conditions and/or router stopping the RSTs, after less than 5 min, I
>> have more than 32 subflow connections created for that MPTCP
>> connection, which is the hardcoded maximum, and from that point I'm
>> unable to create/use new subflows until some of them are closed, which
>> can take quite a lot of time. After this patch is applied, the number
>> of subflows is kind of stable at 4-5 subflows during the whole upload.
>> It's still not 2 (one permanent in iface1 and the recreated one in
>> iface2) because sometimes due to network problems the packet before
>> the RST is lost and thus the RST which arrives is not equal to next
>> expected seq number from the right edge of the SACK block. But still,
>> it makes the situation quite better, specially from user point of
>> view.
>> 
>> Here's an example of the issue without my patch (challenge ACK is sent
>> although the RST is in current place). It shows server acknowledging
>> data with SACK in first line. Then, on 2nd line, client decides to
>> terminate the connexion and uses his next SEQ number available. On 3rd
>> line, server answers with the challenge ACK. Then no answer comes from
>> that challenge ack and the TCP conn is left opened.
>> 
>> 14202    73.086360    10.0.4.2    443    10.67.15.100    53755    TCP
>> 94    [TCP Dup ACK 14192#4] 443→53755 [ACK] Seq=3551806992
>> Ack=1240653972 Win=1573120 Len=0 TSval=4022818 TSecr=1106789
>> SLE=1240836636 SRE=1240942836 SLE=1240770084 SRE=1240835220
>> 14203    73.086363    10.67.15.100    53755    10.0.4.2    443    TCP
>> 74    53755→443 [RST, ACK] Seq=1240942836 Ack=3551806992 Win=90240
>> Len=0 TSval=1106847 TSecr=4022536
>> 14204    73.086368    10.0.4.2    443    10.67.15.100    53755    TCP
>> 94    [TCP Dup ACK 14192#5] 443→53755 [ACK] Seq=3551806992
>> Ack=1240653972 Win=1573120 Len=0 TSval=4022818 TSecr=1106789
>> SLE=1240836636 SRE=1240942836 SLE=1240770084 SRE=1240835220
>> 
>> So, the main point in here is trying to improve the situation to close
>> the connections and free resources in some specific cases without
>> actually going pre RFC5961 state. That would mean when a RST is
>> received, up to 4-5 SEQs are checked to match instead of 1.
>> 
>> I didn't contact the authors of the RFC. I CC them in this e-mail. I
>> hope that's the right thing to do in this case and that they don't
>> mind it in case they want to follow the topic.
>> 
>> I will have a look at packetdrill to try to reproduce it somehow there.
>> 
>> On Tue, May 31, 2016 at 5:12 PM, Eric Dumazet <eric.dumazet@gmail.com>
>> wrote:
>> 
>> On Tue, 2016-05-31 at 13:38 +0200, Pau Espin Pedrol wrote:
>> 
>> RFC 5961 advises to only accept RST packets containing a seq number
>> matching the next expected seq number instead of the whole receive
>> window in order to avoid spoofing attacks.
>> 
>> However, this situation is not optimal in the case SACK is in use at the
>> time the RST is sent. I recently run into a scenario in which packet
>> losses were high while uploading data to a server, and userspace was
>> willing to frequently terminate connections by sending a RST. In
>> this case, the ACK sent on the receiver side is frozen waiting for a lost
>> packet retransmission and a SACK block is used to let the client
>> continue uploading data. At some point later on, the client sends the
>> RST, which matches the next expected seq number of the SACK block on the
>> receiver side which is going forward receiving data.
>> 
>> In this scenario, as RFC 5961 defines, the SEQ doesn't match the frozen
>> main ACK at receiver side and thus gets dropped and a challenge ACK is
>> sent, which gets usually lost due to network conditions. The main
>> consequence is that the connection stays alive for a while even if it
>> made sense to accept the RST. This can get really bad if lots of
>> connections like this one are created in few seconds, allocating all the
>> resources of the server easily.
>> 
>> From security point of view: the maximum number of SACK blocks for a TCP
>> connection is limited to 4 due to options field maximum length, and that
>> 
>> This is not true. The maximum number of SACK blocks for a TCP "packet"
>> is limited to 4. But a TCP connection can keep an arbitrary amount of
>> SACK blocks.
>> 
>> means we match at maximum against 5 seq numbers, which should make it
>> still difficult for attackers to inject a valid RST message.
>> 
>> This patch was tested in a 3.18 kernel and probed to improve the
>> situation in the scenario described above.
>> 
>> Signed-off-by: Pau Espin Pedrol <pau.espin@tessares.net>
>> ---
>> net/ipv4/tcp_input.c | 18 +++++++++++++++++-
>> 1 file changed, 17 insertions(+), 1 deletion(-)
>> 
>> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
>> index d6c8f4cd0..4727dc8 100644
>> --- a/net/ipv4/tcp_input.c
>> +++ b/net/ipv4/tcp_input.c
>> @@ -5159,6 +5159,7 @@ static bool tcp_validate_incoming(struct sock *sk,
>> struct sk_buff *skb,
>>                              const struct tcphdr *th, int syn_inerr)
>> {
>>    struct tcp_sock *tp = tcp_sk(sk);
>> +     bool rst_seq_match = false;
>> 
>>    /* RFC1323: H1. Apply PAWS check first. */
>>    if (tcp_fast_parse_options(skb, th, tp) && tp->rx_opt.saw_tstamp &&
>> @@ -5195,13 +5196,28 @@ static bool tcp_validate_incoming(struct sock *sk,
>> struct sk_buff *skb,
>> 
>>    /* Step 2: check RST bit */
>>    if (th->rst) {
>> -             /* RFC 5961 3.2 :
>> +             /* RFC 5961 3.2 (extended to match against SACK too if
>> available):
>>             * If sequence number exactly matches RCV.NXT, then
>>             *     RESET the connection
>>             * else
>>             *     Send a challenge ACK
>>             */
>>            if (TCP_SKB_CB(skb)->seq == tp->rcv_nxt)
>> +                     rst_seq_match = true;
>> +             else if (tcp_is_sack(tp)) {
>> +                     int this_sack;
>> +                     struct tcp_sack_block *sp = tp->rx_opt.dsack ?
>> +                                     tp->duplicate_sack :
>> tp->selective_acks;
>> +
>> +                     for (this_sack = 0; this_sack < tp->rx_opt.num_sacks;
>> ++this_sack) {
>> +                             if (TCP_SKB_CB(skb)->seq ==
>> sp[this_sack].end_seq) {
>> +                                     rst_seq_match = true;
>> +                                     break;
>> +                             }
>> +                     }
>> +             }
>> +
>> +             if (rst_seq_match)
>>                    tcp_reset(sk);
>>            else
>>                    tcp_send_challenge_ack(sk, skb);
>> --
>> 2.5.0
>> 
>> 
>> It looks like you want to seriously relax RFC 5961 ...
>> 
>> Could you have a problem because of the host-wide RFC 5961 rate limit ?
>> 
>> Have you contacted RFC authors ?
>> 
>> If the peer sends the RST, presumably it should answer to the challenge
>> ACK right away, since it does not care of the SACK blocks anymore.
>> 
>> A packetdrill test demonstrating the issue would be nice.
>> 
>> Thanks.
>> 
>> 
>> 
>> 
>> 
>> 
>> --
>> Pau Espin Pedrol | R&D Engineer - External
>> pau.espin@tessares.net | +32 487 43 36 50
>> Tessares SA | Hybrid Access Solutions
>> www.tessares.net
>> 6 Rue Louis de Geer, 1348 Louvain-la-Neuve, Belgium
>> 
>> --
>> 
>> ------------------------------
>> DISCLAIMER.
>> This email and any files transmitted with it are confidential and intended
>> solely for the use of the individual or entity to whom they are addressed.
>> If you have received this email in error please notify the system manager.
>> This message contains confidential information and is intended only for the
>> individual named. If you are not the named addressee you should not
>> disseminate, distribute or copy this e-mail. Please notify the sender
>> immediately by e-mail if you have received this e-mail by mistake and
>> delete this e-mail from your system. If you are not the intended recipient
>> you are notified that disclosing, copying, distributing or taking any
>> action in reliance on the contents of this information is strictly
>> prohibited.
>> 
>> 
>> --------
>> Randall Stewart
>> rrs@netflix.com
>> 803-317-4952
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> --
>> Pau Espin Pedrol | R&D Engineer - External
>> pau.espin@tessares.net | +32 487 43 36 50
>> Tessares SA | Hybrid Access Solutions
>> www.tessares.net
>> 6 Rue Louis de Geer, 1348 Louvain-la-Neuve, Belgium
>> 
>> --
>> 
>> ------------------------------
>> DISCLAIMER.
>> This email and any files transmitted with it are confidential and intended
>> solely for the use of the individual or entity to whom they are addressed.
>> If you have received this email in error please notify the system manager.
>> This message contains confidential information and is intended only for the
>> individual named. If you are not the named addressee you should not
>> disseminate, distribute or copy this e-mail. Please notify the sender
>> immediately by e-mail if you have received this e-mail by mistake and
>> delete this e-mail from your system. If you are not the intended recipient
>> you are notified that disclosing, copying, distributing or taking any
>> action in reliance on the contents of this information is strictly
>> prohibited.
>> 
>> 
>> --------
>> Randall Stewart
>> rrs@netflix.com
>> 803-317-4952
>> 
>> 
>> 
>> 
>> 
> 
> 
> 
> -- 
> Pau Espin Pedrol | R&D Engineer - External
> pau.espin@tessares.net | +32 487 43 36 50
> Tessares SA | Hybrid Access Solutions
> www.tessares.net
> 6 Rue Louis de Geer, 1348 Louvain-la-Neuve, Belgium
> 
> -- 
> 
> ------------------------------
> DISCLAIMER.
> This email and any files transmitted with it are confidential and intended 
> solely for the use of the individual or entity to whom they are addressed. 
> If you have received this email in error please notify the system manager. 
> This message contains confidential information and is intended only for the 
> individual named. If you are not the named addressee you should not 
> disseminate, distribute or copy this e-mail. Please notify the sender 
> immediately by e-mail if you have received this e-mail by mistake and 
> delete this e-mail from your system. If you are not the intended recipient 
> you are notified that disclosing, copying, distributing or taking any 
> action in reliance on the contents of this information is strictly 
> prohibited.

--------
Randall Stewart
rrs@netflix.com
803-317-4952

Re: [PATCH net-next] tcp: accept RST if SEQ matches right edge of SACK block

[Pau Espin]

On Thu, Jun 2, 2016 at 3:14 PM, Randall Stewart <rrs@netflix.com> wrote:
>
> Pau:
>
> Hopefully me setting the “plain text” in my Mac-Mail preferences will make this
> plain text :-)
>
>
> >>
> >>
> >> Well yes the probability is increased but definitely not assured :-)
> >> Your scenario is specific to a very high loss path. Which is why
> >> the challenge ack is lost...
> >>
> >
> > Correct. But still an improvement in this particular situation with
> > the only drawback of checking against 5 (4+1) SEQ numbers instead of
> > 1.
> >
> >
> >>
> >>
> >> Possibly another alternative is to change the client where when sending a
> >> RST
> >> with a TCB instead of using snd_nxt you could use snd_una. Of course that
> >> could
> >> also result in a challenge ACK if the receiver has not yet received a
> >> ACK that is in flight (that was the whole purpose of the challenge ack). I
> >> think overall
> >> you will always have this problem i.e. the sender of the RST may not know
> >> precisely
> >> the state of the receiver.
> >>
> >>
> >> Indeed, I guess there will still be same problem in other scenarios.
> >> On top of that, it seems a bit weird to me to send a RST packet using
> >> a SEQ number which was already used to send a different packet (that's
> >> what would happen in this case right?).
> >>
> >>
> >> Why the trick here is you want to RST the connection. You need to use
> >> a seq number that is valid.. the seq numbers once a RST is being sent
> >> mean nothing the app will get the same thing.
> >>
> >> In fact snd_nxt in the scenario above is *also* a re-used sequence number.
> >> You
> >> retransmitted from snd_una for one segment, snd_nxt got left 1 segment up,
> >> so
> >> when you sent the RST it would be snd_nxt which was previously a data
> >> segment being marked with RST.
> >>
> >> If you wanted to assure that no other segment had been sent with that
> >> sequence you would have to put snd_max as the value, but of course
> >> that *would* return a challenge ack for sure.
> >>
> >> The trick here is you are trying to “guess” where the peer is. The only
> >> thing you know for sure is snd_una. Anything else in flight won’t  reset
> >> the peer. In fact in your scenario if you had sent snd_una instead of
> >> snd_nxt it would have worked. If you changed the sender to use
> >> snd_una then it would be interesting to see if that also gave you
> >> similar results...
> >>
> >>
> >>
> >>
> >> Your fix happens to work since the receiver happens to have the SACK blocks
> >> in question.. this is fine and if you don’t mind *weakening the security* of
> >> the
> >> RST you could do that. I think for stack I am working on for FreeBSD I will
> >> change
> >> the stack I am working on to recognize the RST going out and use snd_una.
> >>
> >>
> >> You mean here you are always going to use snd_una or that you are
> >> going to try to figure out some heuristics to use either snd_next or
> >> snd_una depending on the scenario?
> >>
> >>
> >> For this scenario I will always use snd_una I am not sure you can reliably
> >> develop any heuristics to tell you to use snd_una/snd_nxt or some other
> >> block that as been sent :-)
> >>
> >> snd_una is actually the most actuate as to what you know at the time not
> >> snd_nxt. I am sure using snd_nxt is a hold over from before the RFC got
> >> implemented.
> >>
> >
> > As, as far as I understand now, it could be useful to improve the
> > situation in the sender too by checking if we recently received SACK
> > blocks from the receiver and in that case sending the RST using
> > snd_una instead of snd_nxt because in that case we will almost surely
> > receive a challenge_ack if we use snd_nxt as SEQ for the RST.
> >
>
> Checking the scoreboard and using snd_una instead of snd_nxt
> might help things.
>
> > On the other hand, using  always snd_una as SEQ for the RST would
> > cause other (even more usual) cases to be discarded or answered with a
> > challenge ACK which are accepted right now. I'm thinking for instance
> > any case in which you send packets (so in flight packets
> > sender->receiver) just before the RST is sent (with the snd_una).
> > Packets are received by the receiver and RCV_NXT is updated and then
> > you receive the RST which is < than RCV_NXT just updated. Am I missing
> > something? Please correct me if I'm wrong.
> >
>
> I think if packets are in flight either way you are taking a gamble on what
> you are sending, snd_nxt/snd_una and snd_max.
>
> If you are idle and send a RST then those should all be the same and
> you will win :)
>
> snd_nxt will be questionable if you are in the middle of retransmitting (your case)
> since you really don’t know where it is.
>
> I do like your idea of using the scoreboard to tell if you need to use
> snd_una or snd_nxt. In theory if the scoreboard is empty using
> snd_nxt should be the equivalent to using snd_max.. but if they
> are not equal then you are doing a retransmit and it becomes a crap
> shoot what you should use here.
>
> All of this IMO is only relevant if you are having high packet loss. If that
> is not the case having to go through a challenge ack to remove a TCB
> does not hurt anything.
>
> > Even in my case (SACK on), I guess it could even happen that due to
> > fast retransmit the first unacked packet (snd_una) is sent and then
> > the sender also sends the RST, and the receiver received the data
> > packet, updates RCV_NXT and the RST packet is discarded.
>
> And the challenge ack is sent back.
>
No, it's not, that why I think so far using snd_una is probably not a
better idea than using snd_nxt. Becasue you still have the same
problems (sometimes the RST is not accepted), plus sometimes, on top
of that, the receiver doesn't send  a challenge_ack or a dupack.

If I understood snd_una correctly, it can happen if sender sends a
packet, and before receiving the ACK for it (which was already
processed by the receiver), it sends a RST.

I use this packetdrill test to emulate the scenario:

0  socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
+0 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
+0 bind(3, {sa_family = AF_INET, sin_port = htons(13000), sin_addr =
inet_addr("192.168.0.1")}, ...) = 0
+0 listen(3, 1) = 0

+0 < S 0:0(0) win 500 <mss 1460,sackOK,nop,nop,nop,wscale 7> sock(3)
+0 > S. 0:0(0) ack 1 win 29200 <mss 1460,nop,nop,sackOK,nop,wscale 7> sock(3)
+0 < . 1:1(0) ack 1 win 500 sock(3)
+0 accept(3, ..., ...) = 4

// First roundtrip of data client->server (upload)
+0 < P. 1:1001(1000) win 500 sock(4)
+0 > . 1:1(0) ack 1001 <...> sock(4)

//In here we assume the case in which 1 packet is sent and then the
RST (using snd_una) is generated before seeing the ACK which is in
flight from receiver->sender:
+0 < P. 1001:2001(1000) win 500 sock(4)
+0 > . 1:1(0) ack 2001 <...>
+0 < R. 1001:1001(0) win 500 sock(4)
// No extra ACK after the RST is generated

// To clearly differentiate closing connections around the same time
due to packetdrill finishing
+0 `sleep 10`

That's basically because it fails in a prior check to the SEQ==RCV_NXT
in the receiver.
The check is actually:
if (!tcp_sequence(tp, TCP_SKB_CB(skb)->seq, TCP_SKB_CB(skb)->end_seq)) {

then, inside the if, as the packet is a RST, it is discarded without
sending any packet back.
The fail inside tcp_sequence is because this condition is true:
"before(end_seq, tp->rcv_wup)"

That's the case in which 1 packets before the RST has been already
acked but it didn't arrive yet to the client when it decided to reset
the connection. I guess it can be quite a common scenario.

Maybe the logic in BSD-alike hosts is different, I don't know.

> >
> > So, as a summary I think we should still keep the patch in the
> > receiver to improve the situation in any case, and also do further
> > work to improve the situation in the sender.
> >
> > All that being said, it's OK for me to add a sysctl to configure it.
> > More opinions on whether it's needed or not for the patch are welcome.
>
> I can’t say whats best for linux.. since I am really not involved in that
> community :-) But I do like the ideas of sysctl’s to control behavior. That
> way the sysadmin can cater the system to his/or/her environment :-D
>
> R
>
> >
> >> I don’t like the idea of weakening the security, I know as you propose below
> >> its
> >> just 5 in 2^^32 instead of 1 in 2^^32 but depending on how you implement it
> >> you could stretch that to be the number of sack blocks the receiver holds.
> >>
> >>
> >> As stated above, I think it would probably be enough to check for
> >> those 4-5 seq numbers and not for all of them, which would on top not
> >> hit on security that much (but still maybe too much for somebody).
> >>
> >>
> >> I would think if you implement it that way I would have an option of sysctl
> >> to turn
> >> it off. But I still think snd_una is a better alternative ;-)
> >>
> >> R
> >>
> >>
> >>
> >> R
> >>
> >>
> >> On Jun 1, 2016, at 1:41 AM, Yuchung Cheng <ycheng@google.com> wrote:
> >>
> >> On Tue, May 31, 2016 at 9:50 AM, Pau Espin <pau.espin@tessares.net> wrote:
> >>
> >>
> >> Hi, sorry re-sending because I didn't have plain text mode enabled and
> >> message didn't arrive to the mailing list. Also dropping some mail
> >> addresses from RFC authors which probed to be unavailable anymore.
> >>
> >> The problem is not caused by the host-wide rate limit.
> >> I analyzed the scenario with tcpdump and I see the challenge ACKs are
> >> being sent, but due to network conditions sometimes they can be lost.
> >> On top of that, the RST being sent from the client to the server after
> >> receiving the challenge ACK can also sometimes be lost. I even found
> >> the client/router had a problem with iptables setup and was actually
> >> not sending back RST for incoming TCP packets without an existing
> >> connection established (should be the one already closed before with
> >> the first RST sent by the client and which originated the challenge
> >> ACK).
> >>
> >> In my test scenario I'm using MultiPathTCP and I'm recreating (destroy
> >> with RST, then create with SYN)  the subflow on one of the interfaces
> >> every aprox 5 seconds while uploading a big file. Due to network
> >> conditions and/or router stopping the RSTs, after less than 5 min, I
> >> have more than 32 subflow connections created for that MPTCP
> >> connection, which is the hardcoded maximum, and from that point I'm
> >> unable to create/use new subflows until some of them are closed, which
> >> can take quite a lot of time. After this patch is applied, the number
> >> of subflows is kind of stable at 4-5 subflows during the whole upload.
> >> It's still not 2 (one permanent in iface1 and the recreated one in
> >> iface2) because sometimes due to network problems the packet before
> >> the RST is lost and thus the RST which arrives is not equal to next
> >> expected seq number from the right edge of the SACK block. But still,
> >> it makes the situation quite better, specially from user point of
> >> view.
> >>
> >> Here's an example of the issue without my patch (challenge ACK is sent
> >> although the RST is in current place). It shows server acknowledging
> >> data with SACK in first line. Then, on 2nd line, client decides to
> >> terminate the connexion and uses his next SEQ number available. On 3rd
> >> line, server answers with the challenge ACK. Then no answer comes from
> >> that challenge ack and the TCP conn is left opened.
> >>
> >> 14202    73.086360    10.0.4.2    443    10.67.15.100    53755    TCP
> >> 94    [TCP Dup ACK 14192#4] 443→53755 [ACK] Seq=3551806992
> >> Ack=1240653972 Win=1573120 Len=0 TSval=4022818 TSecr=1106789
> >> SLE=1240836636 SRE=1240942836 SLE=1240770084 SRE=1240835220
> >> 14203    73.086363    10.67.15.100    53755    10.0.4.2    443    TCP
> >> 74    53755→443 [RST, ACK] Seq=1240942836 Ack=3551806992 Win=90240
> >> Len=0 TSval=1106847 TSecr=4022536
> >> 14204    73.086368    10.0.4.2    443    10.67.15.100    53755    TCP
> >> 94    [TCP Dup ACK 14192#5] 443→53755 [ACK] Seq=3551806992
> >> Ack=1240653972 Win=1573120 Len=0 TSval=4022818 TSecr=1106789
> >> SLE=1240836636 SRE=1240942836 SLE=1240770084 SRE=1240835220
> >>
> >> So, the main point in here is trying to improve the situation to close
> >> the connections and free resources in some specific cases without
> >> actually going pre RFC5961 state. That would mean when a RST is
> >> received, up to 4-5 SEQs are checked to match instead of 1.
> >>
> >> I didn't contact the authors of the RFC. I CC them in this e-mail. I
> >> hope that's the right thing to do in this case and that they don't
> >> mind it in case they want to follow the topic.
> >>
> >> I will have a look at packetdrill to try to reproduce it somehow there.
> >>
> >> On Tue, May 31, 2016 at 5:12 PM, Eric Dumazet <eric.dumazet@gmail.com>
> >> wrote:
> >>
> >> On Tue, 2016-05-31 at 13:38 +0200, Pau Espin Pedrol wrote:
> >>
> >> RFC 5961 advises to only accept RST packets containing a seq number
> >> matching the next expected seq number instead of the whole receive
> >> window in order to avoid spoofing attacks.
> >>
> >> However, this situation is not optimal in the case SACK is in use at the
> >> time the RST is sent. I recently run into a scenario in which packet
> >> losses were high while uploading data to a server, and userspace was
> >> willing to frequently terminate connections by sending a RST. In
> >> this case, the ACK sent on the receiver side is frozen waiting for a lost
> >> packet retransmission and a SACK block is used to let the client
> >> continue uploading data. At some point later on, the client sends the
> >> RST, which matches the next expected seq number of the SACK block on the
> >> receiver side which is going forward receiving data.
> >>
> >> In this scenario, as RFC 5961 defines, the SEQ doesn't match the frozen
> >> main ACK at receiver side and thus gets dropped and a challenge ACK is
> >> sent, which gets usually lost due to network conditions. The main
> >> consequence is that the connection stays alive for a while even if it
> >> made sense to accept the RST. This can get really bad if lots of
> >> connections like this one are created in few seconds, allocating all the
> >> resources of the server easily.
> >>
> >> From security point of view: the maximum number of SACK blocks for a TCP
> >> connection is limited to 4 due to options field maximum length, and that
> >>
> >> This is not true. The maximum number of SACK blocks for a TCP "packet"
> >> is limited to 4. But a TCP connection can keep an arbitrary amount of
> >> SACK blocks.
> >>
> >> means we match at maximum against 5 seq numbers, which should make it
> >> still difficult for attackers to inject a valid RST message.
> >>
> >> This patch was tested in a 3.18 kernel and probed to improve the
> >> situation in the scenario described above.
> >>
> >> Signed-off-by: Pau Espin Pedrol <pau.espin@tessares.net>
> >> ---
> >> net/ipv4/tcp_input.c | 18 +++++++++++++++++-
> >> 1 file changed, 17 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> >> index d6c8f4cd0..4727dc8 100644
> >> --- a/net/ipv4/tcp_input.c
> >> +++ b/net/ipv4/tcp_input.c
> >> @@ -5159,6 +5159,7 @@ static bool tcp_validate_incoming(struct sock *sk,
> >> struct sk_buff *skb,
> >>                              const struct tcphdr *th, int syn_inerr)
> >> {
> >>    struct tcp_sock *tp = tcp_sk(sk);
> >> +     bool rst_seq_match = false;
> >>
> >>    /* RFC1323: H1. Apply PAWS check first. */
> >>    if (tcp_fast_parse_options(skb, th, tp) && tp->rx_opt.saw_tstamp &&
> >> @@ -5195,13 +5196,28 @@ static bool tcp_validate_incoming(struct sock *sk,
> >> struct sk_buff *skb,
> >>
> >>    /* Step 2: check RST bit */
> >>    if (th->rst) {
> >> -             /* RFC 5961 3.2 :
> >> +             /* RFC 5961 3.2 (extended to match against SACK too if
> >> available):
> >>             * If sequence number exactly matches RCV.NXT, then
> >>             *     RESET the connection
> >>             * else
> >>             *     Send a challenge ACK
> >>             */
> >>            if (TCP_SKB_CB(skb)->seq == tp->rcv_nxt)
> >> +                     rst_seq_match = true;
> >> +             else if (tcp_is_sack(tp)) {
> >> +                     int this_sack;
> >> +                     struct tcp_sack_block *sp = tp->rx_opt.dsack ?
> >> +                                     tp->duplicate_sack :
> >> tp->selective_acks;
> >> +
> >> +                     for (this_sack = 0; this_sack < tp->rx_opt.num_sacks;
> >> ++this_sack) {
> >> +                             if (TCP_SKB_CB(skb)->seq ==
> >> sp[this_sack].end_seq) {
> >> +                                     rst_seq_match = true;
> >> +                                     break;
> >> +                             }
> >> +                     }
> >> +             }
> >> +
> >> +             if (rst_seq_match)
> >>                    tcp_reset(sk);
> >>            else
> >>                    tcp_send_challenge_ack(sk, skb);
> >> --
> >> 2.5.0
> >>
> >>
> >> It looks like you want to seriously relax RFC 5961 ...
> >>
> >> Could you have a problem because of the host-wide RFC 5961 rate limit ?
> >>
> >> Have you contacted RFC authors ?
> >>
> >> If the peer sends the RST, presumably it should answer to the challenge
> >> ACK right away, since it does not care of the SACK blocks anymore.
> >>
> >> A packetdrill test demonstrating the issue would be nice.
> >>
> >> Thanks.
> >>
> >>
> >>
> >>
> >>
> >>
> >> --
> >> Pau Espin Pedrol | R&D Engineer - External
> >> pau.espin@tessares.net | +32 487 43 36 50
> >> Tessares SA | Hybrid Access Solutions
> >> www.tessares.net
> >> 6 Rue Louis de Geer, 1348 Louvain-la-Neuve, Belgium
> >>
> >> --
> >>
> >> ------------------------------
> >> DISCLAIMER.
> >> This email and any files transmitted with it are confidential and intended
> >> solely for the use of the individual or entity to whom they are addressed.
> >> If you have received this email in error please notify the system manager.
> >> This message contains confidential information and is intended only for the
> >> individual named. If you are not the named addressee you should not
> >> disseminate, distribute or copy this e-mail. Please notify the sender
> >> immediately by e-mail if you have received this e-mail by mistake and
> >> delete this e-mail from your system. If you are not the intended recipient
> >> you are notified that disclosing, copying, distributing or taking any
> >> action in reliance on the contents of this information is strictly
> >> prohibited.
> >>
> >>
> >> --------
> >> Randall Stewart
> >> rrs@netflix.com
> >> 803-317-4952
> >>
> >>
> >>
> >>
> >>
> >>
> >>
> >>
> >> --
> >> Pau Espin Pedrol | R&D Engineer - External
> >> pau.espin@tessares.net | +32 487 43 36 50
> >> Tessares SA | Hybrid Access Solutions
> >> www.tessares.net
> >> 6 Rue Louis de Geer, 1348 Louvain-la-Neuve, Belgium
> >>
> >> --
> >>
> >> ------------------------------
> >> DISCLAIMER.
> >> This email and any files transmitted with it are confidential and intended
> >> solely for the use of the individual or entity to whom they are addressed.
> >> If you have received this email in error please notify the system manager.
> >> This message contains confidential information and is intended only for the
> >> individual named. If you are not the named addressee you should not
> >> disseminate, distribute or copy this e-mail. Please notify the sender
> >> immediately by e-mail if you have received this e-mail by mistake and
> >> delete this e-mail from your system. If you are not the intended recipient
> >> you are notified that disclosing, copying, distributing or taking any
> >> action in reliance on the contents of this information is strictly
> >> prohibited.
> >>
> >>
> >> --------
> >> Randall Stewart
> >> rrs@netflix.com
> >> 803-317-4952
> >>
> >>
> >>
> >>
> >>
> >
> >
> >
> > --
> > Pau Espin Pedrol | R&D Engineer - External
> > pau.espin@tessares.net | +32 487 43 36 50
> > Tessares SA | Hybrid Access Solutions
> > www.tessares.net
> > 6 Rue Louis de Geer, 1348 Louvain-la-Neuve, Belgium
> >
> > --
> >
> > ------------------------------
> > DISCLAIMER.
> > This email and any files transmitted with it are confidential and intended
> > solely for the use of the individual or entity to whom they are addressed.
> > If you have received this email in error please notify the system manager.
> > This message contains confidential information and is intended only for the
> > individual named. If you are not the named addressee you should not
> > disseminate, distribute or copy this e-mail. Please notify the sender
> > immediately by e-mail if you have received this e-mail by mistake and
> > delete this e-mail from your system. If you are not the intended recipient
> > you are notified that disclosing, copying, distributing or taking any
> > action in reliance on the contents of this information is strictly
> > prohibited.
>
> --------
> Randall Stewart
> rrs@netflix.com
> 803-317-4952
>
>
>
>
>



-- 
Pau Espin Pedrol | R&D Engineer - External
pau.espin@tessares.net | +32 487 43 36 50
Tessares SA | Hybrid Access Solutions
www.tessares.net
6 Rue Louis de Geer, 1348 Louvain-la-Neuve, Belgium

-- 

------------------------------
DISCLAIMER.
This email and any files transmitted with it are confidential and intended 
solely for the use of the individual or entity to whom they are addressed. 
If you have received this email in error please notify the system manager. 
This message contains confidential information and is intended only for the 
individual named. If you are not the named addressee you should not 
disseminate, distribute or copy this e-mail. Please notify the sender 
immediately by e-mail if you have received this e-mail by mistake and 
delete this e-mail from your system. If you are not the intended recipient 
you are notified that disclosing, copying, distributing or taking any 
action in reliance on the contents of this information is strictly 
prohibited.

Re: [PATCH net-next] tcp: accept RST if SEQ matches right edge of SACK block

[Eric Dumazet]

On Tue, 2016-05-31 at 13:38 +0200, Pau Espin Pedrol wrote:
> RFC 5961 advises to only accept RST packets containing a seq number
> matching the next expected seq number instead of the whole receive
> window in order to avoid spoofing attacks.
> 
> However, this situation is not optimal in the case SACK is in use at the
> time the RST is sent. I recently run into a scenario in which packet
> losses were high while uploading data to a server, and userspace was
> willing to frequently terminate connections by sending a RST. In
> this case, the ACK sent on the receiver side is frozen waiting for a lost
> packet retransmission and a SACK block is used to let the client
> continue uploading data. At some point later on, the client sends the
> RST, which matches the next expected seq number of the SACK block on the
> receiver side which is going forward receiving data.
> 
> In this scenario, as RFC 5961 defines, the SEQ doesn't match the frozen
> main ACK at receiver side and thus gets dropped and a challenge ACK is
> sent, which gets usually lost due to network conditions. The main
> consequence is that the connection stays alive for a while even if it
> made sense to accept the RST. This can get really bad if lots of
> connections like this one are created in few seconds, allocating all the
> resources of the server easily.
> 
> From security point of view: the maximum number of SACK blocks for a TCP
> connection is limited to 4 due to options field maximum length, and that
> means we match at maximum against 5 seq numbers, which should make it
> still difficult for attackers to inject a valid RST message.
> 
> This patch was tested in a 3.18 kernel and probed to improve the
> situation in the scenario described above.
> 
> Signed-off-by: Pau Espin Pedrol <pau.espin@tessares.net>
> ---
>  net/ipv4/tcp_input.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index d6c8f4cd0..4727dc8 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -5159,6 +5159,7 @@ static bool tcp_validate_incoming(struct sock *sk, struct sk_buff *skb,
>  				  const struct tcphdr *th, int syn_inerr)
>  {
>  	struct tcp_sock *tp = tcp_sk(sk);
> +	bool rst_seq_match = false;
>  
>  	/* RFC1323: H1. Apply PAWS check first. */
>  	if (tcp_fast_parse_options(skb, th, tp) && tp->rx_opt.saw_tstamp &&
> @@ -5195,13 +5196,28 @@ static bool tcp_validate_incoming(struct sock *sk, struct sk_buff *skb,
>  
>  	/* Step 2: check RST bit */
>  	if (th->rst) {
> -		/* RFC 5961 3.2 :
> +		/* RFC 5961 3.2 (extended to match against SACK too if available):
>  		 * If sequence number exactly matches RCV.NXT, then
>  		 *     RESET the connection
>  		 * else
>  		 *     Send a challenge ACK
>  		 */
>  		if (TCP_SKB_CB(skb)->seq == tp->rcv_nxt)
> +			rst_seq_match = true;
> +		else if (tcp_is_sack(tp)) {
> +			int this_sack;
> +			struct tcp_sack_block *sp = tp->rx_opt.dsack ?
> +					tp->duplicate_sack : tp->selective_acks;
> +

Please reorder your variables :
    long variable name foooooooooo;
    short var bar;


It not clear why we should use duplicate_sack[0] at all ?

dsack should be 0 at this point anyway ?

> +			for (this_sack = 0; this_sack < tp->rx_opt.num_sacks; ++this_sack) {
> +				if (TCP_SKB_CB(skb)->seq == sp[this_sack].end_seq) {
> +					rst_seq_match = true;
> +					break;
> +				}
> +			}
> +		}
> +
> +		if (rst_seq_match)
>  			tcp_reset(sk);
>  		else
>  			tcp_send_challenge_ack(sk, skb);
> -- 
> 2.5.0
> 
> 

Re: [PATCH net-next] tcp: accept RST if SEQ matches right edge of SACK block

[Pau Espin]

I will provide a new patch as soon as I find some time to do it.

Indeed, I think there is no need to check duplicate_sack, I will remove it.

I will also update the description.

Shall I also add a sysctl to disable this patch and still be able to
stick strictly to RFC 5961 3.2 for the checks?

On Wed, Jun 1, 2016 at 5:48 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Tue, 2016-05-31 at 13:38 +0200, Pau Espin Pedrol wrote:
>> RFC 5961 advises to only accept RST packets containing a seq number
>> matching the next expected seq number instead of the whole receive
>> window in order to avoid spoofing attacks.
>>
>> However, this situation is not optimal in the case SACK is in use at the
>> time the RST is sent. I recently run into a scenario in which packet
>> losses were high while uploading data to a server, and userspace was
>> willing to frequently terminate connections by sending a RST. In
>> this case, the ACK sent on the receiver side is frozen waiting for a lost
>> packet retransmission and a SACK block is used to let the client
>> continue uploading data. At some point later on, the client sends the
>> RST, which matches the next expected seq number of the SACK block on the
>> receiver side which is going forward receiving data.
>>
>> In this scenario, as RFC 5961 defines, the SEQ doesn't match the frozen
>> main ACK at receiver side and thus gets dropped and a challenge ACK is
>> sent, which gets usually lost due to network conditions. The main
>> consequence is that the connection stays alive for a while even if it
>> made sense to accept the RST. This can get really bad if lots of
>> connections like this one are created in few seconds, allocating all the
>> resources of the server easily.
>>
>> From security point of view: the maximum number of SACK blocks for a TCP
>> connection is limited to 4 due to options field maximum length, and that
>> means we match at maximum against 5 seq numbers, which should make it
>> still difficult for attackers to inject a valid RST message.
>>
>> This patch was tested in a 3.18 kernel and probed to improve the
>> situation in the scenario described above.
>>
>> Signed-off-by: Pau Espin Pedrol <pau.espin@tessares.net>
>> ---
>>  net/ipv4/tcp_input.c | 18 +++++++++++++++++-
>>  1 file changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
>> index d6c8f4cd0..4727dc8 100644
>> --- a/net/ipv4/tcp_input.c
>> +++ b/net/ipv4/tcp_input.c
>> @@ -5159,6 +5159,7 @@ static bool tcp_validate_incoming(struct sock *sk, struct sk_buff *skb,
>>                                 const struct tcphdr *th, int syn_inerr)
>>  {
>>       struct tcp_sock *tp = tcp_sk(sk);
>> +     bool rst_seq_match = false;
>>
>>       /* RFC1323: H1. Apply PAWS check first. */
>>       if (tcp_fast_parse_options(skb, th, tp) && tp->rx_opt.saw_tstamp &&
>> @@ -5195,13 +5196,28 @@ static bool tcp_validate_incoming(struct sock *sk, struct sk_buff *skb,
>>
>>       /* Step 2: check RST bit */
>>       if (th->rst) {
>> -             /* RFC 5961 3.2 :
>> +             /* RFC 5961 3.2 (extended to match against SACK too if available):
>>                * If sequence number exactly matches RCV.NXT, then
>>                *     RESET the connection
>>                * else
>>                *     Send a challenge ACK
>>                */
>>               if (TCP_SKB_CB(skb)->seq == tp->rcv_nxt)
>> +                     rst_seq_match = true;
>> +             else if (tcp_is_sack(tp)) {
>> +                     int this_sack;
>> +                     struct tcp_sack_block *sp = tp->rx_opt.dsack ?
>> +                                     tp->duplicate_sack : tp->selective_acks;
>> +
>
> Please reorder your variables :
>     long variable name foooooooooo;
>     short var bar;
>
>
> It not clear why we should use duplicate_sack[0] at all ?
>
> dsack should be 0 at this point anyway ?
>
>> +                     for (this_sack = 0; this_sack < tp->rx_opt.num_sacks; ++this_sack) {
>> +                             if (TCP_SKB_CB(skb)->seq == sp[this_sack].end_seq) {
>> +                                     rst_seq_match = true;
>> +                                     break;
>> +                             }
>> +                     }
>> +             }
>> +
>> +             if (rst_seq_match)
>>                       tcp_reset(sk);
>>               else
>>                       tcp_send_challenge_ack(sk, skb);
>> --
>> 2.5.0
>>
>>
>
>



-- 
Pau Espin Pedrol | R&D Engineer - External
pau.espin@tessares.net | +32 487 43 36 50
Tessares SA | Hybrid Access Solutions
www.tessares.net
6 Rue Louis de Geer, 1348 Louvain-la-Neuve, Belgium

-- 

------------------------------
DISCLAIMER.
This email and any files transmitted with it are confidential and intended 
solely for the use of the individual or entity to whom they are addressed. 
If you have received this email in error please notify the system manager. 
This message contains confidential information and is intended only for the 
individual named. If you are not the named addressee you should not 
disseminate, distribute or copy this e-mail. Please notify the sender 
immediately by e-mail if you have received this e-mail by mistake and 
delete this e-mail from your system. If you are not the intended recipient 
you are notified that disclosing, copying, distributing or taking any 
action in reliance on the contents of this information is strictly 
prohibited.

Re: [PATCH net-next] tcp: accept RST if SEQ matches right edge of SACK block

[Eric Dumazet]

On Fri, 2016-06-03 at 14:16 +0200, Pau Espin wrote:
> I will provide a new patch as soon as I find some time to do it.
> 
> Indeed, I think there is no need to check duplicate_sack, I will remove it.
> 
> I will also update the description.
> 
> Shall I also add a sysctl to disable this patch and still be able to
> stick strictly to RFC 5961 3.2 for the checks?

I have no strict opinion on this.

It seems to me that checking at most 4 right edges (at least in current
linux implementation) is not adding a huge risk, and allows for better
interoperability.

I vote for no extra sysctl.

Re: [PATCH net-next] tcp: accept RST if SEQ matches right edge of SACK block

[Neal Cardwell]

On Fri, Jun 3, 2016 at 11:13 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> I have no strict opinion on this.
>
> It seems to me that checking at most 4 right edges (at least in current
> linux implementation) is not adding a huge risk, and allows for better
> interoperability.
>
> I vote for no extra sysctl.

I vote for no extra sysctl as well.

But I would also vote to tighten up the proposed logic slightly, and
only check the seq of the incoming RST against the right edge of the
*right-most* SACK block. That is, the code could loop through the
tp->selective_acks to find the right-most of the right edges of the
SACK blocks (the end_seq that has no other end_seq after() it). AFAICT
it makes sense to expect that a legitimate incoming RST might match
rcv_nxt, or might match the right-most edge of the right-most SACK.
But allowing a RST to match a sequence of some SACK in the middle of
the sequence range would seem to only increase the attack surface for
RST attacks.

neal

Re: [PATCH net-next] tcp: accept RST if SEQ matches right edge of SACK block

[Pau Espin]

Hi Neal,
Sorry I saw your email just after sending the second version of the patch.
Indeed, it could make sense to do it as you say. I'll try to do some
tests in my environment and send a new version with those changes if I
don't find any problem with it.

On Fri, Jun 3, 2016 at 5:45 PM, Neal Cardwell <ncardwell@google.com> wrote:
> On Fri, Jun 3, 2016 at 11:13 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> I have no strict opinion on this.
>>
>> It seems to me that checking at most 4 right edges (at least in current
>> linux implementation) is not adding a huge risk, and allows for better
>> interoperability.
>>
>> I vote for no extra sysctl.
>
> I vote for no extra sysctl as well.
>
> But I would also vote to tighten up the proposed logic slightly, and
> only check the seq of the incoming RST against the right edge of the
> *right-most* SACK block. That is, the code could loop through the
> tp->selective_acks to find the right-most of the right edges of the
> SACK blocks (the end_seq that has no other end_seq after() it). AFAICT
> it makes sense to expect that a legitimate incoming RST might match
> rcv_nxt, or might match the right-most edge of the right-most SACK.
> But allowing a RST to match a sequence of some SACK in the middle of
> the sequence range would seem to only increase the attack surface for
> RST attacks.
>
> neal



-- 
Pau Espin Pedrol | R&D Engineer - External
pau.espin@tessares.net | +32 487 43 36 50
Tessares SA | Hybrid Access Solutions
www.tessares.net
6 Rue Louis de Geer, 1348 Louvain-la-Neuve, Belgium

-- 

------------------------------
DISCLAIMER.
This email and any files transmitted with it are confidential and intended 
solely for the use of the individual or entity to whom they are addressed. 
If you have received this email in error please notify the system manager. 
This message contains confidential information and is intended only for the 
individual named. If you are not the named addressee you should not 
disseminate, distribute or copy this e-mail. Please notify the sender 
immediately by e-mail if you have received this e-mail by mistake and 
delete this e-mail from your system. If you are not the intended recipient 
you are notified that disclosing, copying, distributing or taking any 
action in reliance on the contents of this information is strictly 
prohibited.

Re: [PATCH net-next] tcp: accept RST if SEQ matches right edge of SACK block

[Eric Dumazet]

On Fri, 2016-06-03 at 11:45 -0400, Neal Cardwell wrote:
> On Fri, Jun 3, 2016 at 11:13 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > I have no strict opinion on this.
> >
> > It seems to me that checking at most 4 right edges (at least in current
> > linux implementation) is not adding a huge risk, and allows for better
> > interoperability.
> >
> > I vote for no extra sysctl.
> 
> I vote for no extra sysctl as well.
> 
> But I would also vote to tighten up the proposed logic slightly, and
> only check the seq of the incoming RST against the right edge of the
> *right-most* SACK block. That is, the code could loop through the
> tp->selective_acks to find the right-most of the right edges of the
> SACK blocks (the end_seq that has no other end_seq after() it). AFAICT
> it makes sense to expect that a legitimate incoming RST might match
> rcv_nxt, or might match the right-most edge of the right-most SACK.
> But allowing a RST to match a sequence of some SACK in the middle of
> the sequence range would seem to only increase the attack surface for
> RST attacks.

Well, the most recent info would be in [0], no need to iterate, right ?

So only look at the first sack block in the array, even if we have 3 or
4 blocks there.

Re: [PATCH net-next] tcp: accept RST if SEQ matches right edge of SACK block

[Neal Cardwell]

On Fri, Jun 3, 2016 at 12:24 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Fri, 2016-06-03 at 11:45 -0400, Neal Cardwell wrote:
>> But I would also vote to tighten up the proposed logic slightly, and
>> only check the seq of the incoming RST against the right edge of the
>> *right-most* SACK block. That is, the code could loop through the
>> tp->selective_acks to find the right-most of the right edges of the
>> SACK blocks (the end_seq that has no other end_seq after() it). AFAICT
>> it makes sense to expect that a legitimate incoming RST might match
>> rcv_nxt, or might match the right-most edge of the right-most SACK.
>> But allowing a RST to match a sequence of some SACK in the middle of
>> the sequence range would seem to only increase the attack surface for
>> RST attacks.
>
> Well, the most recent info would be in [0], no need to iterate, right ?
>
> So only look at the first sack block in the array, even if we have 3 or
> 4 blocks there.

Yes, good point. It should only need to check the first SACK block in
the selective_acks  array.

neal

Re: [PATCH net-next] tcp: accept RST if SEQ matches right edge of SACK block

[Neal Cardwell]

On Fri, Jun 3, 2016 at 3:44 PM, Neal Cardwell <ncardwell@google.com> wrote:
> On Fri, Jun 3, 2016 at 12:24 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> On Fri, 2016-06-03 at 11:45 -0400, Neal Cardwell wrote:
>>> But I would also vote to tighten up the proposed logic slightly, and
>>> only check the seq of the incoming RST against the right edge of the
>>> *right-most* SACK block. That is, the code could loop through the
>>> tp->selective_acks to find the right-most of the right edges of the
>>> SACK blocks (the end_seq that has no other end_seq after() it). AFAICT
>>> it makes sense to expect that a legitimate incoming RST might match
>>> rcv_nxt, or might match the right-most edge of the right-most SACK.
>>> But allowing a RST to match a sequence of some SACK in the middle of
>>> the sequence range would seem to only increase the attack surface for
>>> RST attacks.
>>
>> Well, the most recent info would be in [0], no need to iterate, right ?
>>
>> So only look at the first sack block in the array, even if we have 3 or
>> 4 blocks there.
>
> Yes, good point. It should only need to check the first SACK block in
> the selective_acks  array.

Well, hmm. The most recent SACK block may not be the right-most one,
if there is reordering or packets are retransmitted. So AFAICT if we
wanted to try hard to just use the right-most SACK block we'd need to
check all the blocks.

But just checking the first SACK block seems like a reasonable
trade-off in terms of simplicity.

I don't feel strongly either way.

neal

[PATCH net-next] tcp: accept RST if SEQ matches right edge of right-most SACK block

[Pau Espin Pedrol]

RFC 5961 advises to only accept RST packets containing a seq number
matching the next expected seq number instead of the whole receive
window in order to avoid spoofing attacks.

However, this situation is not optimal in the case SACK is in use at the
time the RST is sent. I recently run into a scenario in which packet
losses were high while uploading data to a server, and userspace was
willing to frequently terminate connections by sending a RST. In
this case, the ACK sent on the receiver side (rcv_nxt) is frozen waiting
for a lost packet retransmission and SACK blocks are used to let the
client continue uploading data. At some point later on, the client sends
the RST (snd_nxt), which matches the next expected seq number of the
right-most SACK block on the receiver side which is going forward
receiving data.

In this scenario, as RFC 5961 defines, the RST SEQ doesn't match the
frozen main ACK at receiver side and thus gets dropped and a challenge
ACK is sent, which gets usually lost due to network conditions. The main
consequence is that the connection stays alive for a while even if it
made sense to accept the RST. This can get really bad if lots of
connections like this one are created in few seconds, allocating all the
resources of the server easily.

For security reasons, not all SACK blocks are checked (there could be a
big amount of SACK blocks => acceptable SEQ numbers). Furthermore, it
wouldn't make sense to check for RST in blocks other than the right-most
received one because the sender is not expected to be sending new data
after the RST. For simplicity, only up to the 4 most recently updated
SACK blocks (selective_acks[4] field) are compared to find the
right-most block, as usually those are the ones with bigger probability
to contain it.

This patch was tested in a 3.18 kernel and probed to improve the
situation in the scenario described above.

Signed-off-by: Pau Espin Pedrol <pau.espin@tessares.net>
---
 net/ipv4/tcp_input.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index d6c8f4cd0..d034923 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -5159,6 +5159,7 @@ static bool tcp_validate_incoming(struct sock *sk, struct sk_buff *skb,
 				  const struct tcphdr *th, int syn_inerr)
 {
 	struct tcp_sock *tp = tcp_sk(sk);
+	bool rst_seq_match = false;
 
 	/* RFC1323: H1. Apply PAWS check first. */
 	if (tcp_fast_parse_options(skb, th, tp) && tp->rx_opt.saw_tstamp &&
@@ -5195,13 +5196,30 @@ static bool tcp_validate_incoming(struct sock *sk, struct sk_buff *skb,
 
 	/* Step 2: check RST bit */
 	if (th->rst) {
-		/* RFC 5961 3.2 :
-		 * If sequence number exactly matches RCV.NXT, then
+		/* RFC 5961 3.2 (extended to match against SACK too if available):
+		 * If seq num exactly matches RCV.NXT or the right-most SACK block,
+		 * then
 		 *     RESET the connection
 		 * else
 		 *     Send a challenge ACK
 		 */
-		if (TCP_SKB_CB(skb)->seq == tp->rcv_nxt)
+		if (TCP_SKB_CB(skb)->seq == tp->rcv_nxt) {
+			rst_seq_match = true;
+		} else if (tcp_is_sack(tp) && tp->rx_opt.num_sacks > 0) {
+			struct tcp_sack_block *sp = &tp->selective_acks[0];
+			int max_sack = sp[0].end_seq;
+			int this_sack;
+
+			for (this_sack = 1; this_sack < tp->rx_opt.num_sacks; ++this_sack) {
+				max_sack = after(sp[this_sack].end_seq, max_sack) ?
+							sp[this_sack].end_seq : max_sack;
+			}
+
+			if (TCP_SKB_CB(skb)->seq == max_sack)
+				rst_seq_match = true;
+		}
+
+		if (rst_seq_match)
 			tcp_reset(sk);
 		else
 			tcp_send_challenge_ack(sk, skb);
-- 
2.5.0


-- 

------------------------------
DISCLAIMER.
This email and any files transmitted with it are confidential and intended 
solely for the use of the individual or entity to whom they are addressed. 
If you have received this email in error please notify the system manager. 
This message contains confidential information and is intended only for the 
individual named. If you are not the named addressee you should not 
disseminate, distribute or copy this e-mail. Please notify the sender 
immediately by e-mail if you have received this e-mail by mistake and 
delete this e-mail from your system. If you are not the intended recipient 
you are notified that disclosing, copying, distributing or taking any 
action in reliance on the contents of this information is strictly 
prohibited.

Re: [PATCH net-next] tcp: accept RST if SEQ matches right edge of right-most SACK block

[Neal Cardwell]

The functionality seems OK to me, though there are some
style/formatting issues, which checkpatch.pl picks up:

> ./scripts/checkpatch.pl net-next-tcp-accept-RST-if-SEQ-matches-right-edge-of-right-most-SACK-block.patch
WARNING: line over 80 characters
#73: FILE: net/ipv4/tcp_input.c:5199:
+               /* RFC 5961 3.2 (extended to match against SACK too if
available):

WARNING: line over 80 characters
#74: FILE: net/ipv4/tcp_input.c:5200:
+                * If seq num exactly matches RCV.NXT or the
right-most SACK block,

WARNING: line over 80 characters
#88: FILE: net/ipv4/tcp_input.c:5213:
+                       for (this_sack = 1; this_sack <
tp->rx_opt.num_sacks; ++this_sack) {

WARNING: line over 80 characters
#89: FILE: net/ipv4/tcp_input.c:5214:
+                               max_sack =
after(sp[this_sack].end_seq, max_sack) ?

WARNING: line over 80 characters
#90: FILE: net/ipv4/tcp_input.c:5215:
+
sp[this_sack].end_seq : max_sack;

total: 0 errors, 5 warnings, 0 checks, 40 lines checked

neal

[PATCH net-next] tcp: accept RST if SEQ matches right edge of right-most SACK block

[Pau Espin Pedrol]

RFC 5961 advises to only accept RST packets containing a seq number
matching the next expected seq number instead of the whole receive
window in order to avoid spoofing attacks.

However, this situation is not optimal in the case SACK is in use at the
time the RST is sent. I recently run into a scenario in which packet
losses were high while uploading data to a server, and userspace was
willing to frequently terminate connections by sending a RST. In
this case, the ACK sent on the receiver side (rcv_nxt) is frozen waiting
for a lost packet retransmission and SACK blocks are used to let the
client continue uploading data. At some point later on, the client sends
the RST (snd_nxt), which matches the next expected seq number of the
right-most SACK block on the receiver side which is going forward
receiving data.

In this scenario, as RFC 5961 defines, the RST SEQ doesn't match the
frozen main ACK at receiver side and thus gets dropped and a challenge
ACK is sent, which gets usually lost due to network conditions. The main
consequence is that the connection stays alive for a while even if it
made sense to accept the RST. This can get really bad if lots of
connections like this one are created in few seconds, allocating all the
resources of the server easily.

For security reasons, not all SACK blocks are checked (there could be a
big amount of SACK blocks => acceptable SEQ numbers). Furthermore, it
wouldn't make sense to check for RST in blocks other than the right-most
received one because the sender is not expected to be sending new data
after the RST. For simplicity, only up to the 4 most recently updated
SACK blocks (selective_acks[4] field) are compared to find the
right-most block, as usually those are the ones with bigger probability
to contain it.

This patch was tested in a 3.18 kernel and probed to improve the
situation in the scenario described above.

Signed-off-by: Pau Espin Pedrol <pau.espin@tessares.net>
---
 net/ipv4/tcp_input.c | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index d6c8f4cd0..89dd8d8 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -5159,6 +5159,7 @@ static bool tcp_validate_incoming(struct sock *sk, struct sk_buff *skb,
 				  const struct tcphdr *th, int syn_inerr)
 {
 	struct tcp_sock *tp = tcp_sk(sk);
+	bool rst_seq_match = false;
 
 	/* RFC1323: H1. Apply PAWS check first. */
 	if (tcp_fast_parse_options(skb, th, tp) && tp->rx_opt.saw_tstamp &&
@@ -5195,13 +5196,32 @@ static bool tcp_validate_incoming(struct sock *sk, struct sk_buff *skb,
 
 	/* Step 2: check RST bit */
 	if (th->rst) {
-		/* RFC 5961 3.2 :
-		 * If sequence number exactly matches RCV.NXT, then
+		/* RFC 5961 3.2 (extend to match against SACK too if available):
+		 * If seq num matches RCV.NXT or the right-most SACK block,
+		 * then
 		 *     RESET the connection
 		 * else
 		 *     Send a challenge ACK
 		 */
-		if (TCP_SKB_CB(skb)->seq == tp->rcv_nxt)
+		if (TCP_SKB_CB(skb)->seq == tp->rcv_nxt) {
+			rst_seq_match = true;
+		} else if (tcp_is_sack(tp) && tp->rx_opt.num_sacks > 0) {
+			struct tcp_sack_block *sp = &tp->selective_acks[0];
+			int max_sack = sp[0].end_seq;
+			int this_sack;
+
+			for (this_sack = 1; this_sack < tp->rx_opt.num_sacks;
+			     ++this_sack) {
+				max_sack = after(sp[this_sack].end_seq,
+						 max_sack) ?
+					sp[this_sack].end_seq : max_sack;
+			}
+
+			if (TCP_SKB_CB(skb)->seq == max_sack)
+				rst_seq_match = true;
+		}
+
+		if (rst_seq_match)
 			tcp_reset(sk);
 		else
 			tcp_send_challenge_ack(sk, skb);
-- 
2.5.0


-- 

------------------------------
DISCLAIMER.
This email and any files transmitted with it are confidential and intended 
solely for the use of the individual or entity to whom they are addressed. 
If you have received this email in error please notify the system manager. 
This message contains confidential information and is intended only for the 
individual named. If you are not the named addressee you should not 
disseminate, distribute or copy this e-mail. Please notify the sender 
immediately by e-mail if you have received this e-mail by mistake and 
delete this e-mail from your system. If you are not the intended recipient 
you are notified that disclosing, copying, distributing or taking any 
action in reliance on the contents of this information is strictly 
prohibited.

Re: [PATCH net-next] tcp: accept RST if SEQ matches right edge of right-most SACK block

[Eric Dumazet]

On Tue, 2016-06-07 at 16:30 +0200, Pau Espin Pedrol wrote:
> RFC 5961 advises to only accept RST packets containing a seq number
> matching the next expected seq number instead of the whole receive
> window in order to avoid spoofing attacks.

> Signed-off-by: Pau Espin Pedrol <pau.espin@tessares.net>

Acked-by: Eric Dumazet <edumazet@google.com>

Thanks !

Re: [PATCH net-next] tcp: accept RST if SEQ matches right edge of right-most SACK block

[Neal Cardwell]

On Tue, Jun 7, 2016 at 11:21 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Tue, 2016-06-07 at 16:30 +0200, Pau Espin Pedrol wrote:
>> RFC 5961 advises to only accept RST packets containing a seq number
>> matching the next expected seq number instead of the whole receive
>> window in order to avoid spoofing attacks.
>
>> Signed-off-by: Pau Espin Pedrol <pau.espin@tessares.net>

Acked-by: Neal Cardwell <ncardwell@google.com>
Tested-by: Neal Cardwell <ncardwell@google.com>

I verified that it passes the following packetdrill test, which is
based on the script posted by Pau in a recent thread on this topic.

Thanks!

neal

----

// Test that we accept an incoming RST if it matches the rightmost
// SACK block end.

0     socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
+0    setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
+0    bind(3, ..., ...) = 0
+0    listen(3, 1) = 0

+0    < S 0:0(0) win 500 <mss 1460,sackOK,nop,nop,nop,wscale 7>
+0    > S. 0:0(0) ack 1 win 29200 <mss 1460,nop,nop,sackOK,nop,wscale 7>
+0    < . 1:1(0) ack 1 win 500
+0    accept(3, ..., ...) = 4

// First roundtrip of data client->server (upload)
+0    < P. 1:1001(1000) ack 1 win 500
+0    > . 1:1(0) ack 1001

// A packet arrives without prev packet being seen.
+0    < P. 5001:6001(1000) ack 1 win 500
+0    > . 1:1(0) ack 1001 <nop,nop,sack 5001:6001>

// Another out-of-order packet arrives.
+0    < P. 3001:4001(1000) ack 1 win 500
+0    > . 1:1(0) ack 1001 <nop,nop,sack 3001:4001 5001:6001>

// We send a challenge ACK for a RST matching the right edge of the
inner SACK block:
+0    < R. 4001:4001(0) ack 1 win 500
+0    > . 1:1(0) ack 1001 <nop,nop,sack 3001:4001 5001:6001>

// RST arrives from client, for instance caused by setsockopt(fd,
SOL_SOCKET, SO_LINGER, ...)
// Matches right edge of right-most SACK block, so we reset the connection.
+0    < R. 6001:6001(0) ack 1 win 500

// Verify that the RST caused the kernel to reset the connection:
+.010 write(4, ..., 10000) = -1 ECONNRESET (Connection reset by peer)

// Double-check  that the socket is gone:
+0    < P. 2001:3001(1000) ack 1 win 500
+0    > R 1:1(0) ack 0

Re: [PATCH net-next] tcp: accept RST if SEQ matches right edge of right-most SACK block

[David Miller]

From: Pau Espin Pedrol <pau.espin@tessares.net>
Date: Tue,  7 Jun 2016 16:30:34 +0200

> RFC 5961 advises to only accept RST packets containing a seq number
> matching the next expected seq number instead of the whole receive
> window in order to avoid spoofing attacks.
> 
> However, this situation is not optimal in the case SACK is in use at the
> time the RST is sent. I recently run into a scenario in which packet
> losses were high while uploading data to a server, and userspace was
> willing to frequently terminate connections by sending a RST. In
> this case, the ACK sent on the receiver side (rcv_nxt) is frozen waiting
> for a lost packet retransmission and SACK blocks are used to let the
> client continue uploading data. At some point later on, the client sends
> the RST (snd_nxt), which matches the next expected seq number of the
> right-most SACK block on the receiver side which is going forward
> receiving data.
> 
> In this scenario, as RFC 5961 defines, the RST SEQ doesn't match the
> frozen main ACK at receiver side and thus gets dropped and a challenge
> ACK is sent, which gets usually lost due to network conditions. The main
> consequence is that the connection stays alive for a while even if it
> made sense to accept the RST. This can get really bad if lots of
> connections like this one are created in few seconds, allocating all the
> resources of the server easily.
> 
> For security reasons, not all SACK blocks are checked (there could be a
> big amount of SACK blocks => acceptable SEQ numbers). Furthermore, it
> wouldn't make sense to check for RST in blocks other than the right-most
> received one because the sender is not expected to be sending new data
> after the RST. For simplicity, only up to the 4 most recently updated
> SACK blocks (selective_acks[4] field) are compared to find the
> right-most block, as usually those are the ones with bigger probability
> to contain it.
> 
> This patch was tested in a 3.18 kernel and probed to improve the
> situation in the scenario described above.
> 
> Signed-off-by: Pau Espin Pedrol <pau.espin@tessares.net>

Applied.

[PATCH net-next] tcp: accept RST if SEQ matches right edge of SACK block

[Pau Espin Pedrol]

RFC 5961 advises to only accept RST packets containing a seq number
matching the next expected seq number instead of the whole receive
window in order to avoid spoofing attacks.

However, this situation is not optimal in the case SACK is in use at the
time the RST is sent. I recently run into a scenario in which packet
losses were high while uploading data to a server, and userspace was
willing to frequently terminate connections by sending a RST. In
this case, the ACK sent on the receiver side (rcv_nxt) is frozen waiting
for a lost packet retransmission and a SACK block is used to let the
client continue uploading data. At some point later on, the client sends
the RST (snd_nxt), which matches the next expected seq number of the
SACK block on the receiver side which is going forward receiving data.

In this scenario, as RFC 5961 defines, the RST SEQ doesn't match the
frozen main ACK at receiver side and thus gets dropped and a challenge
ACK is sent, which gets usually lost due to network conditions. The main
consequence is that the connection stays alive for a while even if it
made sense to accept the RST. This can get really bad if lots of
connections like this one are created in few seconds, allocating all the
resources of the server easily.

For security reasons, not all SACK blocks are checked (there could be a
big amount of SACK blocks => acceptable SEQ numbers). Only up to the 4
most recently updated SACK blocks (selective_acks[4] field) are checked,
which are usually the ones with bigger probability to receive a RST as
next packet. This should make it still difficult for attackers to inject
a valid RST message.

This patch was tested in a 3.18 kernel and probed to improve the
situation in the scenario described above.

Signed-off-by: Pau Espin Pedrol <pau.espin@tessares.net>
---
 net/ipv4/tcp_input.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index d6c8f4cd0..8c5712a 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -5159,6 +5159,7 @@ static bool tcp_validate_incoming(struct sock *sk, struct sk_buff *skb,
 				  const struct tcphdr *th, int syn_inerr)
 {
 	struct tcp_sock *tp = tcp_sk(sk);
+	bool rst_seq_match = false;
 
 	/* RFC1323: H1. Apply PAWS check first. */
 	if (tcp_fast_parse_options(skb, th, tp) && tp->rx_opt.saw_tstamp &&
@@ -5195,13 +5196,28 @@ static bool tcp_validate_incoming(struct sock *sk, struct sk_buff *skb,
 
 	/* Step 2: check RST bit */
 	if (th->rst) {
-		/* RFC 5961 3.2 :
-		 * If sequence number exactly matches RCV.NXT, then
+		/* RFC 5961 3.2 (extended to match against SACK too if available):
+		 * If seq num exactly matches RCV.NXT or a recently updated SACK block,
+		 * then
 		 *     RESET the connection
 		 * else
 		 *     Send a challenge ACK
 		 */
-		if (TCP_SKB_CB(skb)->seq == tp->rcv_nxt)
+		if (TCP_SKB_CB(skb)->seq == tp->rcv_nxt) {
+			rst_seq_match = true;
+		} else if (tcp_is_sack(tp)) {
+			struct tcp_sack_block *sp = &tp->selective_acks[0];
+			int this_sack;
+
+			for (this_sack = 0; this_sack < tp->rx_opt.num_sacks; ++this_sack) {
+				if (TCP_SKB_CB(skb)->seq == sp[this_sack].end_seq) {
+					rst_seq_match = true;
+					break;
+				}
+			}
+		}
+
+		if (rst_seq_match)
 			tcp_reset(sk);
 		else
 			tcp_send_challenge_ack(sk, skb);
-- 
2.5.0


-- 

------------------------------
DISCLAIMER.
This email and any files transmitted with it are confidential and intended 
solely for the use of the individual or entity to whom they are addressed. 
If you have received this email in error please notify the system manager. 
This message contains confidential information and is intended only for the 
individual named. If you are not the named addressee you should not 
disseminate, distribute or copy this e-mail. Please notify the sender 
immediately by e-mail if you have received this e-mail by mistake and 
delete this e-mail from your system. If you are not the intended recipient 
you are notified that disclosing, copying, distributing or taking any 
action in reliance on the contents of this information is strictly 
prohibited.