Re: [PATCH] sctp: reject forged cookie peer_addr with unknown address family
From: netdev-bot+sashiko@kernel.org
Date: 2026-09-14 12:13:19
Also in:
linux-sctp
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().
quoted hunk ↗ jump to hunk
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?
quoted hunk ↗ jump to hunk
/* 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