Re: [PATCHv2 net] sctp: delay the authentication for the duplicated cookie-echo chunk

[David Miller]

From: Xin Long 
Date: Sat,  5 May 2018 14:59:47 +0800

> Now sctp only delays the authentication for the normal cookie-echo
> chunk by setting chunk->auth_chunk in sctp_endpoint_bh_rcv(). But
> for the duplicated one with auth, in sctp_assoc_bh_rcv(), it does
> authentication first based on the old asoc, which will definitely
> fail due to the different auth info in the old asoc.
> 
> The duplicated cookie-echo chunk will create a new asoc with the
> auth info from this chunk, and the authentication should also be
> done with the new asoc's auth info for all of the collision 'A',
> 'B' and 'D'. Otherwise, the duplicated cookie-echo chunk with auth
> will never pass the authentication and create the new connection.
> 
> This issue exists since very beginning, and this fix is to make
> sctp_assoc_bh_rcv() follow the way sctp_endpoint_bh_rcv() does
> for the normal cookie-echo chunk to delay the authentication.
> 
> While at it, remove the unused params from sctp_sf_authenticate()
> and define sctp_auth_chunk_verify() used for all the places that
> do the delayed authentication.
> 
> v1->v2:
>   fix the typo in changelog as Marcelo noticed.
> 
> Acked-by: Marcelo Ricardo Leitner 
> Signed-off-by: Xin Long 

Applied, thanks.

Re: [PATCHv2 net] sctp: delay the authentication for the duplicated cookie-echo chunk

[Neil Horman]

