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.
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. ✅
|
|
Jenkins retest this please |
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.
Comments suppressed due to low confidence (1)
tests/api.c:1
baseLenis aword32, and subtractingOPAQUE16_LENwhenbaseLen > 0can underflow ifbaseLenis1(or otherwise less thanOPAQUE16_LEN). Even if current behavior never returns1, this is fragile. UsebaseLen >= (word32)OPAQUE16_LENas the condition (or otherwise clamp) to avoid unsigned wrap impactingbaseInternaland subsequenttickSzcalculations.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Description
Credit for issue report:
"Suryansh Mansharamani, founder of Plainshift AI (plainshift.io)"
Fixes zd21596
Testing
test_tls_ext_word16_overflowaes_eax_testChecklist