New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 838402 link

Starred by 3 users

Security: WebRTC: Out-of-bounds memory access in WebRTC VP9 Frame Processing

Project Member Reported by natashenka@google.com, Apr 30

Issue description

There is a missing check in VP9 frame processing that could lead to memory corruption.

In the file video_coding/rtp_frame_reference_finder.cc, the function RtpFrameReferenceFinder::ManageFrameVp9 fetches the GofInfo based on a pic_idx parsed from the incoming packet header. If the incoming frame is of type kVideoFrameKey, find is called on an iterator and the result is used without checking whether the it succeeds.

 if (frame->frame_type() == kVideoFrameKey) {
    ...
    GofInfo info = gof_info_.find(codec_header.tl0_pic_idx)->second;
    FrameReceivedVp9(frame->id.picture_id, &info);
    UnwrapPictureIds(frame);
    return kHandOff;
  }

This can cause a pointer to memory outside the gof_info_ map to be passed to FrameReceivedVp9. This function both reads and writes the info structure.

This issue does not crash reliably, so I recommend reproducing it using an asan build of Chrome. To reproduce the issue:

1) unzip the attached webrtc-from-chat.zip on a local webserver
2) fetch the webrtc source (https://webrtc.org/native-code/development/), and replace src/modules/rtp_rtcp/source/rtp_format_vp9.cc with the version attached to the code
3) build webrtc, including the examples
4) run the attached webrtcserver.py with python 3.6 or higher
5) start the peerconnection_client sample in the webrtc examples. Connect to the recommended server, and then select test2 as the peer to connect to
6) visit http://127.0.0.1/webrtc-from-chat/index.html in chrome
7) Enter any username and hit "Log in"
8) Type anything into the chat window at the bottom and hit send

Chrome should crash in a few seconds.

Though the attached PoC requires user interaction, it is not necessary to exercise this issue in a browser.

This issue affects any browser that supports VP9, and can be reached by loading a single webpage (though some browsers will prompt for permissions). It also affects native clients (such as mobile applications) that use webrtc and support VP9, though the user has to place or answer a video call for their client to be in the state where this issue is reachable.

I recommend fixing this by changing the above code to:

    auto gof_info_it = gof_info_.find(codec_header.tl0_pic_idx);
    if (gof_info_it == gof_info_.end())
        return kDrop;
    GofInfo info = gof_info_it->second;
    FrameReceivedVp9(frame->id.picture_id, &info);

I have verified that this fix would prevent the crash.

Please note that is is not sufficient to fix this bug in Chrome, it needs to be fixed in WebRTC upstream as well to be fixed for all affected users.

This bug is subject to a 90 day disclosure deadline. After 90 days elapse
or a patch has been made broadly available, the bug report will become
visible to the public.
 
webrtc-from-chat.zip
396 KB Download
webrtcserver.py
3.7 KB View Download
rtp_format_vp9.cc
26.1 KB View Download
Cc: nisse@chromium.org sprang@chromium.org kwiberg@chromium.org
Components: -Blink>WebRTC Blink>WebRTC>Video
Owner: philipel@chromium.org
Labels: M-67 Security_Severity-High Security_Impact-Stable OS-Android OS-Chrome OS-Fuchsia OS-Linux OS-Mac OS-Windows Pri-1
Cc: ssilkin@chromium.org
Cc: danilchap@chromium.org eladalon@chromium.org
I believe this problem should be fixed by https://webrtc-review.googlesource.com/c/src/+/60260

Unfortunately I could not get your repro steps to work, could you test this CL?

You can run "git cl patch 60260" to apply the patch locally.
Project Member

Comment 6 by sheriffbot@chromium.org, May 2

Status: Assigned (was: Unconfirmed)
Status: Fixed (was: Assigned)
Yes, this resolves it. Thank you!
Project Member

Comment 8 by sheriffbot@chromium.org, May 3

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Labels: Merge-Request-67
Project Member

Comment 10 by sheriffbot@chromium.org, May 4

Labels: -Merge-Request-67 Merge-Review-67 Hotlist-Merge-Review
This bug requires manual review: M67 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Cc: awhalley@chromium.org
+awhalley@ (Security TPM) for M67 merge review. 
NextAction: 2018-05-07
The NextAction date has arrived: 2018-05-07
Project Member

