[PATCH net 0/2] sctp: validate INIT in COOKIE-ECHO when auth disabled

[PATCH net 0/2] sctp: validate INIT in COOKIE-ECHO when auth disabled

[Xin Long]

This series fixes a security gap in SCTP's COOKIE-ECHO handling when
cookie authentication is disabled.

Currently, INIT chunks embedded in cookies are not re-verified after
unpacking, creating a vulnerability when cookie_auth_enable=0. This
series first refactors error handling, then adds the missing validation.

Xin Long (2):
  sctp: factor out INIT verification failure handling
  sctp: add INIT verification after cookie unpacking

 net/sctp/sm_make_chunk.c |   2 +-
 net/sctp/sm_statefuns.c  | 200 +++++++++++++++++++--------------------
 2 files changed, 99 insertions(+), 103 deletions(-)

-- 
2.47.1

[PATCH net 1/2] sctp: factor out INIT verification failure handling

[Xin Long]

Extract the duplicated INIT/INIT-ACK error handling logic into a new
helper, sctp_abort_on_init_err().

Several state functions open-code the same pattern after
sctp_verify_init() fails: construct an ABORT with error causes if
available, send it when allocation succeeds, or fall back to T-bit ABORT
handling when no error chunk is present. INIT-ACK handling also includes
additional teardown logic for malformed packets.

Move this logic into sctp_abort_on_init_err() to reduce duplication and
centralize INIT/INIT-ACK failure handling.

No functional change intended. The helper will be used in a subsequent
patch.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sctp/sm_statefuns.c | 171 +++++++++++++++++-----------------------
 1 file changed, 72 insertions(+), 99 deletions(-)

diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
index 9b23c11cbb9e..544f308ee527 100644
--- a/net/sctp/sm_statefuns.c
+++ b/net/sctp/sm_statefuns.c
@@ -68,6 +68,14 @@ static void sctp_send_stale_cookie_err(struct net *net,
 				       const struct sctp_chunk *chunk,
 				       struct sctp_cmd_seq *commands,
 				       struct sctp_chunk *err_chunk);
+static enum sctp_disposition sctp_abort_on_init_err(
+					struct net *net,
+					const struct sctp_endpoint *ep,
+					const struct sctp_association *asoc,
+					const struct sctp_chunk *chunk,
+					void *arg,
+					struct sctp_cmd_seq *commands,
+					struct sctp_chunk *err_chunk);
 static enum sctp_disposition sctp_sf_do_5_2_6_stale(
 					struct net *net,
 					const struct sctp_endpoint *ep,
@@ -325,7 +333,6 @@ enum sctp_disposition sctp_sf_do_5_1B_init(struct net *net,
 	struct sctp_chunk *chunk = arg, *repl, *err_chunk;
 	struct sctp_unrecognized_param *unk_param;
 	struct sctp_association *new_asoc;
-	struct sctp_packet *packet;
 	int len;
 
 	/* 6.10 Bundling
@@ -375,32 +382,9 @@ enum sctp_disposition sctp_sf_do_5_1B_init(struct net *net,
 	err_chunk = NULL;
 	if (!sctp_verify_init(net, ep, asoc, chunk->chunk_hdr->type,
 			      (struct sctp_init_chunk *)chunk->chunk_hdr, chunk,
-			      &err_chunk)) {
-		/* This chunk contains fatal error. It is to be discarded.
-		 * Send an ABORT, with causes if there is any.
-		 */
-		if (err_chunk) {
-			packet = sctp_abort_pkt_new(net, ep, asoc, arg,
-					(__u8 *)(err_chunk->chunk_hdr) +
-					sizeof(struct sctp_chunkhdr),
-					ntohs(err_chunk->chunk_hdr->length) -
-					sizeof(struct sctp_chunkhdr));
-
-			sctp_chunk_free(err_chunk);
-
-			if (packet) {
-				sctp_add_cmd_sf(commands, SCTP_CMD_SEND_PKT,
-						SCTP_PACKET(packet));
-				SCTP_INC_STATS(net, SCTP_MIB_OUTCTRLCHUNKS);
-				return SCTP_DISPOSITION_CONSUME;
-			} else {
-				return SCTP_DISPOSITION_NOMEM;
-			}
-		} else {
-			return sctp_sf_tabort_8_4_8(net, ep, asoc, type, arg,
-						    commands);
-		}
-	}
+			      &err_chunk))
+		return sctp_abort_on_init_err(net, ep, asoc, chunk, arg,
+					      commands, err_chunk);
 
 	/* Grab the INIT header.  */
 	chunk->subh.init_hdr = (struct sctp_inithdr *)chunk->skb->data;