On Sat, May 05, 2018 at 02:59:47PM +0800, Xin Long wrote:
> Now sctp only delays the authentication for the normal cookie-echo
> chunk by setting chunk->auth_chunk in sctp_endpoint_bh_rcv(). But
> for the duplicated one with auth, in sctp_assoc_bh_rcv(), it does
> authentication first based on the old asoc, which will definitely
> fail due to the different auth info in the old asoc.
> 
> The duplicated cookie-echo chunk will create a new asoc with the
> auth info from this chunk, and the authentication should also be
> done with the new asoc's auth info for all of the collision 'A',
> 'B' and 'D'. Otherwise, the duplicated cookie-echo chunk with auth
> will never pass the authentication and create the new connection.
> 
> This issue exists since very beginning, and this fix is to make
> sctp_assoc_bh_rcv() follow the way sctp_endpoint_bh_rcv() does
> for the normal cookie-echo chunk to delay the authentication.
> 
> While at it, remove the unused params from sctp_sf_authenticate()
> and define sctp_auth_chunk_verify() used for all the places that
> do the delayed authentication.
> 
> v1->v2:
>   fix the typo in changelog as Marcelo noticed.
> 
> Acked-by: Marcelo Ricardo Leitner 
> Signed-off-by: Xin Long 
> ---
>  net/sctp/associola.c| 30 -
>  net/sctp/sm_statefuns.c | 86 
> ++---
>  2 files changed, 75 insertions(+), 41 deletions(-)
> 
> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> index 837806d..a47179d 100644
> --- a/net/sctp/associola.c
> +++ b/net/sctp/associola.c
> @@ -1024,8 +1024,9 @@ static void sctp_assoc_bh_rcv(struct work_struct *work)
>   struct sctp_endpoint *ep;
>   struct sctp_chunk *chunk;
>   struct sctp_inq *inqueue;
> - int state;
> + int first_time = 1; /* is this the first time through the loop */
>   int error = 0;
> + int state;
>  
>   /* The association should be held so we should be safe. */
>   ep = asoc->ep;
> @@ -1036,6 +1037,30 @@ static void sctp_assoc_bh_rcv(struct work_struct *work)
>   state = asoc->state;
>   subtype = SCTP_ST_CHUNK(chunk->chunk_hdr->type);
>  
> + /* If the first chunk in the packet is AUTH, do special
> +  * processing specified in Section 6.3 of SCTP-AUTH spec
> +  */
> + if (first_time && subtype.chunk == SCTP_CID_AUTH) {
> + struct sctp_chunkhdr *next_hdr;
> +
> + next_hdr = sctp_inq_peek(inqueue);
> + if (!next_hdr)
> + goto normal;
> +
> + /* If the next chunk is COOKIE-ECHO, skip the AUTH
> +  * chunk while saving a pointer to it so we can do
> +  * Authentication later (during cookie-echo
> +  * processing).
> +  */
> + if (next_hdr->type == SCTP_CID_COOKIE_ECHO) {
> + chunk->auth_chunk = skb_clone(chunk->skb,
> +   GFP_ATOMIC);
> + chunk->auth = 1;
> + continue;
> + }
> + }
> +
> +normal:
>   /* SCTP-AUTH, Section 6.3:
>*The receiver has a list of chunk types which it expects
>*to be received only after an AUTH-chunk.  This list has
> @@ -1074,6 +1099,9 @@ static void sctp_assoc_bh_rcv(struct work_struct *work)
>   /* If there is an error on chunk, discard this packet. */
>   if (error && chunk)
>   chunk->pdiscard = 1;
> +
> + if (first_time)
> + first_time = 0;
>   }
>   sctp_association_put(asoc);
>  }
> diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
> index 28c070e..c9ae340 100644
> --- a/net/sctp/sm_statefuns.c
> +++ b/net/sctp/sm_statefuns.c
> @@ -153,10 +153,7 @@ static enum sctp_disposition sctp_sf_violation_chunk(
>   struct sctp_cmd_seq *commands);
>  
>  static enum sctp_ierror sctp_sf_authenticate(
> - struct net *net,
> - const struct sctp_endpoint *ep,
>   const struct sctp_association *asoc,
> - const union sctp_subtype type,
>   struct sctp_chunk *chunk);
>  
>  static enum sctp_disposition __sctp_sf_do_9_1_abort(
> @@ -626,6 +623,38 @@ enum sctp_disposition sctp_sf_do_5_1C_ack(struct net 
> *net,
>   return SCTP_DISPOSITION_CONSUME;
>  }
>  
> +static bool sctp_auth_chunk_verify(struct net *net, struct sctp_chunk *chunk,
> +const struct sctp_association *asoc)
> +{
> + struct sctp_chunk auth;
> +
> + if 

[PATCHv2 net] sctp: delay the authentication for the duplicated cookie-echo chunk

[Xin Long]

Now sctp only delays the authentication for the normal cookie-echo
chunk by setting chunk->auth_chunk in sctp_endpoint_bh_rcv(). But
for the duplicated one with auth, in sctp_assoc_bh_rcv(), it does
authentication first based on the old asoc, which will definitely
fail due to the different auth info in the old asoc.

The duplicated cookie-echo chunk will create a new asoc with the
auth info from this chunk, and the authentication should also be
done with the new asoc's auth info for all of the collision 'A',
'B' and 'D'. Otherwise, the duplicated cookie-echo chunk with auth
will never pass the authentication and create the new connection.

This issue exists since very beginning, and this fix is to make
sctp_assoc_bh_rcv() follow the way sctp_endpoint_bh_rcv() does
for the normal cookie-echo chunk to delay the authentication.

While at it, remove the unused params from sctp_sf_authenticate()
and define sctp_auth_chunk_verify() used for all the places that
do the delayed authentication.

v1->v2:
  fix the typo in changelog as Marcelo noticed.

Acked-by: Marcelo Ricardo Leitner 
Signed-off-by: Xin Long 
---
 net/sctp/associola.c| 30 -
 net/sctp/sm_statefuns.c | 86 ++---
 2 files changed, 75 insertions(+), 41 deletions(-)

diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index 837806d..a47179d 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -1024,8 +1024,9 @@ static void sctp_assoc_bh_rcv(struct work_struct *work)
struct sctp_endpoint *ep;
struct sctp_chunk *chunk;
struct sctp_inq *inqueue;
-   int state;
+   int first_time = 1; /* is this the first time through the loop */
int error = 0;
+   int state;
 
/* The association should be held so we should be safe. */
ep = asoc->ep;
@@ -1036,6 +1037,30 @@ static void sctp_assoc_bh_rcv(struct work_struct *work)
state = asoc->state;
subtype = SCTP_ST_CHUNK(chunk->chunk_hdr->type);
 
+   /* If the first chunk in the packet is AUTH, do special
+* processing specified in Section 6.3 of SCTP-AUTH spec
+*/
+   if (first_time && subtype.chunk == SCTP_CID_AUTH) {
+   struct sctp_chunkhdr *next_hdr;
+
+   next_hdr = sctp_inq_peek(inqueue);
+   if (!next_hdr)
+   goto normal;
+
+   /* If the next chunk is COOKIE-ECHO, skip the AUTH
+* chunk while saving a pointer to it so we can do
+* Authentication later (during cookie-echo
+* processing).
+*/
+   if (next_hdr->type == SCTP_CID_COOKIE_ECHO) {
+   chunk->auth_chunk = skb_clone(chunk->skb,
+ GFP_ATOMIC);
+   chunk->auth = 1;
+   continue;
+   }
+   }
+
+normal:
/* SCTP-AUTH, Section 6.3:
 *The receiver has a list of chunk types which it expects
 *to be received only after an AUTH-chunk.  This list has
@@ -1074,6 +1099,9 @@ static void sctp_assoc_bh_rcv(struct work_struct *work)
/* If there is an error on chunk, discard this packet. */
if (error && chunk)
chunk->pdiscard = 1;
+
+   if (first_time)
+   first_time = 0;
}
sctp_association_put(asoc);
 }
diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
index 28c070e..c9ae340 100644
--- a/net/sctp/sm_statefuns.c
+++ b/net/sctp/sm_statefuns.c
@@ -153,10 +153,7 @@ static enum sctp_disposition sctp_sf_violation_chunk(
struct sctp_cmd_seq *commands);
 
 static enum sctp_ierror sctp_sf_authenticate(
-   struct net *net,
-   const struct sctp_endpoint *ep,
const struct sctp_association *asoc,
-   const union sctp_subtype type,
struct sctp_chunk *chunk);
 
 static enum sctp_disposition __sctp_sf_do_9_1_abort(
@@ -626,6 +623,38 @@ enum sctp_disposition sctp_sf_do_5_1C_ack(struct net *net,
return SCTP_DISPOSITION_CONSUME;
 }
 
+static bool sctp_auth_chunk_verify(struct net *net, struct sctp_chunk *chunk,
+  const struct sctp_association *asoc)
+{
+   struct sctp_chunk auth;
+
+   if (!chunk->auth_chunk)
+   return true;
+
+   /* SCTP-AUTH:  auth_chunk pointer is only set when the cookie-echo
+* is supposed to be authenticated and we have to do delayed