From 8b1dcbd5e30e8a2fc055efc37a815b51ffff7b00 Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Thu, 28 Mar 2024 13:46:23 +0100 Subject: [PATCH] streaming/buffer: improve integer handling safety Unsafe handling of buffer offset and to be inserted data's length could lead to a integer overflow. This in turn would skip growing the target buffer, which then would be memcpy'd into, leading to an out of bounds write. This issue shouldn't be reachable through any of the consumers of the API, but to be sure some debug validation checks have been added. Bug: #6903. (cherry picked from commit cf6278f95adaba86e0db578dad95cba386a7d509) --- src/util-streaming-buffer.c | 63 +++++++++++++++++++++++++++++++------ 1 file changed, 53 insertions(+), 10 deletions(-) diff --git a/src/util-streaming-buffer.c b/src/util-streaming-buffer.c index 6ff4f438a4..204ef2eb7e 100644 --- a/src/util-streaming-buffer.c +++ b/src/util-streaming-buffer.c @@ -1064,7 +1064,15 @@ void StreamingBufferSlideToOffset( DEBUG_VALIDATE_BUG_ON(sb->region.stream_offset < offset); } -#define DATA_FITS(sb, len) ((sb)->region.buf_offset + (len) <= (sb)->region.buf_size) +static int DataFits(const StreamingBuffer *sb, const uint32_t len) +{ + uint64_t buf_offset64 = sb->region.buf_offset; + uint64_t len64 = len; + if (len64 + buf_offset64 > UINT32_MAX) { + return -1; + } + return sb->region.buf_offset + len <= sb->region.buf_size; +} int StreamingBufferAppend(StreamingBuffer *sb, const StreamingBufferConfig *cfg, StreamingBufferSegment *seg, const uint8_t *data, uint32_t data_len) @@ -1076,7 +1084,11 @@ int StreamingBufferAppend(StreamingBuffer *sb, const StreamingBufferConfig *cfg, return -1; } - if (!DATA_FITS(sb, data_len)) { + int r = DataFits(sb, data_len); + if (r < 0) { + DEBUG_VALIDATE_BUG_ON(1); + return -1; + } else if (r == 0) { if (sb->region.buf_size == 0) { if (GrowToSize(sb, cfg, data_len) != SC_OK) return -1; @@ -1085,7 +1097,7 @@ int StreamingBufferAppend(StreamingBuffer *sb, const StreamingBufferConfig *cfg, return -1; } } - DEBUG_VALIDATE_BUG_ON(!DATA_FITS(sb, data_len)); + DEBUG_VALIDATE_BUG_ON(DataFits(sb, data_len) != 1); memcpy(sb->region.buf + sb->region.buf_offset, data, data_len); seg->stream_offset = sb->region.stream_offset + sb->region.buf_offset; @@ -1111,7 +1123,11 @@ int StreamingBufferAppendNoTrack(StreamingBuffer *sb, const StreamingBufferConfi return -1; } - if (!DATA_FITS(sb, data_len)) { + int r = DataFits(sb, data_len); + if (r < 0) { + DEBUG_VALIDATE_BUG_ON(1); + return -1; + } else if (r == 0) { if (sb->region.buf_size == 0) { if (GrowToSize(sb, cfg, data_len) != SC_OK) return -1; @@ -1120,7 +1136,7 @@ int StreamingBufferAppendNoTrack(StreamingBuffer *sb, const StreamingBufferConfi return -1; } } - DEBUG_VALIDATE_BUG_ON(!DATA_FITS(sb, data_len)); + DEBUG_VALIDATE_BUG_ON(DataFits(sb, data_len) != 1); memcpy(sb->region.buf + sb->region.buf_offset, data, data_len); uint32_t rel_offset = sb->region.buf_offset; @@ -1133,7 +1149,15 @@ int StreamingBufferAppendNoTrack(StreamingBuffer *sb, const StreamingBufferConfi } } -#define DATA_FITS_AT_OFFSET(region, len, offset) ((offset) + (len) <= (region)->buf_size) +static int DataFitsAtOffset( + const StreamingBufferRegion *region, const uint32_t len, const uint32_t offset) +{ + const uint64_t offset64 = offset; + const uint64_t len64 = len; + if (offset64 + len64 > UINT32_MAX) + return -1; + return (offset + len <= region->buf_size); +} #if defined(DEBUG) || defined(DEBUG_VALIDATION) static void Validate(const StreamingBuffer *sb) @@ -1477,8 +1501,6 @@ static StreamingBufferRegion *BufferInsertAtRegion(StreamingBuffer *sb, int StreamingBufferInsertAt(StreamingBuffer *sb, const StreamingBufferConfig *cfg, StreamingBufferSegment *seg, const uint8_t *data, uint32_t data_len, uint64_t offset) { - int r; - DEBUG_VALIDATE_BUG_ON(seg == NULL); DEBUG_VALIDATE_BUG_ON(offset < sb->region.stream_offset); if (offset < sb->region.stream_offset) { @@ -1496,11 +1518,15 @@ int StreamingBufferInsertAt(StreamingBuffer *sb, const StreamingBufferConfig *cf region == &sb->region ? "main" : "aux", region); uint32_t rel_offset = offset - region->stream_offset; - if (!DATA_FITS_AT_OFFSET(region, data_len, rel_offset)) { + int r = DataFitsAtOffset(region, data_len, rel_offset); + if (r < 0) { + DEBUG_VALIDATE_BUG_ON(1); + return SC_ELIMIT; + } else if (r == 0) { if ((r = GrowToSize(sb, cfg, (rel_offset + data_len))) != SC_OK) return r; } - DEBUG_VALIDATE_BUG_ON(!DATA_FITS_AT_OFFSET(region, data_len, rel_offset)); + DEBUG_VALIDATE_BUG_ON(DataFitsAtOffset(region, data_len, rel_offset) != 1); SCLogDebug("offset %" PRIu64 " data_len %u, rel_offset %u into region offset %" PRIu64 ", buf_offset %u, buf_size %u", @@ -2320,6 +2346,22 @@ static int StreamingBufferTest10(void) PASS; } +static int StreamingBufferTest11(void) +{ + StreamingBufferConfig cfg = { 24, 1, STREAMING_BUFFER_REGION_GAP_DEFAULT, NULL, NULL, NULL }; + StreamingBuffer *sb = StreamingBufferInit(&cfg); + FAIL_IF(sb == NULL); + + StreamingBufferSegment seg1; + FAIL_IF(StreamingBufferAppend(sb, &cfg, &seg1, (const uint8_t *)"ABCDEFGH", 8) != 0); + StreamingBufferSegment seg2; + unsigned int data_len = 0xffffffff; + FAIL_IF(StreamingBufferAppend(sb, &cfg, &seg2, (const uint8_t *)"unused", data_len) != -1); + FAIL_IF(StreamingBufferInsertAt( + sb, &cfg, &seg2, (const uint8_t *)"abcdefghij", data_len, 100000) != SC_ELIMIT); + StreamingBufferFree(sb, &cfg); + PASS; +} #endif void StreamingBufferRegisterTests(void) @@ -2333,5 +2375,6 @@ void StreamingBufferRegisterTests(void) UtRegisterTest("StreamingBufferTest08", StreamingBufferTest08); UtRegisterTest("StreamingBufferTest09", StreamingBufferTest09); UtRegisterTest("StreamingBufferTest10", StreamingBufferTest10); + UtRegisterTest("StreamingBufferTest11 Bug 6903", StreamingBufferTest11); #endif } -- 2.47.2