Fix off-by-one over-read in TLS 1.2 ClientHello and CertificateRequest

[nvxbug]

Description

Repro: a TLS 1.2 ClientHello whose body ends right after the session id, i.e. msg_len == 35 + sess_len + 1, passes the session-id length check but the parser then reads the two-byte ciphersuite-list length. A CertificateRequest that ends inside its distinguished-name length field behaves the same on the client side.

Cause: in ssl_parse_client_hello the session-id check only guarantees sess_len + 36 <= msg_len, one byte short of the sess_len + 37 needed for MBEDTLS_GET_UINT16_BE(buf, 35 + sess_len), so the second length byte is read one byte past the message body. The DTLS branch reserves those two bytes correctly through its cookie-length check; only the non-DTLS path is short. The same shortfall exists before the dn_len read in ssl_parse_certificate_request.

Fix (reworked per review): amend the existing checks instead of adding new ones. In ssl_parse_client_hello the session-id check now covers just the session id, the cookie length byte is checked before it is read, and the ciphersuite-list length check covers both transports. In ssl_parse_certificate_request the signature-algorithms check also reserves the two certificate-authorities length bytes, the checks use the available < needed pattern, and an odd-length signature-algorithms list is rejected. Valid handshakes are unaffected; the truncated messages that hit this were already rejected by the consistency check that follows, so the only behavior change is that the over-read no longer happens.

PR checklist

• changelog provided
• framework PR not required
• TF-PSA-Crypto development PR not required because: no crypto core change
• TF-PSA-Crypto 1.1 PR not required because: no crypto core change
• mbedtls development PR provided (this PR)
• mbedtls 4.1 PR provided Backport 4.1: Fix off-by-one over-read in TLS 1.2 ClientHello and CertificateRequest #10929
• mbedtls 3.6 PR provided Backport 3.6: Fix off-by-one over-read in TLS 1.2 ClientHello and CertificateRequest #10928
• tests provided (injected truncated ClientHello cases, run in TLS 1.2-only builds)

[nvxbug]

any update?

[gilles-peskine-arm]

Thank you for contributing this fix. I believe that it is functionally correct, but the code is giving me a headache, so I would prefer to fix the existing checks instead of adding another one.

Adding tests is entirely optional and I can propose something Codex wrote.

[nvxbug]

Pushed the rework, details in the inline replies. Short version: existing checks amended instead of adding new ones, odd sig_alg_len now rejected, and there are two injected truncated ClientHello tests for the server side.

[gilles-peskine-arm]

Thank you for updating the hard-to-understand checks to classic if (read_length > available_size) return ERROR checks. Looks good to me.

Would you mind making pull requests for mbedtls-4.1 and mbedtls-3.6 as well?

[irwir]

mbedtls/library/ssl_tls12_client.c

Line 2220 in 522e2e4

if (ssl->in_hslen <= mbedtls_ssl_hs_hdr_len(ssl)) {

mbedtls/library/ssl_tls12_client.c

Line 2239 in 522e2e4

if (ssl->in_hslen <= mbedtls_ssl_hs_hdr_len(ssl) + 2 + n) {

What is the reason to change these conditions?
The new conditions seems to be equivalent to the previous ones, but with unnecessary extra operation of addition.

[nvxbug]

@irwir yes, those two are equivalent, it's deliberate. The check with the actual off-by-one was written in the available <= needed - 1 shape, which is likely why the error went unnoticed for so long, so the review asked to convert the remaining <= checks in this function to the plain available < needed form while at it (see the thread above). The extra additions are compile-time constants, so the generated code doesn't change.

@gilles-peskine-arm backports are up: #10929 for 4.1 and #10928 for 3.6.

[irwir]

mbedtls_ssl_hs_hdr_len(ssl) is a compile time constant if MBEDTLS_SSL_PROTO_DTLS was not defined.

[nvxbug]

Right, with DTLS compiled in it's a runtime value. It's a static inline returning 4 or 12 though, so after inlining the literal offsets still fold and the operand count stays the same, e.g. hdr + 1 + n + 2 compiles as hdr + 3 + n, one addition, same as the old hdr + 2 + n. I compared the -O2 output of the old and new forms with the transport check compiled in and the new form is no larger. Either way the rewrite was about readability, per the review thread above.