Fix TLS 1.3 CertificateVerify transcript hash length (use ciphersuite hash)

[EdouardMALOT]

Summary

Follow-up to the #377 review discussion (#377 (comment)):
the dynamic transcript-hash length introduced there is keyed to the signature scheme's
hash, but per RFC 8446 §4.4.3 the
CertificateVerify content embeds Transcript-Hash(Handshake Context), whose length is fixed
by the negotiated ciphersuite's hash. The two differ whenever a peer signs with a hash
other than the suite's.

Concrete case with the only TLS 1.3 suite currently enabled (TLS_AES_128_GCM_SHA256):
an ECDSA P-384 certificate signs ecdsa_secp384r1_sha384, so the copy overruns its 32-byte
transcript-hash slot and reads 48 bytes, spilling into the adjacent CLIENT_FINISHED row of
nx_secure_tls_transcript_hashes[5][32]. The read stays inside the array, so this is a
correctness and interoperability regression, not a memory-safety one. It corrupts the signed content
and rejects valid peer signatures on the verify side — TLS 1.3 no longer works with ECDSA
P-384/P-521 certificates (it did before #377, when the copy length was a constant 32,
which was correct for every configuration shipped at the time).

The concern raised in the review remains addressed: the length is no longer hardcoded and
derives from the ciphersuite. Enabling a SHA-384/512 suite needs more than that, though —
NX_SECURE_TLS_MAX_HASH_SIZE is 32, so _nx_secure_tls_1_3_transcript_hash_save would overrun
a transcript row before CertificateVerify is reached. That constant would have to grow first,
and it sizes the key schedule too.

Changes

• nx_secure/src/nx_secure_tls_send_certificate_verify.c — key the transcript-hash copy
  length to nx_secure_tls_session_ciphersuite -> nx_secure_tls_hash (SHA-256 fallback,
  mirroring _nx_secure_tls_1_3_transcript_hash_save). The signature scheme's hash still
  digests the assembled content and parameterizes RSA-PSS.
• nx_secure/src/nx_secure_tls_process_certificate_verify.c — same fix on the verify side.
• test/regression/nx_secure_test/nx_secure_tls_1_3_certverify_transcript_hash_test.c —
  regression test calling both functions directly with a signature-scheme hash (SHA-384) that
  differs from the ciphersuite hash (SHA-256), capturing via a spy hash method the exact byte
  count copied into the CertificateVerify content. Asserts 32 bytes (ciphersuite SHA-256,
  correct), not 48 (signature-scheme SHA-384, what the bug copied).

Test plan

• Regression test added, exercising both the send and process paths directly
• Verified against real hardware: a NetX Duo TLS 1.3 client repeatedly and reliably
  completes the handshake against a broker presenting EC P-384 and P-521 server
  certificates (ecdsa_secp384r1_sha384 / ecdsa_secp521r1_sha512), where it previously
  rejected the broker's valid CertificateVerify signature. A P-256 control case (unaffected
  by the bug, same hash on both sides) continues to pass throughout.
• Regression check: P-256 / RSA (sha256 schemes) handshakes unchanged

Related: #377, #399, #161.

[fdesbiens]

Thank you — this is an unusually well-argued PR. The description states a specific claim, points at the review discussion that caused it, and comes with a test that pins the behaviour. That made it straightforward to check rather than take on trust, which is the best thing a description can do.

