Append injected data at the end of the input buffer - #11044
Append injected data at the end of the input buffer#11044yosuke-wolfssl wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes wolfSSL_inject() so injected transport bytes are appended after any already-buffered (but unconsumed) input data, preventing partial TLS records from being overwritten and ensuring multi-call injection reassembles correctly.
Changes:
- Update
wolfSSL_inject()to append atinputBuffer.length, compute free space relative tolength, and returnBUFFER_ERRORon inconsistent buffer state. - Document additional return codes in both source comments and generated doxygen headers (EN/JA).
- Add a regression test that injects TLS records in multiple chunks, including a grow-with-partial-record path.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/ssl_api_rw.c |
Fixes injection append offset and strengthens buffer-length validation/error returns. |
tests/api.c |
Adds regression coverage for partial-record injection (no-grow and grow paths). |
doc/dox_comments/header_files/ssl.h |
Updates doxygen return-code documentation for wolfSSL_inject(). |
doc/dox_comments/header_files-ja/ssl.h |
Same doxygen documentation update for JA headers. |
Suppressed comments (1)
tests/api.c:37392
- Same capacity issue as above:
recordSzis sourced fromtest_ctx.s_lenand copied intorecord(allocated asTEST_INJECT_BIG_SZ + 512) without checking whether it fits. If the write is split into multiple records/chunks in some build configurations, thisXMEMCPY()can overflowrecord. Ensurerecordis (re)allocated to at leastrecordSzbefore copying.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
e117db8 to
52e0f57
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11044
Scan targets checked: wolfcrypt-rs-bugs, wolfssl-bugs, wolfssl-src
Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
52e0f57 to
79ff640
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11044
Scan targets checked: wolfcrypt-rs-bugs, wolfssl-bugs, wolfssl-src
No new issues found in the changed files. ✅
0667ecc to
5367278
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11044
Scan targets checked: wolfcrypt-rs-bugs, wolfssl-bugs, wolfssl-src
No new issues found in the changed files. ✅
5367278 to
ee59aef
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11044
Scan targets checked: wolfcrypt-rs-bugs, wolfssl-bugs, wolfssl-src
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
ee59aef to
2ddfad5
Compare
2ddfad5 to
11c67b7
Compare
dgarske
left a comment
There was a problem hiding this comment.
I'd like either @SparkiDev or @julek-wolfssl to also review this before merge, but changes look good to me.
Problem
Two defects, both reached by feeding one TLS record to
wolfSSL_inject()overseveral calls.
1.
wolfSSL_inject()appended at the wrong offset (src/ssl_api_rw.c).New bytes belong after data already buffered but not yet consumed, at
inputBuffer.buffer + length; the copy went toinputBuffer.buffer + idx, thestart of the unconsumed region. When the buffer is drained (
idx == length)the two coincide, which is why existing coverage never caught it. Otherwise the
new bytes overwrote a retained partial record and
lengththen claimed thestale trailing bytes as valid, so a record fed in over several calls never
reassembled. The free-space check compounded this, measuring
bufferSize - usedLength(space for an append atlength) while writing atidx. Closes f-7521.2.
GrowInputBuffer()destroys retained data when the allocator aliases(
src/internal.c).XMALLOC(DYNAMIC_TYPE_IN_BUFFER)returns the bufferalready in use under
HAVE_IO_POOL(a single staticpool_in) and under staticmemory with
WOLFMEM_IO_POOL_FIXED(hint->inBuf).GrowInputBuffer()copiesthe retained bytes to the front, then immediately zeroes that same memory:
A
WOLFSSL_STATIC_MEMORY-only guard existed, but it did not coverHAVE_IO_POOL, and it never moved the data — so it also mis-read stale byteswhenever
idx > 0.wolfSSL_inject()is the first caller able to reach this. It sizes the bufferto the chunk it is handed, so a later grow still holds a partial record.
GetInputData_ex()always requests the whole record, so its one grow per recordlands at
usedLength == 0, where there is nothing to preserve.Fix (
src/ssl_api_rw.c)maxLengthmeasures free space pastinputBuffer.length.XMEMCPYappends atinputBuffer.length.BUFFER_ERRORwhenusedLengthormaxLengthis negative, mirroringGetInputData_ex().Also documents the
BUFFER_ERRORandAPP_DATA_READYreturns, which the sourcecomment and the EN/JA doxygen did not both list.
Fix (
src/internal.c)GrowInputBuffer()now detectstmp == inputBuffer.bufferfor any allocator,moves the retained bytes down with
XMEMMOVE, and zeroes only the now-staleremainder. This replaces the
WOLFSSL_STATIC_MEMORYguard rather than adding aparallel
HAVE_IO_POOLcopy: aliasing is a property of the allocator, andXMALLOC_USERlets applications supply their own pooling allocator.Tests
test_wolfSSL_inject_partial_recordintests/api.c:wolfSSL_read()between eachVerification
--enable-iopool--enable-all--enable-staticmemory--enable-dtls --enable-dtls13--enable-iopool+ ASan/UBSanNegative controls: reverting
src/ssl_api_rw.cfails the new test in a defaultbuild; reverting
src/internal.cfails it under--enable-iopool, theconfiguration that was red on PRB. The
WOLFSSL_STATIC_MEMORYhalf wasconfirmed separately with a white-box probe over a
WOLFMEM_IO_POOL_FIXEDpool(retained bytes mis-read when
idx > 0before the change, correct after); thatprobe is not part of this PR.
The out-of-bounds write the bounds mismatch permits needs
idx > 0at injecttime, which requires
--enable-readahead; without itProcessReply()drainsevery buffered record first. The corruption is reachable in a default build and
is what the test covers.