snappy: fix OOB reads in framed data parser#11855
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughflb_snappy_uncompress_framed_data() adds defensive bounds checks: the loop ensures enough bytes exist for frame headers and declared payloads, and COMPRESSED_DATA / UNCOMPRESSED_DATA frames validate that their frame_length includes the 4-byte checksum field. ChangesSnappy Framed Data Bounds Checking
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/flb_snappy.c (1)
169-179:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOff-by-one in frame header bounds check:
uint32_tread atframe_buffer[1]accessesoffset+4.The check at line 169 ensures
offset + 4 <= in_len(indices 0–3 valid). However, line 178 reads auint32_tat&frame_buffer[1], accessing bytes at indices 1, 2, 3, 4. When exactly 4 bytes remain (offset + 4 == in_len), index 4 is out of bounds.This affects valid trailing 4-byte frames (e.g., empty PADDING). Rather than requiring 5 bytes and rejecting valid frames, read only the 3 bytes actually needed:
🔒 Proposed fix: read frame_length as 3 bytes
frame_type = *((uint8_t *) &frame_buffer[0]); - frame_length = *((uint32_t *) &frame_buffer[1]); - frame_length &= 0x00FFFFFF; + frame_length = ((uint32_t)((unsigned char)frame_buffer[1])) | + ((uint32_t)((unsigned char)frame_buffer[2]) << 8) | + ((uint32_t)((unsigned char)frame_buffer[3]) << 16); frame_body = &frame_buffer[4];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/flb_snappy.c` around lines 169 - 179, The bounds check is correct for a 4-byte header (bytes 0–3) but the code incorrectly reads a uint32_t from &frame_buffer[1], which touches byte 4; change the frame length read to assemble only the 3 bytes at frame_buffer[1..3] instead of dereferencing a uint32_t pointer. Concretely, keep frame_type = frame_buffer[0], then compute frame_length = ((uint32_t)frame_buffer[1]) | ((uint32_t)frame_buffer[2] << 8) | ((uint32_t)frame_buffer[3] << 16) (and remove the subsequent &0x00FFFFFF mask), so you no longer perform an out-of-bounds/unaligned 32-bit read on frame_buffer.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/flb_snappy.c`:
- Around line 169-179: The bounds check is correct for a 4-byte header (bytes
0–3) but the code incorrectly reads a uint32_t from &frame_buffer[1], which
touches byte 4; change the frame length read to assemble only the 3 bytes at
frame_buffer[1..3] instead of dereferencing a uint32_t pointer. Concretely, keep
frame_type = frame_buffer[0], then compute frame_length =
((uint32_t)frame_buffer[1]) | ((uint32_t)frame_buffer[2] << 8) |
((uint32_t)frame_buffer[3] << 16) (and remove the subsequent &0x00FFFFFF mask),
so you no longer perform an out-of-bounds/unaligned 32-bit read on frame_buffer.
|
@TristanInSec would you please sign off the commits ? (DCO error / git commit -s ...) |
The snappy framed data parser has two issues: 1. The loop reads a 4-byte frame header without checking that 4 bytes are available. When 1-3 bytes remain after the last valid frame, the uint32_t read at frame_buffer[1] extends past the buffer. Also missing a check that the full frame body (offset + 4 + frame_length) fits within the input. 2. Both compressed and uncompressed data paths subtract sizeof(uint32_t) from frame_length for the checksum without checking frame_length >= 4. When frame_length < 4, the subtraction wraps to a huge size_t value on 64-bit, causing flb_snappy_uncompress() or memcpy() to read far past the buffer. Add bounds checks for the frame header read, total frame extent, and minimum frame_length before the checksum subtraction. Signed-off-by: Tristan <tristan@talencesecurity.com>
TristanInSec
force-pushed
the
fix/snappy-frame-parsing
branch
from
4d15a10 to
eef89da
Compare
The frame header is 4 bytes: 1 byte type + 3 bytes little-endian length. The previous code read a uint32_t at frame_buffer[1] which touches byte index 4, but the bounds check only ensures indices 0-3 are valid. When exactly 4 bytes remain, byte 4 is out of bounds. Read the 3-byte length field byte-by-byte instead. This also fixes a potential unaligned memory access on strict-alignment architectures. Signed-off-by: Tristan <tristan@talencesecurity.com>
3 participants
The snappy framed data parser has two issues:
The loop reads a 4-byte frame header without checking that 4 bytes
are available. When 1-3 bytes remain after the last valid frame,
the uint32_t read at frame_buffer[1] extends past the buffer.
Also missing a check that the full frame body (offset + 4 +
frame_length) fits within the input.
Both compressed and uncompressed data paths subtract sizeof(uint32_t)
from frame_length for the checksum without checking frame_length >= 4.
When frame_length < 4, the subtraction wraps to a huge size_t value
on 64-bit, causing flb_snappy_uncompress() or memcpy() to read far
past the buffer.
Add bounds checks for the frame header read, total frame extent, and
minimum frame_length before the checksum subtraction.
Summary by CodeRabbit