Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR (“Fenrir fixes”) adds regression tests for multiple reported TLS/TLS-extension failure modes (issues 1824, 2126, 2128, 2131, 2913–2916, 2921–2922, 2925, 2927) and hardens several TLS extension size calculations to better handle potential 16-bit length overflows.
Changes:
- Add new API-level tests covering EMS resumption downgrade, AEAD/tag corruption handling, secure renegotiation verify_data mismatches, TLS 1.3 HRR cipher suite mismatches, and TLS 1.3 ticket age window behavior.
- Add a new test validating session ticket HMAC verification failure via the ticket key callback.
- Update multiple TLS extension “GetSize” helpers to use wider intermediates and add overflow checks.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| tests/api/test_tls_ext.h | Exposes new TLS extension test prototypes. |
| tests/api/test_tls_ext.c | Implements new TLS/extension negative-path regression tests. |
| tests/api.c | Registers new tests and adds a session-ticket HMAC corruption test. |
| src/tls.c | Adds overflow checks in several TLS extension size calculators and returns LENGTH_ERROR for PSK oversize conditions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
julek-wolfssl
force-pushed
the
fenrir/20260415
branch
from
6b9b62d to
47ea569
Compare
Member
Author
|
retest this please history lost.. |
Match the TLSX_SNI_GetSize pattern: use a word32 accumulator and return 0 if the aggregate size exceeds WOLFSSL_MAX_16BIT, so a large number of ALPN entries can no longer silently wrap the length computation.
Mirror the TLSX_SNI_GetSize pattern: accumulate into a word32 and return 0 when the aggregate size exceeds WOLFSSL_MAX_16BIT so large idSz values or many TCA entries no longer silently wrap to a small value that undersizes the TLSX_TCA_Write output buffer.
julek-wolfssl
force-pushed
the
fenrir/20260415
branch
from
47ea569 to
e7098e6
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
julek-wolfssl
force-pushed
the
fenrir/20260415
branch
from
e7098e6 to
86c7ea8
Compare
Both TLSX_PreSharedKey_GetSize and TLSX_PreSharedKey_GetSizeBinders accumulate per-identity bytes into a word16. With enough PSK entries (or large binderLen/identityLen values) the accumulator wraps silently and the caller allocates an undersized extension buffer, which TLSX_PreSharedKey_Write then overflows. Switch both accumulators to word32 and return LENGTH_ERROR when the total would exceed the 16-bit wire length field.
The CA Names extension size accumulator was a word16. With enough CA entries (or large DER-encoded names) the running total can wrap silently, leaving TLSX_CA_Names_Write to overflow an undersized extension buffer. Match TLSX_SNI_GetSize: use a word32 accumulator and return 0 when the total exceeds WOLFSSL_MAX_16BIT.
Covers the HandleResumeHistory check that RFC 7627 Section 5.3 requires: if the original session used Extended Master Secret, the server MUST abort when a resumption ClientHello is received without EMS. The new memio test performs a TLS 1.2 handshake with EMS, saves the session, disables EMS on a fresh client, resumes with the saved session, and asserts the server returns EXT_MASTER_SECRET_NEEDED_E.
Cover the Poly1305 ConstantCompare tag check in ChachaAEADDecrypt that no existing test was hitting (VERIFY_MAC_ERROR never expected in the suite). A memio-based TLS 1.2 handshake over ECDHE-RSA-CHACHA20-POLY1305 completes, the server's IORecv is then replaced with a wrapper that flips the final byte of the next record body so the forged Poly1305 tag no longer matches. The server's wolfSSL_read must surface VERIFY_MAC_ERROR.
Tls13IntegrityOnly_Decrypt was completely untouched by existing tests, so any mutation of its ConstantCompare would pass CI. Add a memio TLS 1.3 handshake over TLS13-SHA256-SHA256 (integrity-only NULL cipher), then corrupt the final byte of the next record body via an IORecv wrapper and assert the server surfaces DECRYPT_ERROR.
Cover both branches of TLSX_SecureRenegotiation_Parse's ConstantCompare against the cached Finished verify_data: a single memio test loops over client-side and server-side corruption, renegotiates, and asserts the offending peer surfaces SECURE_RENEGOTIATION_E.
wolfSSL_TicketKeyCb is the built-in ticket callback registered by the OpenSSL-compat wolfSSL_CTX_set_tlsext_ticket_key_cb API. Its ConstantCompare of the ticket HMAC was never reached in any test, so a deletion of the check would silently accept forged tickets. New test sets up the compat callback, establishes a TLS 1.2 session, saves it, flips a byte of the encrypted ticket, and asserts the resumption attempt does not complete.
DoTls13ClientHello enforces RFC 8446 Section 4.1.4 by comparing the cipher suite in the second ClientHello to the hrrCipherSuite cached on the server from the HelloRetryRequest. No existing test covers the mismatch branch, so a deletion of the check would silently allow a client to switch cipher suite between CH1 and CH2. Drive a partial handshake until the server has emitted the HRR, then flip the cached hrrCipherSuite on the server; processing CH2 must surface INVALID_PARAMETER.
DoClientTicketCheck's ticket-age bounds (-1000 ms low bound and MAX_TICKET_AGE_DIFF*1000+1000 ms high bound) were never exercised by any integration test, so mutations of the constants went undetected. Establish a TLS 1.3 session, read the NewSessionTicket, then shift the client's cached ageAdd by well over 1 second so the server's unobfuscated diff falls outside the valid window on resumption. The server must reject the PSK — session_reused stays 0.
julek-wolfssl
force-pushed
the
fenrir/20260415
branch
from
86c7ea8 to
54d96d3
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+442
to
+444
|
|
||
| /* Flip the high bit to push the unobfuscated age out of window. */ | ||
| if (session != NULL) |
There was a problem hiding this comment.
This test can pass without actually exercising the ticket-age window check if the client never receives a NewSessionTicket (e.g., session->ticketLen remains 0). Consider asserting the session has a ticket (e.g., ticketLen > 0) before mutating ticketAdd and attempting resumption, so the test fails if resumption PSK material wasn't established.
Suggested change
| /* Flip the high bit to push the unobfuscated age out of window. */ | |
| if (session != NULL) | |
| if (session != NULL) | |
| ExpectTrue(session->ticketLen > 0); | |
| /* Flip the high bit to push the unobfuscated age out of window. */ | |
| if (session != NULL && session->ticketLen > 0) |
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.
Issue numbers:
1824
2126
2128
2131
2913
2914
2915
2916
2921
2922
2925
2927