Fenrir 2026-06-02: TLS/DTLS correctness, resumption & renegotiation safety fixes#10582
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses multiple Fenrir-tracked TLS/DTLS correctness and safety findings across parsing strictness, renegotiation handling, resumption validation, record-sequence hygiene, session cache invalidation on fatal alerts, key zeroization, and adds regression tests to prevent reintroduction.
Changes:
- Tighten TLS 1.3 message parsing to reject trailing bytes after extension blocks (EncryptedExtensions, CertificateRequest).
- Harden TLS 1.2 renegotiation and resumption behavior (NO_RENEGOTIATION option behavior, SCR renegotiation_info enforcement, EMS + cipher-suite consistency on resumption, refuse sequence-number wrap).
- Add/extend regression tests (memio renegotiation refusal tests; RSA verify/PSS negative tamper tests) and improve DTLS 1.3 key hygiene (zeroization before free).
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| wolfssl/internal.h | Extends secure renegotiation state with a new per-handshake renegInfoSeen flag and repacks SCR flags into bitfields. |
| wolfssl/error-ssl.h | Introduces SEQUENCE_NUMBER_E for TLS record sequence-number wrap prevention. |
| tests/api/test_tls_ext.h | Exposes new memio regression test prototypes for NO_RENEGOTIATION behavior. |
| tests/api/test_tls_ext.c | Adds client/server memio tests ensuring NO_RENEGOTIATION refuses renegotiation via warning alert while keeping the connection usable. |
| tests/api/test_ossl_rsa.c | Adds negative tamper tests for RSA v1.5 verify and RSA-PSS verification paths. |
| tests/api.c | Registers the new TLS extension regression tests in the main test list. |
| src/tls13.c | Enforces strict length equality for TLS 1.3 EncryptedExtensions and rejects trailing bytes in CertificateRequest. |
| src/tls.c | Tracks presence of renegotiation_info when parsing SCR extension in ClientHello. |
| src/internal.c | Implements NO_RENEGOTIATION warning behavior, SCR renegotiation checks, session invalidation on fatal alerts, TLS 1.2 record sequence wrap refusal, DTLS 1.3 ChaCha key zeroization, and resumption EMS/suite consistency checks. |
| src/dtls13.c | Zeroizes DTLS 1.3 record-number ChaCha context on wc_Chacha_SetKey failure before freeing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
9c5a037 to
a31b83b
Compare
|
retest this please |
|
e1b916c to
94d1bb7
Compare
|
Retest this please.. |
fa6ab55 to
a773a88
Compare
Extend test_wolfSSL_RSA_verify and test_wolfSSL_RSA_padding_add_PKCS1_PSS with negative cases that flip a byte in the signature/encoding and in the hash, asserting verification fails. This guards the XMEMCMP-based signature acceptance decision in wolfSSL_RSA_verify_mgf against regressions that would let any decryption result of matching length pass as valid.
DoTls13EncryptedExtensions only bounds-checked the extensions length against the message size, silently ignoring any trailing bytes. RFC 8446 Section 4.3.1 defines the message as solely the extensions block, so enforce length equality and return BUFFER_ERROR (decode_error) on a mismatch.
DoTls13CertificateRequest advanced past the certificate_request_context and extensions blocks but never verified the whole message body was consumed, silently ignoring trailing bytes. RFC 8446 Section 4.3.2 fixes the wire format; enforce that the consumed length equals the message size and return BUFFER_ERROR (decode_error) otherwise.
FreeCiphers released the DTLS 1.3 record-number protection ChaCha contexts with XFREE only, leaving key material in freed heap memory. ForceZero both contexts before freeing, matching the regular TLS ChaCha path in FreeCiphersSide, and also zeroize a partially-set key in Dtls13InitChaChaCipher when wc_Chacha_SetKey fails.
CompleteServerHello's resumption branch derived keys from the cached master secret without checking the resumed session's extended_master_secret state against the abbreviated ServerHello, letting a MITM strip EMS on resumption. Per RFC 7627 Section 5.3, abort with a fatal handshake_failure when the cached session's EMS flag does not match the ServerHello EMS state.
On session-ID resumption the client only checked that the server's selected suite was in its offered list, not that it equaled the resumed session's suite, so a server could resume the session ID under a different cipher suite. Per RFC 5246 Section 7.4.1.2 / F.1.4 a resumed session reuses its negotiated suite; abort with a fatal illegal_parameter on mismatch.
GetSEQIncrement silently rolled the 64-bit write sequence counter from 2^64-1 back to 0, reusing sequence number 0 with the same keys. Per RFC 5246 Section 6.1 sequence numbers MUST NOT wrap. BuildMessage now refuses to emit a TLS 1.2 record once the write sequence number has reached its maximum, returning the new SEQUENCE_NUMBER_E error so the caller renegotiates or closes instead.
DoAlert marked a connection closed on a received fatal alert but left the established session in the resumption cache, and the send path did the same, so a session whose connection ended in a fatal alert remained resumable. Per RFC 5246 Section 7.2.2 the session identifier MUST be invalidated; evict the established session from the cache on both receipt and transmission of a fatal alert via the new InvalidateSessionOnFatalAlert helper.
The documented 'reject peer-initiated renegotiation' option was accepted and stored but never consulted. Now DoHelloRequest replies with a no_renegotiation warning instead of starting SCR when the bit is set (client side), and the server refuses a renegotiation ClientHello with a no_renegotiation warning instead of resetting handshake state.
The server validated client_verify_data only inside TLSX_SecureRenegotiation_Parse, which never runs when the renegotiation_info extension is absent, so a renegotiation ClientHello that omitted it was never checked. Track a per-handshake renegInfoSeen flag and, after parsing the renegotiation ClientHello extensions, abort with handshake_failure if the extension was absent (RFC 5746 3.7). Also reject an SCSV received during renegotiation (RFC 5746 3.5).
a773a88 to
1f4afe9
Compare
In the WOLFSSL_OP_NO_RENEGOTIATION refusal path, WOLFSSL_LEAVE logged a hard-coded 0 while the function actually returned SendAlert()'s result. Capture the return value first so the trace reflects reality (e.g. when SendAlert fails due to write backpressure) and return it.
|
analysis from Opus 4.8-High: F-5811 / F-5807 skip ticket resumption. Both the suite-match and EMS checks are gated behind Worth confirming whether ticket-resumed suite is validated elsewhere on the client path. If not, this is a candidate for a precise follow-up (apply the same |
|
@douzzer addressed |
The TLS 1.2 client only compared the ServerHello suite against the cached session suite for session-ID resumption; ticket resumption was skipped on the assumption the suite is bound in the ticket. But the ticket is opaque to the client, so it must enforce the match itself - otherwise a server could resume a ticket under a different (weaker) suite the client offered and the downgrade would go undetected (RFC 5246 7.4.1.3). The check is skipped only when the client retained no suite for the session (cipherSuite0/cipherSuite both zero), so there is nothing to compare against - as for EAP-FAST, whose PAC is a TLS ticket whose keys come from the session-secret callback and which never populates the cached suite. (0,0) is TLS_NULL_WITH_NULL_NULL, never negotiated, so it unambiguously means "no retained suite". The EMS check remains ticket-gated. Add memio regression tests: a ticket resumption under a different (retained) suite is rejected with MATCH_SUITE_ERROR, and a resumption whose cached suite was not retained still succeeds.
9ce49b9 to
2da5b24
Compare
dgarske
left a comment
There was a problem hiding this comment.
Skoll Code Review
Scan type: reviewOverall recommendation: COMMENT
Findings: 8 total — 3 posted, 5 skipped
2 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [Medium] Client-side EMS resumption check (server-drops-EMS direction) has no test —
src/internal.c:32344-32360 - [Medium] EMS resumption check excludes ALL ticket resumptions, asymmetric with the suite check —
src/internal.c:32350-32360 - [Low] Trailing-byte rejection returns BUFFER_ERROR without a TLS alert —
src/tls13.c:6033, src/tls13.c:6171-6173
Skipped findings
- [Low]
Fatal-alert session invalidation (F-5818) is untested on both send and receive paths - [Low]
TLS 1.3 send path evicts session only on alert_fatal, asymmetric with receive path - [Medium]
No negative tests for TLS 1.3 trailing-byte rejection (EncryptedExtensions / CertificateRequest) - [Low]
New brace-only block introduced solely to declare a variable in test - [Medium]
Most correctness fixes ship without dedicated regression tests
Review generated by Skoll
Address review on PR wolfSSL#10582: - The client-side extended_master_secret consistency check skipped all session-ticket resumptions, leaving a generic ticket resumption open to an undetected EMS downgrade by a malicious server or MITM. The client retains the EMS state for ticket sessions too (SetupSession), so the check now applies to ticket resumption as well, mirroring the adjacent cipher-suite check. Only EAP-FAST style resumption - where the session-secret callback supplies the master secret for an opaque PAC ticket - is exempt, matched precisely via ssl->sessionSecretCb just as the callback invocation in DoServerHello does. - Add test_tls_ems_resumption_server_downgrade, exercising the client-direction downgrade (server resumes but omits EMS from its ServerHello) for both session-ID and session-ticket resumption. This client-side branch previously had no test coverage.
|
Jenkins retest this please: "Build 'PRB-valgrind-check-v3' failed with result: FAILURE" -> "[cd wolfssl && ../testing/Jenkins/PRB-valgrind-check-v3/valgrind-check.sh --enable-opensslall --enable-sniffer CPPFLAGS="-DWOLFSSL_OLD_PRIME_CHECK"]" |
|
retest this please |
dgarske
left a comment
There was a problem hiding this comment.
Skoll Code Review
Scan type: reviewOverall recommendation: REQUEST_CHANGES
Findings: 7 total — 5 posted, 2 skipped
5 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [High] Client EMS/cipher-suite resumption checks fire on unconfirmed ticket resumption, breaking RFC 5077 ticket-decline fallback to full handshake —
src/internal.c:32344-32395 - [Medium] DoAlert invalidates the cached session before the alert is validated; forged TLS 1.3 plaintext alert evicts the session —
src/internal.c:22596-22604 - [Medium] test_tls12_resume_ticket_wrong_suite missing WOLFSSL_NO_DEF_TICKET_ENC_CB gate —
tests/api/test_tls.c:889-896 - [Low] Null-pointer checks in FreeCiphers use truthiness instead of explicit NULL comparison —
src/internal.c:3346-3349 - [Low] BuildMessage refuses the last RFC-legal sequence number (2^64-1) —
src/internal.c:24717-24728
Skipped findings
- [Medium]
No regression tests for F-4867, F-4868, F-5810, F-5813, F-5818, F-5633 - [Low]
SecureRenegotiation struct: new member inserted mid-struct and existing byte fields converted to bitfields
Review generated by Skoll
dgarske
left a comment
There was a problem hiding this comment.
Please review skoll reports: #10582 (review)
The client's resumed-session EMS (F-5807) and cipher-suite (F-5811) checks were enforced in CompleteServerHello at ServerHello-parse time. For stateless ticket resumption the client sends an empty session ID and cannot yet tell whether the server accepted the ticket (RFC 5077 3.4): a server that declines the ticket falls back to a full handshake under a freshly negotiated suite/EMS state, which these checks wrongly aborted with MATCH_SUITE_ERROR, breaking the RFC 5077 ticket-decline fallback to a full handshake. Move both checks into CheckResumptionConsistency and run it only once resumption is confirmed - from whichever the server sends first in the abbreviated flight: a renewed NewSessionTicket (before SetupSession refreshes the cached suite/EMS to the current values) or its ChangeCipherSpec. By then the "Not resuming as thought" path has cleared 'resuming' for any ticket decline, so the full-handshake fallback proceeds. Add test_tls12_resume_ticket_decline_fallback (ticket declined by a fresh server CTX, full handshake under a different suite must succeed) and gate test_tls12_resume_ticket_wrong_suite on WOLFSSL_NO_DEF_TICKET_ENC_CB so it skips rather than fails in builds without the default ticket encryption callback.
DoAlert evicted the cached session from the fatal-alert handling that runs before the plaintext-under-encryption validation, so a forged TLS 1.3 plaintext alert injected on an established connection evicted the session (forcing a full handshake on reconnect) even though the alert is then rejected as PARSE_ERROR. The unexpected_message teardown sent in response also evicted through the SendAlert hook. Move the receive-side eviction past the validation, into the branch that processes a genuine alert, and have InvalidateSessionOnFatalAlert refuse to evict for a TLS 1.3 plaintext alert received while encryption is on (the current record was not decrypted) - covering both the receive path and the unexpected_message teardown sent in response. RFC 8446 6.2 does not require TLS 1.3 invalidation, so this loses nothing; TLS 1.2 (RFC 5246 7.2.2) is unaffected.
Use the project's preferred `ptr != NULL` form for the new DTLS 1.3 ChaCha record-number zeroization guards instead of relying on truthiness.
The sequence number 2^64-1 is itself RFC 5246 6.1-legal; only the wrap to 0 is forbidden. GetSEQIncrement reads the current counter then post-increments it, so the check refuses the final legal sequence number to avoid the wrapping post-increment. Document that this last value is deliberately sacrificed rather than implying 2^64-1 is itself unusable.
Summary
Addresses ten Fenrir-tracked findings (TLS/DTLS correctness, session-resumption safety, renegotiation safety, and key hygiene). Each fix is its own commit, tagged with its
F-<NUMBER>.Fixes
test_wolfSSL_RSA_verifyandtest_wolfSSL_RSA_padding_add_PKCS1_PSS(PKCS#1 v1.5 and PSS), guarding theXMEMCMPsignature-acceptance check.DoTls13EncryptedExtensionsnow rejects trailing bytes after the extensions block (length equality, RFC 8446 §4.3.1).DoTls13CertificateRequestnow rejects trailing bytes after the extensions block (RFC 8446 §4.3.2).FreeCiphers, plus thewc_Chacha_SetKeyfailure path).renegotiation_infoon a renegotiation ClientHello and aborts if absent; also rejects an SCSV received during renegotiation (RFC 5746 §3.5/§3.7).BuildMessagerefuses to emit a TLS 1.2 record once the write sequence number reaches its maximum, instead of silently wrapping (newSEQUENCE_NUMBER_E, RFC 5246 §6.1).WOLFSSL_OP_NO_RENEGOTIATION: refuse peer-initiated renegotiation with ano_renegotiationwarning while keeping the connection established, on both the client (HelloRequest) and server (renegotiation ClientHello) paths.Tests
New memio regression tests in
tests/api/test_tls_ext.c:test_scr_no_renegotiation_option— server refuses renegotiation, stays established, and still passes application data.test_helloRequest_no_renegotiation_option— client refuses a server HelloRequest, stays established, and still passes application data.Both directions of post-refusal application data exchange are asserted.
Verification
./commit-tests.sh(current /--disable-fastmath/ full configs) — all pass, 0 failures../configure --enable-all && make check— pass, 0 failures.make checkon a secure-renegotiation config (exercising the new SCR tests) — pass, 0 failures.