MediaStreamGraph processes runnables at unsafe time

RESOLVED FIXED in Firefox 51

Status

()

defect
P1
normal
Rank:
15
RESOLVED FIXED
3 years ago
23 days ago

People

(Reporter: smaug, Assigned: pehrsons)

Tracking

({sec-high})

50 Branch
mozilla53
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?
qe-verify -

Firefox Tracking Flags

(firefox-esr45 unaffected, firefox50 wontfix, firefox51 fixed, firefox52 fixed, firefox53 fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main51+])

Attachments

(2 attachments, 1 obsolete attachment)

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.
Flags: needinfo?(pehrson)
(Assignee)

Updated

3 years ago
Assignee: nobody → pehrson
Status: NEW → ASSIGNED
Flags: needinfo?(pehrson)
Group: core-security → media-core-security
Rank: 15
Keywords: sec-high
Priority: -- → P1
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.
Attachment #8813088 - Flags: review?(padenot)
(Assignee)

Comment 5

2 years ago
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.
Attachment #8813246 - Flags: review?(drno)
(Assignee)

Updated

2 years ago
Attachment #8813088 - Flags: review?(padenot)
Attachment #8813246 - Flags: review?(drno) → review+
Attachment #8813088 - Flags: review?(padenot) → review+
(Assignee)

Comment 7

2 years ago
Given that smaug is on vacation, bz, do you know what the implications of this bug are? (for sec-approval)
Flags: needinfo?(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...
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 9

2 years ago
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.
Attachment #8813088 - Flags: sec-approval?
(Assignee)

Comment 10

2 years ago
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.
Attachment #8813246 - Flags: sec-approval?
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.
Attachment #8813088 - Flags: sec-approval? → sec-approval+
Attachment #8813246 - Flags: sec-approval?
Flags: in-testsuite?
(Assignee)

Comment 12

2 years ago
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?
Flags: needinfo?(abillings)
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.
Flags: needinfo?(abillings)
(Assignee)

Comment 14

2 years ago
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.
Attachment #8813246 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3bb432e68db4
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Group: media-core-security → core-security-release
(Assignee)

Comment 17

2 years ago
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.
Attachment #8813088 - Flags: approval-mozilla-beta?
Attachment #8813088 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 18

2 years ago
smaug, can you comment on the security aspects of this bug? How far would it make sense to uplift this?
Flags: needinfo?(bugs)
(Reporter)

Comment 19

2 years ago
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.
Flags: needinfo?(bugs)
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.
Attachment #8813088 - Flags: approval-mozilla-beta?
Attachment #8813088 - Flags: approval-mozilla-beta+
Attachment #8813088 - Flags: approval-mozilla-aurora?
Attachment #8813088 - Flags: approval-mozilla-aurora+
needs rebasing for beta
Flags: needinfo?(pehrson)
(Assignee)

Comment 23

2 years ago
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.
(Assignee)

Comment 24

2 years ago
Rebased for beta. This is just half the original patch with the affected method under a different name.
Flags: needinfo?(pehrson)
Attachment #8822146 - Flags: review+
(Assignee)

Comment 25

2 years ago
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.
(Assignee)

Updated

2 years ago
Whiteboard: [checkin-needed-aurora][checkin-needed-beta]
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main51+]
(Assignee)

Updated

2 years ago
Depends on: 1337653
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.