Re: [PATCH] sctp: validate cookie AUTH state before use

On Fri, Aug 7, 2026 at 2:47 PM Xin Long <[email protected]> wrote:
>
> [sashiko-gemini]:
> > > +bool sctp_auth_verify_cookie_params(const struct sctp_endpoint *ep,
> > > +                                    const struct sctp_cookie *cookie)
> > > +{
> > > +        const struct sctp_paramhdr *random;
> > > +        const struct sctp_hmac_algo_param *hmacs;
> > > +        const struct sctp_chunks_param *chunks;
> > > +        u16 hmacs_len, chunks_len;
> > > +        u16 n_hmacs, n_chunks, i;
> > > +        bool has_sha1 = false;
> > > +
> > > +        if (sctp_sk(ep->base.sk)->cookie_auth_enable || !ep->auth_enable)
> > > +                return true;
> > This is a pre-existing issue, but does skipping validation when
> > cookie_auth_enable is true leave the system vulnerable if auth_enable is
> > dynamically toggled?
> > When an endpoint has auth_enable = 0, it generates state cookies with a
> > zero-initialized auth_hmacs array. If the administrator then dynamically
> > enables auth_enable = 1, an attacker could send a COOKIE_ECHO with the
> > previously generated cookie (which has a valid MAC) bundled with an AUTH
> > chunk.
> > Because cookie_auth_enable is true, this check skips validation and the
> > zeroed auth_hmacs is restored. When the AUTH chunk is processed,
> > sctp_auth_asoc_verify_hmac_id() calculates the element count with a zeroed
> > length:
> > sctp_auth_asoc_verify_hmac_id() {
> >         hmacs = (struct sctp_hmac_algo_param *)asoc->c.auth_hmacs;
> >         n_elt = (ntohs(hmacs->param_hdr.length) -
> >                  sizeof(struct sctp_paramhdr)) >> 1;
> >
> >         return __sctp_auth_find_hmacid(hmacs->hmac_ids, n_elt, hmac_id);
> > }
> > Since length is 0, this underflows to 65534, causing an out-of-bounds read
> > of 131,068 bytes past the association allocation in
> > __sctp_auth_find_hmacid().
>
> False positive: when cookie_auth_enable is true, the cookie is already
> authenticated. The cookie content is generated locally, so validation is
> unnecessary.
>
>
> [sashiko-claude]:
> > > +bool sctp_auth_verify_cookie_params(const struct sctp_endpoint *ep,
> > > +                                    const struct sctp_cookie *cookie)
> > > +{
> > > +        const struct sctp_paramhdr *random;
> > > +        const struct sctp_hmac_algo_param *hmacs;
> > > +        const struct sctp_chunks_param *chunks;
> > > +        u16 hmacs_len, chunks_len;
> > > +        u16 n_hmacs, n_chunks, i;
> > > +        bool has_sha1 = false;
> > > +
> > > +        if (sctp_sk(ep->base.sk)->cookie_auth_enable || !ep->auth_enable)
> > > +                return true;
> > Discovered by: opus-5, old-prompts · Missed by: sashiko-gemini
> > This isn't a bug introduced by this patch, but does the new invariant leave
> > the underflowing consumer reachable in the default configuration?  With
> > cookie_auth_enable set (the default from protocol.c), this early return
> > skips all validation, and sctp_auth_asoc_verify_hmac_id() still has no
> > lower bound on the parameter length:
> > net/sctp/auth.c:sctp_auth_asoc_verify_hmac_id() {
> >         hmacs = (struct sctp_hmac_algo_param *)asoc->c.auth_hmacs;
> >         n_elt = (ntohs(hmacs->param_hdr.length) -
> >                  sizeof(struct sctp_paramhdr)) >> 1;
> >         return __sctp_auth_find_hmacid(hmacs->hmac_ids, n_elt, hmac_id);
> > }
> > For a zero-length parameter the unsigned subtraction wraps and the
> > truncation into __u16 n_elt yields 65534, so __sctp_auth_find_hmacid()
> > scans about 128 KB past the 12-byte asoc->c.auth_hmacs array in softirq
> > context.  Note the sibling consumer __sctp_auth_cid() does guard this
> > case:
> >         if (!param || param->param_hdr.length == 0)
> >                 return 0;
> > Is a zero-length asoc->c.auth_hmacs together with peer.auth_capable == 1
> > reachable without any forged cookie?  It looks like it is:
> > sctp_association_init()
> >         if (ep->auth_hmacs_list)
> >                 memcpy(asoc->c.auth_hmacs, ep->auth_hmacs_list,
> >                         ntohs(ep->auth_hmacs_list->param_hdr.length));
> > with ep->auth_hmacs_list NULL while ep->auth_enable == 0 (the default in
> > sctp_endpoint_init()), so c.auth_hmacs stays all zero.  Afterwards
> > sctp_setsockopt_auth_supported() does:
> >         ep->auth_enable = !!params->assoc_value;
> > and sctp_process_ext_param() then sets, for that same already-created
> > association:
> >                         if (asoc->ep->auth_enable)
> >                                 asoc->peer.auth_capable = 1;
> > A remote AUTH chunk then reaches sctp_sf_authenticate() ->
> > sctp_auth_asoc_verify_hmac_id() with the underflow.  If the scan happens
> > to match the attacker's 16-bit hmac_id, sctp_auth_get_hmac() indexes the
> > four-entry sctp_hmac_list[] with that id and the resulting hmac_len gates
> > the 20/32-byte digest write in sctp_auth_calculate_hmac().
> > Would it be worth adding the same length == 0 guard to
> > sctp_auth_asoc_verify_hmac_id(), and bounding the index in
> > sctp_auth_get_hmac(), rather than relying only on the conditionally
> > executed cookie check?  The same applies to
> > sctp_auth_make_key_vector(), which still does:
> >         random_len = ntohs(random->param_hdr.length);
> >         ...
> >         memcpy(new->data, random, random_len);
> > out of the fixed 36-byte asoc->c.auth_random.
>
> False positive: same reason as above.
>
> > > +
> > > +        random = (const struct sctp_paramhdr *)cookie->auth_random;
> > > +        if (random->type != SCTP_PARAM_RANDOM ||
> > > +            ntohs(random->length) != sizeof(*random) + SCTP_AUTH_RANDOM_LENGTH)
> > > +                return false;
> > > +
> > > +        hmacs = (const struct sctp_hmac_algo_param *)cookie->auth_hmacs;
> > > +        hmacs_len = ntohs(hmacs->param_hdr.length);
> > > +        if (hmacs->param_hdr.type != SCTP_PARAM_HMAC_ALGO ||
> > > +            hmacs_len < sizeof(struct sctp_paramhdr) +
> > > +                        sizeof(hmacs->hmac_ids[0]) ||
> > > +            hmacs_len > sizeof(cookie->auth_hmacs) ||
> > > +            (hmacs_len - sizeof(struct sctp_paramhdr)) %
> > > +                        sizeof(hmacs->hmac_ids[0]))
> > > +                return false;
> > Discovered by: opus-5, old-prompts · Missed by: sashiko-gemini
> > Can these checks reject a cookie this endpoint minted itself?
> > The decision to validate is taken from ep->auth_enable sampled at
> > COOKIE-ECHO time, not from the state that produced the INIT-ACK, while the
> > HMACS and CHUNKS tests unconditionally require a well-formed parameter.  An
> > all-zero (absent) auth_hmacs/auth_chunks looks like a state the producer
> > legitimately creates:
> > net/sctp/associola.c:sctp_association_init() {
> >         if (ep->auth_hmacs_list)
> >                 memcpy(asoc->c.auth_hmacs, ep->auth_hmacs_list, ...);
> >         if (ep->auth_chunk_list)
> >                 memcpy(asoc->c.auth_chunks, ep->auth_chunk_list, ...);
> > }
> > auth_random is the only one of the three always written, and the endpoint
> > lists are NULL while ep->auth_enable == 0, which is the default in
> > sctp_endpoint_init().  sctp_make_init() treats the zero-length case as
> > "parameter omitted" rather than malformed:
> >                 auth_hmacs = (struct sctp_paramhdr *)asoc->c.auth_hmacs;
> >                 if (auth_hmacs->length)
> >                         chunksize += SCTP_PAD4(ntohs(auth_hmacs->length));
> >                 else
> >                         auth_hmacs = NULL;
> > sctp_pack_cookie() copies asoc->c verbatim, so the outstanding cookie
> > carries those zeros.  If the application then calls
> > setsockopt(SCTP_AUTH_SUPPORTED) while the cookie is still within
> > Valid.Cookie.Life, and cookie_hmac_alg is none, the early return above is
> > skipped and "hmacs->param_hdr.type != SCTP_PARAM_HMAC_ALGO" fails with
> > type 0.
> > Given that, does the comment "they must satisfy the same constraints as
> > locally generated AUTH parameters" hold?  Locally generated parameters may
> > legitimately be absent.
>
> ep->auth_enable can be changed at any time during the handshake, especially
> on a listening socket, and we should not change this behavior for backward
> compatibility at this time.
>
> If it changes from 0 to 1 while processing a COOKIE-ECHO, the packet will
> be rejected and the connection will eventually fail, but it will not lead
> to a crash. Changing ep->auth_enable during an active handshake should be
> considered invalid SCTP usage.

Acked-by: Xin Long <[email protected]>