Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR fixes size/bounds handling when accumulating TLS extension lengths so oversized extension blocks are rejected (BUFFER_E) instead of silently wrapping 16-bit counters, and adds a regression test for the overflow scenario.
Changes:
- Switch extension-length accumulation to a wider type (word32) and add explicit overflow checks in TLSX sizing/writing paths.
- Add ECH inner ClientHello length bounds checks to prevent word16 truncation.
- Add a regression test that constructs an oversized SessionTicket extension and asserts BUFFER_E.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| wolfssl/internal.h | Exposes several TLSX helpers to tests via WOLFSSL_TEST_VIS. |
| tests/api.c | Adds regression test test_tls_ext_word16_overflow and registers it. |
| src/tls13.c | Adds bounds check before casting ECH inner ClientHello length to word16. |
| src/tls.c | Implements word32 accumulation + overflow detection in TLSX size/write logic and adds ECH expansion length guard. |
Comments suppressed due to low confidence (1)
tests/api.c:1
- The new safety logic also adds wrap/overflow detection in the write path (
TLSX_Write/TLSX_WriteRequest), but the regression test only asserts the sizing path (TLSX_GetRequestSize). To cover the newly added write-side checks, extend this test to also callTLSX_WriteRequest(with a suitably sized output buffer) and assert it returnsBUFFER_Efor the same oversized extension list.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10220
Scan targets checked: wolfssl-bugs, wolfssl-compliance, wolfssl-consttime, wolfssl-defaults, wolfssl-mutation, wolfssl-proptest, wolfssl-src, wolfssl-zeroize
Findings: 5
5 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10220
Scan targets checked: wolfcrypt-api_misuse, wolfcrypt-bugs, wolfcrypt-compliance, wolfcrypt-concurrency, wolfcrypt-consttime, wolfcrypt-defaults, wolfcrypt-mutation, wolfcrypt-portability, wolfcrypt-proptest, wolfcrypt-src, wolfcrypt-zeroize, wolfssl-bugs, wolfssl-compliance, wolfssl-consttime, wolfssl-defaults, wolfssl-mutation, wolfssl-proptest, wolfssl-src, wolfssl-zeroize
Findings: 6
6 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10220
Scan targets checked: wolfcrypt-api_misuse, wolfcrypt-bugs, wolfcrypt-compliance, wolfcrypt-concurrency, wolfcrypt-consttime, wolfcrypt-defaults, wolfcrypt-mutation, wolfcrypt-portability, wolfcrypt-proptest, wolfcrypt-src, wolfcrypt-zeroize, wolfssl-bugs, wolfssl-compliance, wolfssl-consttime, wolfssl-defaults, wolfssl-mutation, wolfssl-proptest, wolfssl-src, wolfssl-zeroize
Findings: 3
3 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
tests/api.c:1
- The boundary sub-test can become configuration-dependent and fail even when the code is correct: if
baseInternal + extHdr >= target,tickSzstays 0, no oversized extension is added, but the test still assertslenBoundary == 0xFFFF. Consider explicitly assertingbaseInternal + extHdr < target(and marking the test failed if not), or skipping this boundary sub-test when the baseline extensions already consume too much space.
wolfcrypt/test/test.c:1 - On
wc_AesEaxInitfailure, the test freeseaxbut does not callwc_AesEaxFree(eax). Ifwc_AesEaxInitcan partially initialize internal fields that require cleanup on failure paths, this can leak state. Consider callingwc_AesEaxFree(eax)beforeXFREE(...)in this failure branch as well (the free routine should be safe on partially initialized contexts).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| } | ||
|
|
||
| if ((word32)*pOffset + (word32)offset > WOLFSSL_MAX_16BIT) { |
There was a problem hiding this comment.
If an extension writer sets ret != 0, the loop breaks, but the post-loop overflow check can still run and return BUFFER_E, masking the original error. To preserve the real failure cause, gate the total-length check on ret == 0 (or return ret immediately after the loop when ret != 0).
| if ((word32)*pOffset + (word32)offset > WOLFSSL_MAX_16BIT) { | |
| if (ret == 0 && (word32)*pOffset + (word32)offset > WOLFSSL_MAX_16BIT) { |
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10220
Scan targets checked: wolfcrypt-api_misuse, wolfcrypt-bugs, wolfcrypt-compliance, wolfcrypt-concurrency, wolfcrypt-consttime, wolfcrypt-defaults, wolfcrypt-mutation, wolfcrypt-portability, wolfcrypt-proptest, wolfcrypt-src, wolfcrypt-zeroize, wolfssl-bugs, wolfssl-compliance, wolfssl-consttime, wolfssl-defaults, wolfssl-mutation, wolfssl-proptest, wolfssl-src, wolfssl-zeroize
No new issues found in the changed files. ✅
|
Description
Credit for issue report:
"Suryansh Mansharamani, founder of Plainshift AI (plainshift.io)"
Fixes zd21596
Testing
test_tls_ext_word16_overflowaes_eax_testChecklist