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

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 2 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] Undisclosed UAPI/behavioural regression on two socket-option
  paths.
- [Medium] sctp_unpack_cookie() validates a stack *copy* of the attacker-
  controlled cookie peer_addr (`paddr = bear_cookie->peer_addr; ...…
- [Low] The kernel-doc block above sctp_v6_addr_valid()
  (net/sctp/ipv6.c:724-730) documents the return value purely as a…

--

sctp: reject forged cookie peer_addr with unknown address family

This validates the attacker-controlled COOKIE-ECHO peer_addr in
sctp_unpack_cookie() with af->addr_valid() on a stack copy, makes
sctp_v6_addr_valid() reject non-PF_INET6 sockets, and reuses
addr_valid() in sctp_process_param() while adding the missing !af
checks.

A few questions below.

> diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
> index ef26878f1282a..c45ac15f3b79d 100644
> --- a/net/sctp/ipv6.c
> +++ b/net/sctp/ipv6.c
> @@ -734,6 +734,9 @@ static int sctp_v6_addr_valid(union sctp_addr *addr,
>  {
>  	int ret = ipv6_addr_type(&addr->v6.sin6_addr);
>  
> +	if (sp && sctp_opt2sk(sp)->sk_family != PF_INET6)
> +		return 0;
> +

[Severity: Medium]
Should this gate sit before the IPV6_ADDR_MAPPED branch?  Placed here it
also rejects a v4-mapped sockaddr_in6 on an AF_INET socket, not only
genuine IPv6 addresses, and it fires for every caller that passes a
non-NULL sctp_sock.

Two socket option callers build af straight from the user-supplied
sa_family with no pf->af_supported() cross-check first, unlike
sctp_verify_addr() which goes through sctp_sockaddr_af() ->
opt->pf->af_supported().

net/sctp/socket.c:sctp_bindx_rem() {
	...
	af = sctp_get_af_specific(sa_addr->sa.sa_family);
	if (!af) {
		retval = -EINVAL;
		goto err_bindx_rem;
	}

	if (!af->addr_valid(sa_addr, sp, NULL)) {
		retval = -EADDRNOTAVAIL;
		goto err_bindx_rem;
	}
	...
}

net/sctp/socket.c:sctp_setsockopt_peer_primary_addr() {
	...
	if (!af->addr_valid((union sctp_addr *)&prim->sspp_addr, sp, NULL))
		return -EADDRNOTAVAIL;
	...
}

Before this change, a v4-mapped sockaddr_in6 handed to a non-v6only
PF_INET socket was rewritten in place by sctp_v6_map_v4() and validated
as IPv4, so sctp_del_bind_addr() and sctp_assoc_lookup_laddr() then
operated on the normalised AF_INET address and could succeed.  With the
new early return, do SCTP_SOCKOPT_BINDX_REM and
SCTP_SET_PEER_PRIMARY_ADDR with ::ffff:a.b.c.d now return -EADDRNOTAVAIL
where they previously worked?

The changelog describes the helper change only in terms of the cookie and
INIT paths ("so that IPv6 addresses are never used on IPv4 sockets") and
does not mention the socket API side effect.  Since the patch carries
Fixes: 1da177e4c3f4 and will be picked up for stable, could this be either
described in the changelog or scoped to the paths that need it?

[Severity: Low]
This isn't a bug, but the comment block above sctp_v6_addr_valid() still
documents the verdict as a property of the address alone:

/* This function checks if the address is a valid address to be used for
 * SCTP.
 *
 * Output:
 * Return 0 - If the address is a non-unicast or an illegal address.
 * Return 1 - If the address is a unicast.
 */

A legal global unicast IPv6 address now returns 0 whenever the supplied
sctp_sock is not PF_INET6, and the callers above turn that into
-EADDRNOTAVAIL.  Could the comment gain a line about the socket family
rejection?

>  	/* Support v4-mapped-v6 address. */
>  	if (ret == IPV6_ADDR_MAPPED) {
>  		/* Note: This routine is used in input, so v4-mapped-v6

> diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> index 84a4c97d0f755..20c0938948843 100644
> --- a/net/sctp/sm_make_chunk.c
> +++ b/net/sctp/sm_make_chunk.c

[ ... ]

> @@ -1841,6 +1843,11 @@ struct sctp_association *sctp_unpack_cookie(
>  		goto fail;
>  	}
>  
> +	paddr = bear_cookie->peer_addr;
> +	af = sctp_get_af_specific(paddr.sa.sa_family);
> +	if (!af || !af->addr_valid(&paddr, sctp_sk(ep->base.sk), NULL))
> +		goto malformed;
> +

[Severity: Medium]
Is the object that gets validated here the same one that is later used?

sctp_v6_addr_valid() only declares a v4-mapped AF_INET6 address valid
after rewriting it in place:

net/sctp/ipv6.c:sctp_v6_addr_valid() {
	...
	if (ret == IPV6_ADDR_MAPPED) {
		...
		sctp_v6_map_v4(addr);
		return sctp_get_af_specific(AF_INET)->addr_valid(addr, sp, skb);
	}
	...
}

The rewrite lands in the throw-away stack copy paddr, which is then
discarded.  The association and its primary transport are built from the
original in-skb address:

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))
	...
}

and memcpy(&retval->c, bear_cookie, sizeof(*bear_cookie)) copies the
un-normalised address into the association.

So for a forged cookie carrying sa_family = AF_INET6 with ::ffff:a.b.c.d
on a dual-stack PF_INET6 listener, the check passes (validated as IPv4)
while sctp_assoc_add_peer() -> sctp_transport_new() installs an AF_INET6
v4-mapped primary with af_specific = IPv6, and the primary path is then
driven through sctp_v6_get_dst() with a v4-mapped destination.  The IPv4
address parameters from the same forged INIT are normalised to AF_INET, so
the same peer ends up with transports of two different families.

The sibling call site added by the same patch does consume the normalised
address:

net/sctp/sm_make_chunk.c:sctp_process_param() {
	...
		if (!af || !af->from_addr_param(&addr, param.addr,
						htons(asoc->peer.port), 0) ||
		    !af->addr_valid(&addr, sctp_sk(asoc->base.sk), NULL))
			break;
		scope = sctp_scope(peer_addr);
		if (sctp_in_scope(net, &addr, scope))
			if (!sctp_assoc_add_peer(asoc, &addr, gfp, SCTP_UNCONFIRMED))
	...
}

Could the two new users of addr_valid() be made to agree, for instance by
storing the validated copy into retval->c.peer_addr and having the state
function pass that instead of re-reading the raw cookie, so nothing is
written into the possibly shared skb?

>  	/* Make a new base association.  */
>  	scope = sctp_scope(sctp_source(chunk));
>  	retval = sctp_association_new(ep, ep->base.sk, scope, gfp);

[ ... ]

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