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 <henrik.lundin@webrtc.org>
Commit-Queue: Philipp Hancke <philipp.hancke@googlemail.com>
Cr-Commit-Position: refs/heads/master@{#32239}

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);
     }

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.

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