Comment 14 by bugdroid1@chromium.org, May 7

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

commit 156436056ec1d9cd56ae3d7bce29b356b404f9ca
Author: philipel <philipel@webrtc.org>
Date: Mon May 07 11:29:44 2018

VP9 RTP frame reference finder cleanup.

Bug:  chromium:838402 
Change-Id: Ib6117db5aeb83fc03f72759be2e42d0a4ceb94b6
Reviewed-on: https://webrtc-review.googlesource.com/60260
Reviewed-by: Stefan Holmer <stefan@webrtc.org>
Reviewed-by: Niels Moller <nisse@webrtc.org>
Commit-Queue: Philip Eliasson <philipel@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#23139}
[modify] https://crrev.com/156436056ec1d9cd56ae3d7bce29b356b404f9ca/modules/video_coding/rtp_frame_reference_finder.cc

Cc: tommi@chromium.org
philipel@, could you pls clarify which CL you're requesting M67 merge for? CL listed at #14 landed in trunk just 4 hrs back so not in canary yet.
The merge request is for the CL in #14. I realize that CL is a bit tricky to review since it is more than just a security fix, so what I could do instead is to implement the solution suggested by natashenka@. Then the CL would be trivial.
Thank you  philipel@. Pls update the bug with canary result for cl listed at #14.

awhalley@, PTAL comment #17. Thank you.
Project Member

Comment 19 by bugdroid1@chromium.org, May 7

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

commit 71098015063adea710cce4616686d08d65fc1940
Author: webrtc-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com <webrtc-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Date: Mon May 07 20:43:08 2018

Roll src/third_party/webrtc/ 823f9135f..d5ef6ff25 (15 commits)

https://webrtc.googlesource.com/src.git/+log/823f9135f858..d5ef6ff258da

$ git log 823f9135f..d5ef6ff25 --date=short --no-merges --format='%ad %ae %s'

Created with:
  roll-dep src/third_party/webrtc
BUG=chromium:None,chromium:None,chromium:840347,chromium:839860,chromium:838402


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

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=master.tryserver.chromium.linux:linux_chromium_archive_rel_ng;master.tryserver.chromium.mac:mac_chromium_archive_rel_ng
TBR=webrtc-chromium-sheriffs-robots@google.com

Change-Id: Ibaedd51469f75941f90aed9f4ba6bb686cd79f6c
Reviewed-on: https://chromium-review.googlesource.com/1048112
Commit-Queue: webrtc-chromium-autoroll <webrtc-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Reviewed-by: webrtc-chromium-autoroll <webrtc-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#556561}
[modify] https://crrev.com/71098015063adea710cce4616686d08d65fc1940/DEPS

Project Member

Comment 20 by bugdroid1@chromium.org, May 8

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

commit d1429ef7c2699d69eec780f6f8beba5ecab9e312
Author: Kunihiko Sakamoto <ksakamoto@chromium.org>
Date: Tue May 08 03:27:37 2018

Revert "Roll src/third_party/webrtc/ 823f9135f..d5ef6ff25 (15 commits)"

This reverts commit 71098015063adea710cce4616686d08d65fc1940.

Reason for revert: ios build is still broken.
https://ci.chromium.org/buildbot/chromium.mac/ios-device-xcode-clang/59679

