rmaucher commented on PR #1073:
URL: https://github.com/apache/tomcat/pull/1073#issuecomment-5847508951
Bad luck, I had it in my latest code review, but only as low, so it didn't
get featured in the overall report (so no fix).
(for the record, this is the report)
### [Low] `unReadReadBuffer()` shifts buffer data with a forward copy that
corrupts overlapping regions (latent data corruption)
- File:
/home/opencode/code-review/tomcat/java/org/apache/tomcat/util/net/SocketBufferHandler.java:152-154
(write-mode branch) and 172-174 (read-mode branch)
- Description: When data is returned to a non-empty read buffer, both
branches shift the existing live bytes "up" to make room by copying forward:
- write mode (lines 152-154): `for (int i = 0; i < readBuffer.position();
i++) readBuffer.put(i + bytesReturned, readBuffer.get(i));` moves `[0,
position)` to `[bytesReturned, position+bytesReturned)`. When `bytesReturned <
position` the source and destination overlap and the forward copy overwrites
bytes before they are read.
- read mode (lines 172-174): `for (int i = readBuffer.position(); i <
oldLimit; i++) readBuffer.put(i + shiftRequired, readBuffer.get(i));` moves
`[position, oldLimit)` by `shiftRequired` with the same forward-overlap
corruption whenever `shiftRequired < oldLimit - position`.
Concrete demonstration (write-mode branch): buffer capacity 10, position
6, contents `A B C D E F`; returning `X Y`. The loop yields `A B A B A B F F`
before insertion, so the final buffer is `X Y A B A B A F` instead of `X Y A B
C D E F` - `C`, `D`, `E` are destroyed and `A`, `B` duplicated. Since this is
the socket *read* path (used on HTTP/HTTP-2 upgrades to re-queue data from
untrusted clients), corruption here could desynchronize protocol parsing
(potential request-smuggling vector) if ever reached.
- Verification: Proven by hand-tracing the byte-by-byte copy above.
Reachability was then checked against all call sites of
`SocketWrapperBase.unRead()` (the only caller of `unReadReadBuffer()`):
`AbstractProtocol.java:1315` (HTTP upgrade) and `Http2AsyncParser.java:121,315`
(h2c upgrade / direct h2). In all current flows the wrapper read buffer is
empty at the `unRead()` point: HTTP/1.1 header parsing fills
`Http11InputBuffer`'s own larger buffer directly from the socket
(`NioEndpoint.NioSocketWrapper.read(boolean, ByteBuffer)` direct-read path,
since `to.remaining() >= readBuffer.capacity()` always holds while parsing
headers), and the h2 async parser first `expand()`s the read buffer to
`maxFrameSize` (16384) while every vectored read's destination set is `9 +
maxFrameSize` (or `24 + 9 + maxFrameSize`), so the main buffer is always fully
drained before `unRead()` is called. The AJP byte-array read path
(`NioEndpoint.java:1574-1597`) can leave the buffer partially consumed, but
AJP never triggers the upgrade/`unRead` flow. Hence the corrupting branch is
not currently reachable - hence Low - but the code is objectively wrong for the
public `unRead()` API and would corrupt data if a future caller (or endpoint)
returns data while the buffer still holds unconsumed bytes.
- Proposed fix: copy backwards in both branches:
```java
// write mode
for (int i = readBuffer.position() - 1; i >= 0; i--) {
readBuffer.put(i + bytesReturned, readBuffer.get(i));
}
// read mode
for (int i = oldLimit - 1; i >= readBuffer.position(); i--) {
readBuffer.put(i + shiftRequired, readBuffer.get(i));
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
