[PATCH 39/43]: Protect against Reset/Sync floods due to buggy applications

[PATCH 39/43]: Protect against Reset/Sync floods due to buggy applications

[Gerrit Renker]

[DCCP]: Protect against Reset/Sync floods due to buggy applications

This patch protects against Reset/Sync floods which happens as a result
of either buggy or crashing client applications. The Reset/Sync flood
is triggered as follows:

 1. Client establishes connection to listening server;
 2. before server can write data to client, client crashes;
 3. crashing client removes connection state at client host;
 4. server still thinks client is alive and sends data;
 5. client responds to server packet with Reset packet Code 3, 
    "No Connection", with seqno=0 - as per RFC 4340, 8.3.1;
 6. server thinks that seqno=0 is out of synch (step 6), sends Sync;
 7. goto (6).

The result is a drastic flood of packets: In one occasion I counted
345549 Reset/Sync packets, before the server finally killed itself.

Fix:
----
Since this condition is peculiar and can be distinguished from other
sequence-invalid packets, a special case has been added. The Reset
is accepted if
 * it has Reset Code 3, "No Connection" AND
 * it has sequence number 0 as described in RFC 4340, 8.3.1.

If both conditions are satisfied, the Reset is enqueued in the receive queue
as usual, and will very soon terminate the crashed connection.

Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
---
 net/dccp/input.c |   17 +++++++++++++++++
 1 file changed, 17 insertions(+)

--- a/net/dccp/input.c
+++ b/net/dccp/input.c
@@ -155,6 +155,22 @@ static int dccp_check_seqno(struct sock 
 		    (DCCP_SKB_CB(skb)->dccpd_ack_seq ! 		     DCCP_PKT_WITHOUT_ACK_SEQ))
 			dp->dccps_gar = DCCP_SKB_CB(skb)->dccpd_ack_seq;
