Closed
Bug 1255298
Opened 8 years ago
Closed 8 years ago
ServiceWorkerRegistration.showNotification violates cross-compartment access invariants
Categories
(Core :: DOM: Service Workers, defect)
Core
DOM: Service Workers
Tracking
()
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
(Keywords: sec-high, Whiteboard: [post-critsmash-triage][adv-main46+] btpp-active)
Attachments
(3 files)
1.22 KB,
patch
|
Details | Diff | Splinter Review | |
7.44 KB,
patch
|
wchen
:
review+
lizzard
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
4.00 KB,
patch
|
wchen
:
review+
|
Details | Diff | Splinter Review |
Found this while auditing AutoJSAPI usage in the tree. I'm not sure whether this actually needs to be security-sensitive or whether it would be a safe crash in release. The basic violation is that we call JS_WriteStructuredClone with "cx" and "value" in different compartments. Bobby, do you know what the release situation there is? Anyway, the bug is that the refactoring done in bug 1114554 made it so the code that actually creates a notification in Notification::CreateAndShow uses an AutoJSAPI with a passed-in nsIGlobalObject to set up a JSContext. It then structured-clones the "data" passed in via the NotificationOptions dictionary. The problem is that the "data" in that dictionary is in the caller compartment (further up the callstack; all codepaths that land in Notification::CreateAndShow start out in binding code coming from JS). And the caller compartment need not match the compartment of the nsIGlobalObject we're using. I don't know enough about serviceworkers yet to write a minimal testcase (for which I just need the smallest possible serviceworker test that can reach Notification::CreateAndShow. But I did verify that I get nice fatal asserts if I hack up test_notification_get.html in the way shown in the attached diff and then run it. I would expect that calling showNotification over Xrays would likewise fail if it passed "data" in the NotificationOptions. I can probably fix the actual code here, but could use a hand with writing a test quickly.
Flags: needinfo?(bobbyholley)
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8728810 -
Flags: review?(wchen)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Comment 2•8 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #0) > Created attachment 8728805 [details] [diff] [review] > Test patch that triggers some nice fatal asserts when the test is run > > Found this while auditing AutoJSAPI usage in the tree. > > I'm not sure whether this actually needs to be security-sensitive or whether > it would be a safe crash in release. The basic violation is that we call > JS_WriteStructuredClone with "cx" and "value" in different compartments. > > Bobby, do you know what the release situation there is? I believe compartment mismatches are fatal on nightly only. Andrew can confirm.
Flags: needinfo?(bobbyholley) → needinfo?(continuation)
Comment 3•8 years ago
|
||
(In reply to Bobby Holley (busy) from comment #2) > I believe compartment mismatches are fatal on nightly only. Andrew can > confirm. They used to be fatal in Nightly and Aurora only, but I haven't seen any evidence of them for around a year, so they may not be fatal anywhere. But yes, these are still a sec issue.
Flags: needinfo?(continuation)
Keywords: sec-high
Updated•8 years ago
|
Attachment #8728810 -
Flags: review?(wchen) → review+
Assignee | ||
Comment 4•8 years ago
|
||
Comment on attachment 8728810 [details] [diff] [review] Just pass through the JSContext when passing through the NotificationOptions in notification code [Security approval request comment] How easily could an exploit be constructed based on the patch? Probably not that easily. The patch doesn't make it obvious that the problem is a cross-compartment access, for example. In fact, if this bug were not security-sensitive, this patch would not give off a "security fix" smell at all. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No, but that's because there are no tests. I'm about to upload a second patch that contains tests. Which older supported branches are affected by this flaw? I believe everything starting with Firefox 44 (when we enabled this API on beta/release) is affected. If not all supported branches, which bug introduced the flaw? Bug 1114554 (which introduced the functionality) or bug 1234054 (which exposed it in release). Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? I think this patch should apply to the various affected branches. If not, it would not be difficult to adapt. How likely is this patch to cause regressions; how much testing does it need? I don't know. I'm not an expert on this code. Note that I would _really_ like to land the tests for this so they don't get lost. I believe I can write the test in a way that doesn't make it smell like a test for a security bug and land it in a different (not sec-sensitive) bug. Would that be acceptable?
Attachment #8728810 -
Flags: sec-approval?
Assignee | ||
Comment 5•8 years ago
|
||
Attachment #8729214 -
Flags: review?(wchen)
Comment 6•8 years ago
|
||
sec-approval+ for trunk. We're going to want this for Beta, Aurora, and ESR45, I believe. Can you land tests under a not-security cover bug so they don't have attention drawn to them?
status-firefox45:
--- → wontfix
status-firefox46:
--- → affected
status-firefox47:
--- → affected
status-firefox-esr45:
--- → affected
tracking-firefox46:
--- → ?
tracking-firefox47:
--- → ?
tracking-firefox48:
--- → +
tracking-firefox-esr45:
--- → ?
Updated•8 years ago
|
Attachment #8728810 -
Flags: sec-approval? → sec-approval+
Updated•8 years ago
|
Attachment #8729214 -
Flags: review?(wchen) → review+
Comment 7•8 years ago
|
||
FYI, we've disabled service workers in 45ESR.
Assignee | ||
Comment 8•8 years ago
|
||
Hmm. ESR 45 still has pref("dom.webnotifications.serviceworker.enabled", true); in firefox.js. Is it just that you can't get your hands on a ServiceWorkerRegistration at all there?
Assignee | ||
Comment 9•8 years ago
|
||
Comment on attachment 8728810 [details] [diff] [review] Just pass through the JSContext when passing through the NotificationOptions in notification code Approval Request Comment [Feature/regressing bug #]: Bug 1114554 (which introduced the functionality) or bug 1234054 (which exposed it in release) [User impact if declined]: Possible to trigger crashes or other badness via invalid cross-compartment access. [Describe test coverage new/current, TreeHerder]: We have decent test coverage for service workers, actually. [Risks and why]: I can't think of obvious ones offhand, but I'm not an expert on this code. [String/UUID change made/needed]: None.
Attachment #8728810 -
Flags: approval-mozilla-beta?
Attachment #8728810 -
Flags: approval-mozilla-aurora?
Comment 10•8 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #8) > Hmm. ESR 45 still has pref("dom.webnotifications.serviceworker.enabled", > true); in firefox.js. Is it just that you can't get your hands on a > ServiceWorkerRegistration at all there? We set dom.serviceworkers.enabled to false which should make this evaluate to false: https://dxr.mozilla.org/mozilla-central/source/dom/webidl/ServiceWorkerRegistration.webidl#11
Assignee | ||
Comment 11•8 years ago
|
||
Right, but the normal way you get a ServiceWorkerRegistration is via some other API, not via its (nonexistent) constructor, right? I guess that would be off of ServiceWorkerContainer which is controlled by ServiceWorkerContainer::IsEnabled which checks that same pref. So we're good, ok.
Assignee | ||
Comment 12•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f59de9dba41
Assignee | ||
Comment 13•8 years ago
|
||
I landed the test in bug 1255692.
Updated•8 years ago
|
Whiteboard: btpp-active
Comment 14•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4f59de9dba41
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment 15•8 years ago
|
||
Comment on attachment 8728810 [details] [diff] [review] Just pass through the JSContext when passing through the NotificationOptions in notification code May fix some crashes, sec-high, let's take it for beta and aurora
Attachment #8728810 -
Flags: approval-mozilla-beta?
Attachment #8728810 -
Flags: approval-mozilla-beta+
Attachment #8728810 -
Flags: approval-mozilla-aurora?
Attachment #8728810 -
Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/cc357481aa2b https://hg.mozilla.org/releases/mozilla-beta/rev/b31b7bd99b5f
Tracked as it's a sec-high.
Updated•8 years ago
|
Group: dom-core-security → core-security-release
Comment 18•8 years ago
|
||
ServiceWorkers are disabled in ESR45
Updated•8 years ago
|
Flags: qe-verify-
Whiteboard: btpp-active → [post-critsmash-triage] btpp-active
Updated•8 years ago
|
Updated•8 years ago
|
Whiteboard: [post-critsmash-triage] btpp-active → [post-critsmash-triage][adv-main46+] btpp-active
Updated•8 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•