1317501 - MediaStreamGraph processes runnables at unsafe time

Status: RESOLVED FIXED

(Core :: Audio/Video: MediaStreamGraph, defect, P1)

Description

[Olli Pettay [:smaug][[e-mail]]]

http://searchfox.org/mozilla-central/source/dom/media/MediaStreamGraph.cpp#1767-1768 is called when we're in stable state. That should not run scripts, but
http://searchfox.org/mozilla-central/rev/886d5186f0598ab2f8a9953eb5a4dab9750ef834/dom/media/DOMMediaStream.cpp#222 ends up there, meaning that we dispatch DOM events
http://searchfox.org/mozilla-central/rev/886d5186f0598ab2f8a9953eb5a4dab9750ef834/dom/media/MediaStreamTrack.cpp#459

I noticed this while changing Promise scheduling to follow the spec.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f3850fed17768eee8fc5fec4b5a47970ece6a640&selectedJob=31021145 [mda] shows the assertion.

Comment 1

[Paul Adenot (:padenot)]

Maybe we can simply queue another task to fire this event? Andreas, want to have a look? Looks like this is in code you wrote more or less recently.

Comment 2

[Andreas Pehrson [:pehrsons]]

Attachment: Don't notify dom objects synchronously from MSG runnables in DOMMediaStream

Comment 3

[Andreas Pehrson [:pehrsons]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6cc02d739f661a0d298765993b60aaf6f34922cd

Comment 4

[Paul Adenot (:padenot)]

Comment on attachment 8813088 [details] [diff] [review]
Don't notify dom objects synchronously from MSG runnables in DOMMediaStream

Review of attachment 8813088 [details] [diff] [review]:
-----------------------------------------------------------------

The try looks broken.

Comment 5

[Andreas Pehrson [:pehrsons]]

Attachment: Don't test media element flow for tracks that were not expected

We were failing on try when testing media element flow for a track that was no longer sent. We don't have checks that it ends properly either but I'll set up another bug for that.

Comment 6

[Andreas Pehrson [:pehrsons]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe594ee41a009836377555e93bb98cc913ec3f78

Comment 7

[Andreas Pehrson [:pehrsons]]

Given that smaug is on vacation, bz, do you know what the implications of this bug are? (for sec-approval)

Comment 8

[Boris Zbarsky [:bzbarsky]]

I'm not actually sure.  It can certainly cause event mis-ordering due to the event loop spinning when it should not.  But I don't know the extent to which this is a security problem, or whether there are other ones...

Comment 9

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8813088 [details] [diff] [review]
Don't notify dom objects synchronously from MSG runnables in DOMMediaStream

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Unclear, see comment 8.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No.

Which older supported branches are affected by this flaw?
50 and onwards.

If not all supported branches, which bug introduced the flaw?
Bug 1208373

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
It doesn't go clean on beta and release. It should be low risk to apply the same principals of an extra dispatch there too.

How likely is this patch to cause regressions; how much testing does it need?
Not likely. Automation coverage is good.

Comment 10

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8813246 [details] [diff] [review]
Don't test media element flow for tracks that were not expected

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Unclear, see comment 8.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No.

Which older supported branches are affected by this flaw?
50 and onwards.

If not all supported branches, which bug introduced the flaw?
Bug 1208373

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
This patch should go clean on all branches.

How likely is this patch to cause regressions; how much testing does it need?
Not likely. Automation coverage is good.

Comment 11

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8813088 [details] [diff] [review]
Don't notify dom objects synchronously from MSG runnables in DOMMediaStream

Giving sec-approval+ for the PATCH only. The test can't go in until we ship the fix in a public release. You should nominate Aurora and Beta patches as well.

I'm clearing sec-approval on the test as it is just going to clog up the sec-approval queue and sit around for over a month. I suggest setting in-testsuite? or similar.

Comment 12

[Andreas Pehrson [:pehrsons]]

I see now that I should have been clearer when answering the first question in the request for the test patch. Sorry for the hassle.

I understand the general concern around tests for sec issues, but this test change actually fixes fallout in automation unrelated to the flaw in this bug, and is required for the fix to land without causing oranges.

Can we re-evaluate the test patch or should I maybe land it in a separate bug, seeing that it's unrelated to the sec issue?

Comment 13

[Al Billings [:abillings - ex-MoCo]]

Yes, that's fine. Just put it in a separate bug so it doesn't get flagged by any malicious watchers as a security checkin.

Comment 14

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8813246 [details] [diff] [review]
Don't test media element flow for tracks that were not expected

See bug 1321609 for this patch.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3bb432e68db4e89ce6b5e11f022cfe3ca04fd751

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3bb432e68db4

Comment 17

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8813088 [details] [diff] [review]
Don't notify dom objects synchronously from MSG runnables in DOMMediaStream

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1208373
[User impact if declined]: Unclear, see comment 8.
[Is this code covered by automated tests?]: It will be when smaug has finished his work on promise scheduling.
[Has the fix been verified in Nightly?]: No, but it's simple enough and shouldn't lead to anything unexpected.
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: We did see timing related fallout on Nightly. If that appears again, bug 1321609 might have to be uplifted.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: There's a slight risk that some events change order, but they shouldn't be related to each other.
[String changes made/needed]: None.

Comment 18

[Andreas Pehrson [:pehrsons]]

smaug, can you comment on the security aspects of this bug? How far would it make sense to uplift this?

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Looking at the stable state handling code, it seems like calling js or processing runnables during stable state shouldn't crash, since CycleCollectedJSContext::ProcessStableStateQueue() goes through the array in a safe way. But ordering of various things could be wrong.

So, I think this is more about correctness than security after all.

Comment 20

[Gerry Chang [:gchang]]

Comment on attachment 8813088 [details] [diff] [review]
Don't notify dom objects synchronously from MSG runnables in DOMMediaStream

Fix a sec-high. Beta51+ & Aurora52+. Should be in 51 beta 9.

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/242d8710b25a

Comment 22

[Carsten Book [:Tomcat]]

needs rebasing for beta

Comment 23

[Andreas Pehrson [:pehrsons]]

Aurora backed out due to mda failures: https://treeherder.mozilla.org/logviewer.html#?job_id=4481921&repo=mozilla-aurora

This is the same intermittent that we saw on m-c, bug 1321609.

Comment 24

[Andreas Pehrson [:pehrsons]]

Attachment: [beta] Don't notify dom objects synchronously from MSG runnables in DOMMediaStream

Rebased for beta. This is just half the original patch with the affected method under a different name.

Comment 25

[Andreas Pehrson [:pehrsons]]

We need to wait for aurora uplift of bug 1321609 before uplifting this again, but I think beta can go as is per bug 1321609 comment 5.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/daf432f7c84e
https://hg.mozilla.org/releases/mozilla-beta/rev/cb96747de8e2