Fenrir: TLS/DTLS conformance and crypto hardening fixes#10575
Open
julek-wolfssl wants to merge 11 commits into
Open
Fenrir: TLS/DTLS conformance and crypto hardening fixes#10575julek-wolfssl wants to merge 11 commits into
julek-wolfssl wants to merge 11 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR batches multiple TLS/DTLS conformance fixes and crypto hardening changes across the core protocol implementation, OpenSSL-compat APIs, and tests, aligning behavior with relevant RFC requirements and strengthening correctness around edge cases.
Changes:
- Tighten TLS 1.3 / DTLS 1.3 protocol conformance (CertificateRequest rules, ALPN response constraints, DTLS 1.3 epoch ceiling handling).
- Harden API/crypto correctness (safe handling of NULL in
wolfSSL_get_verify_result, chunkedwolfSSL_CRYPTO_memcmp, DH shared-secret right-justification). - Improve key-material hygiene and DTLS epoch management (Camellia schedule zeroization on free, epoch slot eviction behavior).
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
wolfssl/ssl.h |
Adds OpenSSL-aligned verify error code used for NULL-ssl verify-result behavior. |
wolfssl/internal.h |
Introduces DTLS 1.3 epoch max constants (2^48-1) for shared enforcement. |
tests/api.c |
Updates API test to assert non-zero verify error on NULL ssl. |
src/tls13.c |
Enforces TLS 1.3 CertificateRequest requirements and DTLS 1.3 KeyUpdate epoch ceiling handling. |
src/tls.c |
Tightens group setter argument validation; enforces ALPN/MFL response conformance checks. |
src/ssl.c |
Adjusts wolfSSL_get_verify_result(NULL) behavior; hardens wolfSSL_CRYPTO_memcmp against oversized lengths. |
src/pk.c |
Fixes DH shared-secret right-justification for short results. |
src/internal.c |
Ensures Camellia key schedule is zeroized via wc_CamelliaFree before freeing. |
src/dtls13.c |
Fixes epoch slot eviction selection and adds DTLS 1.3 epoch ceiling check during KeyUpdate ACK processing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
retest this please |
|
julek-wolfssl
assigned
wolfSSL-Bot and
julek-wolfssl and unassigned
julek-wolfssl and
wolfSSL-Bot
julek-wolfssl
force-pushed
the
fenrir-fixes-20260601
branch
from
77017f7 to
03e5eac
Compare
RFC 9147 Section 4.2.1 limits the DTLS 1.3 epoch to 2^48-1. SendTls13KeyUpdate now refuses to send a KeyUpdate when the sending epoch is already at the maximum, and Dtls13KeyUpdateAckReceived rejects an epoch that would exceed the limit after incrementing (previously only a full 64-bit wrap to zero was checked).
The size_t length was cast directly to the int taken by ConstantCompare. On 64-bit builds a size with a negative low 32-bit value (e.g. 0x80000000) became a negative length, so ConstantCompare ran zero iterations and returned 0 (equal) without comparing. Reject size > INT_MAX with a non-zero (mismatch) result before narrowing.
WOLFSSL_FAILURE is 0, which equals X509_V_OK, so a NULL ssl was indistinguishable from successful verification under the standard "SSL_get_verify_result(ssl) \!= X509_V_OK" idiom. Return WOLFSSL_X509_V_ERR_APPLICATION_VERIFICATION (50, matching the OpenSSL compat value) instead, and add it to the X509 verify-error enum.
TLSX_MFL_Parse only checked that the extension was requested and that the value was recognized, not that the server echoed the requested value. Per RFC 6066 Section 4, compare the ServerHello value against the locally stored request and abort with a fatal illegal_parameter alert on mismatch.
A response ProtocolNameList was passed whole to ALPN_find_match, which accepts the first configured match, so a server could return more than one protocol. Per RFC 7301 Section 3.1 the ServerHello list must contain exactly one name; enforce that the single name spans the whole list and reject otherwise with a fatal illegal_parameter alert.
DoTls13CertificateRequest parsed the extensions but never verified the mandatory signature_algorithms extension was present. A request with none left peerSuites.hashSigAlgoSz at zero and was accepted. Per RFC 8446 Section 4.3.2, reject such a request with a fatal missing_extension alert before selecting a certificate response.
DoTls13CertificateRequest rejected a non-empty context during the handshake but did the complementary check for post-handshake auth. Per RFC 8446 Section 4.3.2 a post-handshake certificate_request_context must be non-empty and unique; reject a zero-length context and one that duplicates a still-pending request context with a fatal illegal_parameter alert.
wolfSSL_CTX_set_groups/wolfSSL_set_groups only rejected counts above WOLFSSL_MAX_GROUP_COUNT; a negative count skipped the copy loop and was cast to byte (e.g. 255) into numGroups, which InitSSL later trusts for a fixed-size copy. Reject count <= 0 in both, and in the set1_groups OpenSSL-compat wrappers.
Dtls13NewEpochSlot never updated oldestNumber after picking a candidate, so every eligible epoch compared "older" than the sentinel and the last eligible slot was returned instead of the lowest epoch number. Update oldestNumber alongside oldest so the true minimum is evicted.
FreeCiphersSide freed cipher->cam with XFREE only, leaving the expanded key schedule and IV in freed heap memory. Call wc_CamelliaFree (which ForceZeros the context) before XFREE, matching the ARIA cleanup above.
The constant-time path of _DH_compute_key (DH_compute_key_padded) had the XMEMMOVE source/dest swapped and used (padded_keySz - keySz) as the length instead of keySz, overwriting the secret with junk when keySz < padded_keySz. Move key[0..keySz-1] to the high end, matching the idiom used in tls.c/sniffer.c.
julek-wolfssl
force-pushed
the
fenrir-fixes-20260601
branch
from
03e5eac to
da719da
Compare
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.
A batch of TLS/DTLS protocol-conformance and crypto correctness/hardening fixes.
Fixes
wolfSSL_CRYPTO_memcmpwolfSSL_get_verify_resulton NULLsslmax_fragment_lengthresponse that differs from request (RFC 6066 §4)signature_algorithmsin TLS 1.3 CertificateRequest (RFC 8446 §4.3.2)Testing
./commit-tests.shpasses (current, basic--disable-fastmath, and full configs)../configure --enable-all && make checkpasses (22 total, 0 failures).