Prevent OOB reads for truncated H264 STAP-A packets.

webrtc/modules/video_coding/main/source/session_info.cc

 /*
  *  Copyright (c) 2012 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 98 matching lines @@
   size_t length = 0;
   for (PacketIteratorConst it = packets_.begin(); it != packets_.end(); ++it)
     length += (*it).sizeBytes;
   return length;
 }
 
 int VCMSessionInfo::NumPackets() const {
   return packets_.size();
 }
 
-size_t VCMSessionInfo::InsertBuffer(uint8_t* frame_buffer,
-                                    PacketIterator packet_it) {
-  VCMPacket& packet = *packet_it;
+bool VCMSessionInfo::InsertBuffer(uint8_t* frame_buffer,
+                                  size_t* inserted_length,
+                                  PacketIterator packet_it) {
+  VCMPacket* packet = &(*packet_it);
   PacketIterator it;
 
   // Calculate the offset into the frame buffer for this packet.
   size_t offset = 0;
   for (it = packets_.begin(); it != packet_it; ++it)
     offset += (*it).sizeBytes;
 
   // Set the data pointer to pointing to the start of this packet in the
   // frame buffer.
-  const uint8_t* packet_buffer = packet.dataPtr;
-  packet.dataPtr = frame_buffer + offset;
+  const uint8_t* packet_buffer = packet->dataPtr;
+  packet->dataPtr = frame_buffer + offset;
 
   // We handle H.264 STAP-A packets in a special way as we need to remove the
   // two length bytes between each NAL unit, and potentially add start codes.
   const size_t kH264NALHeaderLengthInBytes = 1;
   const size_t kLengthFieldLength = 2;
-  if (packet.codecSpecificHeader.codec == kRtpVideoH264 &&
-      packet.codecSpecificHeader.codecHeader.H264.packetization_type ==
+  if (packet->codecSpecificHeader.codec == kRtpVideoH264 &&
+      packet->codecSpecificHeader.codecHeader.H264.packetization_type ==
           kH264StapA) {
-    size_t required_length = 0;
+    size_t required_length = kH264NALHeaderLengthInBytes;
     const uint8_t* nalu_ptr = packet_buffer + kH264NALHeaderLengthInBytes;
-    while (nalu_ptr < packet_buffer + packet.sizeBytes) {
+    while (nalu_ptr < packet_buffer + packet->sizeBytes) {
       size_t length = BufferToUWord16(nalu_ptr);
       required_length +=
-          length + (packet.insertStartCode ? kH264StartCodeLengthBytes : 0);
+          length + (packet->insertStartCode ? kH264StartCodeLengthBytes : 0);

[stefan-webrtc, 2015/07/23 12:36:04]

As discussed, the problem is that this start code overflows the packet size. We
have to take into account the number of nal units when allocating the frame
buffer size in frame_buffer.cc.

       nalu_ptr += kLengthFieldLength + length;
     }
-    ShiftSubsequentPackets(packet_it, required_length);
+    if (required_length > packet->sizeBytes)
+      return false;
+    ShiftSubsequentPackets(packet_it,
+                           required_length - kH264NALHeaderLengthInBytes);
     nalu_ptr = packet_buffer + kH264NALHeaderLengthInBytes;
     uint8_t* frame_buffer_ptr = frame_buffer + offset;
-    while (nalu_ptr < packet_buffer + packet.sizeBytes) {
+    while (nalu_ptr < packet_buffer + packet->sizeBytes) {
       size_t length = BufferToUWord16(nalu_ptr);
       nalu_ptr += kLengthFieldLength;
-      frame_buffer_ptr += Insert(nalu_ptr,
-                                 length,
-                                 packet.insertStartCode,
-                                 const_cast<uint8_t*>(frame_buffer_ptr));
+      frame_buffer_ptr +=
+          Insert(nalu_ptr, length, packet->insertStartCode, frame_buffer_ptr);
       nalu_ptr += length;
     }
-    packet.sizeBytes = required_length;
-    return packet.sizeBytes;
+    packet->sizeBytes = required_length - kH264NALHeaderLengthInBytes;

[pbos-webrtc, 2015/07/17 14:08:53]

Was this part (not including kH264NALHeaderLengthInBytes) correct before? This
should be a no-op now since kH264NALHeaderLengthInBytes is added at line 142.

+    *inserted_length = packet->sizeBytes;
+    return true;
   }
   ShiftSubsequentPackets(
       packet_it,
-      packet.sizeBytes +
-          (packet.insertStartCode ? kH264StartCodeLengthBytes : 0));
+      packet->sizeBytes +
+          (packet->insertStartCode ? kH264StartCodeLengthBytes : 0));
 
-  packet.sizeBytes = Insert(packet_buffer,
-                            packet.sizeBytes,
-                            packet.insertStartCode,
-                            const_cast<uint8_t*>(packet.dataPtr));
-  return packet.sizeBytes;
+  packet->sizeBytes = Insert(packet_buffer,
+                            packet->sizeBytes,
+                            packet->insertStartCode,
+                            const_cast<uint8_t*>(packet->dataPtr));
+  *inserted_length = packet->sizeBytes;
+  return true;
 }
 
 size_t VCMSessionInfo::Insert(const uint8_t* buffer,
                               size_t length,
                               bool insert_start_code,
                               uint8_t* frame_buffer) {
   if (insert_start_code) {
     const unsigned char startCode[] = {0, 0, 0, 1};
     memcpy(frame_buffer, startCode, kH264StartCodeLengthBytes);
   }
@@ skipping 322 matching lines @@
                IsNewerSequenceNumber(packet.seqNum, last_packet_seq_num_)) {
       LOG(LS_WARNING) << "Received packet with a sequence number which is out "
                          "of frame boundaries";
       return -3;
     }
   }
 
   // The insert operation invalidates the iterator |rit|.
   PacketIterator packet_list_it = packets_.insert(rit.base(), packet);
 
-  size_t returnLength = InsertBuffer(frame_buffer, packet_list_it);
+  size_t returnLength;
+  if (!InsertBuffer(frame_buffer, &returnLength, packet_list_it))
+    return kSizeError;
   UpdateCompleteSession();
-  if (decode_error_mode == kWithErrors)
+  if (decode_error_mode == kWithErrors) {
     decodable_ = true;
-  else if (decode_error_mode == kSelectiveErrors)
+  } else if (decode_error_mode == kSelectiveErrors) {
     UpdateDecodableSession(frame_data);
+  }
   return static_cast<int>(returnLength);
 }
 
 void VCMSessionInfo::InformOfEmptyPacket(uint16_t seq_num) {
   // Empty packets may be FEC or filler packets. They are sequential and
   // follow the data packets, therefore, we should only keep track of the high
   // and low sequence numbers and may assume that the packets in between are
   // empty packets belonging to the same frame (timestamp).
   if (empty_seq_num_high_ == -1)
     empty_seq_num_high_ = seq_num;
   else
     empty_seq_num_high_ = LatestSequenceNumber(seq_num, empty_seq_num_high_);
   if (empty_seq_num_low_ == -1 || IsNewerSequenceNumber(empty_seq_num_low_,
                                                         seq_num))
     empty_seq_num_low_ = seq_num;
 }
 
 }  // namespace webrtc