New issue
Advanced search Search tips
Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 1
Type: Bug-Security



Sign in to add a comment

MediaStreamTrack.applyConstraints will crash the tab if executed in quick succession

Reported by johber...@gmail.com, Aug 28 2017

Issue description

UserAgent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/61.0.3163.59 Safari/537.36

Steps to reproduce the problem:
1. Use navigator.mediaDevices.getUserMedia to acquire a MediaStream and MediaStreamTrack.
2. Add the MediaStream to an HTML video element and delay for a short while, around 2.5 seconds.
3. execute MediaStreamTrack.applyConstraints({ advanced: [{ brightness: value }] }) *1000 times in quick succession

* in reality it has crashed in as little as 50 times, but a 1000 times should crash it most of the time.

What is the expected behavior?
The expected behavior is for the constraint to update without crashing the tab (Or Chrome App).

What went wrong?
If run locally in a tab, an 'Aw, Snap!' message will appear and DevTools will disconnect. If run in a Chrome App, the app will crash.

I've attached a screenshot of this.

Did this work before? Yes 60

Does this work in other browsers? N/A

Chrome version: 61.0.3163.59  Channel: beta
OS Version: 17.04
Flash Version: 

I've tried this on Ubuntu Linux 17.04 and Chrome OS and it is the same on both operating systems. It works fine on Chrome 60 and previous versions but crashes on Chrome 61.
 
constraints-test-case.zip
110 KB Download

Comment 1 by guidou@chromium.org, Aug 28 2017

Components: Blink>ImageCapture

Comment 2 by guidou@chromium.org, Aug 28 2017

Cc: emir...@chromium.org
Owner: mcasas@chromium.org
I bisected this and tracked the problem to the following range:
https://chromium.googlesource.com/chromium/src/+log/470096afdb28947722a3ecb946a4677f6ee9fa59..210bf791475500ca214d8111cda81da94bfd79ab

Suspect change is r480447. Assigning to mcasas@ for further investigation.

Comment 3 by guidou@chromium.org, Aug 28 2017

Status: Assigned (was: Unconfirmed)

Comment 4 by mcasas@chromium.org, Sep 18 2017

Cc: mcasas@chromium.org
Labels: Needs-Feedback
NextAction: 2017-09-29
johberlvi@ would you navigate to chrome://crashes after crashing and
let us know the crash id? Thanks

Comment 5 Deleted

Comment 6 by johber...@gmail.com, Sep 20 2017

I posted it and it got deleted, I'll try again:

Crash ID: crash/1c8df9715db93296

Comment 7 by mcasas@chromium.org, Sep 25 2017

Owner: guidou@chromium.org
guidou@ could you PTAL? Seems to be a generic problem with the
autogenerated MediaTrack Constraint Set.
The NextAction date has arrived: 2017-09-29

Comment 9 by guidou@chromium.org, Sep 29 2017

Cc: guidou@chromium.org
Components: -Blink>GetUserMedia
Owner: reillyg@chromium.org
The problem seems to be a null-pointer dereference when invoking |resolve_function| in OnMojoGetPhotoState(), at https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp?rcl=b6b9e4f8137ec1c2d1fd91df539de2746f642b81&l=691

I guess some bound argument becomes null.

I get this trace:
#0 0x7f15f3cbee57 base::debug::StackTrace::StackTrace()
#1 0x7f15f3cbe92f base::debug::(anonymous namespace)::StackDumpSignalHandler()
#2 0x7f15f3e44330 <unknown>
#3 0x7f15ea883f0a WTF::String::String()
#4 0x7f15eaa84bac blink::StringOrStringSequence::StringOrStringSequence()
#5 0x7f15eaa90a25 blink::ConstrainDOMStringParameters::ConstrainDOMStringParameters()
#6 0x7f15eaa8566c blink::StringOrStringSequenceOrConstrainDOMStringParameters::StringOrStringSequenceOrConstrainDOMStringParameters()
#7 0x7f15eaa9218c blink::MediaTrackConstraintSet::MediaTrackConstraintSet()
#8 0x7f15eaa92d4b blink::MediaTrackConstraints::MediaTrackConstraints()
#9 0x7f15eac55427 _ZN4base8internal7InvokerINS0_9BindStateIMN5blink12ImageCaptureEFvNS3_21MediaTrackConstraintsEPNS3_21ScriptPromiseResolverEEJNS3_10PersistentIS4_EES5_EEEFvS7_EE3RunEPNS0_13BindStateBaseEOS7_
#10 0x7f15eac4cec8 blink::ImageCapture::OnMojoGetPhotoState()


Assigning to reillyg@, who is an owner of ImageCapture for further investigation.
Labels: -Needs-Feedback Stability-Crash
NextAction: ----
The issue is that in ImageCapture::SetMediaTrackConstraints a MediaTrackConstraints is bound as an argument to a WTF::Function. Bound arguments don't get properly traced during garbage collection so its internal HeapVector<MediaTrackConstraintSet> is freed leading to a use-after-free when the Promise is eventually resolved.
Status: Started (was: Assigned)
A fix is available in this patch: https://chromium-review.googlesource.com/c/chromium/src/+/701358

I have filed issue 771781 to track adding additional checks to prevent this class of issue in the future.
Project Member

Comment 12 by bugdroid1@chromium.org, Oct 6 2017

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

commit 84ca1ee18bbc32f3cb035d071e8271e064dfd4d7
Author: Reilly Grant <reillyg@chromium.org>
Date: Fri Oct 06 21:11:22 2017

