From afdbf8e6f881bda6cfe8c8c8c46f2167db8b0b05 Mon Sep 17 00:00:00 2001 From: Chema Gonzalez Date: Wed, 24 Jun 2020 10:05:22 -0700 Subject: [PATCH] H264: Fix stap-a-to-annex-b loop over-read While converting the aggregated (stap-a) packet transform packet framing input into an annex-b framing copy, the two loops (both the required size calculation and the stap-a-to-annex-b copy) may over-read the input buffer. In both buffers, `nalu_ptr` follows the input (stap-a) buffer, which is located in `data`, and whose length is `data_size`. Buffer is read until `nalu_ptr` reaches the end of the buffer. Issues is that the 5th line in the loop: ``` uint16_t segment_length = nalu_ptr[0] << 8 | nalu_ptr[1]; ``` This line accesses `nalu_ptr[1]`, which needs to be protected in the loop condition. Let's assume `data_size = 4`, and that we restart the loop with `nalu_ptr = data + 3`. The condition of the loop does hold (`nalu_ptr = data + 3 < data + data_size`), but the 5th line will access to `data[3+1] = data[4]`, which is an over-read. Tested: ``` $ ninja -C out/Default $ out/Default/modules_unittests --gtest_filter=PacketBuffer*:H264*:RtpPacketizerH264Test*:VideoRtpDepacketizerH264Test*:TestH264SpsPpsTracker* --logs ... [ PASSED ] 97 tests. ``` Change-Id: I8b8aaf7d12b0bb154430b8922f099cd49e684762 Bug: webrtc:11698 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/177140 Reviewed-by: Sergey Silkin Reviewed-by: Rasmus Brandt Commit-Queue: Niklas Enbom Cr-Commit-Position: refs/heads/master@{#31561} --- modules/video_coding/h264_sps_pps_tracker.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/modules/video_coding/h264_sps_pps_tracker.cc b/modules/video_coding/h264_sps_pps_tracker.cc index 3965b28e8e..4becdb7608 100644 --- a/modules/video_coding/h264_sps_pps_tracker.cc +++ b/modules/video_coding/h264_sps_pps_tracker.cc @@ -49,6 +49,7 @@ H264SpsPpsTracker::FixedBitstream H264SpsPpsTracker::CopyAndFixBitstream( RTPVideoHeader* video_header) { RTC_DCHECK(video_header); RTC_DCHECK(video_header->codec == kVideoCodecH264); + RTC_DCHECK_GT(bitstream.size(), 0); auto& h264_header = absl::get(video_header->video_type_header); @@ -128,7 +129,7 @@ H264SpsPpsTracker::FixedBitstream H264SpsPpsTracker::CopyAndFixBitstream( if (h264_header.packetization_type == kH264StapA) { const uint8_t* nalu_ptr = bitstream.data() + 1; - while (nalu_ptr < bitstream.data() + bitstream.size()) { + while (nalu_ptr < bitstream.data() + bitstream.size() - 1) { RTC_DCHECK(video_header->is_first_packet_in_frame); required_size += sizeof(start_code_h264); @@ -180,7 +181,7 @@ H264SpsPpsTracker::FixedBitstream H264SpsPpsTracker::CopyAndFixBitstream( // Copy the rest of the bitstream and insert start codes. if (h264_header.packetization_type == kH264StapA) { const uint8_t* nalu_ptr = bitstream.data() + 1; - while (nalu_ptr < bitstream.data() + bitstream.size()) { + while (nalu_ptr < bitstream.data() + bitstream.size() - 1) { fixed.bitstream.AppendData(start_code_h264); // The first two bytes describe the length of a segment.