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

Issue 679306: WebRTC crash (?) on appear.in

Reported by philipp....@googlemail.com, Jan 9 2017 Project Member

Issue description

Chrome Version: 57.0.2970.0
Operating System: Linux 4.4.0-57-generic

Hard to reproduce. I was trying to hunt down simulcast issues where when rewriting packets into a single stream which lead to video corruption. And close to the point where I suspected this happens it crashed.


****DO NOT CHANGE BELOW THIS LINE****
Crash ID: crash/d31282d080000000
 

Comment 1 by philipp....@googlemail.com, Jan 9 2017

another one:
Crash ID: crash/0e6e5e2480000000

Comment 2 by philipp....@googlemail.com, Jan 9 2017

and another one. Not even close to the point where I suspected this happened (which was rtp sequence number wraparound)

Crash ID: crash/099622d080000000

Comment 3 by ajha@chromium.org, Jan 10 2017

Components: Blink>WebRTC>Video
Labels: Needs-Triage-M57 OS-Linux

Comment 5 by bugdroid1@chromium.org, Jan 31 2017

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

commit 1c056254a6b8b6521ad2799c8c705b8bbfb9c210
Author: philipel <philipel@webrtc.org>
Date: Tue Jan 31 17:53:12 2017

Fix race condition in FrameBuffer2

If the frame buffer is cleared while the decoding thread is waiting to acquire
the lock in order to return the |next_frame_it| will be invalidated.

BUG= chromium:679306 

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

[modify] https://crrev.com/1c056254a6b8b6521ad2799c8c705b8bbfb9c210/webrtc/modules/video_coding/frame_buffer2.cc
[modify] https://crrev.com/1c056254a6b8b6521ad2799c8c705b8bbfb9c210/webrtc/modules/video_coding/frame_buffer2.h

Comment 6 by philipel@chromium.org, Feb 1 2017

The fix in #5 fix only one bug that caused webrtc::video_coding::FrameBuffer::NextFrame to crash, but there is another bug that also cause crashes in this function.

Comment 7 by philipel@chromium.org, Apr 19 2017

Cc: philipel@chromium.org rsesek@chromium.org
 Issue 708074  has been merged into this issue.

Comment 8 by philipel@chromium.org, Apr 19 2017

Copy pasting from a duplicate:

I have found what is causing the crash, and it is due to the FrameBuffer2 not handling large jumps in picture id correctly.

For example when receiving these frames in a stream:
5453, 5454, 15670, 29804, 29805, 29806, 33819, 41248

Then the order will be considered to be:
5453 < 5454 < 15670 < 29804 < 29805 < 29806 < 33819

But when inserting 41248 it is both 33819 < 41248 and 41248 < 5453 due to the picture id wrapping at 2^16. This cause problems in the NextFrame function when we want to iterate over all continuous frames to find the best frame to return next.

Comment 9 by philipp....@googlemail.com, Apr 19 2017

\o/

the typical case where that happens is when simulcast is used -- I don't think SFUs will rewrite the picture id. Can you clear the buffer when receiving a keyframe?

Comment 10 by holmer@chromium.org, Apr 20 2017

Cc: holmer@chromium.org
Labels: -OS-Linux -Needs-Triage-M57 ReleaseBlock-Stable M-58 OS-All

Comment 11 by holmer@chromium.org, Apr 20 2017

Labels: Pri-1 Type-Bug-Security

Comment 12 by philipel@chromium.org, Apr 20 2017

Since this bug can cause us to dereference non-nullptrs we consider this to be a security risk.

Fix here: https://codereview.chromium.org/2830723002/

Comment 13 by bugdroid1@chromium.org, Apr 20 2017

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

commit 146a48b0fabef7e74f2a2b62fb5cb45f9b393408
Author: philipel <philipel@webrtc.org>
Date: Thu Apr 20 11:04:38 2017

Check if the order of frames becomes ambiguous if we were to insert the incoming frame, and if so, clear the FrameBuffer.

BUG= chromium:679306 

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

