Re: [PATCH net] sctp: validate cached peer INIT chunk length in COOKIE_ECHO processing

From: 'Simon Horman' <[email protected]>

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://sashiko.dev
---
sctp: validate cached peer INIT chunk length in COOKIE_ECHO processing

This commit adds a bounds check to ensure the cached peer INIT chunk
length does not exceed the available data in the COOKIE_ECHO buffer
during unpacking.

Simon says:

Hi Xin,

Although the issues flagged below are flagged as pre-existing I am
wondering if they directly impact the effectiveness of this patch.
Could you take a look?

> diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> index de86ac088289e..85264862fb6b6 100644
> --- a/net/sctp/sm_make_chunk.c
> +++ b/net/sctp/sm_make_chunk.c

[ ... ]

> @@ -1759,6 +1760,10 @@ struct sctp_association *sctp_unpack_cookie(
>  	cookie = chunk->subh.cookie_hdr;
>  	bear_cookie = &cookie->c;
>  
> +	ch = (struct sctp_chunkhdr *)(bear_cookie + 1);
> +	if (ntohs(ch->length) > len - fixed_size)
> +		goto malformed;
> +

[Severity: High]
This isn't a bug introduced by this patch, but does this check also
need to ensure the length is at least the size of the INIT header?

If ch->length is small, for example 4 bytes for just the chunk header,
this check passes. However, sctp_process_init() appears to unconditionally
read the init_hdr fields:

net/sctp/sm_make_chunk.c:sctp_process_init() {
    ...
    asoc->peer.i.init_tag = ntohl(peer_init->init_hdr.init_tag);
    asoc->peer.i.a_rwnd = ntohl(peer_init->init_hdr.a_rwnd);
    ...
}

Could an attacker send a COOKIE_ECHO chunk with an embedded INIT chunk
truncated to 4 bytes and trigger an out-of-bounds read past the SKB payload?

[Severity: High]
This is a pre-existing issue, but is cookie->raw_addr_list_len properly
validated against the remaining payload length before use?

If SCTP cookie authentication is disabled, an unvalidated
raw_addr_list_len could lead to out-of-bounds reads when
sctp_assoc_set_bind_addr_from_cookie() calls sctp_raw_to_bind_addrs()
to parse the bind addresses:

net/sctp/bind_addr.c:sctp_raw_to_bind_addrs() {
    ...
    while (addrs_len >= sizeof(struct sctp_paramhdr)) {
        param = (struct sctp_paramhdr *)raw_addr_list;
        len = ntohs(param->length);
        addrs_len -= len;
        raw_addr_list += len;
    }
    ...
}

If the unvalidated addrs_len is artificially large, or if a supplied
param->length is larger than addrs_len causing an integer underflow on
addrs_len -= len, could this bypass the loop condition, read into unmapped
pages, and cause a panic?

>  	/* Verify the cookie's MAC, if cookie authentication is enabled. */
>  	if (sctp_sk(ep->base.sk)->cookie_auth_enable) {
>  		u8 mac[SHA256_DIGEST_SIZE];