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

[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.