Automatically adding ccs based on suspected regression changelists:

Add fuzzer for H264 bitstream parser. by pbos@webrtc.org - https://chromium.googlesource.com/external/webrtc/trunk/webrtc/+/8f5983ab0154050b5b36dec57e4a827fac4b228d

If this is incorrect, please let us know why and apply the Test-Predator-Wrong-CLs label.

phoglund@ -- ClusterFuzz can't find the culprit CL so can you please help triage this security issue? Thanks.

phoglund: Uh oh! This issue still open and hasn't been updated in the last 14 days. This is a serious vulnerability, and we want to ensure that there's progress. Could you please leave an update with the current status and any potential blockers?

If you're not the right owner for this issue, could you please remove yourself as soon as possible or help us find the right one?

If the issue is fixed or you can't reproduce it, please close the bug. If you've started working on a fix, please set the status to Started.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Stefan, can you help triage this one? Not sure who should work on h264 or bitbuffer.h.

Elad, is this the same bitbuffer that you intend to use? Could you look into this? Maybe we should see if the BitBuffer has changed in Chrome?

eladalon: Uh oh! This issue still open and hasn't been updated in the last 28 days. This is a serious vulnerability, and we want to ensure that there's progress. Could you please leave an update with the current status and any potential blockers?

If you're not the right owner for this issue, could you please remove yourself as soon as possible or help us find the right one?

If the issue is fixed or you can't reproduce it, please close the bug. If you've started working on a fix, please set the status to Started.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Assigning to nisse@ as eladalon is ooo.

I've done a little debugging, and I see at least two problems:

1: If BitBuffer::PeekBits is called at the end of the buffer and with bit_count == 0, it will read one byte after the end of the buffer.

I'm not sure what's the intended behavior. Should it RTC_CHECK that bit_count > 0? Or return always true for a request for zero bits?

2: SpsParser assigns members log2_max_pic_order_cnt_lsb_minus4 and log2_max_frame_num_minus4 using BitBuffer::ReadExponentialGolomb, with no validity checks beyond the return value from that function.

For the fuzzer testcase, we get log2_max_pic_order_cnt_lsb_minus4 == 0xfffc. Later on we add 4 to get the bit count, which overflows (perfectly well-defined behavior) to give 0. And this triggers problem (1).

For problem 1, we should decide what the BitBuffer should do with bit_count == 0, and do that without over-reads. Test cases give no guidance on intended behavior.

For problem 2, I'd suggest not having any "minus 4" members, and read values like

    uint32_t log_minus4;
    RETURN_EMPTY_ON_FAIL(
        buffer->ReadExponentialGolomb(&log_minus4));

    sps.log2_max_pic_order_cnt_lsb = log_minus4 + 4;
    if (sps.log2_max_pic_order_cnt_lsb < 4) {  // Overflow check
      return absl::nullopt;
    }

Or, since we don't support reading more than 32 bits anyway,

    uint32_t log_minus4;
    RETURN_EMPTY_ON_FAIL(
        buffer->ReadExponentialGolomb(&log_minus4));
    if (log_minus_4 > (32 - 4)) {
      return absl::nullopt;
    }
    sps.log2_max_pic_order_cnt_lsb = log_minus4 + 4;
  
If the h264 specification gives tighter bounds on these values, we should check those instead.

I've seen some cases where reading or writing zero bytes actually simplifies the code. If we can think of a realistic use case for reading zero bits, then I think it would be reasonable to always return true. However, until we have such a use case, I'd prefer to DCHECK with a comment in the code that returning true would be just as reasonable.

The following revision refers to this bug:
  https://webrtc.googlesource.com/src.git/+/827cf3c1b28ef8c824aaff50d381e78bb7733f00

