From 2291fb36cf89987b828a778ad9cd5175711207fc Mon Sep 17 00:00:00 2001 From: Philipp Hancke Date: Tue, 29 Sep 2020 10:51:42 +0200 Subject: [PATCH] red: ensure minimum amount of header bytes avoids out-of-bounds reads when splitting RED packets. Bug: webrtc:11640 Change-Id: I38beb5b373c4faa878f627a5df17dd4db9ea20cf Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/185804 Reviewed-by: Henrik Lundin Commit-Queue: Philipp Hancke Cr-Commit-Position: refs/heads/master@{#32239} --- .../neteq/red_payload_splitter.cc | 31 +++++++++++++------ .../audio_coding/neteq/red_payload_splitter.h | 3 ++ .../neteq/red_payload_splitter_unittest.cc | 22 ++++++++++++- 3 files changed, 46 insertions(+), 10 deletions(-) diff --git a/modules/audio_coding/neteq/red_payload_splitter.cc b/modules/audio_coding/neteq/red_payload_splitter.cc index 1343690999..3e983dc2d4 100644 --- a/modules/audio_coding/neteq/red_payload_splitter.cc +++ b/modules/audio_coding/neteq/red_payload_splitter.cc @@ -29,10 +29,10 @@ namespace webrtc { // The method loops through a list of packets {A, B, C, ...}. Each packet is // split into its corresponding RED payloads, {A1, A2, ...}, which is // temporarily held in the list |new_packets|. -// When the first packet in |packet_list| has been processed, the orignal packet -// is replaced by the new ones in |new_packets|, so that |packet_list| becomes: -// {A1, A2, ..., B, C, ...}. The method then continues with B, and C, until all -// the original packets have been replaced by their split payloads. +// When the first packet in |packet_list| has been processed, the original +// packet is replaced by the new ones in |new_packets|, so that |packet_list| +// becomes: {A1, A2, ..., B, C, ...}. The method then continues with B, and C, +// until all the original packets have been replaced by their split payloads. bool RedPayloadSplitter::SplitRed(PacketList* packet_list) { // Too many RED blocks indicates that something is wrong. Clamp it at some // reasonable value. @@ -43,6 +43,7 @@ bool RedPayloadSplitter::SplitRed(PacketList* packet_list) { const Packet& red_packet = *it; assert(!red_packet.payload.empty()); const uint8_t* payload_ptr = red_packet.payload.data(); + size_t payload_length = red_packet.payload.size(); // Read RED headers (according to RFC 2198): // @@ -67,6 +68,10 @@ bool RedPayloadSplitter::SplitRed(PacketList* packet_list) { bool last_block = false; size_t sum_length = 0; while (!last_block) { + if (payload_length == 0) { + RTC_LOG(LS_WARNING) << "SplitRed header too short"; + return false; + } RedHeader new_header; // Check the F bit. If F == 0, this was the last block. last_block = ((*payload_ptr & 0x80) == 0); @@ -74,11 +79,16 @@ bool RedPayloadSplitter::SplitRed(PacketList* packet_list) { new_header.payload_type = payload_ptr[0] & 0x7F; if (last_block) { // No more header data to read. - ++sum_length; // Account for RED header size of 1 byte. + sum_length += kRedLastHeaderLength; // Account for RED header size. new_header.timestamp = red_packet.timestamp; new_header.payload_length = red_packet.payload.size() - sum_length; - payload_ptr += 1; // Advance to first payload byte. + payload_ptr += kRedLastHeaderLength; // Advance to first payload byte. + payload_length -= kRedLastHeaderLength; } else { + if (payload_length < kRedHeaderLength) { + RTC_LOG(LS_WARNING) << "SplitRed header too short"; + return false; + } // Bits 8 through 21 are timestamp offset. int timestamp_offset = (payload_ptr[1] << 6) + ((payload_ptr[2] & 0xFC) >> 2); @@ -86,10 +96,13 @@ bool RedPayloadSplitter::SplitRed(PacketList* packet_list) { // Bits 22 through 31 are payload length. new_header.payload_length = ((payload_ptr[2] & 0x03) << 8) + payload_ptr[3]; - payload_ptr += 4; // Advance to next RED header. + + sum_length += new_header.payload_length; + sum_length += kRedHeaderLength; // Account for RED header size. + + payload_ptr += kRedHeaderLength; // Advance to next RED header. + payload_length -= kRedHeaderLength; } - sum_length += new_header.payload_length; - sum_length += 4; // Account for RED header size of 4 bytes. // Store in new list of packets. new_headers.push_back(new_header); } diff --git a/modules/audio_coding/neteq/red_payload_splitter.h b/modules/audio_coding/neteq/red_payload_splitter.h index c2e0a445d0..c54ffc0dae 100644 --- a/modules/audio_coding/neteq/red_payload_splitter.h +++ b/modules/audio_coding/neteq/red_payload_splitter.h @@ -18,6 +18,9 @@ namespace webrtc { class DecoderDatabase; +static const size_t kRedHeaderLength = 4; // 4 bytes RED header. +static const size_t kRedLastHeaderLength = + 1; // reduced size for last RED header. // This class handles splitting of RED payloads into smaller parts. // Codec-specific packet splitting can be performed by // AudioDecoder::ParsePayload. diff --git a/modules/audio_coding/neteq/red_payload_splitter_unittest.cc b/modules/audio_coding/neteq/red_payload_splitter_unittest.cc index b4cd8d7d0b..5956971b33 100644 --- a/modules/audio_coding/neteq/red_payload_splitter_unittest.cc +++ b/modules/audio_coding/neteq/red_payload_splitter_unittest.cc @@ -31,7 +31,6 @@ namespace webrtc { static const int kRedPayloadType = 100; static const size_t kPayloadLength = 10; -static const size_t kRedHeaderLength = 4; // 4 bytes RED header. static const uint16_t kSequenceNumber = 0; static const uint32_t kBaseTimestamp = 0x12345678; @@ -368,4 +367,25 @@ TEST(RedPayloadSplitter, WrongPayloadLength) { packet_list.pop_front(); } +// Test that we reject packets too short to contain a RED header. +TEST(RedPayloadSplitter, RejectsIncompleteHeaders) { + RedPayloadSplitter splitter; + + uint8_t payload_types[] = {0, 0}; + const int kTimestampOffset = 160; + + PacketList packet_list; + + // Truncate the packet such that the first block can not be parsed. + packet_list.push_back(CreateRedPayload(2, payload_types, kTimestampOffset)); + packet_list.front().payload.SetSize(4); + EXPECT_FALSE(splitter.SplitRed(&packet_list)); + EXPECT_FALSE(packet_list.empty()); + + // Truncate the packet such that the first block can not be parsed. + packet_list.front().payload.SetSize(3); + EXPECT_FALSE(splitter.SplitRed(&packet_list)); + EXPECT_FALSE(packet_list.empty()); +} + } // namespace webrtc