[modify] https://crrev.com/146a48b0fabef7e74f2a2b62fb5cb45f9b393408/webrtc/modules/video_coding/frame_buffer2.cc
[modify] https://crrev.com/146a48b0fabef7e74f2a2b62fb5cb45f9b393408/webrtc/modules/video_coding/frame_buffer2_unittest.cc

Comment 14 by philipel@chromium.org, Apr 20 2017

Cc: tommi@chromium.org

Comment 15 by gov...@chromium.org, Apr 20 2017

Cc: awhalley@chromium.org

Comment 16 by mea...@chromium.org, Apr 20 2017

Labels: Security_Impact-Stable Security_Severity-High
> Since this bug can cause us to dereference non-nullptrs we consider this to be a security risk.

In this case I'm marking this as a high severity bug as the security sheriff.

philipel: Can you please confirm that this bug can be triggered by an attacker supplied stream?

Comment 17 by gov...@chromium.org, Apr 20 2017

Cc: ligim...@chromium.org

Comment 18 by philipel@chromium.org, Apr 20 2017

#16
An attacker can craft a malicious stream which will cause this crash. 

What happens is that we iterate past the end of the map containing all frames, so the pointer we dereference is whatever the invalid iterator was pointing to.

I'm not sure if the attacker can decide which memory address that will be dereferences, but there might be some clever way.

Comment 19 by mea...@chromium.org, Apr 20 2017

#18: Thanks for confirming!

Comment 20 by philipp....@googlemail.com, Apr 20 2017

the effort to reliably trigger it is quite high, it would take me a week of effort to create the preconditions.

meacer@ (or someone else): if you could view-restrict 
 https://bugs.chromium.org/p/chromium/issues/detail?id=708074 that would be good as it contains information that would allow a targeted reproduction (to someone who knows webrtc and vp8 sufficiently well... that is an extremly limited audience)

Comment 21 by awhalley@google.com, Apr 20 2017

Done.

Comment 22 by tommi@chromium.org, Apr 21 2017

Cc: deadbeef@chromium.org

Comment 23 by philipel@chromium.org, Apr 21 2017

Labels: Merge-Request-58 Merge-Request-59
We still don't have a canary version with the fix in #13 included, but it should be built later today.

Comment 24 by sheriffbot@chromium.org, Apr 21 2017

Project Member
Labels: -Merge-Request-59 Hotlist-Merge-Approved Merge-Approved-59
Your change meets the bar and is auto-approved for M59. Please go ahead and merge the CL to branch 3071 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), gkihumba@(ChromeOS), Abdul Syed@(Desktop)

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

Comment 25 by sheriffbot@chromium.org, Apr 21 2017

Project Member
Labels: -Merge-Request-58 Merge-Review-58 Hotlist-Merge-Review
This bug requires manual review: Only 3 days from stable, we might already have a stable candidate build
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop)

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

Comment 26 by sheriffbot@chromium.org, Apr 21 2017

Project Member
Labels: Deadline-Exceeded
We commit ourselves to a 60 day deadline for fixing for high severity vulnerabilities, and have exceeded it here. If you're unable to look into this soon, could you please find another owner or remove yourself so that this gets back into the security triage queue?

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

Comment 27 by sheriffbot@chromium.org, Apr 21 2017

Project Member
Status: Fixed (was: Assigned)
Please mark security bugs as fixed as soon as the fix lands, and before requesting merges. This update is based on the merge- labels applied to this issue. Please reopen if this update was incorrect.

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

Comment 28 by tommi@chromium.org, Apr 21 2017

Cc: blum@chromium.org

Comment 29 by philipel@chromium.org, Apr 21 2017

I have tested canary version 60.0.3077.0 on mac and I can no longer reproduce the crash.

Comment 30 by ligim...@chromium.org, Apr 21 2017

For Mac in stable channel-58.0.3029.81 this crash ranks #5, 45 reports with 41 unique clients.

Comment 31 by gov...@chromium.org, Apr 21 2017

Cc: abdulsyed@chromium.org amineer@chromium.org
+ awhalley@ for M58 merge review.

