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

[Simon Horman <horms@kernel.org>]

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: add INIT verification after cookie unpacking

This commit adds sctp_verify_init() checks in the COOKIE-ECHO processing
paths sctp_sf_do_5_1D_ce() and sctp_sf_do_5_2_4_dupcook() when
cookie_auth_enable is disabled, and updates sctp_verify_init() to bound
the parameter walk against peer_init length rather than chunk->chunk_end.
On verification failure the new association is freed and an ABORT is
generated via sctp_abort_on_init_err().

> diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
> index 8c636f045e45..6967e889d1bd 100644
> --- a/net/sctp/sm_statefuns.c
> +++ b/net/sctp/sm_statefuns.c
> @@ -650,11 +650,12 @@ 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;
> +	enum sctp_cid cid;
>  	struct sock *sk;
>  	int error = 0;
>  
> @@ -728,6 +729,18 @@ enum sctp_disposition sctp_sf_do_5_1D_ce(struct net *net,
>  		}
>  	}
>  
> +	peer_init = (struct sctp_init_chunk *)(chunk->subh.cookie_hdr + 1);
> +	cid = peer_init->chunk_hdr.type;
> +	if (!sctp_sk(sk)->cookie_auth_enable &&
> +	    !sctp_verify_init(net, ep, asoc, cid, peer_init, chunk,
> +			      &err_chk_p)) {
> +		sctp_association_free(new_asoc);
> +		return sctp_abort_on_init_err(net, ep, asoc, cid, arg, commands,
> +					      err_chk_p);
> +	}

[Critical]
When cookie_auth_enable is disabled the cookie HMAC is not validated by
sctp_unpack_cookie(), so every byte of the embedded INIT (including
peer_init->chunk_hdr.type) is attacker-controlled.  Is it safe to feed
that byte directly into both sctp_verify_init() and
sctp_abort_on_init_err() as cid?

Inside the COOKIE-ECHO body the embedded chunk is by RFC always an INIT.
Could the cid argument be hard-coded to SCTP_CID_INIT (or
peer_init->chunk_hdr.type be validated against SCTP_CID_INIT before use)
instead of trusting the wire byte?

Two attacker-reachable consequences look possible if cid is forged to
SCTP_CID_INIT_ACK (3) and the STATE_COOKIE parameter is omitted:

sctp_verify_init() then returns 0 via this branch with *errp populated:

	if ((SCTP_CID_INIT_ACK == cid) && !has_cookie)
		return sctp_process_missing_param(asoc, SCTP_PARAM_STATE_COOKIE,
						  chunk, errp);

control transfers to sctp_abort_on_init_err(net, ep, asoc, cid, ...) with
cid == SCTP_CID_INIT_ACK and a non-NULL err_chunk.  After sending the
ABORT packet, that helper falls through to its out: label because
cid == SCTP_CID_INIT_ACK:

	if (cid != SCTP_CID_INIT_ACK) {
		if (!packet)
			return SCTP_DISPOSITION_NOMEM;
		return SCTP_DISPOSITION_CONSUME;
	}

out:
	...
	return sctp_stop_t1_and_abort(net, commands, error, ECONNREFUSED,
				      asoc, chunk->transport);

In the sctp_sf_do_5_1D_ce() context, asoc is typically NULL (the function
itself opens with if (asoc && !sctp_vtag_verify(chunk, asoc))).  Can the
SCTP_CMD_NEW_STATE / SCTP_CMD_TIMER_STOP / SCTP_CMD_INIT_FAILED commands
queued by sctp_stop_t1_and_abort() against a NULL asoc be dispatched
without the sideeffect interpreter dereferencing asoc?

> @@ -2205,6 +2219,19 @@ 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);
> +		cid = peer_init->chunk_hdr.type;
> +		if (!sctp_sk(ep->base.sk)->cookie_auth_enable &&
> +		    !sctp_verify_init(net, ep, asoc, cid, peer_init, chunk,
> +				      &err_chk_p)) {
> +			sctp_association_free(new_asoc);
> +			return sctp_abort_on_init_err(net, ep, asoc, cid, arg,
> +						      commands, err_chk_p);
> +		}
> +		if (err_chk_p)
> +			sctp_chunk_free(err_chk_p);
> +		fallthrough;

[Critical]
The same attacker-controlled cid is propagated here as well.  In
sctp_sf_do_5_2_4_dupcook() asoc is the existing established association.
With a forged cid of SCTP_CID_INIT_ACK and missing STATE_COOKIE, can a
single unauthenticated COOKIE-ECHO drive sctp_abort_on_init_err() into
its out: path and have sctp_stop_t1_and_abort() tear down the established
asoc and report ECONNREFUSED to the application?

Would constraining the verification to the well-defined INIT case (for
example, hard-coding SCTP_CID_INIT or rejecting any cid !=
SCTP_CID_INIT before calling sctp_verify_init()/sctp_abort_on_init_err())
avoid both of these paths?