commit 827cf3c1b28ef8c824aaff50d381e78bb7733f00
Author: Niels Möller <nisse@webrtc.org>
Date: Thu Sep 27 11:50:10 2018

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 
Change-Id: I684f92b8f0f2fcdbab24732d8e8381bc51a92752
Reviewed-on: https://webrtc-review.googlesource.com/101760
Reviewed-by: Stefan Holmer <stefan@webrtc.org>
Reviewed-by: Björn Terelius <terelius@webrtc.org>
Reviewed-by: Karl Wiberg <kwiberg@webrtc.org>
Commit-Queue: Niels Moller <nisse@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#24862}
[modify] https://crrev.com/827cf3c1b28ef8c824aaff50d381e78bb7733f00/common_video/h264/h264_bitstream_parser.cc
[modify] https://crrev.com/827cf3c1b28ef8c824aaff50d381e78bb7733f00/common_video/h264/sps_parser.cc
[modify] https://crrev.com/827cf3c1b28ef8c824aaff50d381e78bb7733f00/common_video/h264/sps_parser.h
[modify] https://crrev.com/827cf3c1b28ef8c824aaff50d381e78bb7733f00/common_video/h264/sps_parser_unittest.cc
[modify] https://crrev.com/827cf3c1b28ef8c824aaff50d381e78bb7733f00/rtc_base/bitbuffer.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/99f8db7dfcfbbeabc7987fb2fe9fec2c23d7fcce

commit 99f8db7dfcfbbeabc7987fb2fe9fec2c23d7fcce
Author: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Date: Thu Sep 27 16:13:37 2018

Roll src/third_party/webrtc 529d0d9795b8..efb94d57eb88 (6 commits)

https://webrtc.googlesource.com/src.git/+log/529d0d9795b8..efb94d57eb88


git log 529d0d9795b8..efb94d57eb88 --date=short --no-merges --format='%ad %ae %s'
2018-09-27 oprypin@webrtc.org Revert "Revert "Replace VideoDecoder with VideoDecoderFactory in VideoReceiveStream config.""
2018-09-27 nisse@webrtc.org Revert "Replace VideoDecoder with VideoDecoderFactory in VideoReceiveStream config."
2018-09-27 mbonadei@webrtc.org Fix global_constructors, exit_time_destructors in audio device pulse.
2018-09-27 srte@webrtc.org Adds scenario test framework.
2018-09-27 saza@webrtc.org Remove echo_cancellation() and echo_control_mobile() interface access outside APM
2018-09-27 nisse@webrtc.org Avoid overflow when parsing H.264 SPS.


Created with:
  gclient setdep -r src/third_party/webrtc@efb94d57eb88

The AutoRoll server is located here: https://autoroll.skia.org/r/webrtc-chromium-autoroll

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.

CQ_INCLUDE_TRYBOTS=luci.chromium.try:linux_chromium_archive_rel_ng;luci.chromium.try:mac_chromium_archive_rel_ng

BUG= chromium:877843 
TBR=webrtc-chromium-sheriffs-robots@google.com

Change-Id: Ie0f958230b4d5565bebed96f68e31e4a67a8d20b
Reviewed-on: https://chromium-review.googlesource.com/1249621
Reviewed-by: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Commit-Queue: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#594741}
[modify] https://crrev.com/99f8db7dfcfbbeabc7987fb2fe9fec2c23d7fcce/DEPS

ClusterFuzz has detected this issue as fixed in range 594740:594741.

Detailed report: https://clusterfuzz.com/testcase?key=4722189266583552

Fuzzer: libFuzzer_h264_bitstream_parser_fuzzer
Job Type: libfuzzer_chrome_asan
Platform Id: linux

Crash Type: Heap-buffer-overflow READ 1
Crash Address: 0x605000002162
Crash State:
  rtc::BitBuffer::PeekBits
  rtc::BitBuffer::ReadBits
  webrtc::H264BitstreamParser::ParseNonParameterSetNalu
  
Sanitizer: address (ASAN)

Recommended Security Severity: Medium

Regressed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_asan&range=430550:430593
Fixed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_asan&range=594740:594741

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=4722189266583552

See https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reference.md for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.

ClusterFuzz testcase 4722189266583552 is verified as fixed, so closing issue as verified.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.

I think the security impact is very low; we might read and ignore one byte beyond the end of the buffer. Which might crash (i.e., denial of service) in the unlikely event that the buffer was located just at the edge of accessible memory.

So I don't think merging the fix into release branch(es) is motivated.

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot