tls: bound the ServerHello ciphersuite and compression method

[tinic]

_nx_secure_tls_process_serverhello() checked that the session ID fitted and then read three more bytes without checking anything.

The floor is message_length < 38. After the protocol version and the 32-byte random, length is 35, and the session ID check is

if ((length + session_id_length) > message_length)

so a 38-byte ServerHello may carry a session_id_length of 3 and leave length at exactly 38. The two ciphersuite bytes are then read at [38] and [39], and the compression method at [40] -- three bytes past the message.

A server chooses both fields, so this is reachable by any peer a client connects to.

The session ID check bounds the session ID. This adds the one the fields after it need.

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

[fdesbiens]

Thank you — this is the second of these you have found and the description is again precise enough that I could check it directly rather than guess at it.

The arithmetic is exactly as you state. The floor at :98 admits message_length == 38. After the version and the 32-byte random, length is 34; the session ID length byte at [34] takes it to 35; the check (35 + session_id_length) > message_length accepts session_id_length == 3 because 38 is not greater than 38; the session ID copy takes length to 38. The ciphersuite is then read at [38] and [39] and the compression method at [40], three bytes past a 38-byte message. Nothing between the floor check and the ciphersuite read constrains it.

Reproduced under AddressSanitizer. I compiled the real translation unit with the real headers, stubbed its four externals, and passed a 38-byte exact-size heap allocation so any read past the message is visible:

unpatched: AddressSanitizer: heap-buffer-overflow, READ of size 1
           at nx_secure_tls_process_serverhello.c:174
           0 bytes after 38-byte region
patched:   returns 0x10a (NX_SECURE_TLS_INCORRECT_MESSAGE_LENGTH), no report

Built -m32 so ULONG is 32 bits as on every ThreadX port, since a 64-bit build would not be a faithful model of the target.

The check is the right size and in the right place. (length + 3) > message_length covers all three bytes at indices length, length + 1 and length + 2, with no off-by-one in either direction, and length cannot exceed 290 at that point so the addition cannot wrap. Combining all three into one check rather than two is also correct: the ciphersuite and compression method are both mandatory fields, so there is no legitimate ServerHello carrying one without the other.

And the rest of the function is bounded once this lands. I enumerated all nine reads of packet_buffer: the version, the HelloRetryRequest comparison and the server random are covered by the 38-byte floor; the session ID length byte at [34] likewise; the session ID copy by the check at :161; the ciphersuite and compression by your new one; and the extension length and extension body by message_length >= (length + 2) at :209 and (length + total_extensions_length) > message_length at :217. Your one check closes the only gap.

Three call sites reach this, all client-side and all server-controlled as you say: nx_secure_tls_client_handshake.c:173, nx_secure_tls_1_3_client_handshake.c:248 and nx_secure_dtls_client_handshake.c:287. I checked the DTLS one specifically because a fragment_length versus message_length mix-up there would have been worse than the bug you are fixing; it is fine, since reassembly returns early while nx_secure_dtls_fragment_length > 0 and only dispatches once the full message is present.

On impact, stated carefully because there are two distinct cases and only one of them is a memory-safety issue. When the ServerHello is the last handshake message in the record, the three bytes are genuinely past the end of the data and the read is out of bounds — stale adjacent memory, which will normally fail the ciphersuite lookup and abort the handshake. When another handshake message follows in the same record, the same three bytes are in bounds but belong to the next message, so the client selects a ciphersuite from bytes that are not part of the ServerHello at all. The second case is a parsing-confusion bug rather than an unsafe one, and it grants an attacker nothing they did not already have, since a server can simply put its chosen ciphersuite in a well-formed ServerHello. So: a real out-of-bounds read with limited consequence, which is the same place #413 landed.

Two asks and a note follow.

[fdesbiens]

Anchor: nx_secure/src/nx_secure_dtls_process_helloverifyrequest.c line 80 (not part of this diff)

I went looking for the same pattern in the other client-side parsers a server can drive, on the assumption that if the ServerHello path was missed then a sibling might be too. One is.

    length = 0;

    /* First two bytes of the server hello following the header are the TLS major and minor version numbers. */
    length += 2;

    /* Get the cookie length. */
    dtls_session -> nx_secure_dtls_cookie_length = packet_buffer[length];   /* :80 — read */
    length += 1;

    if (dtls_session -> nx_secure_dtls_cookie_length > NX_SECURE_DTLS_MAX_COOKIE_LENGTH)
    {
        ...
    }

    if ((3u + dtls_session -> nx_secure_dtls_cookie_length) > message_length)   /* :89 — check */
    {
        ...
    }

There is no floor check anywhere in this function — no message_length < 3 equivalent to the ServerHello's < 38. The read of packet_buffer[2] at :80 happens before the only length check, at :89. So a HelloVerifyRequest whose handshake header declares a length of 0, 1 or 2 reads past the end of the message, and the check that would have rejected it runs afterwards. This is the same shape as your #413 finding: the guard exists and returns the right status; it is just on the wrong side of the load.

