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

[sashiko-gemini]:

quoted

+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]:

quoted

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

quoted

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