[Backport] Security bug 877843

Avoid overflow when parsing H.264 SPS.

Check that |log2_max_frame_num_minus4| and
|log2_max_pic_order_cnt_lsb_minus4| are at most 28, resulting in a
field width of at most 32 bits.

Bug: chromium:877843
Reviewed-on: https://webrtc-review.googlesource.com/101760
Change-Id: I684f92b8f0f2fcdbab24732d8e8381bc51a92752
Reviewed-by: Michael Brüning <michael.bruning@qt.io>

5 files changed, 83 insertions, 16 deletions

diff --git a/chromium/third_party/webrtc/common_video/h264/h264_bitstream_parser.cc b/chromium/third_party/webrtc/common_video/h264/h264_bitstream_parser.cc
index d8f8a62378c..2e63b9d4a16 100644
--- a/chromium/third_party/webrtc/common_video/h264/h264_bitstream_parser.cc
+++ b/chromium/third_party/webrtc/common_video/h264/h264_bitstream_parser.cc
@@ -75,9 +75,9 @@ H264BitstreamParser::Result H264BitstreamParser::ParseNonParameterSetNalu(
     RETURN_INV_ON_FAIL(slice_reader.ReadBits(&bits_tmp, 2));
   }
   // frame_num: u(v)
-  // Represented by log2_max_frame_num_minus4 + 4 bits.
+  // Represented by log2_max_frame_num bits.
   RETURN_INV_ON_FAIL(
-      slice_reader.ReadBits(&bits_tmp, sps_->log2_max_frame_num_minus4 + 4));
+      slice_reader.ReadBits(&bits_tmp, sps_->log2_max_frame_num));
   uint32_t field_pic_flag = 0;
   if (sps_->frame_mbs_only_flag == 0) {
     // field_pic_flag: u(1)
@@ -92,10 +92,10 @@ H264BitstreamParser::Result H264BitstreamParser::ParseNonParameterSetNalu(
     RETURN_INV_ON_FAIL(slice_reader.ReadExponentialGolomb(&golomb_tmp));
   }
   // pic_order_cnt_lsb: u(v)
-  // Represented by sps_.log2_max_pic_order_cnt_lsb_minus4 + 4 bits.
+  // Represented by sps_.log2_max_pic_order_cnt_lsb bits.
   if (sps_->pic_order_cnt_type == 0) {
-    RETURN_INV_ON_FAIL(slice_reader.ReadBits(
-        &bits_tmp, sps_->log2_max_pic_order_cnt_lsb_minus4 + 4));
+    RETURN_INV_ON_FAIL(
+        slice_reader.ReadBits(&bits_tmp, sps_->log2_max_pic_order_cnt_lsb));
     if (pps_->bottom_field_pic_order_in_frame_present_flag &&
         field_pic_flag == 0) {
       // delta_pic_order_cnt_bottom: se(v)
diff --git a/chromium/third_party/webrtc/common_video/h264/sps_parser.cc b/chromium/third_party/webrtc/common_video/h264/sps_parser.cc
index b313f487883..647d5e01011 100644
--- a/chromium/third_party/webrtc/common_video/h264/sps_parser.cc
+++ b/chromium/third_party/webrtc/common_video/h264/sps_parser.cc
@@ -131,15 +131,30 @@ absl::optional<SpsParser::SpsState> SpsParser::ParseSpsUpToVui(
       }
     }
   }
+  // log2_max_frame_num and log2_max_pic_order_cnt_lsb are used with
+  // BitBuffer::ReadBits, which can read at most 32 bits at a time. We also have
+  // to avoid overflow when adding 4 to the on-wire golomb value, e.g., for evil
+  // input data, ReadExponentialGolomb might return 0xfffc.
+  const uint32_t kMaxLog2Minus4 = 32 - 4;
+
   // log2_max_frame_num_minus4: ue(v)
-  RETURN_EMPTY_ON_FAIL(
-      buffer->ReadExponentialGolomb(&sps.log2_max_frame_num_minus4));
+  uint32_t log2_max_frame_num_minus4;
+  if (!buffer->ReadExponentialGolomb(&log2_max_frame_num_minus4) ||
+      log2_max_frame_num_minus4 > kMaxLog2Minus4) {
+    return OptionalSps();
+  }
+  sps.log2_max_frame_num = log2_max_frame_num_minus4 + 4;
+
   // pic_order_cnt_type: ue(v)
   RETURN_EMPTY_ON_FAIL(buffer->ReadExponentialGolomb(&sps.pic_order_cnt_type));
   if (sps.pic_order_cnt_type == 0) {
     // log2_max_pic_order_cnt_lsb_minus4: ue(v)
-    RETURN_EMPTY_ON_FAIL(
-        buffer->ReadExponentialGolomb(&sps.log2_max_pic_order_cnt_lsb_minus4));
+    uint32_t log2_max_pic_order_cnt_lsb_minus4;
+    if (!buffer->ReadExponentialGolomb(&log2_max_pic_order_cnt_lsb_minus4) ||
+        log2_max_pic_order_cnt_lsb_minus4 > kMaxLog2Minus4) {
+      return OptionalSps();
+    }
+    sps.log2_max_pic_order_cnt_lsb = log2_max_pic_order_cnt_lsb_minus4 + 4;
   } else if (sps.pic_order_cnt_type == 1) {
     // delta_pic_order_always_zero_flag: u(1)
     RETURN_EMPTY_ON_FAIL(
diff --git a/chromium/third_party/webrtc/common_video/h264/sps_parser.h b/chromium/third_party/webrtc/common_video/h264/sps_parser.h
index d4294b25589..000fcb1bfb5 100644
--- a/chromium/third_party/webrtc/common_video/h264/sps_parser.h
+++ b/chromium/third_party/webrtc/common_video/h264/sps_parser.h
@@ -32,8 +32,8 @@ class SpsParser {
     uint32_t delta_pic_order_always_zero_flag = 0;
     uint32_t separate_colour_plane_flag = 0;
     uint32_t frame_mbs_only_flag = 0;
-    uint32_t log2_max_frame_num_minus4 = 0;
-    uint32_t log2_max_pic_order_cnt_lsb_minus4 = 0;
+    uint32_t log2_max_frame_num = 4;          // Smallest valid value.
+    uint32_t log2_max_pic_order_cnt_lsb = 4;  // Smallest valid value.
     uint32_t pic_order_cnt_type = 0;
     uint32_t max_num_ref_frames = 0;
     uint32_t vui_params_present = 0;
diff --git a/chromium/third_party/webrtc/common_video/h264/sps_parser_unittest.cc b/chromium/third_party/webrtc/common_video/h264/sps_parser_unittest.cc
index 50227ed870a..dc1c7ba1343 100644
--- a/chromium/third_party/webrtc/common_video/h264/sps_parser_unittest.cc
+++ b/chromium/third_party/webrtc/common_video/h264/sps_parser_unittest.cc
@@ -43,6 +43,8 @@ static const size_t kSpsBufferMaxSize = 256;
 void GenerateFakeSps(uint16_t width,
                      uint16_t height,
                      int id,
+                     uint32_t log2_max_frame_num_minus4,
+                     uint32_t log2_max_pic_order_cnt_lsb_minus4,
                      rtc::Buffer* out_buffer) {
   uint8_t rbsp[kSpsBufferMaxSize] = {0};
   rtc::BitBufferWriter writer(rbsp, kSpsBufferMaxSize);
@@ -57,12 +59,12 @@ void GenerateFakeSps(uint16_t width,
   // Profile is not special, so we skip all the chroma format settings.
 
   // Now some bit magic.
-  // log2_max_frame_num_minus4: ue(v). 0 is fine.
-  writer.WriteExponentialGolomb(0);
+  // log2_max_frame_num_minus4: ue(v).
+  writer.WriteExponentialGolomb(log2_max_frame_num_minus4);
   // pic_order_cnt_type: ue(v). 0 is the type we want.
   writer.WriteExponentialGolomb(0);
   // log2_max_pic_order_cnt_lsb_minus4: ue(v). 0 is fine.
-  writer.WriteExponentialGolomb(0);
+  writer.WriteExponentialGolomb(log2_max_pic_order_cnt_lsb_minus4);
   // max_num_ref_frames: ue(v). 0 is fine.
   writer.WriteExponentialGolomb(0);
   // gaps_in_frame_num_value_allowed_flag: u(1).
@@ -104,9 +106,11 @@ void GenerateFakeSps(uint16_t width,
     byte_count++;
   }
 
+  out_buffer->Clear();
   H264::WriteRbsp(rbsp, byte_count, out_buffer);
 }
 
+// TODO(nisse): Delete test fixture.
 class H264SpsParserTest : public ::testing::Test {
  public:
   H264SpsParserTest() {}
@@ -153,7 +157,7 @@ TEST_F(H264SpsParserTest, TestSampleSPSWeirdResolution) {
 
 TEST_F(H264SpsParserTest, TestSyntheticSPSQvgaLandscape) {
   rtc::Buffer buffer;
-  GenerateFakeSps(320u, 180u, 1, &buffer);
+  GenerateFakeSps(320u, 180u, 1, 0, 0, &buffer);
   EXPECT_TRUE(static_cast<bool>(
       sps_ = SpsParser::ParseSps(buffer.data(), buffer.size())));
   EXPECT_EQ(320u, sps_->width);
@@ -163,7 +167,7 @@ TEST_F(H264SpsParserTest, TestSyntheticSPSQvgaLandscape) {
 
 TEST_F(H264SpsParserTest, TestSyntheticSPSWeirdResolution) {
   rtc::Buffer buffer;
-  GenerateFakeSps(156u, 122u, 2, &buffer);
+  GenerateFakeSps(156u, 122u, 2, 0, 0, &buffer);
   EXPECT_TRUE(static_cast<bool>(
       sps_ = SpsParser::ParseSps(buffer.data(), buffer.size())));
   EXPECT_EQ(156u, sps_->width);
@@ -184,4 +188,48 @@ TEST_F(H264SpsParserTest, TestSampleSPSWithScalingLists) {
   EXPECT_EQ(1080u, sps_->height);
 }
 
+TEST_F(H264SpsParserTest, TestLog2MaxFrameNumMinus4) {
+  rtc::Buffer buffer;
+  GenerateFakeSps(320u, 180u, 1, 0, 0, &buffer);
+  EXPECT_TRUE(static_cast<bool>(
+      sps_ = SpsParser::ParseSps(buffer.data(), buffer.size())));
+  EXPECT_EQ(320u, sps_->width);
+  EXPECT_EQ(180u, sps_->height);
+  EXPECT_EQ(1u, sps_->id);
+  EXPECT_EQ(4u, sps_->log2_max_frame_num);
+
+  GenerateFakeSps(320u, 180u, 1, 28, 0, &buffer);
+  EXPECT_TRUE(static_cast<bool>(
+      sps_ = SpsParser::ParseSps(buffer.data(), buffer.size())));
+  EXPECT_EQ(320u, sps_->width);
+  EXPECT_EQ(180u, sps_->height);
+  EXPECT_EQ(1u, sps_->id);
+  EXPECT_EQ(32u, sps_->log2_max_frame_num);
+
+  GenerateFakeSps(320u, 180u, 1, 29, 0, &buffer);
+  EXPECT_FALSE(SpsParser::ParseSps(buffer.data(), buffer.size()));
+}
+
+TEST_F(H264SpsParserTest, TestLog2MaxPicOrderCntMinus4) {
+  rtc::Buffer buffer;
+  GenerateFakeSps(320u, 180u, 1, 0, 0, &buffer);
+  EXPECT_TRUE(static_cast<bool>(
+      sps_ = SpsParser::ParseSps(buffer.data(), buffer.size())));
+  EXPECT_EQ(320u, sps_->width);
+  EXPECT_EQ(180u, sps_->height);
+  EXPECT_EQ(1u, sps_->id);
+  EXPECT_EQ(4u, sps_->log2_max_pic_order_cnt_lsb);
+
+  GenerateFakeSps(320u, 180u, 1, 0, 28, &buffer);
+  EXPECT_TRUE(static_cast<bool>(
+      sps_ = SpsParser::ParseSps(buffer.data(), buffer.size())));
+  EXPECT_EQ(320u, sps_->width);
+  EXPECT_EQ(180u, sps_->height);
+  EXPECT_EQ(1u, sps_->id);
+  EXPECT_EQ(32u, sps_->log2_max_pic_order_cnt_lsb);
+
+  GenerateFakeSps(320u, 180u, 1, 0, 29, &buffer);
+  EXPECT_FALSE(SpsParser::ParseSps(buffer.data(), buffer.size()));
+}
+
 }  // namespace webrtc
diff --git a/chromium/third_party/webrtc/rtc_base/bitbuffer.cc b/chromium/third_party/webrtc/rtc_base/bitbuffer.cc
index 025cfe1516a..be72d558f1e 100644
--- a/chromium/third_party/webrtc/rtc_base/bitbuffer.cc
+++ b/chromium/third_party/webrtc/rtc_base/bitbuffer.cc
@@ -108,6 +108,10 @@ bool BitBuffer::ReadUInt32(uint32_t* val) {
 }
 
 bool BitBuffer::PeekBits(uint32_t* val, size_t bit_count) {
+  // TODO(nisse): Could allow bit_count == 0 and always return success. But
+  // current code reads one byte beyond end of buffer in the case that
+  // RemainingBitCount() == 0 and bit_count == 0.
+  RTC_DCHECK(bit_count > 0);
   if (!val || bit_count > RemainingBitCount() || bit_count > 32) {
     return false;
   }