@@ -525,7 +509,6 @@ enum sctp_disposition sctp_sf_do_5_1C_ack(struct net *net,
 	struct sctp_init_chunk *initchunk;
 	struct sctp_chunk *chunk = arg;
 	struct sctp_chunk *err_chunk;
-	struct sctp_packet *packet;
 
 	if (!sctp_vtag_verify(chunk, asoc))
 		return sctp_sf_pdiscard(net, ep, asoc, type, arg, commands);
@@ -548,50 +531,9 @@ enum sctp_disposition sctp_sf_do_5_1C_ack(struct net *net,
 	err_chunk = NULL;
 	if (!sctp_verify_init(net, ep, asoc, chunk->chunk_hdr->type,
 			      (struct sctp_init_chunk *)chunk->chunk_hdr, chunk,
-			      &err_chunk)) {
-
-		enum sctp_error error = SCTP_ERROR_NO_RESOURCE;
-
-		/* This chunk contains fatal error. It is to be discarded.
-		 * Send an ABORT, with causes.  If there are no causes,
-		 * then there wasn't enough memory.  Just terminate
-		 * the association.
-		 */
-		if (err_chunk) {
-			packet = sctp_abort_pkt_new(net, ep, asoc, arg,
-					(__u8 *)(err_chunk->chunk_hdr) +
-					sizeof(struct sctp_chunkhdr),
-					ntohs(err_chunk->chunk_hdr->length) -
-					sizeof(struct sctp_chunkhdr));
-
-			sctp_chunk_free(err_chunk);
-
-			if (packet) {
-				sctp_add_cmd_sf(commands, SCTP_CMD_SEND_PKT,
-						SCTP_PACKET(packet));
-				SCTP_INC_STATS(net, SCTP_MIB_OUTCTRLCHUNKS);
-				error = SCTP_ERROR_INV_PARAM;
-			}
-		}
-
-		/* SCTP-AUTH, Section 6.3:
-		 *    It should be noted that if the receiver wants to tear
-		 *    down an association in an authenticated way only, the
-		 *    handling of malformed packets should not result in
-		 *    tearing down the association.
-		 *
-		 * This means that if we only want to abort associations
-		 * in an authenticated way (i.e AUTH+ABORT), then we
-		 * can't destroy this association just because the packet
-		 * was malformed.
-		 */
-		if (sctp_auth_recv_cid(SCTP_CID_ABORT, asoc))
-			return sctp_sf_pdiscard(net, ep, asoc, type, arg, commands);
-
-		SCTP_INC_STATS(net, SCTP_MIB_ABORTEDS);
-		return sctp_stop_t1_and_abort(net, commands, error, ECONNREFUSED,
-						asoc, chunk->transport);
-	}
+			      &err_chunk))
+		return sctp_abort_on_init_err(net, ep, asoc, chunk, arg,
+					      commands, err_chunk);
 
 	/* Tag the variable length parameters.  Note that we never
 	 * convert the parameters in an INIT chunk.
@@ -1522,7 +1464,6 @@ static enum sctp_disposition sctp_sf_do_unexpected_init(
 	struct sctp_unrecognized_param *unk_param;
 	struct sctp_association *new_asoc;
 	enum sctp_disposition retval;
-	struct sctp_packet *packet;
 	int len;
 
 	/* 6.10 Bundling
@@ -1566,31 +1507,9 @@ static enum sctp_disposition sctp_sf_do_unexpected_init(
 	err_chunk = NULL;
 	if (!sctp_verify_init(net, ep, asoc, chunk->chunk_hdr->type,
 			      (struct sctp_init_chunk *)chunk->chunk_hdr, chunk,
-			      &err_chunk)) {
-		/* This chunk contains fatal error. It is to be discarded.
-		 * Send an ABORT, with causes if there is any.
-		 */
-		if (err_chunk) {
-			packet = sctp_abort_pkt_new(net, ep, asoc, arg,
-					(__u8 *)(err_chunk->chunk_hdr) +
-					sizeof(struct sctp_chunkhdr),
-					ntohs(err_chunk->chunk_hdr->length) -
-					sizeof(struct sctp_chunkhdr));
-
-			if (packet) {
-				sctp_add_cmd_sf(commands, SCTP_CMD_SEND_PKT,
-						SCTP_PACKET(packet));
-				SCTP_INC_STATS(net, SCTP_MIB_OUTCTRLCHUNKS);
-				retval = SCTP_DISPOSITION_CONSUME;
-			} else {
-				retval = SCTP_DISPOSITION_NOMEM;
-			}
-			goto cleanup;
-		} else {
-			return sctp_sf_tabort_8_4_8(net, ep, asoc, type, arg,
-						    commands);
-		}
-	}
+			      &err_chunk))
+		return sctp_abort_on_init_err(net, ep, asoc, chunk, arg,
+					      commands, err_chunk);
 
 	/*
 	 * Other parameters for the endpoint SHOULD be copied from the
@@ -1691,7 +1610,6 @@ static enum sctp_disposition sctp_sf_do_unexpected_init(
 nomem_retval:
 	if (new_asoc)
 		sctp_association_free(new_asoc);
-cleanup:
 	if (err_chunk)
 		sctp_chunk_free(err_chunk);
 	return retval;
@@ -6485,6 +6403,61 @@ static void sctp_send_stale_cookie_err(struct net *net,
 	}
 }
 
+static enum sctp_disposition sctp_abort_on_init_err(
+					struct net *net,
+					const struct sctp_endpoint *ep,
+					const struct sctp_association *asoc,
+					const struct sctp_chunk *chunk,
+					void *arg,
+					struct sctp_cmd_seq *commands,
+					struct sctp_chunk *err_chunk)
+{
+	enum sctp_error error = SCTP_ERROR_NO_RESOURCE;
+	struct sctp_packet *packet;
+	struct sctp_chunkhdr *ch;
+
+	if (!err_chunk)
+		return sctp_sf_tabort_8_4_8(net, ep, asoc, SCTP_ST_CHUNK(0),
+					    arg, commands);
+
+	ch = err_chunk->chunk_hdr;
+	packet = sctp_abort_pkt_new(net, ep, asoc, arg,
+				    (__u8 *)ch + sizeof(*ch),
+				    ntohs(ch->length) - sizeof(*ch));
+
+	sctp_chunk_free(err_chunk);
+
+	if (packet) {
+		sctp_add_cmd_sf(commands, SCTP_CMD_SEND_PKT,
+				SCTP_PACKET(packet));
+		SCTP_INC_STATS(net, SCTP_MIB_OUTCTRLCHUNKS);
+		error = SCTP_ERROR_INV_PARAM;
+	}
+
+	if (chunk->chunk_hdr->type != SCTP_CID_INIT_ACK) {
+		if (!packet)
+			return SCTP_DISPOSITION_NOMEM;
+		return SCTP_DISPOSITION_CONSUME;
+	}
+	/* SCTP-AUTH, Section 6.3:
+	 *    It should be noted that if the receiver wants to tear
+	 *    down an association in an authenticated way only, the
+	 *    handling of malformed packets should not result in
+	 *    tearing down the association.
+	 *
+	 * This means that if we only want to abort associations
+	 * in an authenticated way (i.e AUTH+ABORT), then we
+	 * can't destroy this association just because the packet
+	 * was malformed.
+	 */
+	if (sctp_auth_recv_cid(SCTP_CID_ABORT, asoc))
+		return sctp_sf_pdiscard(net, ep, asoc, SCTP_ST_CHUNK(0), arg,
+					commands);
+
+	SCTP_INC_STATS(net, SCTP_MIB_ABORTEDS);
+	return sctp_stop_t1_and_abort(net, commands, error, ECONNREFUSED,
+				      asoc, chunk->transport);
+}
 
 /* Process a data chunk */
 static int sctp_eat_data(const struct sctp_association *asoc,
-- 
2.47.1

[PATCH net 2/2] sctp: add INIT verification after cookie unpacking

[Xin Long]

In SCTP handshake, the INIT chunk is initially processed by the server
and embedded into the cookie carried in INIT-ACK. The client then
returns this cookie via COOKIE-ECHO, where the server unpacks it and
reconstructs the original INIT chunk.

When cookie authentication is enabled, the cookie contents are protected
against tampering, so reusing the unpacked INIT without re-verification
is safe.

However, when cookie authentication is disabled, the reconstructed INIT
can no longer be trusted. In this case, the INIT must be explicitly
validated after unpacking to avoid processing potentially tampered data.

Add sctp_verify_init() checks after cookie unpacking in COOKIE-ECHO
processing paths (sctp_sf_do_5_1D_ce() and sctp_sf_do_5_2_4_dupcook())
when cookie_auth_enable is disabled. On failure, the association is
freed and an ABORT is generated via sctp_abort_on_init_err().

Also update sctp_verify_init() to validate parameter bounds against the
actual peer_init length rather than chunk->chunk_end, since peer_init
may not span the full chunk buffer in COOKIE-ECHO.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sctp/sm_make_chunk.c |  2 +-
 net/sctp/sm_statefuns.c  | 29 ++++++++++++++++++++++++++---
 2 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index 41958b8e59fd..21b9eb1c02e9 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -2298,7 +2298,7 @@ int sctp_verify_init(struct net *net, const struct sctp_endpoint *ep,
 	 * VIOLATION error.  We build the ERROR chunk here and let the normal
 	 * error handling code build and send the packet.
 	 */
-	if (param.v != (void *)chunk->chunk_end)
+	if (param.v != (void *)peer_init + ntohs(peer_init->chunk_hdr.length))
 		return sctp_process_inv_paramlength(asoc, param.p, chunk, errp);
 
 	/* The only missing mandatory param possible today is
diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
index 544f308ee527..a11a18678279 100644
--- a/net/sctp/sm_statefuns.c
+++ b/net/sctp/sm_statefuns.c
@@ -647,10 +647,10 @@ enum sctp_disposition sctp_sf_do_5_1D_ce(struct net *net,
 					 struct sctp_cmd_seq *commands)
 {
 	struct sctp_ulpevent *ev, *ai_ev = NULL, *auth_ev = NULL;
+	struct sctp_chunk *err_chk_p = NULL;
 	struct sctp_association *new_asoc;
 	struct sctp_init_chunk *peer_init;
 	struct sctp_chunk *chunk = arg;
-	struct sctp_chunk *err_chk_p;
 	struct sctp_chunk *repl;
 	struct sock *sk;
 	int error = 0;
@@ -725,6 +725,17 @@ enum sctp_disposition sctp_sf_do_5_1D_ce(struct net *net,
 		}
 	}
 
+	peer_init = (struct sctp_init_chunk *)(chunk->subh.cookie_hdr + 1);
+	if (!sctp_sk(sk)->cookie_auth_enable &&
+	    !sctp_verify_init(net, ep, asoc, peer_init->chunk_hdr.type,
+			      peer_init, chunk, &err_chk_p)) {
+		sctp_association_free(new_asoc);
+		return sctp_abort_on_init_err(net, ep, asoc, chunk, arg,
+					      commands, err_chk_p);
+	}
+	if (err_chk_p)
+		sctp_chunk_free(err_chk_p);
+
 	if (security_sctp_assoc_request(new_asoc, chunk->head_skb ?: chunk->skb)) {
 		sctp_association_free(new_asoc);
 		return sctp_sf_pdiscard(net, ep, asoc, type, arg, commands);
@@ -738,7 +749,6 @@ enum sctp_disposition sctp_sf_do_5_1D_ce(struct net *net,
 	/* This is a brand-new association, so these are not yet side
 	 * effects--it is safe to run them here.
 	 */
-	peer_init = (struct sctp_init_chunk *)(chunk->subh.cookie_hdr + 1);
 	if (!sctp_process_init(new_asoc, chunk,
 			       &chunk->subh.cookie_hdr->c.peer_addr,
 			       peer_init, GFP_ATOMIC))
@@ -2128,10 +2138,11 @@ enum sctp_disposition sctp_sf_do_5_2_4_dupcook(
 					void *arg,
 					struct sctp_cmd_seq *commands)
 {
+	struct sctp_chunk *err_chk_p = NULL;
 	struct sctp_association *new_asoc;
+	struct sctp_init_chunk *peer_init;
 	struct sctp_chunk *chunk = arg;
 	enum sctp_disposition retval;
-	struct sctp_chunk *err_chk_p;
 	int error = 0;
 	char action;
 
@@ -2200,6 +2211,18 @@ enum sctp_disposition sctp_sf_do_5_2_4_dupcook(
 	switch (action) {
 	case 'A': /* Association restart. */
 	case 'B': /* Collision case B. */
+		peer_init = (struct sctp_init_chunk *)
+				(chunk->subh.cookie_hdr + 1);
+		if (!sctp_sk(ep->base.sk)->cookie_auth_enable &&
+		    !sctp_verify_init(net, ep, asoc, peer_init->chunk_hdr.type,
+				      peer_init, chunk, &err_chk_p)) {
+			sctp_association_free(new_asoc);
+			return sctp_abort_on_init_err(net, ep, asoc, chunk, arg,
+						      commands, err_chk_p);
+		}
+		if (err_chk_p)
+			sctp_chunk_free(err_chk_p);
+		fallthrough;
 	case 'D': /* Collision case D. */
 		/* Update socket peer label if first association. */
 		if (security_sctp_assoc_request((struct sctp_association *)asoc,
-- 
2.47.1

Re: [PATCH net 1/2] sctp: factor out INIT verification failure handling

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
sctp: factor out INIT verification failure handling

This refactor moves the duplicated INIT/INIT-ACK error handling out of
sctp_sf_do_5_1B_init(), sctp_sf_do_5_1C_ack(), and
sctp_sf_do_unexpected_init() into a new helper sctp_abort_on_init_err().
The commit message states "No functional change intended."

> diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
> index 9b23c11cbb9e..544f308ee527 100644
> --- a/net/sctp/sm_statefuns.c
> +++ b/net/sctp/sm_statefuns.c

[ ... ]

> @@ -548,50 +531,9 @@ enum sctp_disposition sctp_sf_do_5_1C_ack(struct net *net,
>  	err_chunk = NULL;
>  	if (!sctp_verify_init(net, ep, asoc, chunk->chunk_hdr->type,
>  			      (struct sctp_init_chunk *)chunk->chunk_hdr, chunk,
> -			      &err_chunk)) {
> -
> -		enum sctp_error error = SCTP_ERROR_NO_RESOURCE;
> -
> -		/* This chunk contains fatal error. It is to be discarded.
> -		 * Send an ABORT, with causes.  If there are no causes,
> -		 * then there wasn't enough memory.  Just terminate
> -		 * the association.
> -		 */
> -		if (err_chunk) {
> -			packet = sctp_abort_pkt_new(net, ep, asoc, arg,
> -					(__u8 *)(err_chunk->chunk_hdr) +
> -					sizeof(struct sctp_chunkhdr),
> -					ntohs(err_chunk->chunk_hdr->length) -
> -					sizeof(struct sctp_chunkhdr));
> -
> -			sctp_chunk_free(err_chunk);
> -
> -			if (packet) {
> -				sctp_add_cmd_sf(commands, SCTP_CMD_SEND_PKT,
> -						SCTP_PACKET(packet));
> -				SCTP_INC_STATS(net, SCTP_MIB_OUTCTRLCHUNKS);
> -				error = SCTP_ERROR_INV_PARAM;
> -			}
> -		}
> -
> -		/* SCTP-AUTH, Section 6.3:
> -		 *    It should be noted that if the receiver wants to tear
> -		 *    down an association in an authenticated way only, the
> -		 *    handling of malformed packets should not result in
> -		 *    tearing down the association.
> -		 *
> -		 * This means that if we only want to abort associations
> -		 * in an authenticated way (i.e AUTH+ABORT), then we
> -		 * can't destroy this association just because the packet
> -		 * was malformed.
> -		 */
> -		if (sctp_auth_recv_cid(SCTP_CID_ABORT, asoc))
> -			return sctp_sf_pdiscard(net, ep, asoc, type, arg, commands);
> -
> -		SCTP_INC_STATS(net, SCTP_MIB_ABORTEDS);
> -		return sctp_stop_t1_and_abort(net, commands, error, ECONNREFUSED,
> -						asoc, chunk->transport);
> -	}
> +			      &err_chunk))
> +		return sctp_abort_on_init_err(net, ep, asoc, chunk, arg,
> +					      commands, err_chunk);

[High]
In the original sctp_sf_do_5_1C_ack() the "if (err_chunk) { ... }"
block is non-controlling: when err_chunk is NULL the code skips the
block, falls through with error == SCTP_ERROR_NO_RESOURCE, consults
sctp_auth_recv_cid(SCTP_CID_ABORT, asoc) and either pdiscards or runs
sctp_stop_t1_and_abort() to tear the half-open association down.

[High]
In the new helper, can the INIT-ACK + NULL err_chunk case still reach
that path?  Looking at sctp_abort_on_init_err():

  if (!err_chunk)
          return sctp_sf_tabort_8_4_8(net, ep, asoc, SCTP_ST_CHUNK(0),
                                      arg, commands);

  ...
  if (chunk->chunk_hdr->type != SCTP_CID_INIT_ACK) {
          ...
  }
  /* SCTP-AUTH, Section 6.3 ... */
  if (sctp_auth_recv_cid(SCTP_CID_ABORT, asoc))
          return sctp_sf_pdiscard(...);
  SCTP_INC_STATS(net, SCTP_MIB_ABORTEDS);
  return sctp_stop_t1_and_abort(net, commands, error, ECONNREFUSED,
                                asoc, chunk->transport);

[High]
The !err_chunk early return runs before the INIT-ACK type check, so
when sctp_verify_init() returns 0 with *errp left as NULL on an
INIT-ACK, control now goes to sctp_sf_tabort_8_4_8() rather than the
auth-check + sctp_stop_t1_and_abort() path.

[High]
Is the NULL-err_chunk case actually reachable on an INIT-ACK?  In
sctp_process_inv_mandatory() and sctp_process_inv_paramlength() in
net/sctp/sm_make_chunk.c, *errp is allocated via sctp_make_op_error_space()
or sctp_make_violation_paramlen() and the function returns 0
unconditionally:

  if (!*errp)
          *errp = sctp_make_op_error_space(asoc, chunk, 0);
  if (*errp)
          sctp_init_cause(...);
  return 0;

So an allocation failure on a malformed INIT-ACK reaches the helper
with err_chunk == NULL.

[High]
On that path, does the new code skip several actions that the original
performed?  sctp_sf_tabort_8_4_8() emits an OOTB ABORT and pdiscards;
it does not stop T1-init, does not call SCTP_CMD_SET_SK_ERR(ECONNREFUSED),
does not issue SCTP_CMD_INIT_FAILED, does not transition the asoc to
SCTP_STATE_CLOSED, and does not increment SCTP_MIB_ABORTEDS.

[High]
As a result, would connect(2) wait for T1-init to exhaust its
retransmissions instead of receiving ECONNREFUSED promptly, and would
SCTP_MIB_ABORTEDS no longer be incremented (with SCTP_MIB_IN_PKT_DISCARDS
being incremented in its place)?

[High]
Does this also bypass the SCTP-AUTH section 6.3 check quoted in the
comment?  Previously, when AUTH-ABORT was negotiated and err_chunk
was NULL, sctp_auth_recv_cid() caused sctp_sf_pdiscard() to be returned
and no ABORT was emitted on the wire.  In the new path, sctp_sf_tabort_8_4_8()
sends an unauthenticated OOTB ABORT regardless of AUTH negotiation.

[High]
Finally, does the disposition return value also change from
SCTP_DISPOSITION_ABORT to SCTP_DISPOSITION_CONSUME on this path?

[High]
Would moving the chunk->chunk_hdr->type check ahead of the !err_chunk
early return restore the original semantics for INIT-ACK while keeping
the sctp_sf_tabort_8_4_8() fallback for INIT?

[ ... ]

> @@ -6485,6 +6403,61 @@ static void sctp_send_stale_cookie_err(struct net *net,
>  	}
>  }
>  
> +static enum sctp_disposition sctp_abort_on_init_err(
> +					struct net *net,
> +					const struct sctp_endpoint *ep,
> +					const struct sctp_association *asoc,
> +					const struct sctp_chunk *chunk,
> +					void *arg,
> +					struct sctp_cmd_seq *commands,
> +					struct sctp_chunk *err_chunk)
> +{
> +	enum sctp_error error = SCTP_ERROR_NO_RESOURCE;
> +	struct sctp_packet *packet;
> +	struct sctp_chunkhdr *ch;
> +
> +	if (!err_chunk)
> +		return sctp_sf_tabort_8_4_8(net, ep, asoc, SCTP_ST_CHUNK(0),
> +					    arg, commands);
> +
> +	ch = err_chunk->chunk_hdr;
> +	packet = sctp_abort_pkt_new(net, ep, asoc, arg,
> +				    (__u8 *)ch + sizeof(*ch),
> +				    ntohs(ch->length) - sizeof(*ch));
> +
> +	sctp_chunk_free(err_chunk);
> +
> +	if (packet) {
> +		sctp_add_cmd_sf(commands, SCTP_CMD_SEND_PKT,
> +				SCTP_PACKET(packet));
> +		SCTP_INC_STATS(net, SCTP_MIB_OUTCTRLCHUNKS);
> +		error = SCTP_ERROR_INV_PARAM;
> +	}
> +
> +	if (chunk->chunk_hdr->type != SCTP_CID_INIT_ACK) {
> +		if (!packet)
> +			return SCTP_DISPOSITION_NOMEM;
> +		return SCTP_DISPOSITION_CONSUME;
> +	}
> +	/* SCTP-AUTH, Section 6.3:
> +	 *    It should be noted that if the receiver wants to tear
> +	 *    down an association in an authenticated way only, the
> +	 *    handling of malformed packets should not result in
> +	 *    tearing down the association.
> +	 *
> +	 * This means that if we only want to abort associations
> +	 * in an authenticated way (i.e AUTH+ABORT), then we
> +	 * can't destroy this association just because the packet
> +	 * was malformed.
> +	 */
> +	if (sctp_auth_recv_cid(SCTP_CID_ABORT, asoc))
> +		return sctp_sf_pdiscard(net, ep, asoc, SCTP_ST_CHUNK(0), arg,
> +					commands);
> +
> +	SCTP_INC_STATS(net, SCTP_MIB_ABORTEDS);
> +	return sctp_stop_t1_and_abort(net, commands, error, ECONNREFUSED,
> +				      asoc, chunk->transport);
> +}

Re: [PATCH net 2/2] sctp: add INIT verification after cookie unpacking

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
Now I have enough verified information to produce the report.

commit 9d74027bfd0cb22a103cca034883f0fcb4e2535a
Author: Xin Long <lucien.xin@gmail.com>

sctp: add INIT verification after cookie unpacking

This commit adds sctp_verify_init() checks after cookie unpacking in
the COOKIE-ECHO processing paths when cookie_auth_enable is disabled.
It also changes the bounds comparison inside sctp_verify_init() from
chunk->chunk_end to peer_init + ntohs(peer_init->chunk_hdr.length).

> diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> index 41958b8e59fd..21b9eb1c02e9 100644
> --- a/net/sctp/sm_make_chunk.c
> +++ b/net/sctp/sm_make_chunk.c
> @@ -2298,7 +2298,7 @@ int sctp_verify_init(struct net *net, const struct sctp_endpoint *ep,
>  	 * VIOLATION error.  We build the ERROR chunk here and let the normal
>  	 * error handling code build and send the packet.
>  	 */
> -	if (param.v != (void *)chunk->chunk_end)
> +	if (param.v != (void *)peer_init + ntohs(peer_init->chunk_hdr.length))
>  		return sctp_process_inv_paramlength(asoc, param.p, chunk, errp);

[High]
Can this new comparison reject INIT chunks whose final parameter has a
length that is not a multiple of 4?

sctp_walk_params() advances pos.v by SCTP_PAD4(ntohs(pos.p->length)) on
every iteration:

  include/net/sctp/sctp.h:
  #define _sctp_walk_params(pos, chunk, end)\
  for (pos.v = (u8 *)(chunk + 1);\
       ...
       pos.v += SCTP_PAD4(ntohs(pos.p->length)))

so when the final parameter has an unpadded length L that is not 4-byte
aligned, pos.v ends at peer_init + offset_of_last_param + SCTP_PAD4(L),
which exceeds peer_init + ntohs(peer_init->chunk_hdr.length) by 1..3.

The previous comparison used chunk->chunk_end, which inqueue.c computes
with the padding included:

  net/sctp/inqueue.c:
  chunk->chunk_end = ((__u8 *)ch) + SCTP_PAD4(ntohs(ch->length));

so the old check accepted unaligned-length final parameters; the new
check returns 0 from sctp_verify_init(), and the callers then invoke
sctp_abort_on_init_err() and emit an ABORT.

Note that this change is in sctp_verify_init() itself, so it affects
every caller, not just the new COOKIE-ECHO sites: sctp_sf_do_5_1B_init,
sctp_sf_do_5_1C_ack, sctp_sf_do_unexpected_init, sctp_sf_do_5_1D_ce
and sctp_sf_do_5_2_4_dupcook all reach this comparison.

Realistic triggers include:

  - Locally-generated INITs constructed by sctp_make_init() when both
    auth_enable and asconf_enable are set: with the default chunk IDs
    ASCONF + ASCONF_ACK the AUTH-CHUNKS parameter has length 6 and is
    typically the last parameter, leaving chunk_hdr.length unaligned.

  - Peer INITs containing any final parameter with non-4-aligned
    length, e.g. SCTP_PARAM_SUPPORTED_ADDRESS_TYPES with 1 or 3 types
    last, AUTH-CHUNKS with N=1,2,3 entries last, or any unknown TLV.

Even in the COOKIE-ECHO context the new bound seems too strict, since
sctp_pack_cookie() copies the INIT verbatim with memcpy(cookie + 1,
init_chunk->chunk_hdr, ntohs(...->length)) followed immediately by
raw_addrs, so unpacked cookies recovered from peers that sent an INIT
with an unaligned-length final parameter would also be spuriously
rejected.

Would comparing against peer_init + SCTP_PAD4(ntohs(peer_init->
chunk_hdr.length)) preserve the long-standing semantics of chunk_end
while still validating the cookie-derived peer_init bounds?

> diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
[ ... ]