New issue
Advanced search Search tips
Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Nov 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 1
Type: Bug-Security



Sign in to add a comment
link

Issue 667504: WebRTC UsingFlexibleMode OOB memory write from picture id

Reported by i...@vulture.fm, Nov 21 2016

Issue description

UserAgent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/54.0.2840.99 Safari/537.36

Steps to reproduce the problem:
The problem is reproducible in our web app while playing and swapping around lots of video. Unfortunately, the repro is not fully deterministic. Though it does almost always crash after a period of several minutes.

However, the bug itself is easily visible by looking at the source code, so nailing down an exact repro (which may be related to a race condition or other randomness) is less a priority for us than just reporting this. Details in the "What went wrong" box.

What is the expected behavior?
Chrome tab doesn't crash while playing video.

What went wrong?
There are several potentials for OOB memory writes in webrtc/modules/video_coding/main/source/decoding_state.cc in VCMDecodingState::SetState, with at least one confirmed in several minidumps.

We suspect this commit introduced the bug: https://chromium.googlesource.com/external/webrtc/+blame/cfc319be1d6afec77bd41eeb70d3e7886dd524db/webrtc/modules/video_coding/main/source/decoding_state.cc#76

The one we have recorded in several minidumps happens at line 93 (different line in master due to other commits since that one) :
 frame_decoded_[frame_index] = true;
Here, frame_index is (uint16_t)0xffff, and 
  bool frame_decoded_[kFrameDecodedLength=(1<<7)];
Tracing back:
 uint16_t frame_index = picture_id_ % kFrameDecodedLength;
picture_id_ is a signed int with value -1 (kNoPictureId). thus the % operation returns -1 and truncates down to 0xffff when converting to uint16_t. Since frame_decoded_ has only 128 entries, this allows out of bounds accesses. The while loop on line 86 may also be able to write large blocks of memory beyond the end of the array. However, all the minidumps we collected are on the above line mentioned.

It seems the problem stems from:

  picture_id_ = frame->PictureId();

which bubbles to the session_info.cc code:

int VCMSessionInfo::PictureId() const {
  if (packets_.empty())
    return kNoPictureId;
  if (packets_.front().video_header.codec == kRtpVideoVp8) {
    return packets_.front().video_header.codecHeader.VP8.pictureId;
  } else if (packets_.front().video_header.codec == kRtpVideoVp9) {
    return packets_.front().video_header.codecHeader.VP9.picture_id;
  } else {
    return kNoPictureId;
  }
}

for which there are many cases where it can return kNoPictureId (-1), but a lack of error(?) checking allows  this -1 value to be used as an unsigned index, allowing OOB memory writes.

Without knowing the code myself, I can't suggest the best bugfix for this, but at the very least, checking for a -1 picture id value as well as making sure frame_decoded_[]'s length is never exceeded, would be a good start.

Did this work before? Yes Before the commit that introduced the bug

Chrome version: 54.0.2840.99  Channel: stable
OS Version: 6.3
Flash Version: 

The minidumps we have are for an internal version of our Electron app, so probably not terribly useful to you without symbols etc, or I would upload the .dmp files. However, it also reproduces in a Chrome 54 browser tab (crashes). We won't be able to provide an external repro website until a later date, but if the explanation given for the code was insufficient, we will try to follow up with that after release dates etc.
 

Comment 1 by i...@vulture.fm, Nov 22 2016