Convert MediaTrackConstraints to a ScriptValue

IDLDictionaries such as MediaTrackConstraints should not be stored on
the heap which would happen when binding one as a parameter to a
callback. This change converts the object to a ScriptValue ahead of
time. This is fine because the value will be passed to a
ScriptPromiseResolver that will converted it to a V8 value if it
isn't already.

Bug:  759457 
Change-Id: I3009a0f7711cc264aeaae07a36c18a6db8c915c8
Reviewed-on: https://chromium-review.googlesource.com/701358
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#507177}
[modify] https://crrev.com/84ca1ee18bbc32f3cb035d071e8271e064dfd4d7/third_party/WebKit/LayoutTests/imagecapture/MediaStreamTrack-applyConstraints.html
[modify] https://crrev.com/84ca1ee18bbc32f3cb035d071e8271e064dfd4d7/third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp
[modify] https://crrev.com/84ca1ee18bbc32f3cb035d071e8271e064dfd4d7/third_party/WebKit/Source/modules/imagecapture/ImageCapture.h

Labels: OS-Android OS-Chrome OS-Mac OS-Windows
Status: Fixed (was: Started)
Labels: -Type-Bug-Regression Type-Bug-Security
Project Member

Comment 15 by sheriffbot@chromium.org, Oct 7 2017

Labels: Restrict-View-SecurityNotify
Labels: M-62 Security_Impact-Stable
AFAICT, this is at least Sev_Medium, and thus a candidate for merging to M-62.
Not seeing any crashes that appear to be related to this on Canary.
Labels: Merge-Request-62
Project Member

Comment 19 by sheriffbot@chromium.org, Oct 9 2017

Labels: -Merge-Request-62 Merge-Review-62 Hotlist-Merge-Review
This bug requires manual review: We are only 7 days from stable.
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Cc: awhalley@chromium.org
+awhalley for security review.
abdulsyed@ - good for 62
Labels: -Merge-Review-62 Merge-Approved-62
Approving merge to M62. Branch:3202. Please merge before 4PM PDT to ensure it makes it to M62 Beta release candidate for tomorrow. 
Project Member

Comment 23 by bugdroid1@chromium.org, Oct 10 2017

Labels: -merge-approved-62 merge-merged-3202
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/19765bd888949485b1e7ee2892b6502b1a653025

commit 19765bd888949485b1e7ee2892b6502b1a653025
Author: Reilly Grant <reillyg@chromium.org>
Date: Tue Oct 10 20:27:35 2017

Convert MediaTrackConstraints to a ScriptValue

IDLDictionaries such as MediaTrackConstraints should not be stored on
the heap which would happen when binding one as a parameter to a
callback. This change converts the object to a ScriptValue ahead of
time. This is fine because the value will be passed to a
ScriptPromiseResolver that will converted it to a V8 value if it
isn't already.

Bug:  759457 
Change-Id: I3009a0f7711cc264aeaae07a36c18a6db8c915c8
Reviewed-on: https://chromium-review.googlesource.com/701358
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Reilly Grant <reillyg@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#507177}(cherry picked from commit 84ca1ee18bbc32f3cb035d071e8271e064dfd4d7)
Reviewed-on: https://chromium-review.googlesource.com/710173
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/branch-heads/3202@{#642}
Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098}
[modify] https://crrev.com/19765bd888949485b1e7ee2892b6502b1a653025/third_party/WebKit/LayoutTests/imagecapture/MediaStreamTrack-applyConstraints.html
[modify] https://crrev.com/19765bd888949485b1e7ee2892b6502b1a653025/third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp
[modify] https://crrev.com/19765bd888949485b1e7ee2892b6502b1a653025/third_party/WebKit/Source/modules/imagecapture/ImageCapture.h

Project Member

Comment 24 by bugdroid1@chromium.org, Oct 10 2017

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

commit e1f5e579ed01070bc96cdc87da69147393a4a10a
Author: Reilly Grant <reillyg@chromium.org>
Date: Tue Oct 10 22:00:57 2017

[M-62] Restore forward declaration of MediaTrackConstraints

The cherry-pick in 19765bd888949485b1e7ee2892b6502b1a653025 removed the
forward declaration of MediaTrackConstraints from ImageCapture.h. This
was fine on head but this symbol was still used by another function
prototype on branch 3202.

Bug:  759457 
Change-Id: Iba689e0e8222cb742d7eb2a8762e4b555bc3a256
Reviewed-on: https://chromium-review.googlesource.com/710531
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/branch-heads/3202@{#649}
Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098}
[modify] https://crrev.com/e1f5e579ed01070bc96cdc87da69147393a4a10a/third_party/WebKit/Source/modules/imagecapture/ImageCapture.h

Labels: Release-0-M62
Labels: CVE-2017-15395
Labels: reward-topanel
Labels: -reward-topanel reward-unpaid reward-1000
*** 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 eligible 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.
*********************************
Labels: Security_Severity-High
Nice one! The Chrome VRP Panel decided to award $1,000 for this report! A member of our finance team will be in touch to arrange for payment.
Labels: -reward-unpaid reward-inprocess
Thank you and your team, very much appreciated! :)
Project Member

Comment 32 by sheriffbot@chromium.org, Jan 13 2018

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
Labels: CVE_description-submitted
Project Member

Comment 34 by sheriffbot@chromium.org, Jul 28

Labels: -Pri-2 Pri-1

Sign in to add a comment