Conversation
`ticket->lifetime` is never set to 0xfffffff anywhere F-430
There was a problem hiding this comment.
Pull request overview
Hardening and cleanup changes across TLS/QUIC parsing, crypto error handling, and CI/container tooling to improve security and robustness.
Changes:
- Strengthen input/length validation in TLS 1.3 parsing and sniffer logic; remove dead session ticket validation code.
- Improve security posture by clearing plaintext on AEAD failure and tightening CRL callback return handling.
- Update CI and Docker dependencies (pin/upgrade actions, switch Docker git clone to HTTPS).
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
wolfcrypt/src/chacha20_poly1305.c |
Clears plaintext buffer on decrypt error to avoid returning unauthenticated data. |
wolfcrypt/src/asn.c |
Adjusts Dilithium-related logging condition in wc_GetKeyOID. |
src/tls13.c |
Relaxes TLS 1.3 ClientHello legacy session ID length check. |
src/tls.c |
Removes dead session ticket request validation hook and macro usage. |
src/sniffer.c |
Adds additional bounds checks for TLS 1.3 ticket nonce and ServerHello extensions. |
src/quic.c |
Updates QUIC transport parameter length upper bound. |
src/crl.c |
Fixes retry condition to check cbRet after callback. |
Docker/Dockerfile |
Switches git clone URL from HTTP to HTTPS. |
.github/workflows/win-csharp-test.yml |
Pins/updates actions/checkout and setup-msbuild. |
.github/workflows/ada.yml |
Pins actions/checkout version. |
Comments suppressed due to low confidence (1)
wolfcrypt/src/asn.c:1
- The logging condition depends on the current value of
*algoID, which may be influenced by its prior value rather than the Dilithium init result in this call. To make the behavior deterministic and easier to reason about, prefer logging based on the initialization return status (or explicitly initialize/reset*algoIDbefore attempting initialization so*algoID == 0reliably reflects 'init failed').
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (ret != 0) { | ||
| /* zero plaintext on error */ | ||
| ForceZero(outPlaintext, inCiphertextLen); | ||
| } |
There was a problem hiding this comment.
This changes the API behavior to wipe outPlaintext for any error (including parameter/initialization failures), which may be surprising to callers and can break code that expects the output buffer to remain untouched unless authentication fails. Consider limiting the wipe to the authentication/tag failure path (or only after decryption has produced plaintext) so the semantic change is narrowly scoped to 'unauthenticated plaintext'.
| if (extLen < 2) { | ||
| SetError(SERVER_HELLO_INPUT_STR, error, session, | ||
| FATAL_ERROR_STATE); | ||
| return WOLFSSL_FATAL_ERROR; | ||
| } |
There was a problem hiding this comment.
For a ServerHello, supported_versions is a fixed-size selected version and should be exactly 2 bytes. With the current check, extLen > 2 will be accepted and the parser will ignore trailing bytes, potentially allowing malformed messages to pass validation. Consider rejecting extLen != 2 here (similar to the strict extLen != 1 check added for EXT_MAX_FRAGMENT_LENGTH).
| steps: | ||
| - name: Pull wolfssl | ||
| uses: actions/checkout@master | ||
| uses: actions/checkout@v4 |
There was a problem hiding this comment.
Using major version tags (e.g., @v4, @v2) is better than @master, but still allows the referenced code to change over time. For stronger supply-chain protection, pin GitHub Actions to an immutable commit SHA (optionally keeping the tag in a comment for readability).
| - name: Add MSBuild to PATH | ||
| uses: microsoft/setup-msbuild@v1 | ||
| uses: microsoft/setup-msbuild@v2 |
There was a problem hiding this comment.
Using major version tags (e.g., @v4, @v2) is better than @master, but still allows the referenced code to change over time. For stronger supply-chain protection, pin GitHub Actions to an immutable commit SHA (optionally keeping the tag in a comment for readability).
| uses: microsoft/setup-msbuild@v2 | |
| uses: microsoft/setup-msbuild@012f192c5cc5a2d4956f93a4f5a5f8449dcd08f2 # microsoft/setup-msbuild@v2 |
actions/checkoutversioncbRetafter callback