Re: [PATCH] sctp: reject forged cookie peer_addr with unknown address family

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 1 (1) · Medium: 0 · Low: 0

- [High] Incomplete fix: sctp_unpack_cookie()
  (net/sctp/sm_make_chunk.c:1858-1860) only checks that the forged…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch
  establishes the NULL-check contract for sctp_get_af_specific() at one…

--

sctp: reject forged cookie peer_addr with unknown address family

This adds a family whitelist for the COOKIE-ECHO cookie peer_addr in
sctp_unpack_cookie(), and makes sctp_transport_new() bail out when
sctp_get_af_specific() has no implementation for the requested family.
Both changes target the null dereference seen from sctp_transport_init().

> diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> index 84a4c97d0f755..7c8fe4b38cd1c 100644
> --- a/net/sctp/sm_make_chunk.c
> +++ b/net/sctp/sm_make_chunk.c
> @@ -1855,6 +1855,10 @@ struct sctp_association *sctp_unpack_cookie(
>  	if (!sctp_auth_verify_cookie_params(ep, bear_cookie))
>  		goto malformed;
>  
> +	if (bear_cookie->peer_addr.sa.sa_family != AF_INET &&
> +	    bear_cookie->peer_addr.sa.sa_family != AF_INET6)
> +		goto malformed;
> +

[Severity: High]
Does this check still leave the mismatched-family case open?  It only asks
whether sa_family is one of AF_INET or AF_INET6, it never relates that family
to the family of the endpoint that received the packet.

On a PF_INET listener with cookie authentication disabled the cookie contents
are attacker supplied, so sa_family can be AF_INET6.  The new guard in
sctp_transport_new() passes as well, since sctp_get_af_specific(AF_INET6) is
non-NULL on CONFIG_IPV6 kernels, and the transport ends up with
af_specific = sctp_af_inet6.

sctp_assoc_add_peer() then routes that transport using the IPv4 socket:

net/sctp/associola.c:sctp_assoc_add_peer() {
	sp = sctp_sk(asoc->base.sk);
	...
	/* Initialize the pmtu of the transport. */
	sctp_transport_route(peer, NULL, sp);
}

sctp_transport_route() calls af->get_dst(), which is sctp_v6_get_dst() here:

net/sctp/ipv6.c:sctp_v6_get_dst() {
	struct ipv6_pinfo *np = inet6_sk(sk);
	...
	final_p = fl6_update_dst(fl6, rcu_dereference(np->opt), &final);
}

inet6_sk() returns inet_sk(sk)->pinet6, which is only populated by
inet6_create(), so np is NULL for an AF_INET socket and np->opt is
dereferenced with no guard.

Isn't that the same remotely triggered null dereference in softirq context
that this patch is meant to close, only at a different offset?

For comparison, sctp_process_param() already refuses IPv6 addresses on
sockets that are not PF_INET6:

	case SCTP_PARAM_IPV6_ADDRESS:
		if (PF_INET6 != asoc->base.sk->sk_family)
			break;

Would it make more sense to validate the cookie family against the receiving
socket family, or against SCTP_INPUT_CB(skb)->af->sa_family, rather than
accepting AF_INET6 unconditionally?

>  	/* Populate the association from the cookie.  */
>  	memcpy(&retval->c, bear_cookie, sizeof(*bear_cookie));
>  
> diff --git a/net/sctp/transport.c b/net/sctp/transport.c
> index 6ea55b9fbde45..cd1a604d6f8e3 100644
> --- a/net/sctp/transport.c
> +++ b/net/sctp/transport.c
> @@ -92,6 +92,9 @@ struct sctp_transport *sctp_transport_new(struct net *net,
>  {
>  	struct sctp_transport *transport;
>  
> +	if (!sctp_get_af_specific(addr->sa.sa_family))
> +		return NULL;
> +

[Severity: High]
This isn't a bug introduced by this patch, but the same
sctp_get_af_specific() result is used unchecked a little further along the
same COOKIE-ECHO path, in sctp_process_init():

net/sctp/sm_make_chunk.c:sctp_process_init() {
	sctp_walk_params(param, peer_init) {
		if (!src_match &&
		    (param.p->type == SCTP_PARAM_IPV4_ADDRESS ||
		     param.p->type == SCTP_PARAM_IPV6_ADDRESS)) {
			af = sctp_get_af_specific(param_type2af(param.p->type));
			if (!af->from_addr_param(&addr, param.addr,
						 chunk->sctp_hdr->source, 0))
				continue;
	...
}

The five other sctp_get_af_specific() callers in sm_make_chunk.c,
bind_addr.c and input.c all test the result for NULL first.

On a CONFIG_IPV6=n build sctp_get_af_specific(AF_INET6) is permanently NULL,
and sctp_verify_param() accepts SCTP_PARAM_IPV6_ADDRESS with a bare break, so
an attacker supplied IPv6 address parameter reaches af->from_addr_param with
af == NULL.

That block only runs when src_match is 0, and sctp_sf_do_5_1D_ce() is the one
sctp_process_init() caller that does not pass sctp_source(chunk) as peer_addr:

net/sctp/sm_statefuns.c:sctp_sf_do_5_1D_ce() {
	if (!sctp_process_init(new_asoc, chunk,
			       &chunk->subh.cookie_hdr->c.peer_addr,
			       peer_init, GFP_ATOMIC))
	...
}

So src_match stays 0 whenever the COOKIE-ECHO source address differs from the
cookie peer_addr, which is attacker chosen with cookie authentication
disabled, and also reachable with cookie authentication enabled by replaying a
legitimately obtained cookie from another source address while keeping the
same source port.

Since this is the same failure mode the patch hardens in sctp_transport_new(),
would it be worth adding the missing !af check in sctp_process_init() in the
same series?

>  	transport = kzalloc_obj(*transport, gfp);
>  	if (!transport)
>  		return NULL;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260913113522.2674588-1-hdthky0%40gmail.com