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

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

[Xin Long]

When a listening SCTP server processes a COOKIE_ECHO chunk, the cached
peer INIT chunk embedded after the cookie is parsed and its parameters
are later walked by sctp_process_init() using sctp_walk_params().

However, the chunk header length of this cached INIT chunk was not
validated against the remaining buffer in the COOKIE_ECHO payload. If
the length field is inflated, the parameter walk can run beyond the
actual received data, leading to out-of-bounds reads and potential
memory corruption during later parameter handling (e.g. STATE_COOKIE
processing and kmemdup() copies).

Add a bounds check in sctp_unpack_cookie() to ensure the cached INIT
chunk length does not exceed the available data in the COOKIE_ECHO
buffer before it is used.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-by: Brian Geffon <bgeffon@google.com>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sctp/sm_make_chunk.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index de86ac088289..85264862fb6b 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -1730,6 +1730,7 @@ struct sctp_association *sctp_unpack_cookie(
 	struct sctp_signed_cookie *cookie;
 	struct sk_buff *skb = chunk->skb;
 	struct sctp_cookie *bear_cookie;
+	struct sctp_chunkhdr *ch;
 	enum sctp_scope scope;
 	unsigned int len;
 	ktime_t kt;
@@ -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;
+
 	/* Verify the cookie's MAC, if cookie authentication is enabled. */
 	if (sctp_sk(ep->base.sk)->cookie_auth_enable) {
 		u8 mac[SHA256_DIGEST_SIZE];
-- 
2.47.1

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

[Simon Horman]

From: '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://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];