Fix TLSX_EchChangeSNI to check hostname termination#10182
Fix TLSX_EchChangeSNI to check hostname termination#10182embhorn wants to merge 1 commit intowolfSSL:masterfrom
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 addresses a crash/overflow risk in ECH SNI handling by ensuring truncated hostnames are NUL-terminated, and adds a regression test to prevent reintroducing the issue.
Changes:
- Ensure the hostname buffer is NUL-terminated when truncating in
TLSX_EchChangeSNI. - Add a regression test that uses an overlong inner SNI hostname to exercise truncation and confirm no crash.
- Register the new test in the API test suite.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/tls.c |
Adds NUL termination to the truncated SNI hostname buffer to prevent out-of-bounds reads in TLSX_EchRestoreSNI. |
tests/api.c |
Adds and registers a regression test that sets an overlong SNI to exercise the truncation path during ECH handshake. |
Comments suppressed due to low confidence (1)
tests/api.c:1
- The PR description’s “Testing” section says “Added
TLSX_EchChangeSNI”, but the actual addition is a regression testtest_wolfSSL_Tls13_ECH_long_SNI. Please update the PR description to reflect the real test name (and/or what test coverage was added) so it’s easier to track in CI and future triage.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| XMEMCPY(serverName, hostName, hostNameSz); | ||
| /* Guarantee NUL termination after truncation so that | ||
| * TLSX_EchRestoreSNI's XSTRLEN cannot read past the buffer. */ | ||
| serverName[hostNameSz - 1] = '\0'; |
There was a problem hiding this comment.
serverName[hostNameSz - 1] = '\0'; will drop the last character for any hostNameSz > 0 (even when not truncated), and can underflow if hostNameSz == 0. A safer pattern is to bound the copy to MAX_PUBLIC_NAME_SZ - 1, then set serverName[copySz] = '\0' (or, if you keep the current clamping, set serverName[hostNameSz] = '\0' when there is room, and only use [MAX_PUBLIC_NAME_SZ - 1] when clamped).
Description
Add NUL to terminate hostname string
Fixes zd21576
Testing
Added
TLSX_EchChangeSNIChecklist