Moved the CertificateVerify buffers into the TLS session metadata - #431
Open
fdesbiens wants to merge 1 commit into
Open
Moved the CertificateVerify buffers into the TLS session metadata#431fdesbiens wants to merge 1 commit into
fdesbiens wants to merge 1 commit into
Conversation
_nx_secure_tls_send_certificate_verify and _nx_secure_tls_process_certificate_verify built and checked every CertificateVerify signature in file-scope statics: handshake_hash, the PKCS#1 block (_nx_secure_padded_signature / _nx_secure_decrypted_signature) and, on the verify side, _nx_secure_pss_scratch. _nx_secure_tls_protection is released before the handshake runs, so two sessions can be inside those routines at the same time and overwrite each other's working data. A client authenticating on two sessions at once could emit a CertificateVerify covering the other session's transcript; a server verifying two TLS 1.3 clients at once could reject a valid signature. The signature is invalid rather than forgeable, so the visible symptom is an intermittent authentication failure under concurrency that never reproduces in single-session testing. Fixes eclipse-threadx#430. The buffers are now carved out of the application-supplied crypto metadata area at session create time, which is already per-session, so no new fixed-size field is added to NX_SECURE_TLS_SESSION. Sending and processing a CertificateVerify never overlap within one session, so the two paths share the same area: 764 bytes per session, or 1364 with TLS 1.3 enabled, and nothing at all when NX_SECURE_DISABLE_X509 is defined. The sizes come from NX_SECURE_TLS_CERTIFICATE_VERIFY_SIGNATURE_SIZE and NX_SECURE_TLS_CERTIFICATE_VERIFY_PSS_SCRATCH_SIZE, both of which an application can lower when its largest certificate key is known to be smaller than the 4096-bit default of NX_CRYPTO_MAX_RSA_MODULUS_SIZE. Because the required metadata grows, an application that passes a hard-coded metadata buffer instead of the value from nx_secure_tls_metadata_size_calculate will now get NX_INVALID_PARAMETERS from nx_secure_tls_session_create until it resizes the buffer. Both routines also now reject a modulus larger than the signature buffer instead of writing past the end of it. That check was missing while the buffers were fixed 600-byte statics, and it matters more now that the size is configurable. nx_secure_tls_certificate_verify_concurrency_test is new. It preempts one session inside each routine at the point where the handshake hash has been written but not yet consumed, runs a second session to completion, and checks that the first session still produced or accepted its own transcript. It fails on the code before this change, on the signing side and the verifying side alike, and it also covers the two new modulus checks. nx_secure_tls_client_handshake_coverage_test assembles a TLS session by hand rather than through nx_secure_tls_session_create, so it now supplies the scratch area the same way it already supplies the crypto table. Assisted-by: Claude Code (Opus 5)
Contributor
Author
|
@EdouardMALOT would you be willing to review this one? A formal review request is not possible because review requests are limited to repository collaborators, hence the ping. You found this bug while working on #399, and that PR is the part of the tree most affected: the send path's PSS scratch will want to come from the per-session region this adds rather than a new static. The two review questions I would most value your eyes on are whether sharing one region between the send and process paths is safe in every handshake ordering you have exercised, and whether the growth in the required metadata size is acceptable for the targets you care about. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #430.
The problem
_nx_secure_tls_send_certificate_verifyand_nx_secure_tls_process_certificate_verifybuilt and checked every CertificateVerify signature in file-scopestaticbuffers:handshake_hash64 + 34 + 64_nx_secure_padded_signature_nx_secure_decrypted_signature_nx_secure_pss_scratch_nx_secure_tls_protectionis released before the handshake runs, so two sessions can be inside those routines at the same time and overwrite each other's working data. A client authenticating on two sessions at once could emit a CertificateVerify covering the other session's transcript; a server verifying two TLS 1.3 clients at once could reject a valid signature. The signature is invalid rather than forgeable, so the visible symptom is an intermittent authentication failure under concurrency that never reproduces in single-session testing.The fix
Option 2 from the issue: the buffers are carved out of the application-supplied crypto metadata area at session create time, which is already per-session, so no new fixed-size field is added to
NX_SECURE_TLS_SESSION.Sending and processing a CertificateVerify never overlap within one session, so the two paths share one region:
NX_SECURE_DISABLE_X509is definedThe sizes come from
NX_SECURE_TLS_CERTIFICATE_VERIFY_SIGNATURE_SIZEandNX_SECURE_TLS_CERTIFICATE_VERIFY_PSS_SCRATCH_SIZE, both of which an application can lower when its largest certificate key is known to be smaller than the 4096-bit default ofNX_CRYPTO_MAX_RSA_MODULUS_SIZE.The issue was filed against the signing side only. The verify side has the identical bug with the opposite symptom — rejecting a valid peer signature — so it is fixed here too rather than left for a follow-up.
Please note: metadata requirement grows
An application that passes a hard-coded metadata buffer instead of the value from
nx_secure_tls_metadata_size_calculatewill now getNX_INVALID_PARAMETERSfromnx_secure_tls_session_createuntil it resizes the buffer. This is the main review question on the change.One thing beyond the issue
Neither routine bounded
data_size(the certificate's RSA modulus length) against the signature buffer. It could not bite at the old fixed 600 bytes withNX_CRYPTO_MAX_RSA_MODULUS_SIZEat its 4096-bit default, but it becomes reachable now that the size is configurable, so both routines now reject an oversized modulus withNX_SECURE_TLS_INVALID_CERTIFICATEinstead of writing past the end of the buffer.Tests
nx_secure_tls_certificate_verify_concurrency_testis new. It hooks the SHA-256 handshake hash method to preempt one session at the exact point where the handshake hash has been written but not yet consumed, resumes a higher-priority thread that runs a second session to completion, and then checks that the first session still produced or accepted its own transcript. Verified to fail on the code before this change, by restoring each source file fromdevin turn:0x131NX_SECURE_TLS_CERTIFICATE_VERIFY_FAILUREon a valid signatureA third phase covers the two new modulus checks.
nx_secure_tls_client_handshake_coverage_testassembles anNX_SECURE_TLS_SESSIONby hand rather than throughnx_secure_tls_session_create, so it now supplies the scratch area the same way it already hand-assignsnx_secure_tls_crypto_table.build_nxd_fast)Two coverage-report steps abort in my environment, both pre-existing and unrelated to this change:
addons/webhits a gcovr 7.0 parser limitation on gcc 13's%%%%%:NNNNN-blockoutput, andtest/cmake/crypto/coverage.sh(unchanged fromdev) passes a--gcov-ignore-parse-errorsvalue gcovr 7.0 does not accept. All tests in both suites pass.Documentation
Matching documentation PR against
rtos-docs-asciidoc: the two new configuration options in the TLS and DTLS option tables, and a paragraph in the TLS "Cryptographic Metadata" section covering the per-session reservation and the hard-coded-buffer consequence.Note for #399
@EdouardMALOT — this touches the ground #399 builds on.
_nx_secure_pss_scratchin the send path does not exist yet ondev, so #399 will need its PSS scratch to come from the same per-session region rather than a new static. The area already reservesNX_SECURE_TLS_CERTIFICATE_VERIFY_PSS_SCRATCH_SIZEunder TLS 1.3 atNX_SECURE_TLS_CERTIFICATE_VERIFY_PSS_OFFSET, and the send path can use it exactly as the verify path does, with no further sizing change. The comment on_nx_secure_pss_scratchin #399 that points at this constraint can point at this PR.