Comment 32 by gov...@chromium.org, Apr 21 2017

Please merge your change to M59 branch #3071 latest before 4:00 PM PT, Monday (04/24) so we can take it for next week last M59 dev release. Thank you.

Comment 33 by sheriffbot@chromium.org, Apr 22 2017

Project Member
Labels: Restrict-View-SecurityNotify

Comment 34 by bugdroid1@chromium.org, Apr 24 2017

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

commit 6d56d2ebf0e40bc73dee99093ac7c223ddc7b6e5
Author: philipel <philipel@webrtc.org>
Date: Mon Apr 24 08:56:55 2017

Check if the order of frames becomes ambiguous if we were to insert the incoming frame, and if so, clear the FrameBuffer.

BUG= chromium:679306 

Review-Url: https://codereview.webrtc.org/2830723002
Cr-Commit-Position: refs/heads/master@{#17785}
(cherry picked from commit 146a48b0fabef7e74f2a2b62fb5cb45f9b393408)

R=stefan@webrtc.org

Review-Url: https://codereview.webrtc.org/2835963002 .
Cr-Commit-Position: refs/branch-heads/59@{#4}
Cr-Branched-From: 10d095d4f743bc16f8e486e156c48a6d023b32c5-refs/heads/master@{#17657}

[modify] https://crrev.com/6d56d2ebf0e40bc73dee99093ac7c223ddc7b6e5/webrtc/modules/video_coding/frame_buffer2.cc
[modify] https://crrev.com/6d56d2ebf0e40bc73dee99093ac7c223ddc7b6e5/webrtc/modules/video_coding/frame_buffer2_unittest.cc

Comment 35 by sheriffbot@chromium.org, Apr 24 2017

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 36 by gov...@chromium.org, Apr 24 2017

Labels: -Merge-Approved-59
Per comment #34, this is already merged to M59. Hence, removing "Merge-Approved-59" label. Thank you.

Comment 37 by awhalley@google.com, Apr 24 2017

Labels: -ReleaseBlock-Stable

Comment 38 by awhalley@chromium.org, Apr 24 2017

Labels: reward-topanel

Comment 39 by kjellander@chromium.org, Apr 25 2017

Cc: anatolid@chromium.org

Comment 40 by anatolid@chromium.org, Apr 25 2017

awhalley@ did you have the chance to review the merge for m58 (requested in #23)?

Comment 41 by anatolid@chromium.org, Apr 25 2017

Cc: huib@chromium.org

Comment 42 by awhalley@chromium.org, Apr 27 2017

anatolid@, govind@ - good for 58

Comment 43 by gov...@chromium.org, Apr 27 2017

Per comment #37, this is NOT a Stable blocker. 
Is it absolutely necessary to take this merge in for next M58 refresh and will be a fully safe merge? We can't take any risk at all as M58 is already in stable currently rolling out to 50% of users on Windows & Mac, 100% Linux.

Comment 44 by awhalley@chromium.org, Apr 28 2017

Labels: -reward-topanel reward-unpaid reward-500

Comment 45 by awhalley@chromium.org, Apr 28 2017

Nice one! The VRP panel decided to award $500 for this report. A member of our finance team will be in touch shortly to arrange payment.

*** Boilerplate reminders! ***
Please do NOT publicly disclose details until a fix has been released to all our users. Early public disclosure may cancel the provisional reward. Also, please be considerate about disclosure when the bug affects a core library that may be used by other products. Please do NOT share this information with third parties who are not directly involved in fixing the bug. Doing so may cancel the provisional reward. Please be honest if you have already disclosed anything publicly or to third parties. Lastly, we understand that some of you are not interested in money. We offer the option to donate your reward to an established charity. If you prefer this option, let us know and we will also match your donation - subject to our discretion. Any rewards that are unclaimed after 12 months will be donated to a charity of our choosing.
*********************************

Comment 46 by awhalley@chromium.org, Apr 28 2017

Labels: -reward-unpaid reward-inprocess

Comment 47 by abdulsyed@chromium.org, May 1 2017

Labels: -Merge-Review-58 Merge-Approved-58
Based on comments by holmer@ and confirmation that this is a safe merge from both developer and security team, approving this merge for M58.

Comment 48 by bugdroid1@chromium.org, May 1 2017

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

commit 6c9caaad7dfdea48a72a691d760ea0d89cb64816
Author: Zijie He <zijiehe@chromium.org>
Date: Mon May 01 17:48:01 2017

Check if the order of frames becomes ambiguous if we were to insert the incoming frame, and if so, clear the FrameBuffer.

On behalf of Philip Eliasson (philipel@webrtc.org).

BUG= chromium:679306 

Review-Url: https://codereview.webrtc.org/2830723002
Cr-Commit-Position: refs/heads/master@{#17785}
(cherry picked from commit 146a48b0fabef7e74f2a2b62fb5cb45f9b393408)

Review-Url: https://codereview.webrtc.org/2848283002 .
Cr-Commit-Position: refs/branch-heads/58@{#19}
Cr-Branched-From: f31969a584bcafe9406c214a9d4c3afb49d19650-refs/heads/master@{#16937}

[modify] https://crrev.com/6c9caaad7dfdea48a72a691d760ea0d89cb64816/webrtc/modules/video_coding/frame_buffer2.cc
[modify] https://crrev.com/6c9caaad7dfdea48a72a691d760ea0d89cb64816/webrtc/modules/video_coding/frame_buffer2_unittest.cc

Comment 49 by gov...@chromium.org, May 1 2017

Cc: zijiehe@chromium.org
Labels: -Merge-Approved-58
Per comment #48, this is already merged to M58. Hence removing "Merge-Approved-58" label. Thank you zijiehe@ for merging the change to M58.

Comment 50 by awhalley@chromium.org, May 1 2017

Labels: Release-1-M58

Comment 51 by awhalley@chromium.org, May 2 2017

Labels: CVE-2017-5068

Comment 52 by amineer@chromium.org, May 2 2017

Cc: niklase@chromium.org

Comment 53 by huib@chromium.org, May 4 2017

Cc: juberti@chromium.org

Comment 54 by philipel@chromium.org, May 5 2017

Cc: terelius@chromium.org

Comment 55 by palmer@google.com, May 9 2017

Labels: -Restrict-View-EditIssue

Comment 56 by juberti@chromium.org, May 10 2017

Cc: emadomara@google.com

Comment 57 by bugdroid1@chromium.org, May 14 2017

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

commit e95b78bd8da556fe85d1603f6ef5558e50c81189
Author: tommi <tommi@webrtc.org>
Date: Sun May 14 14:23:11 2017

Add a couple of checks to FrameBuffer while we're continuing to look at RtpFrameReferenceFinder.

BUG= chromium:679306 
TBR=terelius@webrtc.org, philipel@webrtc.org

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

[modify] https://crrev.com/e95b78bd8da556fe85d1603f6ef5558e50c81189/webrtc/modules/video_coding/frame_buffer2.cc

Comment 58 by bugdroid1@chromium.org, May 18 2017

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

commit 7daab660ce0e35fecad717fefab4cf935d3c253e
Author: Stefan Holmer <stefan@webrtc.org>
Date: Thu May 18 15:20:13 2017

Add a couple of checks to FrameBuffer while we're continuing to look at RtpFrameReferenceFinder.

BUG= chromium:679306 
R=philipel@webrtc.org
TBR=terelius@webrtc.org

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

Review-Url: https://codereview.webrtc.org/2891073002 .
Cr-Commit-Position: refs/branch-heads/59@{#11}
Cr-Branched-From: 10d095d4f743bc16f8e486e156c48a6d023b32c5-refs/heads/master@{#17657}

[modify] https://crrev.com/7daab660ce0e35fecad717fefab4cf935d3c253e/webrtc/modules/video_coding/frame_buffer2.cc

Comment 59 by sheriffbot@chromium.org, Jul 28 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 60 by awhalley@chromium.org, Apr 25 2018

Labels: CVE_description-submitted

Sign in to add a comment