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
link

Issue 759457: 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.

Comment 8 by monor...@bugs.chromium.org, Sep 29 2017

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.

Comment 10 by reillyg@chromium.org, Oct 4 2017

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.

Comment 11 by reillyg@chromium.org, Oct 4 2017

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.

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

Project Member
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

Comment 13 by reillyg@chromium.org, Oct 6 2017

Labels: OS-Android OS-Chrome OS-Mac OS-Windows
Status: Fixed (was: Started)

Comment 14 by dcheng@chromium.org, Oct 6 2017

Labels: -Type-Bug-Regression Type-Bug-Security

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

Project Member
Labels: Restrict-View-SecurityNotify

Comment 16 by elawrence@chromium.org, Oct 8 2017

Labels: M-62 Security_Impact-Stable
AFAICT, this is at least Sev_Medium, and thus a candidate for merging to M-62.

Comment 17 by reillyg@chromium.org, Oct 9 2017

Not seeing any crashes that appear to be related to this on Canary.

Comment 18 by reillyg@chromium.org, Oct 9 2017

Labels: Merge-Request-62

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

Project Member
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

Comment 20 by abdulsyed@chromium.org, Oct 10 2017

Cc: awhalley@chromium.org
+awhalley for security review.

Comment 21 by awhalley@google.com, Oct 10 2017

abdulsyed@ - good for 62

Comment 22 by abdulsyed@chromium.org, Oct 10 2017

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.

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

Project Member
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

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

Project Member
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

Comment 25 by awhalley@google.com, Oct 16 2017

Labels: Release-0-M62

Comment 26 by awhalley@chromium.org, Oct 18 2017

Labels: CVE-2017-15395

Comment 27 by awhalley@google.com, Nov 14 2017

Labels: reward-topanel

Comment 28 by awhalley@chromium.org, Nov 16 2017

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.
*********************************

Comment 29 by awhalley@google.com, Nov 16 2017

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.

Comment 30 by awhalley@chromium.org, Nov 16 2017

Labels: -reward-unpaid reward-inprocess

Comment 31 by johber...@gmail.com, Nov 21 2017

Thank you and your team, very much appreciated! :)

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

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 33 by awhalley@chromium.org, Apr 25 2018

Labels: CVE_description-submitted

Comment 34 by sheriffbot@chromium.org, Jul 28 2018

Project Member
Labels: -Pri-2 Pri-1

Sign in to add a comment