Original change's description:
> Roll src/third_party/webrtc/ 823f9135f..d5ef6ff25 (15 commits)
> 
> https://webrtc.googlesource.com/src.git/+log/823f9135f858..d5ef6ff258da
> 
> $ git log 823f9135f..d5ef6ff25 --date=short --no-merges --format='%ad %ae %s'
> 
> Created with:
>   roll-dep src/third_party/webrtc
> BUG=chromium:None,chromium:None,chromium:840347,chromium:839860,chromium:838402
> 
> 
> The AutoRoll server is located here: https://webrtc-chromium-roll.skia.org
> 
> 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=master.tryserver.chromium.linux:linux_chromium_archive_rel_ng;master.tryserver.chromium.mac:mac_chromium_archive_rel_ng
> TBR=webrtc-chromium-sheriffs-robots@google.com
> 
> Change-Id: Ibaedd51469f75941f90aed9f4ba6bb686cd79f6c
> Reviewed-on: https://chromium-review.googlesource.com/1048112
> Commit-Queue: webrtc-chromium-autoroll <webrtc-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
> Reviewed-by: webrtc-chromium-autoroll <webrtc-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
> Cr-Commit-Position: refs/heads/master@{#556561}

TBR=webrtc-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com,webrtc-chromium-sheriffs-robots@google.com

Change-Id: I2823387c7aadad799befbf88cc5b4503a379dd3c
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: chromium:None, chromium:840347, chromium:839860, chromium:838402
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_archive_rel_ng;master.tryserver.chromium.mac:mac_chromium_archive_rel_ng
Reviewed-on: https://chromium-review.googlesource.com/1049345
Commit-Queue: Kunihiko Sakamoto <ksakamoto@chromium.org>
Reviewed-by: Kunihiko Sakamoto <ksakamoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556675}
[modify] https://crrev.com/d1429ef7c2699d69eec780f6f8beba5ecab9e312/DEPS

Project Member

Comment 21 by bugdroid1@chromium.org, May 8

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

commit ebc7dc2c2cccb2e04ed594fa68c0a1fb0d7a59e6
Author: webrtc-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com <webrtc-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Date: Tue May 08 14:16:42 2018

Roll src/third_party/webrtc/ 823f9135f..826738b78 (29 commits)

https://webrtc.googlesource.com/src.git/+log/823f9135f858..826738b78c6a

$ git log 823f9135f..826738b78 --date=short --no-merges --format='%ad %ae %s'

Created with:
  roll-dep src/third_party/webrtc
BUG=chromium:None,chromium:None,chromium:None,chromium:None,chromium:None,chromium:840347,chromium:839860,chromium:838402


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

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=master.tryserver.chromium.linux:linux_chromium_archive_rel_ng;master.tryserver.chromium.mac:mac_chromium_archive_rel_ng
TBR=webrtc-chromium-sheriffs-robots@google.com

Change-Id: I555e922aefb9dac0e262c32fe8562a47f7a15144
Reviewed-on: https://chromium-review.googlesource.com/1049945
Reviewed-by: webrtc-chromium-autoroll <webrtc-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Commit-Queue: webrtc-chromium-autoroll <webrtc-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#556789}
[modify] https://crrev.com/ebc7dc2c2cccb2e04ed594fa68c0a1fb0d7a59e6/DEPS

This CL https://webrtc-review.googlesource.com/c/src/+/74923 is intended to be merged to M67. It is only a small part of the CL that landed on master (https://webrtc-review.googlesource.com/c/src/+/60260) that fixed this problem.

We are waiting for the CL on master to reach canary before we land the CL in 67.
Pls update the bug with canary result.
NextAction: 2018-05-09
The NextAction date has arrived: 2018-05-09
Don't really have an update yet. Still waiting for the CL (https://chromiumdash.appspot.com/commit/156436056ec1d9cd56ae3d7bce29b356b404f9ca) to reach canary.


NextAction: 2018-05-10
Pls update the bug with canary result tomorrow. Thank you.
NextAction: 2018-05-11
I will be OoO tomorrow but back on friday, I will update the bug then.
The NextAction date has arrived: 2018-05-11
The CL was first part of 68.0.3425.0, which was released 9th of May on android/mac/windows (not linux). Canary results look good.
thanks philipel@

govind@ - good for 67
Labels: -Merge-Review-67 Merge-Approved-67
Approving merge to M67 branch 3396 based on comment #30 and #31. Assuming only cl listed at #14 need a merge. Pls merge ASAP. Thank you.
Labels: -Merge-Approved-67 Merge-Merged
This CL has now been merged: https://webrtc-review.googlesource.com/c/src/+/74923
Labels: -Merge-Merged merge-merged-67
Reverted the merge since it broke the build.
Sorry for messing up the merge :(

The updated CL has now landed: https://webrtc-review.googlesource.com/c/src/+/76480
Cc: huib@chromium.org
Labels: Release-0-M67
Labels: CVE-2018-6130 CVE_description-missing
Cc: anatolid@google.com
Project Member

Comment 42 by sheriffbot@chromium.org, Aug 9

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

Sign in to add a comment