inspector: fix out-of-bounds read in ws frame decoding - #64618
Conversation
Signed-off-by: nashit hayyat <nashit@bugqore.com>
|
Review requested:
|
| reinterpret_cast<uv_handle_t*>(&client_socket))); | ||
| } | ||
|
|
||
| TEST_F(InspectorSocketTest, WaitsForFrameBodyToArrive) { |
There was a problem hiding this comment.
The test also passed on the main branch without the patch.
There was a problem hiding this comment.
Good catch, you're right. The two do_write calls were landing in the same read, so the decoder never saw the header on its own and the bug wasn't reached. I added a server->client round trip between them, which forces the header to be read by itself; I confirmed the decoder now gets a 2 byte buffer first and an 11 byte one second.
With that, reverting the src change makes the test die with SIGBUS (exit 138) instead of passing. Full cctest run is 201/201 with the fix in place.
| if (remaining < kMaskingKeyWidthInBytes || | ||
| remaining - kMaskingKeyWidthInBytes < payload_length) |
There was a problem hiding this comment.
This reads to me equivalent to this one liner:
| if (remaining < kMaskingKeyWidthInBytes || | |
| remaining - kMaskingKeyWidthInBytes < payload_length) | |
| if (remaining < kMaskingKeyWidthInBytes + payload_length) |
There was a problem hiding this comment.
Yep, that's equivalent here - the earlier check rejects anything with payload_length64 > SIZE_MAX - kMaskingKeyWidthInBytes, so the addition can't wrap. Switched to the one liner.
Signed-off-by: nashit hayyat <nashit@bugqore.com>
decode_frame_hybi17 checks the announced payload length against buffer.size(), but the payload is read from it + kMaskingKeyWidthInBytes, after the iterator has already walked past the two byte header and the extended length field. A client that sends only the two header bytes of a masked frame, 0x81 0xFD, underflows buffer.size() - kMaskingKeyWidthInBytes to SIZE_MAX, passes the check, and the unmasking loop reads 125 bytes past a two byte vector; the bytes it picks up go to OnWsFrame and back out to the peer. Measuring against what is left after the header fixes both that underflow and the header bytes the old check never accounted for.