I reproduced it the same way as the ServerHello case, with a 1-byte allocation and message_length == 0:

AddressSanitizer: heap-buffer-overflow, READ of size 1
  at nx_secure_dtls_process_helloverifyrequest.c:80
  1 bytes after 1-byte region

And it is reachable, which I checked rather than assumed. _nx_secure_dtls_process_handshake_header() imposes no minimum on the length it decodes from the 3-byte field. In _nx_secure_dtls_client_handshake(), fragment_length == 0 passes (header_bytes + fragment_length) > data_length, nx_secure_dtls_fragment_length is set to 0 so the > 0 early return is not taken, and the switch dispatches to the parser with message_length == 0. A DTLS server sending a bare 12-byte handshake header with no body gets there.

Two caveats on how much this matters: it needs NX_SECURE_ENABLE_DTLS, and the over-read is at most three bytes with the value landing in nx_secure_dtls_cookie_length, which is then bounded against NX_SECURE_DTLS_MAX_COOKIE_LENGTH and reset to 0 on any failure. So the consequence is small, as with the ServerHello.

How I would like to handle it: please add it to this PR if you are willing, since it is the same class, the same subsystem and a one-line fix. A floor check matching the ServerHello's would do it:

    if (message_length < 3)
    {
        /* Message was not the minimum required size for a HelloVerifyRequest. */
        return(NX_SECURE_TLS_INCORRECT_MESSAGE_LENGTH);
    }

That also lets the existing (3u + cookie_length) > message_length check stand unchanged. If you would rather keep this PR to the single ServerHello fix, that is a reasonable call and I will open it separately — just say which you prefer, because I do not want it to fall between us.

[fdesbiens]

On #413 I explicitly did not ask for a test, because the over-read there was only observable under a sanitiser and a test would have passed either way. This one is different in the way that matters, so I am asking.

The unpatched code does not merely read out of bounds — it returns a different status. With garbage ciphersuite bytes the lookup fails and the function returns NX_SECURE_TLS_UNKNOWN_CIPHERSUITE (0x105), or NX_SECURE_TLS_BAD_COMPRESSION_METHOD (0x118) if the stale byte happens to make the ciphersuite match. With your check it returns NX_SECURE_TLS_INCORRECT_MESSAGE_LENGTH (0x10A). So an assertion on the specific status code fails before the fix and passes after it, with no sanitiser required — a genuinely discriminating test in an ordinary build.

There is also direct precedent for the shape. test/regression/nx_secure_test/nx_secure_tls_1_3_serverhello_length_checking_test.c is exactly this: a static serverhello[] byte array fed to a client through the RAM driver, asserting the client rejects it. A case with message_length == 38 and session_id_length == 3 drops straight into that family. nx_secure_tls_serverhello_coverage_test.c and nx_secure_tls_serverhello_extension_test.c are the other two files in the neighbourhood if one fits better.

Worth adding a message_length == 38 with session_id_length == 4 case alongside it, so the test also pins the boundary your check now enforces from the other side — that one should already be rejected by the session ID check at :161, and having both makes the intended boundary explicit rather than incidental.

If the DTLS fix from finding 1 lands here too, the same reasoning applies to it, though that one is harder to assert on status alone since a zero-length HelloVerifyRequest returns NX_SECURE_TLS_INCORRECT_MESSAGE_LENGTH both before and after the fix. Coverage of the code path is still worth having; just do not expect it to discriminate.

[fdesbiens]

Supporting evidence for the shape of your fix, and the thing that convinced me this is an oversight in the ServerHello path specifically rather than a deliberate reliance on some invariant I had not spotted.

_nx_secure_tls_process_clienthello() parses the mirror-image message on the server side, and it bounds every field after the session ID:

• :243 — (length + session_id_length) > message_length, the same session ID check the ServerHello has
• :257 — (length + 1) >= message_length, guarding the two-byte ciphersuite list length
• :267 — ciphersuite_list_length < 2 || (length + ciphersuite_list_length) > message_length
• :277 — length >= message_length, guarding the one-byte compression methods length
• :287 — (length + compression_methods_length) > message_length
• :312 and :320 — the extension length and body

The ServerHello parser has the first of those and then nothing until the extension block. So the two parsers were held to different standards, and your patch brings the ServerHello up to the one the ClientHello already meets. That is a better argument for merging it than the reproduction is.

A small stylistic difference, which I mention only so it is a deliberate choice rather than an accident: the ClientHello code uses one check per field in the (length + 1) >= message_length form, where you use a single combined (length + 3) > message_length. Both are correct and yours is the clearer of the two. I am not asking you to match the older style.