MediaRecorder's RegisterActivityObserver()/ UnRegisterActivityObserver() usage looks suspicious

RESOLVED FIXED in Firefox -esr45

Status

()

Core
Audio/Video
RESOLVED FIXED
11 months ago
9 months ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

({csectype-uaf, regression, sec-high})

50 Branch
mozilla53
csectype-uaf, regression, sec-high
Points:
---

Firefox Tracking Flags

(firefox-esr4550+ fixed, firefox50+ fixed, firefox51+ fixed, firefox52+ fixed, firefox53+ fixed)

Details

(Whiteboard: [adv-main50.1+][adv-esr45.6+])

Attachments

(2 attachments)

Nothing guarantees UnRegisterActivityObserver() ever succeeds to remove the registration since the current document of the window may have already changed.
I'll try to write a testcase.
Assignee: nobody → bugs
Regression from bug 969372.

I don't have testcase yet, but it would be something like:

var win = window.open("foobar.html");
new win.MediaRecorder(stream);
// trigger GC/CC synchronously
Created attachment 8811050 [details] [diff] [review]
v1

Haven't managed to test this yet, since mach refuses to run media tests on my machine :/

Updated

11 months ago
Group: core-security → media-core-security
Comment on attachment 8811050 [details] [diff] [review]
v1

Finally got media tests running locally.
Seems to pass them.
Attachment #8811050 - Flags: review?(bechen)

Comment 4

11 months ago
Comment on attachment 8811050 [details] [diff] [review]
v1

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

The patch looks great.
So the testcase scenario is that CC happened but GC never come?
Attachment #8811050 - Flags: review?(bechen) → review+
the testcase scenario is that we swap the initial about:blank document from inner window after creating MediaRecorder. (remember, window<-> document relationship is _not_ 1-1 relationship. One inner window may have 1 or 2 documents during its lifetime, and one document may have any number of inner windows).

Updated

11 months ago
Keywords: sec-audit
Comment on attachment 8811050 [details] [diff] [review]
v1

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
I'd say not too easily, but the patch does pinpoint where the issue is. Bug 1317409 is related.
The issue was found when reading code, but I think the issue is rather obvious, even if hard to actually crash the browser unsafely.
	
Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Commit message could be " Bug 1317805, ensure to update MediaRecorder when document's activity state changes, r=bechen"

Which older supported branches are affected by this flaw?
all

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
esr45 needs a different patch because of some renaming in the code. Patch coming real soon.

How likely is this patch to cause regressions; how much testing does it need?
Should be reasonable safe. If there is some weird memory management somewhere, adding a new strong reference could be an issue.
 
String or UUID changes made by this patch: 
NA
Attachment #8811050 - Flags: sec-approval?
Attachment #8811050 - Flags: approval-mozilla-esr45?
Attachment #8811050 - Flags: approval-mozilla-beta?
Attachment #8811050 - Flags: approval-mozilla-aurora?
Created attachment 8811861 [details] [diff] [review]
esr45
As far as I see this is sec-high or critical
This was marked as an audit because it was found by a code audit, I believe, with no specific examples of exploitability. 

I'll give sec-approval but I'm not sure we're going to take it in branches without more explanation as to why we'd need to bump it up the trains.

Updated

11 months ago
Attachment #8811050 - Flags: sec-approval? → sec-approval+
As far as I see, when the bug occurs, results are similar to Bug 1317409.
Would probably take couple of hours to write a crasher testcase.
(In reply to Olli Pettay [:smaug] (vacation Nov 19-26) from comment #8)
> As far as I see this is sec-high or critical

Thanks. As a member of the security team please feel free to correct these ratings when you think they're wrong. If you'd prefer us to make the final decision but want to "appeal" a rating a bug comment is likely going to be missed. Make the comment (we need something to look at) but then
 * ping us in #security ("please see https://bugzilla.mozilla.org/show_bug.cgi?id=1317805#c8")
 * mail security@mozilla.org
 * needinfo? Al or myself (or whoever replaces us in the future)
 * clear the sec- keyword to bump it back into triage (but then please leave
   a comment so we don't scratch our heads and then assume it was a mistake).

those are roughly in my order of preference but they're all about the same.
Blocks: 969372
status-firefox50: --- → affected
status-firefox51: --- → affected
status-firefox52: --- → affected
status-firefox53: --- → affected
status-firefox-esr45: --- → affected
tracking-firefox51: --- → +
tracking-firefox52: --- → +
tracking-firefox53: --- → +
tracking-firefox-esr45: --- → 51+
Keywords: sec-audit → csectype-uaf, regression, sec-high
I guess we should backport this then!

Updated

11 months ago
Attachment #8811050 - Flags: approval-mozilla-beta?
Attachment #8811050 - Flags: approval-mozilla-beta+
Attachment #8811050 - Flags: approval-mozilla-aurora?
Attachment #8811050 - Flags: approval-mozilla-aurora+

Comment 13

11 months ago
uplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/dd7ae5c51e8d
status-firefox52: affected → fixed
Target Milestone: --- → mozilla52
https://hg.mozilla.org/integration/mozilla-inbound/rev/c138e5675e8c09579bd06d0b5963ea255e5aa78a
https://hg.mozilla.org/mozilla-central/rev/c138e5675e8c
Status: NEW → RESOLVED
Last Resolved: 11 months ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: mozilla52 → mozilla53

Comment 16

11 months ago
uplift
https://hg.mozilla.org/releases/mozilla-beta/rev/cc60d25f18bc
status-firefox51: affected → fixed

Updated

11 months ago
Group: media-core-security → core-security-release
tracking-firefox50: --- → ?
tracking-firefox-esr45: 51+ → ?
Attachment #8811050 - Flags: approval-mozilla-release?

Updated

11 months ago
tracking-firefox50: ? → +
tracking-firefox-esr45: ? → 51+

Comment 17

11 months ago
Comment on attachment 8811050 [details] [diff] [review]
v1

Sec-high, meets the triage bar for inclusion in 50.1.0, ESR45.6
Attachment #8811050 - Flags: approval-mozilla-release?
Attachment #8811050 - Flags: approval-mozilla-release+
Attachment #8811050 - Flags: approval-mozilla-esr45?
Attachment #8811050 - Flags: approval-mozilla-esr45+
https://hg.mozilla.org/releases/mozilla-release/rev/a68998d0287f
status-firefox50: affected → fixed
https://hg.mozilla.org/releases/mozilla-esr45/rev/cde2a37100f5
status-firefox-esr45: affected → fixed

Updated

11 months ago
Whiteboard: [adv-main50.1+]

Updated

11 months ago
tracking-firefox-esr45: 51+ → 50+
Whiteboard: [adv-main50.1+] → [adv-main50.1+][adv-esr45.6+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.