From 827cf3c1b28ef8c824aaff50d381e78bb7733f00 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niels=20M=C3=B6ller?= Date: Thu, 27 Sep 2018 11:02:08 +0200 Subject: [PATCH] Avoid overflow when parsing H.264 SPS. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Change-Id: I684f92b8f0f2fcdbab24732d8e8381bc51a92752 Reviewed-on: https://webrtc-review.googlesource.com/101760 Reviewed-by: Stefan Holmer Reviewed-by: Björn Terelius Reviewed-by: Karl Wiberg Commit-Queue: Niels Moller Cr-Commit-Position: refs/heads/master@{#24862} --- common_video/h264/h264_bitstream_parser.cc | 10 ++-- common_video/h264/sps_parser.cc | 23 +++++++-- common_video/h264/sps_parser.h | 4 +- common_video/h264/sps_parser_unittest.cc | 58 ++++++++++++++++++++-- rtc_base/bitbuffer.cc | 4 ++ 5 files changed, 83 insertions(+), 16 deletions(-) diff --git a/common_video/h264/h264_bitstream_parser.cc b/common_video/h264/h264_bitstream_parser.cc index d8f8a62378..2e63b9d4a1 100644 --- a/common_video/h264/h264_bitstream_parser.cc +++ b/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/common_video/h264/sps_parser.cc b/common_video/h264/sps_parser.cc index ce12834fe1..b6799a3be4 100644 --- a/common_video/h264/sps_parser.cc +++ b/common_video/h264/sps_parser.cc @@ -133,15 +133,30 @@ absl::optional 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/common_video/h264/sps_parser.h b/common_video/h264/sps_parser.h index 386d468936..76e627d27a 100644 --- a/common_video/h264/sps_parser.h +++ b/common_video/h264/sps_parser.h @@ -34,8 +34,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/common_video/h264/sps_parser_unittest.cc b/common_video/h264/sps_parser_unittest.cc index e88d0c3119..94c9a0c744 100644 --- a/common_video/h264/sps_parser_unittest.cc +++ b/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( 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( 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( + 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( + 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( + 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( + 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/rtc_base/bitbuffer.cc b/rtc_base/bitbuffer.cc index 025cfe1516..be72d558f1 100644 --- a/rtc_base/bitbuffer.cc +++ b/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; }