wolfCrypt and TLS 1.3 correctness fixes (F-1376, F-2653, F-1972, F-4593)#10930
Open
julek-wolfssl wants to merge 4 commits into
Open
wolfCrypt and TLS 1.3 correctness fixes (F-1376, F-2653, F-1972, F-4593)#10930julek-wolfssl wants to merge 4 commits into
julek-wolfssl wants to merge 4 commits into
Conversation
wc_AesCbcEncryptWithKey did not check out/in/key/iv for NULL before calling wc_AesSetKey/wc_AesCbcEncrypt, unlike its counterpart wc_AesCbcDecryptWithKey. A NULL key can reach wc_AesSetKey implementations that XMEMCPY userKey without a NULL guard (e.g. the STM32 path), causing a crash at the API boundary. Add the same NULL guard wc_AesCbcDecryptWithKey uses.
wolfSSL_EVP_CipherInit reconstructed the 32-bit ChaCha20 counter with (word32)(iv[1] << 8) etc., which promotes the unsigned char to int and shifts before the cast. On 16-bit int platforms iv[1] << 8 is signed-overflow UB and iv[2] << 16 / iv[3] << 24 are UB unconditionally (shift >= int width). Even on 32-bit int, iv[3] << 24 with the high bit set is signed-overflow UB. Cast each byte to word32 before shifting so the computation is well-defined.
PrintPubKeyDH declared length as word32 but passed (int*)&length to GetSequence/GetLength, which write through an int*. On platforms where sizeof(int) != sizeof(word32) this updates only part of the variable, leaving stale bytes. Declare length as int (matching the callee out-parameter type) and drop the (int*) casts; the values are non-negative ASN.1 lengths already used via (int) casts and pointer arithmetic.
…593) When a TLS 1.3 client received a second HelloRetryRequest, DoTls13ServerHello returned DUPLICATE_MSG_E without sending an alert. TranslateErrorToAlert had no mapping for DUPLICATE_MSG_E, so it returned invalid_alert and the dispatch tail skipped SendAlert - the connection aborted silently instead of with the RFC 8446-mandated unexpected_message alert. Map DUPLICATE_MSG_E to unexpected_message in TranslateErrorToAlert, alongside OUT_OF_ORDER_E. This mirrors the sanity-check path and covers any TLS 1.3 handler that returns DUPLICATE_MSG_E to the dispatch loop.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR applies four small correctness fixes across wolfCrypt helpers and the TLS 1.3 alert mapping, primarily improving API robustness and standards-compliant error signaling.
Changes:
- Add NULL-parameter validation to
wc_AesCbcEncryptWithKey()to returnBAD_FUNC_ARGinstead of crashing in lower layers. - Fix ChaCha20 EVP counter reconstruction to avoid shift undefined behavior on 16-bit-
int(and signed overflow cases) by casting before shifting and composing with|. - Fix
PrintPubKeyDH()ASN.1 length parsing by using anintlength variable to matchGetSequence()/GetLength()out-parameter types. - Map
DUPLICATE_MSG_Eto the TLSunexpected_messagealert so the TLS 1.3 stack sends an appropriate alert for duplicate handshake messages.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| wolfcrypt/src/wc_encrypt.c | Adds NULL argument checks to AES-CBC encrypt-with-key helper to match decrypt counterpart behavior. |
| wolfcrypt/src/evp.c | Fixes ChaCha20 counter byte reconstruction and corrects DH public key parsing length type to avoid aliasing/size issues. |
| src/internal.c | Updates TranslateErrorToAlert() to map DUPLICATE_MSG_E to unexpected_message for proper TLS alerting. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
julek-wolfssl
marked this pull request as ready for review
July 16, 2026 14:29
|
retest this please |
|
Member
Author
|
retest this please |
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.
Four independent fixes in wolfCrypt and the TLS 1.3 handshake. Each is self-contained and reviewable on its own commit.
wc_AesCbcEncryptWithKey: validate NULL parameters (F-1376)The function did not check
out/in/key/ivfor NULL before callingwc_AesSetKey/wc_AesCbcEncrypt, unlike its counterpartwc_AesCbcDecryptWithKey. A NULLkeyreacheswc_AesSetKeyimplementations thatXMEMCPYuserKeywithout a NULL guard (e.g. the STM32 path), crashing at the API boundary instead of returningBAD_FUNC_ARG.Adds the same guard
wc_AesCbcDecryptWithKeyalready uses.ChaCha20 EVP: fix counter reconstruction shift UB (F-2653)
wolfSSL_EVP_CipherInitrebuilt the 32-bit ChaCha20 counter with(word32)(iv[1] << 8)and friends. The cast applies after the shift, so eachunsigned charpromotes tointand shifts as a signed value: on 16-bitintplatformsiv[1] << 8is signed-overflow UB andiv[2] << 16/iv[3] << 24are UB unconditionally (shift >= int width). Even with 32-bitint,iv[3] << 24with the high bit set is signed overflow.Casts each byte to
word32before shifting, and uses|rather than+to compose the bytes.PrintPubKeyDH: fix strict-aliasing violation in length (F-1972)lengthwas declaredword32but passed as(int*)&lengthtoGetSequence/GetLength, which write through anint*. Wheresizeof(int) != sizeof(word32)this updates only part of the object and leaves stale bytes behind.Declares
lengthasintto match the callee out-parameter type and drops the casts. The values are non-negative ASN.1 lengths, already consumed via(int)casts and pointer arithmetic.TLS 1.3: send
unexpected_messageon a duplicate handshake message (F-4593)When a TLS 1.3 client received a second HelloRetryRequest,
DoTls13ServerHelloreturnedDUPLICATE_MSG_Ewithout sending an alert:TranslateErrorToAlerthad no mapping for it, so it returnedinvalid_alertand the dispatch tail skippedSendAlert. The connection aborted silently rather than with the alert RFC 8446 mandates.Maps
DUPLICATE_MSG_Etounexpected_messagealongsideOUT_OF_ORDER_E, mirroring the sanity-check path and covering any TLS 1.3 handler that returnsDUPLICATE_MSG_Eto the dispatch loop.No regression tests here: three of the four only misbehave on platforms where
sizeof(int) != sizeof(word32)orintis 16-bit, and the fourth needs a server that replies with a second HelloRetryRequest. Happy to add a unit test for the alert path if reviewers want one.