+
+	} else if (dh->dccph_type = DCCP_PKT_RESET  &&
+		   dccp_hdr_reset(skb)->dccph_reset_code =
+		   DCCP_RESET_CODE_NO_CONNECTION &&
+		   DCCP_SKB_CB(skb)->dccpd_seq = 0) {
+		/*
+		 * This happens when connection is established and client app
+		 * crashes before server can send data. The crashing client
+		 * removes connection state, so the server gets a Code 3 Reset
+		 * packet with seqno 0 (RFC 4340, 8.3.1). Responding here with
+		 * a Sync leads to a Reset-Storm which will flood the network
+		 * until the server gives up on this connection or is killed.
+		 * We let this case pass so that the Reset gets enqueued and
+		 * will terminate the erratic connection.
+		 */
+		DCCP_WARN("DCCP: Peer sent RESET with seqno 0\n");
 	} else {
 		DCCP_WARN("DCCP: Step 6 failed for %s packet, "
 			  "(LSWL(%llu) <= P.seqno(%llu) <= S.SWH(%llu)) and "
@@ -168,6 +184,7 @@ static int dccp_check_seqno(struct sock 
 			  (unsigned long long) lawl,
 			  (unsigned long long) DCCP_SKB_CB(skb)->dccpd_ack_seq,
 			  (unsigned long long) dp->dccps_awh);
+		/* FIXME: Rate-limit DCCP-Sync packets as per RFC 4340, 7.5.4 */
 		dccp_send_sync(sk, DCCP_SKB_CB(skb)->dccpd_seq, DCCP_PKT_SYNC);
 		return -1;
 	}

Re: [PATCH 39/43]: Protect against Reset/Sync floods due to buggy

[Eddie Kohler]

Hi Gerrit,

I'm surprised this kind of flood happens & think it may represent a bug in the 
stack.  What is the acknowledgement number on the Sync packet sent in step 
(6)?  It should be GSR, according to Step 6 of the pseudocode in Section 8.5. 
  If it was GSR, I would expect the following denouement:

5. client responds with Reset packet Code 3 with seqno=0
6. server thinks that seqno=0 is out of synch (step 6), sends Sync with 
seqno=GSS, ackno=GSR, then increments GSS;
7. client responds with Reset packet Code 3 with seqno=GSR+1 and ackno=GSS (it 
can do this because it takes the seqno from the received packet's ackno, per 
Section 8.3.1);
8. the Reset is now in synch, so the server kills the connection.

No flood should be possible with valid stacks; some people actually verified 
this theoretically.

Of course stacks can be INvalid, in which case one should rate-limit Syncs, as 
allowed by the protocol.

Your solution, which is to accept Resets with seqno 0, makes it trivially 
simple for an attacker to kill any connection.  It should not be committed! 
Can we figure out why the stack has the chatter problem first?

Eddie


Gerrit Renker wrote:
> [DCCP]: Protect against Reset/Sync floods due to buggy applications
> 
> This patch protects against Reset/Sync floods which happens as a result
> of either buggy or crashing client applications. The Reset/Sync flood
> is triggered as follows:
> 
>  1. Client establishes connection to listening server;
>  2. before server can write data to client, client crashes;
>  3. crashing client removes connection state at client host;
>  4. server still thinks client is alive and sends data;
>  5. client responds to server packet with Reset packet Code 3, 
>     "No Connection", with seqno=0 - as per RFC 4340, 8.3.1;
>  6. server thinks that seqno=0 is out of synch (step 6), sends Sync;
>  7. goto (6).
> 
> The result is a drastic flood of packets: In one occasion I counted
> 345549 Reset/Sync packets, before the server finally killed itself.
> 
> Fix:
> ----
> Since this condition is peculiar and can be distinguished from other
> sequence-invalid packets, a special case has been added. The Reset
> is accepted if
>  * it has Reset Code 3, "No Connection" AND
>  * it has sequence number 0 as described in RFC 4340, 8.3.1.
> 
> If both conditions are satisfied, the Reset is enqueued in the receive queue
> as usual, and will very soon terminate the crashed connection.
> 
> Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
> ---
>  net/dccp/input.c |   17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> --- a/net/dccp/input.c
> +++ b/net/dccp/input.c
> @@ -155,6 +155,22 @@ static int dccp_check_seqno(struct sock 
>  		    (DCCP_SKB_CB(skb)->dccpd_ack_seq !>  		     DCCP_PKT_WITHOUT_ACK_SEQ))
>  			dp->dccps_gar = DCCP_SKB_CB(skb)->dccpd_ack_seq;
> +
> +	} else if (dh->dccph_type = DCCP_PKT_RESET  &&
> +		   dccp_hdr_reset(skb)->dccph_reset_code =
> +		   DCCP_RESET_CODE_NO_CONNECTION &&
> +		   DCCP_SKB_CB(skb)->dccpd_seq = 0) {
> +		/*
> +		 * This happens when connection is established and client app
> +		 * crashes before server can send data. The crashing client
> +		 * removes connection state, so the server gets a Code 3 Reset
> +		 * packet with seqno 0 (RFC 4340, 8.3.1). Responding here with
> +		 * a Sync leads to a Reset-Storm which will flood the network
> +		 * until the server gives up on this connection or is killed.
> +		 * We let this case pass so that the Reset gets enqueued and
> +		 * will terminate the erratic connection.
> +		 */
> +		DCCP_WARN("DCCP: Peer sent RESET with seqno 0\n");
>  	} else {
>  		DCCP_WARN("DCCP: Step 6 failed for %s packet, "
>  			  "(LSWL(%llu) <= P.seqno(%llu) <= S.SWH(%llu)) and "
> @@ -168,6 +184,7 @@ static int dccp_check_seqno(struct sock 
>  			  (unsigned long long) lawl,
>  			  (unsigned long long) DCCP_SKB_CB(skb)->dccpd_ack_seq,
>  			  (unsigned long long) dp->dccps_awh);
> +		/* FIXME: Rate-limit DCCP-Sync packets as per RFC 4340, 7.5.4 */
>  		dccp_send_sync(sk, DCCP_SKB_CB(skb)->dccpd_seq, DCCP_PKT_SYNC);
>  		return -1;
>  	}
> -
> To unsubscribe from this list: send the line "unsubscribe dccp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 39/43]: Protect against Reset/Sync floods due to buggy applications

[Gerrit Renker]

Eddie,

I have spent half a day or so trying to reconstruct the condition which triggered this bug.
With the new patches regarding closing state I couldn't trigger this bug anymore. Which is
not to say that it has gone away. 

All I can say is that I have observed this bug as described and that it is pretty scary -
the computer freezes for several minutes (until the server connection timer declares the
connection as dead) and it generates a massive load of packets. 

I will try to follow this up when I have some more time; as indicated by the comment in 
the patch below, I am lacking the resources to implement rate-limiting for DCCP-Sync; too 
many other things to fix.

Anyone else any ideas / suggestions?

Gerrit


Quoting Eddie Kohler:
|  Hi Gerrit,
|  
|  I'm surprised this kind of flood happens & think it may represent a bug in the 
|  stack.  What is the acknowledgement number on the Sync packet sent in step 
|  (6)?  It should be GSR, according to Step 6 of the pseudocode in Section 8.5. 
|    If it was GSR, I would expect the following denouement:
|  
|  5. client responds with Reset packet Code 3 with seqno=0
|  6. server thinks that seqno=0 is out of synch (step 6), sends Sync with 
|  seqno=GSS, ackno=GSR, then increments GSS;
|  7. client responds with Reset packet Code 3 with seqno=GSR+1 and ackno=GSS (it 
|  can do this because it takes the seqno from the received packet's ackno, per 
|  Section 8.3.1);
|  8. the Reset is now in synch, so the server kills the connection.
|  
|  No flood should be possible with valid stacks; some people actually verified 
|  this theoretically.
|  
|  Of course stacks can be INvalid, in which case one should rate-limit Syncs, as 
|  allowed by the protocol.
|  
|  Your solution, which is to accept Resets with seqno 0, makes it trivially 
|  simple for an attacker to kill any connection.  It should not be committed! 
|  Can we figure out why the stack has the chatter problem first?
|  
|  Eddie
|  
|  
|  Gerrit Renker wrote:
|  > [DCCP]: Protect against Reset/Sync floods due to buggy applications
|  > 
|  > This patch protects against Reset/Sync floods which happens as a result
|  > of either buggy or crashing client applications. The Reset/Sync flood
|  > is triggered as follows:
|  > 
|  >  1. Client establishes connection to listening server;
|  >  2. before server can write data to client, client crashes;
|  >  3. crashing client removes connection state at client host;
|  >  4. server still thinks client is alive and sends data;
|  >  5. client responds to server packet with Reset packet Code 3, 
|  >     "No Connection", with seqno=0 - as per RFC 4340, 8.3.1;
|  >  6. server thinks that seqno=0 is out of synch (step 6), sends Sync;
|  >  7. goto (6).
|  > 
|  > The result is a drastic flood of packets: In one occasion I counted
|  > 345549 Reset/Sync packets, before the server finally killed itself.
|  > 
|  > Fix:
|  > ----
|  > Since this condition is peculiar and can be distinguished from other
|  > sequence-invalid packets, a special case has been added. The Reset
|  > is accepted if
|  >  * it has Reset Code 3, "No Connection" AND
|  >  * it has sequence number 0 as described in RFC 4340, 8.3.1.
|  > 
|  > If both conditions are satisfied, the Reset is enqueued in the receive queue
|  > as usual, and will very soon terminate the crashed connection.
|  > 
|  > Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
|  > ---
|  >  net/dccp/input.c |   17 +++++++++++++++++
|  >  1 file changed, 17 insertions(+)
|  > 
|  > --- a/net/dccp/input.c
|  > +++ b/net/dccp/input.c
|  > @@ -155,6 +155,22 @@ static int dccp_check_seqno(struct sock 
|  >  		    (DCCP_SKB_CB(skb)->dccpd_ack_seq !|  >  		     DCCP_PKT_WITHOUT_ACK_SEQ))
|  >  			dp->dccps_gar = DCCP_SKB_CB(skb)->dccpd_ack_seq;
|  > +
|  > +	} else if (dh->dccph_type = DCCP_PKT_RESET  &&
|  > +		   dccp_hdr_reset(skb)->dccph_reset_code =
|  > +		   DCCP_RESET_CODE_NO_CONNECTION &&
|  > +		   DCCP_SKB_CB(skb)->dccpd_seq = 0) {
|  > +		/*
|  > +		 * This happens when connection is established and client app
|  > +		 * crashes before server can send data. The crashing client
|  > +		 * removes connection state, so the server gets a Code 3 Reset
|  > +		 * packet with seqno 0 (RFC 4340, 8.3.1). Responding here with
|  > +		 * a Sync leads to a Reset-Storm which will flood the network
|  > +		 * until the server gives up on this connection or is killed.
|  > +		 * We let this case pass so that the Reset gets enqueued and
|  > +		 * will terminate the erratic connection.
|  > +		 */
|  > +		DCCP_WARN("DCCP: Peer sent RESET with seqno 0\n");
|  >  	} else {
|  >  		DCCP_WARN("DCCP: Step 6 failed for %s packet, "
|  >  			  "(LSWL(%llu) <= P.seqno(%llu) <= S.SWH(%llu)) and "
|  > @@ -168,6 +184,7 @@ static int dccp_check_seqno(struct sock 
|  >  			  (unsigned long long) lawl,
|  >  			  (unsigned long long) DCCP_SKB_CB(skb)->dccpd_ack_seq,
|  >  			  (unsigned long long) dp->dccps_awh);
|  > +		/* FIXME: Rate-limit DCCP-Sync packets as per RFC 4340, 7.5.4 */
|  >  		dccp_send_sync(sk, DCCP_SKB_CB(skb)->dccpd_seq, DCCP_PKT_SYNC);
|  >  		return -1;
|  >  	}
|  > -
|  > To unsubscribe from this list: send the line "unsubscribe dccp" in
|  > the body of a message to majordomo@vger.kernel.org
|  > More majordomo info at  http://vger.kernel.org/majordomo-info.html
|  
|  

Re: [PATCH 39/43]: Protect against Reset/Sync floods due to buggy

[Eddie Kohler]

Hi Gerrit,

I agree that the bug is scary!  It would be good to fix.  But this fix isn't a 
good fix, especially because it makes it trivially easy to shoot down a 
connection if you know the port numbers.  Would you mind withdrawing the patch 
until the bug reappears?

Eddie


Gerrit Renker wrote:
> Eddie,
> 
> I have spent half a day or so trying to reconstruct the condition which triggered this bug.
> With the new patches regarding closing state I couldn't trigger this bug anymore. Which is
> not to say that it has gone away. 
> 
> All I can say is that I have observed this bug as described and that it is pretty scary -
> the computer freezes for several minutes (until the server connection timer declares the
> connection as dead) and it generates a massive load of packets. 
> 
> I will try to follow this up when I have some more time; as indicated by the comment in 
> the patch below, I am lacking the resources to implement rate-limiting for DCCP-Sync; too 
> many other things to fix.
> 
> Anyone else any ideas / suggestions?
> 
> Gerrit
> 
> 
> Quoting Eddie Kohler:
> |  Hi Gerrit,
> |  
> |  I'm surprised this kind of flood happens & think it may represent a bug in the 
> |  stack.  What is the acknowledgement number on the Sync packet sent in step 
> |  (6)?  It should be GSR, according to Step 6 of the pseudocode in Section 8.5. 
> |    If it was GSR, I would expect the following denouement:
> |  
> |  5. client responds with Reset packet Code 3 with seqno=0
> |  6. server thinks that seqno=0 is out of synch (step 6), sends Sync with 
> |  seqno=GSS, ackno=GSR, then increments GSS;
> |  7. client responds with Reset packet Code 3 with seqno=GSR+1 and ackno=GSS (it 
> |  can do this because it takes the seqno from the received packet's ackno, per 
> |  Section 8.3.1);
> |  8. the Reset is now in synch, so the server kills the connection.
> |  
> |  No flood should be possible with valid stacks; some people actually verified 
> |  this theoretically.
> |  
> |  Of course stacks can be INvalid, in which case one should rate-limit Syncs, as 
> |  allowed by the protocol.
> |  
> |  Your solution, which is to accept Resets with seqno 0, makes it trivially 
> |  simple for an attacker to kill any connection.  It should not be committed! 
> |  Can we figure out why the stack has the chatter problem first?
> |  
> |  Eddie
> |  
> |  
> |  Gerrit Renker wrote:
> |  > [DCCP]: Protect against Reset/Sync floods due to buggy applications
> |  > 
> |  > This patch protects against Reset/Sync floods which happens as a result
> |  > of either buggy or crashing client applications. The Reset/Sync flood
> |  > is triggered as follows:
> |  > 
> |  >  1. Client establishes connection to listening server;
> |  >  2. before server can write data to client, client crashes;
> |  >  3. crashing client removes connection state at client host;
> |  >  4. server still thinks client is alive and sends data;
> |  >  5. client responds to server packet with Reset packet Code 3, 
> |  >     "No Connection", with seqno=0 - as per RFC 4340, 8.3.1;
> |  >  6. server thinks that seqno=0 is out of synch (step 6), sends Sync;
> |  >  7. goto (6).
> |  > 
> |  > The result is a drastic flood of packets: In one occasion I counted
> |  > 345549 Reset/Sync packets, before the server finally killed itself.
> |  > 
> |  > Fix:
> |  > ----
> |  > Since this condition is peculiar and can be distinguished from other
> |  > sequence-invalid packets, a special case has been added. The Reset
> |  > is accepted if
> |  >  * it has Reset Code 3, "No Connection" AND
> |  >  * it has sequence number 0 as described in RFC 4340, 8.3.1.
> |  > 
> |  > If both conditions are satisfied, the Reset is enqueued in the receive queue
> |  > as usual, and will very soon terminate the crashed connection.
> |  > 
> |  > Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
> |  > ---
> |  >  net/dccp/input.c |   17 +++++++++++++++++
> |  >  1 file changed, 17 insertions(+)
> |  > 
> |  > --- a/net/dccp/input.c
> |  > +++ b/net/dccp/input.c
> |  > @@ -155,6 +155,22 @@ static int dccp_check_seqno(struct sock 
> |  >  		    (DCCP_SKB_CB(skb)->dccpd_ack_seq !> |  >  		     DCCP_PKT_WITHOUT_ACK_SEQ))
> |  >  			dp->dccps_gar = DCCP_SKB_CB(skb)->dccpd_ack_seq;
> |  > +
> |  > +	} else if (dh->dccph_type = DCCP_PKT_RESET  &&
> |  > +		   dccp_hdr_reset(skb)->dccph_reset_code =
> |  > +		   DCCP_RESET_CODE_NO_CONNECTION &&
> |  > +		   DCCP_SKB_CB(skb)->dccpd_seq = 0) {
> |  > +		/*
> |  > +		 * This happens when connection is established and client app
> |  > +		 * crashes before server can send data. The crashing client
> |  > +		 * removes connection state, so the server gets a Code 3 Reset
> |  > +		 * packet with seqno 0 (RFC 4340, 8.3.1). Responding here with
> |  > +		 * a Sync leads to a Reset-Storm which will flood the network
> |  > +		 * until the server gives up on this connection or is killed.
> |  > +		 * We let this case pass so that the Reset gets enqueued and
> |  > +		 * will terminate the erratic connection.
> |  > +		 */
> |  > +		DCCP_WARN("DCCP: Peer sent RESET with seqno 0\n");
> |  >  	} else {
> |  >  		DCCP_WARN("DCCP: Step 6 failed for %s packet, "
> |  >  			  "(LSWL(%llu) <= P.seqno(%llu) <= S.SWH(%llu)) and "
> |  > @@ -168,6 +184,7 @@ static int dccp_check_seqno(struct sock 
> |  >  			  (unsigned long long) lawl,
> |  >  			  (unsigned long long) DCCP_SKB_CB(skb)->dccpd_ack_seq,
> |  >  			  (unsigned long long) dp->dccps_awh);
> |  > +		/* FIXME: Rate-limit DCCP-Sync packets as per RFC 4340, 7.5.4 */
> |  >  		dccp_send_sync(sk, DCCP_SKB_CB(skb)->dccpd_seq, DCCP_PKT_SYNC);
> |  >  		return -1;
> |  >  	}
> |  > -
> |  > To unsubscribe from this list: send the line "unsubscribe dccp" in
> |  > the body of a message to majordomo@vger.kernel.org
> |  > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> |  
> |  

Re: [PATCH 39/43]: Protect against Reset/Sync floods due to buggy applications

[Gerrit Renker]

Hi Eddie,

first of all thank you for help with this bug. 

You are correct, the cause is somewhere else, it was an oversight. 

Arnaldo, can you please ignore this patch (16g); I have withdrawn it from the online directory. 

Instead, 

 * a proper bug fix with description follows;

 * the bug and its cause are documented on 
      http://www.erg.abdn.ac.uk/users/gerrit/dccp/docs/seqno_bug/
   (in short, the Sync did not acknowledge GSR, but rather P.seqno);

 * I have further implemented rate-limiting for Syncs as suggested in 7.5.4. Rate-limiting
   proved to be an efficient countermeasure for this bug: it broke the vicious circle of
   invalid-Reset => Sync => invalid Reset. This was because the second received Reset is
   already subject to rate-limiting; hence there is no Sync in reply; hence no further 
   sequence-invalid Reset is triggered. 
   I believe that rate-limiting can be of similiar help in other situations involving 
   sequence-invalid packets.

 * The reason that the bug does not show with the more recent patches is that previously
   it was possible for a DCCP application to silently crash, be killed, terminated without
   its connected peer taking notice of it. Patches 17a and 17b implement an ABORT function
   which sends a DCCP-Reset Code 2 "Aborted" which will terminate the connection. Thus
   mayhem due to half-closed connections is avoided.

Thanks again for pointing this out,
Gerrit
   

Quoting Eddie Kohler:
|  Hi Gerrit,
|  
|  I agree that the bug is scary!  It would be good to fix.  But this fix isn't a 
|  good fix, especially because it makes it trivially easy to shoot down a 
|  connection if you know the port numbers.  Would you mind withdrawing the patch 
|  until the bug reappears?
|  
|  Eddie
|  
|  
|  Gerrit Renker wrote:
|  > Eddie,
|  > 
|  > I have spent half a day or so trying to reconstruct the condition which triggered this bug.
|  > With the new patches regarding closing state I couldn't trigger this bug anymore. Which is
|  > not to say that it has gone away. 
|  > 
|  > All I can say is that I have observed this bug as described and that it is pretty scary -
|  > the computer freezes for several minutes (until the server connection timer declares the
|  > connection as dead) and it generates a massive load of packets. 
|  > 
|  > I will try to follow this up when I have some more time; as indicated by the comment in 
|  > the patch below, I am lacking the resources to implement rate-limiting for DCCP-Sync; too 
|  > many other things to fix.
|  > 
|  > Anyone else any ideas / suggestions?
|  > 
|  > Gerrit
|  > 
|  > 
|  > Quoting Eddie Kohler:
|  > |  Hi Gerrit,
|  > |  
|  > |  I'm surprised this kind of flood happens & think it may represent a bug in the 
|  > |  stack.  What is the acknowledgement number on the Sync packet sent in step 
|  > |  (6)?  It should be GSR, according to Step 6 of the pseudocode in Section 8.5. 
|  > |    If it was GSR, I would expect the following denouement:
|  > |  
|  > |  5. client responds with Reset packet Code 3 with seqno=0
|  > |  6. server thinks that seqno=0 is out of synch (step 6), sends Sync with 
|  > |  seqno=GSS, ackno=GSR, then increments GSS;
|  > |  7. client responds with Reset packet Code 3 with seqno=GSR+1 and ackno=GSS (it 
|  > |  can do this because it takes the seqno from the received packet's ackno, per 
|  > |  Section 8.3.1);
|  > |  8. the Reset is now in synch, so the server kills the connection.
|  > |  
|  > |  No flood should be possible with valid stacks; some people actually verified 
|  > |  this theoretically.
|  > |  
|  > |  Of course stacks can be INvalid, in which case one should rate-limit Syncs, as 
|  > |  allowed by the protocol.
|  > |  
|  > |  Your solution, which is to accept Resets with seqno 0, makes it trivially 
|  > |  simple for an attacker to kill any connection.  It should not be committed! 
|  > |  Can we figure out why the stack has the chatter problem first?
|  > |  
|  > |  Eddie
|  > |  
|  > |  
|  > |  Gerrit Renker wrote:
|  > |  > [DCCP]: Protect against Reset/Sync floods due to buggy applications
|  > |  > 
|  > |  > This patch protects against Reset/Sync floods which happens as a result
|  > |  > of either buggy or crashing client applications. The Reset/Sync flood
|  > |  > is triggered as follows:
|  > |  > 
|  > |  >  1. Client establishes connection to listening server;
|  > |  >  2. before server can write data to client, client crashes;
|  > |  >  3. crashing client removes connection state at client host;
|  > |  >  4. server still thinks client is alive and sends data;
|  > |  >  5. client responds to server packet with Reset packet Code 3, 
|  > |  >     "No Connection", with seqno=0 - as per RFC 4340, 8.3.1;
|  > |  >  6. server thinks that seqno=0 is out of synch (step 6), sends Sync;
|  > |  >  7. goto (6).
|  > |  > 
|  > |  > The result is a drastic flood of packets: In one occasion I counted
|  > |  > 345549 Reset/Sync packets, before the server finally killed itself.
|  > |  > 
|  > |  > Fix:
|  > |  > ----
|  > |  > Since this condition is peculiar and can be distinguished from other
|  > |  > sequence-invalid packets, a special case has been added. The Reset
|  > |  > is accepted if
|  > |  >  * it has Reset Code 3, "No Connection" AND
|  > |  >  * it has sequence number 0 as described in RFC 4340, 8.3.1.
|  > |  > 
|  > |  > If both conditions are satisfied, the Reset is enqueued in the receive queue
|  > |  > as usual, and will very soon terminate the crashed connection.
|  > |  > 
|  > |  > Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
|  > |  > ---
|  > |  >  net/dccp/input.c |   17 +++++++++++++++++
|  > |  >  1 file changed, 17 insertions(+)
|  > |  > 
|  > |  > --- a/net/dccp/input.c
|  > |  > +++ b/net/dccp/input.c
|  > |  > @@ -155,6 +155,22 @@ static int dccp_check_seqno(struct sock 
|  > |  >  		    (DCCP_SKB_CB(skb)->dccpd_ack_seq !|  > |  >  		     DCCP_PKT_WITHOUT_ACK_SEQ))
|  > |  >  			dp->dccps_gar = DCCP_SKB_CB(skb)->dccpd_ack_seq;
|  > |  > +
|  > |  > +	} else if (dh->dccph_type = DCCP_PKT_RESET  &&
|  > |  > +		   dccp_hdr_reset(skb)->dccph_reset_code =
|  > |  > +		   DCCP_RESET_CODE_NO_CONNECTION &&
|  > |  > +		   DCCP_SKB_CB(skb)->dccpd_seq = 0) {
|  > |  > +		/*
|  > |  > +		 * This happens when connection is established and client app
|  > |  > +		 * crashes before server can send data. The crashing client
|  > |  > +		 * removes connection state, so the server gets a Code 3 Reset
|  > |  > +		 * packet with seqno 0 (RFC 4340, 8.3.1). Responding here with
|  > |  > +		 * a Sync leads to a Reset-Storm which will flood the network
|  > |  > +		 * until the server gives up on this connection or is killed.
|  > |  > +		 * We let this case pass so that the Reset gets enqueued and
|  > |  > +		 * will terminate the erratic connection.
|  > |  > +		 */
|  > |  > +		DCCP_WARN("DCCP: Peer sent RESET with seqno 0\n");
|  > |  >  	} else {
|  > |  >  		DCCP_WARN("DCCP: Step 6 failed for %s packet, "
|  > |  >  			  "(LSWL(%llu) <= P.seqno(%llu) <= S.SWH(%llu)) and "
|  > |  > @@ -168,6 +184,7 @@ static int dccp_check_seqno(struct sock 
|  > |  >  			  (unsigned long long) lawl,
|  > |  >  			  (unsigned long long) DCCP_SKB_CB(skb)->dccpd_ack_seq,
|  > |  >  			  (unsigned long long) dp->dccps_awh);
|  > |  > +		/* FIXME: Rate-limit DCCP-Sync packets as per RFC 4340, 7.5.4 */
|  > |  >  		dccp_send_sync(sk, DCCP_SKB_CB(skb)->dccpd_seq, DCCP_PKT_SYNC);
|  > |  >  		return -1;
|  > |  >  	}
|  > |  > -
|  > |  > To unsubscribe from this list: send the line "unsubscribe dccp" in
|  > |  > the body of a message to majordomo@vger.kernel.org
|  > |  > More majordomo info at  http://vger.kernel.org/majordomo-info.html
|  > |  
|  > |  
|  
|  

Re: [PATCH 39/43]: Protect against Reset/Sync floods due to buggy

[Eddie Kohler]

Hi Gerrit,

Gerrit Renker wrote:
> Listen Eddie, 
> 
> I am not interested in your offline email. All further such email will
> be directed back to the list.
> 
> Eddie Kohler wrote:
> |  Glad to help!
> |  E

As you requested, this mail is being cc'd to the kernel mailing list.

In future, every time I send you a three-word note, I will make sure to cc the 
kernel mailing list.

I deeply, deeply apologize for whatever it is I have done this time to make 
you angry, although I have no idea what that could be.  I hereby acknowledge 
all of your many thousands of contributions to the DCCP kernel implementation.

All hail Gerrit!!~*$716617*&!197`987(*&!(*&(*@!&(*#&(*@!

Eddie


> |  
> |  
> |  Gerrit Renker wrote:
> |  > Hi Eddie,
> |  > 
> |  > first of all thank you for help with this bug. 
> |  > 
> |  > You are correct, the cause is somewhere else, it was an oversight. 
> |  > 
> |  > Arnaldo, can you please ignore this patch (16g); I have withdrawn it from the online directory. 
> |  > 
> |  > Instead, 
> |  > 
> |  >  * a proper bug fix with description follows;
> |  > 
> |  >  * the bug and its cause are documented on 
> |  >       http://www.erg.abdn.ac.uk/users/gerrit/dccp/docs/seqno_bug/
> |  >    (in short, the Sync did not acknowledge GSR, but rather P.seqno);
> |  > 
> |  >  * I have further implemented rate-limiting for Syncs as suggested in 7.5.4. Rate-limiting
> |  >    proved to be an efficient countermeasure for this bug: it broke the vicious circle of
> |  >    invalid-Reset => Sync => invalid Reset. This was because the second received Reset is
> |  >    already subject to rate-limiting; hence there is no Sync in reply; hence no further 
> |  >    sequence-invalid Reset is triggered. 
> |  >    I believe that rate-limiting can be of similiar help in other situations involving 
> |  >    sequence-invalid packets.
> |  > 
> |  >  * The reason that the bug does not show with the more recent patches is that previously
> |  >    it was possible for a DCCP application to silently crash, be killed, terminated without
> |  >    its connected peer taking notice of it. Patches 17a and 17b implement an ABORT function
> |  >    which sends a DCCP-Reset Code 2 "Aborted" which will terminate the connection. Thus
> |  >    mayhem due to half-closed connections is avoided.
> |  > 
> |  > Thanks again for pointing this out,
> |  > Gerrit
> |  >    
> |  > 
> |  > Quoting Eddie Kohler:
> |  > |  Hi Gerrit,
> |  > |  
> |  > |  I agree that the bug is scary!  It would be good to fix.  But this fix isn't a 
> |  > |  good fix, especially because it makes it trivially easy to shoot down a 
> |  > |  connection if you know the port numbers.  Would you mind withdrawing the patch 
> |  > |  until the bug reappears?
> |  > |  
> |  > |  Eddie
> |  > |  
> |  > |  
> |  > |  Gerrit Renker wrote:
> |  > |  > Eddie,
> |  > |  > 
> |  > |  > I have spent half a day or so trying to reconstruct the condition which triggered this bug.
> |  > |  > With the new patches regarding closing state I couldn't trigger this bug anymore. Which is
> |  > |  > not to say that it has gone away. 
> |  > |  > 
> |  > |  > All I can say is that I have observed this bug as described and that it is pretty scary -
> |  > |  > the computer freezes for several minutes (until the server connection timer declares the
> |  > |  > connection as dead) and it generates a massive load of packets. 
> |  > |  > 
> |  > |  > I will try to follow this up when I have some more time; as indicated by the comment in 
> |  > |  > the patch below, I am lacking the resources to implement rate-limiting for DCCP-Sync; too 
> |  > |  > many other things to fix.
> |  > |  > 
> |  > |  > Anyone else any ideas / suggestions?
> |  > |  > 
> |  > |  > Gerrit
> |  > |  > 
> |  > |  > 
> |  > |  > Quoting Eddie Kohler:
> |  > |  > |  Hi Gerrit,
> |  > |  > |  
> |  > |  > |  I'm surprised this kind of flood happens & think it may represent a bug in the 
> |  > |  > |  stack.  What is the acknowledgement number on the Sync packet sent in step 
> |  > |  > |  (6)?  It should be GSR, according to Step 6 of the pseudocode in Section 8.5. 
> |  > |  > |    If it was GSR, I would expect the following denouement:
> |  > |  > |  
> |  > |  > |  5. client responds with Reset packet Code 3 with seqno=0
> |  > |  > |  6. server thinks that seqno=0 is out of synch (step 6), sends Sync with 
> |  > |  > |  seqno=GSS, ackno=GSR, then increments GSS;
> |  > |  > |  7. client responds with Reset packet Code 3 with seqno=GSR+1 and ackno=GSS (it 
> |  > |  > |  can do this because it takes the seqno from the received packet's ackno, per 
> |  > |  > |  Section 8.3.1);
> |  > |  > |  8. the Reset is now in synch, so the server kills the connection.
> |  > |  > |  
> |  > |  > |  No flood should be possible with valid stacks; some people actually verified 
> |  > |  > |  this theoretically.
> |  > |  > |  
> |  > |  > |  Of course stacks can be INvalid, in which case one should rate-limit Syncs, as 
> |  > |  > |  allowed by the protocol.
> |  > |  > |  
> |  > |  > |  Your solution, which is to accept Resets with seqno 0, makes it trivially 
> |  > |  > |  simple for an attacker to kill any connection.  It should not be committed! 
> |  > |  > |  Can we figure out why the stack has the chatter problem first?
> |  > |  > |  
> |  > |  > |  Eddie
> |  > |  > |  
> |  > |  > |  
> |  > |  > |  Gerrit Renker wrote:
> |  > |  > |  > [DCCP]: Protect against Reset/Sync floods due to buggy applications
> |  > |  > |  > 
> |  > |  > |  > This patch protects against Reset/Sync floods which happens as a result
> |  > |  > |  > of either buggy or crashing client applications. The Reset/Sync flood
> |  > |  > |  > is triggered as follows:
> |  > |  > |  > 
> |  > |  > |  >  1. Client establishes connection to listening server;
> |  > |  > |  >  2. before server can write data to client, client crashes;
> |  > |  > |  >  3. crashing client removes connection state at client host;
> |  > |  > |  >  4. server still thinks client is alive and sends data;
> |  > |  > |  >  5. client responds to server packet with Reset packet Code 3, 
> |  > |  > |  >     "No Connection", with seqno=0 - as per RFC 4340, 8.3.1;
> |  > |  > |  >  6. server thinks that seqno=0 is out of synch (step 6), sends Sync;
> |  > |  > |  >  7. goto (6).
> |  > |  > |  > 
> |  > |  > |  > The result is a drastic flood of packets: In one occasion I counted
> |  > |  > |  > 345549 Reset/Sync packets, before the server finally killed itself.
> |  > |  > |  > 
> |  > |  > |  > Fix:
> |  > |  > |  > ----
> |  > |  > |  > Since this condition is peculiar and can be distinguished from other
> |  > |  > |  > sequence-invalid packets, a special case has been added. The Reset
> |  > |  > |  > is accepted if
> |  > |  > |  >  * it has Reset Code 3, "No Connection" AND
> |  > |  > |  >  * it has sequence number 0 as described in RFC 4340, 8.3.1.
> |  > |  > |  > 
> |  > |  > |  > If both conditions are satisfied, the Reset is enqueued in the receive queue
> |  > |  > |  > as usual, and will very soon terminate the crashed connection.
> |  > |  > |  > 
> |  > |  > |  > Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
> |  > |  > |  > ---
> |  > |  > |  >  net/dccp/input.c |   17 +++++++++++++++++
> |  > |  > |  >  1 file changed, 17 insertions(+)
> |  > |  > |  > 
> |  > |  > |  > --- a/net/dccp/input.c
> |  > |  > |  > +++ b/net/dccp/input.c
> |  > |  > |  > @@ -155,6 +155,22 @@ static int dccp_check_seqno(struct sock 
> |  > |  > |  >  		    (DCCP_SKB_CB(skb)->dccpd_ack_seq !> |  > |  > |  >  		     DCCP_PKT_WITHOUT_ACK_SEQ))
> |  > |  > |  >  			dp->dccps_gar = DCCP_SKB_CB(skb)->dccpd_ack_seq;
> |  > |  > |  > +
> |  > |  > |  > +	} else if (dh->dccph_type = DCCP_PKT_RESET  &&
> |  > |  > |  > +		   dccp_hdr_reset(skb)->dccph_reset_code =
> |  > |  > |  > +		   DCCP_RESET_CODE_NO_CONNECTION &&
> |  > |  > |  > +		   DCCP_SKB_CB(skb)->dccpd_seq = 0) {
> |  > |  > |  > +		/*
> |  > |  > |  > +		 * This happens when connection is established and client app
> |  > |  > |  > +		 * crashes before server can send data. The crashing client
> |  > |  > |  > +		 * removes connection state, so the server gets a Code 3 Reset
> |  > |  > |  > +		 * packet with seqno 0 (RFC 4340, 8.3.1). Responding here with
> |  > |  > |  > +		 * a Sync leads to a Reset-Storm which will flood the network
> |  > |  > |  > +		 * until the server gives up on this connection or is killed.
> |  > |  > |  > +		 * We let this case pass so that the Reset gets enqueued and
> |  > |  > |  > +		 * will terminate the erratic connection.
> |  > |  > |  > +		 */
> |  > |  > |  > +		DCCP_WARN("DCCP: Peer sent RESET with seqno 0\n");
> |  > |  > |  >  	} else {
> |  > |  > |  >  		DCCP_WARN("DCCP: Step 6 failed for %s packet, "
> |  > |  > |  >  			  "(LSWL(%llu) <= P.seqno(%llu) <= S.SWH(%llu)) and "
> |  > |  > |  > @@ -168,6 +184,7 @@ static int dccp_check_seqno(struct sock 
> |  > |  > |  >  			  (unsigned long long) lawl,
> |  > |  > |  >  			  (unsigned long long) DCCP_SKB_CB(skb)->dccpd_ack_seq,
> |  > |  > |  >  			  (unsigned long long) dp->dccps_awh);
> |  > |  > |  > +		/* FIXME: Rate-limit DCCP-Sync packets as per RFC 4340, 7.5.4 */
> |  > |  > |  >  		dccp_send_sync(sk, DCCP_SKB_CB(skb)->dccpd_seq, DCCP_PKT_SYNC);
> |  > |  > |  >  		return -1;
> |  > |  > |  >  	}
> |  > |  > |  > -
> |  > |  > |  > To unsubscribe from this list: send the line "unsubscribe dccp" in
> |  > |  > |  > the body of a message to majordomo@vger.kernel.org
> |  > |  > |  > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> |  > |  > |  
> |  > |  > |  
> |  > |  
> |  > |  
> |  
> |  

Re: [PATCH 39/43]: Protect against Reset/Sync floods due to buggy applications

[Arnaldo Carvalho de Melo]

On 4/10/07, Eddie Kohler <kohler@cs.ucla.edu> wrote:
> Hi Gerrit,
>
> Gerrit Renker wrote:
> > Listen Eddie,
> >
> > I am not interested in your offline email. All further such email will
> > be directed back to the list.
> >
> > Eddie Kohler wrote:
> > |  Glad to help!
> > |  E
>
> As you requested, this mail is being cc'd to the kernel mailing list.
>
> In future, every time I send you a three-word note, I will make sure to cc the
> kernel mailing list.
>
> I deeply, deeply apologize for whatever it is I have done this time to make
> you angry, although I have no idea what that could be.  I hereby acknowledge
> all of your many thousands of contributions to the DCCP kernel implementation.
>
> All hail Gerrit!!~*$716617*&!197`987(*&!(*&(*@!&(*#&(*@!

Oh, the fun, keep up the good work on reviewing the patches, as usual
for the not so recent past I've been unable to participate fully, and
was feeling ashamed for that, but in hindsight its not that bad, at
least the patches can brew in this so joyfull exchange of messages, at
some point I'll just jump in and read those messages to harvest the
patches for inclusion in mainline, as such I really hope that all
interested parties look at Gerrit and Ian patches and test and report
with it on top of mainline.

Best Regards,

- Arnaldo

Re: [PATCH 39/43]: Protect against Reset/Sync floods due to buggy applications

[Ian McDonald]

On 4/9/07, Gerrit Renker <gerrit@erg.abdn.ac.uk> wrote:
>  * the bug and its cause are documented on
>       http://www.erg.abdn.ac.uk/users/gerrit/dccp/docs/seqno_bug/
>    (in short, the Sync did not acknowledge GSR, but rather P.seqno);
>

Yes I had noticed that bug also at times and never quite got around to
fixing it. Will have to read more about rate limiting though as I'm
not convinced normally about rate limiting schemes etc as they
invariably end up causing problems when doing things like running a
huge server with lots of connections. However as I said, haven't read
it yet so might be wrong on your intent.

Ian
-- 
Web: http://wand.net.nz/~iam4/
Blog: http://iansblog.jandi.co.nz
WAND Network Research Group

Re: [PATCH 39/43]: Protect against Reset/Sync floods due to buggy applications

[Gerrit Renker]

Quoting Ian McDonald:
|  Will have to read more about rate limiting though as I'm
|  not convinced normally about rate limiting schemes etc as they
|  invariably end up causing problems when doing things like running a
|  huge server with lots of connections. However as I said, haven't read
|  it yet so might be wrong on your intent.
RFC 4340. 7.5.4 says that this rate-limiting is a SHOULD. I had similar
thoughts in mind regarding the overhead that this mechanism incurs. That
is why it is kept as simple as possible. Something like xrlim_allow is
already too complicated (cf. comments).

Re: [PATCH 39/43]: Protect against Reset/Sync floods due to buggy applications

[Gerrit Renker]

|  least the patches can brew in this so joyfull exchange of messages, at
|  some point I'll just jump in and read those messages to harvest the
|  patches for inclusion in mainline, as such I really hope that all
|  interested parties look at Gerrit and Ian patches and test and report
|  with it on top of mainline.
Can we please go back to the first round of patches,

 http://www.mail-archive.com/dccp@vger.kernel.org/msg01439.html

and sort those out?
These lay the groundwork for the CCID3 work, Ian acked most of these. 
If there are any issues with these, they will affect the later batch.
 

Re: [PATCH 39/43]: Protect against Reset/Sync floods due to buggy applications

[Gerrit Renker]

No offense taken. Constructive technical feedback remains welcome. 


Quoting Eddie Kohler:
|  Hi Gerrit,
|  
|  Gerrit Renker wrote:
|  > Listen Eddie, 
|  > 
|  > I am not interested in your offline email. All further such email will
|  > be directed back to the list.
|  > 
|  > Eddie Kohler wrote:
|  > |  Glad to help!
|  > |  E
|  
|  As you requested, this mail is being cc'd to the kernel mailing list.
|  
|  In future, every time I send you a three-word note, I will make sure to cc the 
|  kernel mailing list.
|  
|  I deeply, deeply apologize for whatever it is I have done this time to make 
|  you angry, although I have no idea what that could be.  I hereby acknowledge 
|  all of your many thousands of contributions to the DCCP kernel implementation.
|  
|  All hail Gerrit!!~*$716617*&!197`987(*&!(*&(*@!&(*#&(*@!
|  
|  Eddie
|  
|  
|  > |  
|  > |  
|  > |  Gerrit Renker wrote:
|  > |  > Hi Eddie,
|  > |  > 
|  > |  > first of all thank you for help with this bug. 
|  > |  > 
|  > |  > You are correct, the cause is somewhere else, it was an oversight. 
|  > |  > 
|  > |  > Arnaldo, can you please ignore this patch (16g); I have withdrawn it from the online directory. 
|  > |  > 
|  > |  > Instead, 
|  > |  > 
|  > |  >  * a proper bug fix with description follows;
|  > |  > 
|  > |  >  * the bug and its cause are documented on 
|  > |  >       http://www.erg.abdn.ac.uk/users/gerrit/dccp/docs/seqno_bug/
|  > |  >    (in short, the Sync did not acknowledge GSR, but rather P.seqno);
|  > |  > 
|  > |  >  * I have further implemented rate-limiting for Syncs as suggested in 7.5.4. Rate-limiting
|  > |  >    proved to be an efficient countermeasure for this bug: it broke the vicious circle of
|  > |  >    invalid-Reset => Sync => invalid Reset. This was because the second received Reset is
|  > |  >    already subject to rate-limiting; hence there is no Sync in reply; hence no further 
|  > |  >    sequence-invalid Reset is triggered. 
|  > |  >    I believe that rate-limiting can be of similiar help in other situations involving 
|  > |  >    sequence-invalid packets.
|  > |  > 
|  > |  >  * The reason that the bug does not show with the more recent patches is that previously
|  > |  >    it was possible for a DCCP application to silently crash, be killed, terminated without
|  > |  >    its connected peer taking notice of it. Patches 17a and 17b implement an ABORT function
|  > |  >    which sends a DCCP-Reset Code 2 "Aborted" which will terminate the connection. Thus
|  > |  >    mayhem due to half-closed connections is avoided.
|  > |  > 
|  > |  > Thanks again for pointing this out,
|  > |  > Gerrit
|  > |  >    
|  > |  > 
|  > |  > Quoting Eddie Kohler:
|  > |  > |  Hi Gerrit,
|  > |  > |  
|  > |  > |  I agree that the bug is scary!  It would be good to fix.  But this fix isn't a 
|  > |  > |  good fix, especially because it makes it trivially easy to shoot down a 
|  > |  > |  connection if you know the port numbers.  Would you mind withdrawing the patch 
|  > |  > |  until the bug reappears?
|  > |  > |  
|  > |  > |  Eddie
|  > |  > |  
|  > |  > |  
|  > |  > |  Gerrit Renker wrote:
|  > |  > |  > Eddie,
|  > |  > |  > 
|  > |  > |  > I have spent half a day or so trying to reconstruct the condition which triggered this bug.
|  > |  > |  > With the new patches regarding closing state I couldn't trigger this bug anymore. Which is
|  > |  > |  > not to say that it has gone away. 
|  > |  > |  > 
|  > |  > |  > All I can say is that I have observed this bug as described and that it is pretty scary -
|  > |  > |  > the computer freezes for several minutes (until the server connection timer declares the
|  > |  > |  > connection as dead) and it generates a massive load of packets. 
|  > |  > |  > 
|  > |  > |  > I will try to follow this up when I have some more time; as indicated by the comment in 
|  > |  > |  > the patch below, I am lacking the resources to implement rate-limiting for DCCP-Sync; too 
|  > |  > |  > many other things to fix.
|  > |  > |  > 
|  > |  > |  > Anyone else any ideas / suggestions?
|  > |  > |  > 
|  > |  > |  > Gerrit
|  > |  > |  > 
|  > |  > |  > 
|  > |  > |  > Quoting Eddie Kohler:
|  > |  > |  > |  Hi Gerrit,
|  > |  > |  > |  
|  > |  > |  > |  I'm surprised this kind of flood happens & think it may represent a bug in the 
|  > |  > |  > |  stack.  What is the acknowledgement number on the Sync packet sent in step 
|  > |  > |  > |  (6)?  It should be GSR, according to Step 6 of the pseudocode in Section 8.5. 
|  > |  > |  > |    If it was GSR, I would expect the following denouement:
|  > |  > |  > |  
|  > |  > |  > |  5. client responds with Reset packet Code 3 with seqno=0
|  > |  > |  > |  6. server thinks that seqno=0 is out of synch (step 6), sends Sync with 
|  > |  > |  > |  seqno=GSS, ackno=GSR, then increments GSS;
|  > |  > |  > |  7. client responds with Reset packet Code 3 with seqno=GSR+1 and ackno=GSS (it 
|  > |  > |  > |  can do this because it takes the seqno from the received packet's ackno, per 
|  > |  > |  > |  Section 8.3.1);
|  > |  > |  > |  8. the Reset is now in synch, so the server kills the connection.
|  > |  > |  > |  
|  > |  > |  > |  No flood should be possible with valid stacks; some people actually verified 
|  > |  > |  > |  this theoretically.
|  > |  > |  > |  
|  > |  > |  > |  Of course stacks can be INvalid, in which case one should rate-limit Syncs, as 
|  > |  > |  > |  allowed by the protocol.
|  > |  > |  > |  
|  > |  > |  > |  Your solution, which is to accept Resets with seqno 0, makes it trivially 
|  > |  > |  > |  simple for an attacker to kill any connection.  It should not be committed! 
|  > |  > |  > |  Can we figure out why the stack has the chatter problem first?
|  > |  > |  > |  
|  > |  > |  > |  Eddie
|  > |  > |  > |  
|  > |  > |  > |  
|  > |  > |  > |  Gerrit Renker wrote:
|  > |  > |  > |  > [DCCP]: Protect against Reset/Sync floods due to buggy applications
|  > |  > |  > |  > 
|  > |  > |  > |  > This patch protects against Reset/Sync floods which happens as a result
|  > |  > |  > |  > of either buggy or crashing client applications. The Reset/Sync flood
|  > |  > |  > |  > is triggered as follows:
|  > |  > |  > |  > 
|  > |  > |  > |  >  1. Client establishes connection to listening server;
|  > |  > |  > |  >  2. before server can write data to client, client crashes;
|  > |  > |  > |  >  3. crashing client removes connection state at client host;
|  > |  > |  > |  >  4. server still thinks client is alive and sends data;
|  > |  > |  > |  >  5. client responds to server packet with Reset packet Code 3, 
|  > |  > |  > |  >     "No Connection", with seqno=0 - as per RFC 4340, 8.3.1;
|  > |  > |  > |  >  6. server thinks that seqno=0 is out of synch (step 6), sends Sync;
|  > |  > |  > |  >  7. goto (6).
|  > |  > |  > |  > 
|  > |  > |  > |  > The result is a drastic flood of packets: In one occasion I counted
|  > |  > |  > |  > 345549 Reset/Sync packets, before the server finally killed itself.
|  > |  > |  > |  > 
|  > |  > |  > |  > Fix:
|  > |  > |  > |  > ----
|  > |  > |  > |  > Since this condition is peculiar and can be distinguished from other
|  > |  > |  > |  > sequence-invalid packets, a special case has been added. The Reset
|  > |  > |  > |  > is accepted if
|  > |  > |  > |  >  * it has Reset Code 3, "No Connection" AND
|  > |  > |  > |  >  * it has sequence number 0 as described in RFC 4340, 8.3.1.
|  > |  > |  > |  > 
|  > |  > |  > |  > If both conditions are satisfied, the Reset is enqueued in the receive queue
|  > |  > |  > |  > as usual, and will very soon terminate the crashed connection.
|  > |  > |  > |  > 
|  > |  > |  > |  > Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
|  > |  > |  > |  > ---
|  > |  > |  > |  >  net/dccp/input.c |   17 +++++++++++++++++
|  > |  > |  > |  >  1 file changed, 17 insertions(+)
|  > |  > |  > |  > 
|  > |  > |  > |  > --- a/net/dccp/input.c
|  > |  > |  > |  > +++ b/net/dccp/input.c
|  > |  > |  > |  > @@ -155,6 +155,22 @@ static int dccp_check_seqno(struct sock 
|  > |  > |  > |  >  		    (DCCP_SKB_CB(skb)->dccpd_ack_seq !|  > |  > |  > |  >  		     DCCP_PKT_WITHOUT_ACK_SEQ))
|  > |  > |  > |  >  			dp->dccps_gar = DCCP_SKB_CB(skb)->dccpd_ack_seq;
|  > |  > |  > |  > +
|  > |  > |  > |  > +	} else if (dh->dccph_type = DCCP_PKT_RESET  &&
|  > |  > |  > |  > +		   dccp_hdr_reset(skb)->dccph_reset_code =
|  > |  > |  > |  > +		   DCCP_RESET_CODE_NO_CONNECTION &&
|  > |  > |  > |  > +		   DCCP_SKB_CB(skb)->dccpd_seq = 0) {
|  > |  > |  > |  > +		/*
|  > |  > |  > |  > +		 * This happens when connection is established and client app
|  > |  > |  > |  > +		 * crashes before server can send data. The crashing client
|  > |  > |  > |  > +		 * removes connection state, so the server gets a Code 3 Reset
|  > |  > |  > |  > +		 * packet with seqno 0 (RFC 4340, 8.3.1). Responding here with
|  > |  > |  > |  > +		 * a Sync leads to a Reset-Storm which will flood the network
|  > |  > |  > |  > +		 * until the server gives up on this connection or is killed.
|  > |  > |  > |  > +		 * We let this case pass so that the Reset gets enqueued and
|  > |  > |  > |  > +		 * will terminate the erratic connection.
|  > |  > |  > |  > +		 */
|  > |  > |  > |  > +		DCCP_WARN("DCCP: Peer sent RESET with seqno 0\n");
|  > |  > |  > |  >  	} else {
|  > |  > |  > |  >  		DCCP_WARN("DCCP: Step 6 failed for %s packet, "
|  > |  > |  > |  >  			  "(LSWL(%llu) <= P.seqno(%llu) <= S.SWH(%llu)) and "
|  > |  > |  > |  > @@ -168,6 +184,7 @@ static int dccp_check_seqno(struct sock 
|  > |  > |  > |  >  			  (unsigned long long) lawl,
|  > |  > |  > |  >  			  (unsigned long long) DCCP_SKB_CB(skb)->dccpd_ack_seq,
|  > |  > |  > |  >  			  (unsigned long long) dp->dccps_awh);
|  > |  > |  > |  > +		/* FIXME: Rate-limit DCCP-Sync packets as per RFC 4340, 7.5.4 */
|  > |  > |  > |  >  		dccp_send_sync(sk, DCCP_SKB_CB(skb)->dccpd_seq, DCCP_PKT_SYNC);
|  > |  > |  > |  >  		return -1;
|  > |  > |  > |  >  	}
|  > |  > |  > |  > -
|  > |  > |  > |  > To unsubscribe from this list: send the line "unsubscribe dccp" in
|  > |  > |  > |  > the body of a message to majordomo@vger.kernel.org
|  > |  > |  > |  > More majordomo info at  http://vger.kernel.org/majordomo-info.html
|  > |  > |  > |  
|  > |  > |  > |  
|  > |  > |  
|  > |  > |  
|  > |  
|  > |  
|  
|  

Re: [PATCH 39/43]: Protect against Reset/Sync floods due to buggy

[Eddie Kohler]

Gerrit Renker wrote:
> Quoting Ian McDonald:
> |  Will have to read more about rate limiting though as I'm
> |  not convinced normally about rate limiting schemes etc as they
> |  invariably end up causing problems when doing things like running a
> |  huge server with lots of connections.

We pictured the rate limit (if implemented) as per connection, not for the 
whole stack, which is what Gerrit has done.  If the rate limit is considered 
too expensive it's fine to take out, of course; it is only a SHOULD.

Eddie


> |  However as I said, haven't read
> |  it yet so might be wrong on your intent.
> RFC 4340. 7.5.4 says that this rate-limiting is a SHOULD. I had similar
> thoughts in mind regarding the overhead that this mechanism incurs. That
> is why it is kept as simple as possible. Something like xrlim_allow is
> already too complicated (cf. comments).
> -
> To unsubscribe from this list: send the line "unsubscribe dccp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 39/43]: Protect against Reset/Sync floods due to buggy applications

[Gerrit Renker]

I do suggest to keep it, otherwise there is a security hole 
which can very easily be exploited. 

Especially after the experiences with a computer freezing up 
for several minutes due to Sync floods,
where rate-limiting alone has shown to be a viable countermeasure. 

Rate-limiting is on a per-socket basis, not for the whole stack. 

If one wants to take it out, it can be disabled by setting the parameter to 0. 

The implementation is lightweight and does not require complex calculations.

|  Gerrit Renker wrote:
|  > Quoting Ian McDonald:
|  > |  Will have to read more about rate limiting though as I'm
|  > |  not convinced normally about rate limiting schemes etc as they
|  > |  invariably end up causing problems when doing things like running a
|  > |  huge server with lots of connections.
|  
|  We pictured the rate limit (if implemented) as per connection, not for the 
|  whole stack, which is what Gerrit has done.  If the rate limit is considered 
|  too expensive it's fine to take out, of course; it is only a SHOULD.
|  
|  Eddie
|  
|  
|  > |  However as I said, haven't read
|  > |  it yet so might be wrong on your intent.
|  > RFC 4340. 7.5.4 says that this rate-limiting is a SHOULD. I had similar
|  > thoughts in mind regarding the overhead that this mechanism incurs. That
|  > is why it is kept as simple as possible. Something like xrlim_allow is
|  > already too complicated (cf. comments).
|  > -
|  > To unsubscribe from this list: send the line "unsubscribe dccp" in
|  > the body of a message to majordomo@vger.kernel.org
|  > More majordomo info at  http://vger.kernel.org/majordomo-info.html
|  
|