Closed Bug 1255298 Opened 4 years ago Closed 4 years ago

ServiceWorkerRegistration.showNotification violates cross-compartment access invariants

Categories

(Core :: DOM: Service Workers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox45 --- wontfix
firefox46 + fixed
firefox47 + fixed
firefox48 + fixed
firefox-esr38 --- disabled
firefox-esr45 - disabled

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

(Keywords: sec-high, Whiteboard: [post-critsmash-triage][adv-main46+] btpp-active)

Attachments

(3 files)

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: nobody → bzbarsky
Status: NEW → ASSIGNED
(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)
(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
Attachment #8728810 - Flags: review?(wchen) → review+
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?
Attached patch testsSplinter Review
Attachment #8729214 - Flags: review?(wchen)
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?
Attachment #8728810 - Flags: sec-approval? → sec-approval+
Attachment #8729214 - Flags: review?(wchen) → review+
FYI, we've disabled service workers in 45ESR.
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?
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?
(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
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.
I landed the test in bug 1255692.
Whiteboard: btpp-active
https://hg.mozilla.org/mozilla-central/rev/4f59de9dba41
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
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+
Tracked as it's a sec-high.
Group: dom-core-security → core-security-release
ServiceWorkers are disabled in ESR45
Flags: qe-verify-
Whiteboard: btpp-active → [post-critsmash-triage] btpp-active
Whiteboard: [post-critsmash-triage] btpp-active → [post-critsmash-triage][adv-main46+] btpp-active
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.