-  if (UsingFlexibleMode(frame)) {
+  if (picture_id_ >= 0 && UsingFlexibleMode(frame)) {

No idea if this is a correct fix for the problem, but... we tried using this patch, and it seems to have fixed not only crashes, but another completely unrelated (we thought) video freeze issue that was occurring. I suppose the same issue, when not crashing, could still have caused other unintended consequences resulting from OOB memory write.

You guys can probably figure out a better/more real solution, just thought I'd share some preliminary results.

Comment 2 by mea...@chromium.org, Nov 22 2016

Components: Blink>WebRTC>Video
Labels: Security_Impact-Stable Security_Severity-High
Owner: philipel@chromium.org
Status: Assigned (was: Unconfirmed)
Thanks for the report and the detailed analysis!

philipel: Can you please take a look and see if this is related to https://codereview.webrtc.org/1431283002? Thanks.

Comment 3 by sheriffbot@chromium.org, Nov 22 2016

Project Member
Labels: M-54

Comment 4 by sheriffbot@chromium.org, Nov 22 2016

Project Member
Labels: -Pri-2 Pri-1

Comment 5 by bugdroid1@chromium.org, Nov 29 2016

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/external/webrtc.git/+/ee414d90b046e3e7feb4130b0567091a43c36702

commit ee414d90b046e3e7feb4130b0567091a43c36702
Author: philipel <philipel@webrtc.org>
Date: Tue Nov 29 15:01:23 2016

Added sanity check to VCMDecodingState::UsingFlexibleMode to prevent OOB error.

BUG= chromium:667504 

Review-Url: https://codereview.webrtc.org/2534883003
Cr-Commit-Position: refs/heads/master@{#15298}

[modify] https://crrev.com/ee414d90b046e3e7feb4130b0567091a43c36702/webrtc/modules/video_coding/decoding_state.cc

Comment 6 by philipel@chromium.org, Nov 29 2016

Status: Fixed (was: Assigned)
As meacer@ said, thanks for the detailed bug report!

A fix has now landed, isu@ could you try it out and verify that it solved your problem?

Comment 7 by i...@vulture.fm, Nov 30 2016

Removed our patch and added yours instead -- seems to be working! :)

Comment 8 by philipel@chromium.org, Nov 30 2016

Status: Verified (was: Fixed)
Thanks for testing it!

Comment 9 by sheriffbot@chromium.org, Nov 30 2016

Project Member
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify

Comment 10 by awhalley@chromium.org, Dec 2 2016

Labels: -M-54 reward-topanel M-56

Comment 11 by sheriffbot@chromium.org, Dec 9 2016

Project Member
Labels: Merge-Request-56

Comment 12 by dimu@chromium.org, Dec 9 2016

Labels: -Merge-Request-56 Merge-Approved-56 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M56 (branch: 2924)

Comment 13 by philipel@chromium.org, Dec 9 2016

dimu@, do I have to create a merge CL or is it automated in the case of the sheriffbot making the merge request?

Comment 14 by sheriffbot@chromium.org, Dec 12 2016

Project Member
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

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

Comment 15 by bugdroid1@chromium.org, Dec 12 2016

Project Member
Labels: merge-merged-56
The following revision refers to this bug:
  https://chromium.googlesource.com/external/webrtc.git/+/598cf6f23ad635d07ecc78406e82607bbb70c048

commit 598cf6f23ad635d07ecc78406e82607bbb70c048
Author: philipel <philipel@webrtc.org>
Date: Mon Dec 12 16:35:30 2016

Added sanity check to VCMDecodingState::UsingFlexibleMode to prevent OOB error.

BUG= chromium:667504 

TBR=stefan@webrtc.org

Review-Url: https://codereview.webrtc.org/2534883003
Cr-Commit-Position: refs/heads/master@{#15298}
(cherry picked from commit ee414d90b046e3e7feb4130b0567091a43c36702)

Review URL: https://codereview.webrtc.org/2573483002 .

Cr-Commit-Position: refs/branch-heads/56@{#6}
Cr-Branched-From: 613152af11d6e9a4d046af3c48a7be7642dfcc68-refs/heads/master@{#15101}

[modify] https://crrev.com/598cf6f23ad635d07ecc78406e82607bbb70c048/webrtc/modules/video_coding/decoding_state.cc

Comment 16 by philipel@chromium.org, Dec 12 2016

Labels: -Merge-Approved-56 Merge-Merged

Comment 17 by awhalley@chromium.org, Dec 12 2016

Labels: -reward-topanel reward-unpaid reward-3000

Comment 18 by awhalley@google.com, Dec 12 2016

Congratulations, the panel awarded $3,000 for this bug!

Comment 19 by awhalley@chromium.org, Dec 12 2016

Labels: -reward-unpaid reward-inprocess

Comment 20 by awhalley@google.com, Dec 19 2016

Labels: -Hotlist-Merge-Approved

Comment 21 by awhalley@chromium.org, Jan 24 2017

Labels: Release-0-M56

Comment 22 by awhalley@chromium.org, Jan 25 2017

Labels: CVE-2017-5009

Comment 23 by sheriffbot@chromium.org, Mar 8 2017

Project Member
Labels: -Restrict-View-SecurityNotify allpublic
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

Comment 24 by awhalley@chromium.org, Apr 25 2018

Labels: CVE_description-submitted

Sign in to add a comment