tls: bound the CertificateRequest certificate-type count

[tinic]

_nx_secure_tls_process_certificate_request() read the certificate-type count out of the message before establishing that the message had a byte in it.

The only guard is

if (length >= message_length)

which sits inside the NX_SECURE_TLS_TLS_1_3_ENABLED arm, above the else. For TLS 1.2 nothing runs before

cert_types_length = packet_buffer[length];

with length still 0, so a zero-length CertificateRequest reads one byte past the record buffer. The sanity test that follows compares cert_types_length against message_length, which is after the read.

A server sends CertificateRequest and chooses its length, so this is reachable by any peer a client connects to.

Found by a fuzz driver over the client handshake path and confirmed under AddressSanitizer.

[fdesbiens]

Thank you — third one of these and the description is again precise enough to check directly. Your reading of the control flow is exactly right, and the situation is slightly worse than you describe it.

Confirmed, and it is the default path rather than a TLS 1.2 special case. You describe the if (length >= message_length) guard as sitting inside the NX_SECURE_TLS_TLS_1_3_ENABLED arm above the else, which is correct. What is worth adding is that NX_SECURE_TLS_TLS_1_3_ENABLED defaults to 0 — nx_secure_tls.h:379-385 only defines it as 1 when NX_SECURE_TLS_ENABLE_TLS_1_3 is defined. So in a default build the #if arm is not compiled at all and the else block is the only code path, with nothing whatsoever preceding the read. This is not a bug that shows up in an unusual configuration; it is the ordinary one.

Reproduced under AddressSanitizer. Real translation unit, real headers, its one external stubbed to return no local certificate, and the message passed as a tail pointer with message_length == 0:

unpatched: heap-buffer-overflow, READ of size 1
           at nx_secure_tls_process_certificate_request.c:222
patched:   returns 0x10a (NX_SECURE_TLS_INCORRECT_MESSAGE_LENGTH), no report

Built -m32 so ULONG matches its width on ThreadX ports.

Your fix is complete for the certificate-types path, tightly so. With message_length >= 1 guaranteed, cert_types_length is read at [0], length becomes 1, and cert_types_length + 1 > message_length at :233 then bounds it to message_length - 1. The loop at :240 reads indices 1 through cert_types_length, so the highest index touched is exactly message_length - 1. No slack and no overrun — that check and yours together make the block exact.

But the function has two more over-reads, and this PR does not touch either. I only went looking because the cert_types_length + 1 > message_length check at :233 is unusual enough in shape that I wanted to satisfy myself the rest of the function was sound. It is not. Both are in findings 1 and 2, both reproduced, and I confirmed both still fire with your patch applied. Neither is your responsibility, and I am not asking you to fix things you did not break — but I would rather put them in front of you than merge a partial fix silently, because finding 1 in particular is a one-line addition in a block you are already editing.

A note on scope, so my ask is clear: I would like finding 1 in this PR if you are willing, and I think finding 2 should be handled separately because the correct fix there is not a pure bounds addition. Details in each.

[fdesbiens]

Finding 1

This is the one I would like folded into this PR.

The signature-algorithms list is bounded correctly in total:

        if ((length + sign_algs_length) > message_length)      /* :277 */
        {
            return(NX_SECURE_TLS_INCORRECT_MESSAGE_LENGTH);
        }

but the loop that consumes it reads two bytes per iteration while stepping the index by two:

        for (i = 0; i < sign_algs_length; i += 2)
        {
            if ((UINT)((packet_buffer[length] << 8) + packet_buffer[length + 1]) == expected_sign_alg)   /* :287 */

With sign_algs_length odd, the loop runs ceil(sign_algs_length / 2) times, so the final iteration reads [length] and [length + 1] where length + 1 is L0 + sign_algs_length. The check above permits that to equal message_length exactly, which is one past the last valid index. The first byte of the last pair is in bounds and the second is not — verbatim the wording of the published advisory: "The first byte is guaranteed to be within bounds, the second byte could be out of bound if the length of the content is odd."

Reproduced with a four-byte CertificateRequest — cert_types_length 0, sign_algs_length 1:

heap-buffer-overflow, READ of size 1
  at nx_secure_tls_process_certificate_request.c:280   (:287 with this PR applied)
  0 bytes after 4-byte region

I re-ran it with your patch applied to confirm it is untouched by this change: it still fires, at :287.

Reachable on the default build, and cheaply: a zero certificate-type count is accepted when no local certificate is configured, because expected_cert_type stays NX_SECURE_TLS_CERT_TYPE_NONE and cert_type is initialised to the same value, so the equality check at :251 passes and the parse falls straight through to the signature-algorithms block.

The fix the project already uses for this exact situation is an explicit divisibility check. nx_secure_tls_process_clienthello_extensions.c:985-990, which is the remediation for that advisory:

    /* TLS ProtocolVersion is defined to be a uint16. Thus the list of supported
    versions must have a length divisible by 2. */
    if (packet_buffer[0] % 2 != 0)
    {
        return(NX_SECURE_TLS_INCORRECT_MESSAGE_LENGTH);
    }

The same thing here, after the check at :277:

        /* SignatureAndHashAlgorithm is a pair of bytes, so the list length
           must be divisible by 2. */
        if (sign_algs_length % 2 != 0)
        {
            return(NX_SECURE_TLS_INCORRECT_MESSAGE_LENGTH);
        }

I prefer this to changing the loop bound to i + 1 < sign_algs_length, for the same reason the project preferred it in the ClientHello case: RFC 5246 §7.4.4 defines the list as a vector of two-byte SignatureAndHashAlgorithm values, so an odd length is malformed input and should be rejected rather than silently truncated.

[fdesbiens]

Finding 2

Also confirmed, also unfixed by this PR, but I am not asking you to put it in this PR — see the end of this comment.

        while (length < message_length)                    /* :196 */
        {
            extension_type = (UINT)((packet_buffer[length] << 8) + packet_buffer[length + 1]);    /* :198 */
            length += 2;

            extension_length = (UINT)((packet_buffer[length] << 8) + packet_buffer[length + 1]);  /* :201 */
            length += 2;

The condition admits any length up to message_length - 1, and the body then reads four bytes starting there. So up to three bytes past the end. The guard at :189 bounds extension_total_length, but the loop is bounded by message_length rather than by that total, and length += extension_length at the bottom can advance by an attacker-chosen amount.

Reproduced with NX_SECURE_TLS_ENABLE_TLS_1_3 defined, an empty certificate_request_context, extension_total_length of 1 and message_length of 4 — every guard at the top of the 1.3 arm satisfied:

heap-buffer-overflow, READ of size 1
  at nx_secure_tls_process_certificate_request.c:198
  0 bytes after 4-byte region

The obvious minimal correction is to require a full extension header before entering the body:

        /* Each extension header is four bytes: two of type and two of length. */
        while ((length + 4) <= message_length)

The reason I want this separate from your PR is that the change is not purely additive, and I do not think it should be made without tracing the consequence. The post-loop check at :212, if (length >= message_length) return(NX_SECURE_TLS_UNSUPPORTED_CERT_SIGN_ALG);, currently doubles as the "no signature-algorithms extension was found" signal. Tightening the loop condition means the loop can now exit with length < message_length, so that check stops detecting the not-found case and the function would fall through with sign_alg handling that I have not fully traced for TLS 1.3 — the nx_secure_tls_signature_algorithm assignment at :304 is inside the TLS 1.2 protocol-version block, and whether a 1.3 session reaches it depends on how nx_secure_tls_protocol_version is set on that path. Doing this properly probably means an explicit found flag rather than inferring it from length.

That is a small design decision in TLS 1.3 code, and it deserves its own change with its own reasoning rather than riding along on a one-line 1.2 bounds fix. I am happy to open it, or leave it to you if you would rather — just say which, so it does not fall between us. Same request I made on #414, and the same reason.

Lower priority than finding 1 in practice, since it needs NX_SECURE_TLS_ENABLE_TLS_1_3, which is off by default.

[fdesbiens]

Same reasoning as on #414, and it applies here too: the fix changes the returned status, so a test does not need a sanitiser to be meaningful.

A zero-length CertificateRequest on the unpatched code reads a stale byte as the type count and then either fails the cert_types_length + 1 > message_length check or fails the certificate-type match, so it returns NX_SECURE_TLS_INCORRECT_MESSAGE_LENGTH or NX_SECURE_TLS_UNSUPPORTED_CERT_SIGN_TYPE depending on what that byte happened to be. Patched, it deterministically returns NX_SECURE_TLS_INCORRECT_MESSAGE_LENGTH. The determinism is the point worth asserting — the unpatched behaviour depends on memory contents, which is precisely the defect.

There is no CertificateRequest length-checking test in test/regression/nx_secure_test/ today. The closest existing patterns to copy are nx_secure_tls_1_3_serverhello_length_checking_test.c and nx_secure_tls_1_3_clienthello_length_checking_test.c, both of which drive a static malformed-message byte array through the RAM driver. A CertificateRequest equivalent would want mutual authentication configured so the client actually processes one.

If finding 1 lands here as well, please cover the odd-sign_algs_length case in the same test — that one also discriminates on status, cleanly and without depending on memory contents: unpatched it reads a stale byte as half of a signature algorithm and returns NX_SECURE_TLS_UNSUPPORTED_CERT_SIGN_ALG, patched it returns NX_SECURE_TLS_INCORRECT_MESSAGE_LENGTH.