Re: [PATCH v4] 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: 0 · Medium: 1 · Low: 0 (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] The patch now validates peer_addr in place.

Pre-existing issues:
- [Low] Two comments next to the code this patch changes contradict the
  implementation.

--

sctp: reject forged cookie peer_addr with unknown address family

This patch validates the COOKIE-ECHO peer_addr with af->addr_valid() in
sctp_unpack_cookie(). Bad families are rejected, and v4-mapped addresses
are normalised in place. It also makes sctp_v6_addr_valid() reject
non-v4-mapped addresses on non-PF_INET6 sockets, uses addr_valid() to
simplify sctp_process_param(), and adds a missing !af check in
sctp_process_init().

> diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
> index ef26878f1282a..97390569c4bdc 100644
> --- a/net/sctp/ipv6.c
> +++ b/net/sctp/ipv6.c
> @@ -725,7 +725,8 @@ static int sctp_v6_available(union sctp_addr *addr, struct sctp_sock *sp)
>   * SCTP.
>   *
>   * Output:
> - * Return 0 - If the address is a non-unicast or an illegal address.
> + * Return 0 - If the address is a non-unicast or an illegal address,
> + *            or a non-v4-mapped address on a non-PF_INET6 socket.
>   * Return 1 - If the address is a unicast.
>   */
>  static int sctp_v6_addr_valid(union sctp_addr *addr,

[Severity: Low]

This isn't a bug introduced by this patch, but two nearby comments
don't match the code that this patch now relies on.

The v4-mapped branch of sctp_v6_addr_valid() says:

	/* Note: This routine is used in input, so v4-mapped-v6
	 * are disallowed here when there is no sctp_sock.
	 */
	if (sp && ipv6_only_sock(sctp_opt2sk(sp)))
		return 0;
	sctp_v6_map_v4(addr);

When sp is NULL, as in the sctp_rcv() input path, the code maps and
accepts the address instead of rejecting it.

The header comment of sctp_v4_addr_valid() in net/sctp/protocol.c only
lists non-unicast or illegal addresses as reasons to return 0. The
function also returns 0 for any IPv4 address on an IPV6_V6ONLY socket:

	/* IPv4 addresses not allowed */
	if (sp && ipv6_only_sock(sctp_opt2sk(sp)))
		return 0;

This patch removes the explicit ipv6_only_sock() test from
sctp_process_param(), so that function now depends on this check.

The sctp_v6_addr_valid() header comment is already being updated here.
Could these two comments be updated as well?

[ ... ]

> diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> index 84a4c97d0f755..175dbb242e1cb 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]

Can this modify packet data that is shared with a clone? bear_cookie
points into the received skb. sctp_sf_do_5_1D_ce() and
sctp_sf_do_5_2_4_dupcook() both set:

	chunk->subh.cookie_hdr =
		(struct sctp_signed_cookie *)chunk->skb->data;

For an AF_INET6 v4-mapped peer_addr, sctp_v6_addr_valid() calls
sctp_v6_map_v4(), which writes directly into that buffer:

	addr->v4.sin_family = AF_INET;
	addr->v4.sin_port = addr->v6.sin6_port;
	addr->v4.sin_addr.s_addr = addr->v6.sin6_addr.s6_addr32[3];

No skb_cloned() or skb_unshare() check happens before this write.

If an AUTH chunk is bundled before the COOKIE-ECHO,
sctp_endpoint_bh_rcv() and sctp_assoc_bh_rcv() do:

	chunk->auth_chunk = skb_clone(chunk->skb,
				GFP_ATOMIC);

The clone shares the same data.

sctp_sf_do_5_1D_ce() calls sctp_unpack_cookie() first. AUTH is verified
only afterwards:

sctp_auth_chunk_verify()
  sctp_sf_authenticate()
    sctp_auth_calculate_hmac()
      data_len = skb_tail_pointer(skb) - (unsigned char *)auth;

That HMAC covers the COOKIE-ECHO body, including the rewritten
peer_addr. The peer computed its HMAC over the original bytes. Would
verification then fail and the COOKIE-ECHO be dropped through
sctp_sf_pdiscard()? The duplicate-cookie handlers use the same order:
unpack first, then verify AUTH.

Other holders of a clone, such as raw IPPROTO_SCTP sockets and non-mmap
AF_PACKET readers, would also see the changed bytes. sctp_rcv() doesn't
prevent this, because skb_linearize() does nothing to an skb that is
already linear but cloned.

A v4-mapped peer_addr doesn't have to be forged. For an INIT that
arrives over IPv6 from a ::ffff:a.b.c.d source, sctp_make_temp_asoc()
stores the raw source in the signed cookie:

	SCTP_INPUT_CB(skb)->af->from_skb(&asoc->c.peer_addr, skb, 1);

It can also be forged directly when cookie authentication is disabled.

In that case, the commit message statement "peer_addr is validated in
place, so a v4-mapped v6 address is normalised to AF_INET before the
association and its primary transport are built from it" would not hold
when AUTH is bundled.

Would it work to validate and normalise a stack copy of peer_addr, and
pass that copy to the association and transport setup, instead of
writing into the packet buffer?

[ ... ]

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