The regression is real and I traced it to the commit. 17d3abf (PR #377) replaced a constant with a derived length:

-         NX_SECURE_MEMCPY(&handshake_hash[64 + 34], transcript_hash, 32);
-         handshake_hash_length = 130;
+         UINT transcript_hash_len = (UINT)(hash_method -> nx_crypto_ICV_size_in_bits >> 3);
+         NX_SECURE_MEMCPY(&handshake_hash[64 + 34], transcript_hash, transcript_hash_len);
+         handshake_hash_length = 64u + 34u + transcript_hash_len;

with hash_method being crypto_methods -> nx_secure_x509_hash_method, i.e. the signature scheme's hash. That commit's own message describes the intent as fixing "SHA-384/512 transcript hash truncation" — the goal was right and the source of the length was wrong. Your reading of it is exact.

And your reading of RFC 8446 is right. §4.4.3 defines the signed content as 64 octets of 0x20, the context string with its zero separator, then Transcript-Hash(Handshake Context, Certificate); §4.4.1 defines Transcript-Hash in terms of the negotiated ciphersuite's hash. Those are two different hashes and the code was conflating them. The signature scheme's hash still correctly digests the assembled content and parameterizes PSS, which your change preserves — the comment rewrite at :244 makes that distinction clear in a way the old comment actively obscured.

The fix matches the existing idiom exactly. _nx_secure_tls_1_3_transcript_hash_save.c:79-88 makes the same choice with the same SHA-256 fallback for a null ciphersuite. Keying off the same source in both directions is what makes the send and verify sides agree with the peer, and copying the reference function's structure rather than inventing a variant is the right call.

The test discriminates — I checked rather than assumed. I built tls_1_3_enable_build_coverage, reverted only the two source files to dev while keeping the new test, and rebuilt:

ERROR! ... nx_secure_tls_1_3_certverify_transcript_hash_test.c Line: 136
Expected: 0x82, (130) Got: 0x92 (146)

146 is 98 + 48 and 130 is 98 + 32, which is the predicted arithmetic to the byte. With the fix restored it passes, and the full suite is 151/151 on the branch, so nothing else moved. The spy-hash approach is a good choice here: it observes the assembled content length directly rather than inferring it from a handshake outcome, which is what makes the assertion sharp.

Two things I would like on the record, in findings 1 and 2. Neither asks you to change the code.

[fdesbiens]

Not a change request — the fix is right regardless. This is about how the defect gets characterised, because the description could reasonably be read as describing a buffer overflow, and I want the record accurate before anyone decides whether this needs a security advisory. My conclusion is that it does not.

The description says "48 bytes are copied from a 32-byte transcript-hash slot". True of the slot, but the slot is a row of a two-dimensional array:

    UCHAR nx_secure_tls_transcript_hashes[NX_SECURE_TLS_1_3_MAX_TRANSCRIPT_HASHES][NX_SECURE_TLS_MAX_HASH_SIZE];

with NX_SECURE_TLS_1_3_MAX_TRANSCRIPT_HASHES at 5 and NX_SECURE_TLS_MAX_HASH_SIZE at 32, so 160 bytes in total. NX_SECURE_TLS_TRANSCRIPT_IDX_CERTIFICATE is 2, so the copy starts at offset 64 and reads 48 bytes, ending at 112. Even a 64-byte SHA-512 signature scheme ends at 128. Both are comfortably inside the 160-byte array — the bytes read are the neighbouring CLIENT_FINISHED transcript row, which is wrong data but is memory the session already owns.

The destination is fine too: handshake_hash is [64 + 34 + 64], 162 bytes, and #377 resized it from 130 precisely so a SHA-512 transcript would fit. The longest write here is 146.

So the consequence is exactly the interop failure you describe — corrupted signed content on the send side, valid peer signatures rejected on the verify side — with no read or write outside any object. That makes it a correctness and interoperability regression rather than a vulnerability, and I do not intend to open an advisory for it. Worth saying explicitly, because had IDX_CERTIFICATE been the last row rather than the third, the same code would have been a genuine out-of-bounds read and this would be a different conversation.

If you are willing, softening that sentence in the PR description to say the copy overruns the slot into the adjacent transcript row would make the commit history read accurately later. Entirely optional.

[fdesbiens]

The description says the review concern from #377 "remains fully addressed: the length is not hardcoded, and SHA-384/512 ciphersuites will produce 48/64-byte transcript hashes when they are enabled." The first half is right and is the point of this PR. The second half is where I would add a caveat, because it is the part someone will rely on later.

The length derivation would indeed produce 48 or 64. The storage cannot hold it. Each transcript row is NX_SECURE_TLS_MAX_HASH_SIZE bytes, which is 32. So the first thing that breaks when a SHA-384 ciphersuite is enabled is not this code at all — it is _nx_secure_tls_1_3_transcript_hash_save.c, which computes hash_size from the same ciphersuite hash and writes that many bytes to &...nx_secure_tls_transcript_hashes[hash_index][0]. That is a 48-byte write into a 32-byte row, i.e. a real overflow into the following row, and it happens before CertificateVerify is ever reached.

This is latent rather than live, and deliberately so — the SHA-384 suite is commented out in crypto_libraries/src/nx_crypto_generic_ciphersuites.c:199 with the note "SHA-384 ciphersuites not yet supported", and every enabled TLS 1.3 suite is SHA-256. So nothing is broken today and this PR does not make it worse. But it means enabling one is not a matter of uncommenting a table row: NX_SECURE_TLS_MAX_HASH_SIZE would have to grow first, and it is load-bearing well beyond the transcript array — tls_early_secret, tls_binder_key, tls_handshake_secret, tls_master_secret, the traffic secrets and NX_SECURE_TLS_KEY_MATERIAL_SIZE are all sized from it, so widening it has a RAM cost on every session and deserves its own review.

Where this touches your PR: case 2 of the new test synthesises a SHA-384 ciphersuite and asserts a 48-byte transcript length. As a unit test of the length derivation that is exactly right and I am glad it is there. It is worth a comment noting that the configuration it simulates cannot yet be run end to end, so nobody later reads a passing test as evidence that SHA-384 suites work. A sentence in the file's header comment would do it.

No action needed beyond that. I am recording it here so the constraint is written down somewhere other than a commented-out table row.

[EdouardMALOT]

@fdesbiens :
Both done, thank you, the second finding in particular, which I had not traced that far.

The description now says the copy overruns its slot into the adjacent transcript row, with the read staying inside the array, and the SHA-384 claim is caveated. The test file's header comment records that configuration 2 is synthetic and that NX_SECURE_TLS_MAX_HASH_SIZE would have to grow first — confirmed at nx_secure_tls_1_3_transcript_hash_save.c:113 and :127-128.