Bug 1419166 (CVE-2018-5136)

Cross-origin Shared Worker using data: url

RESOLVED FIXED in Firefox 59

Status

()

defect
P2
normal
RESOLVED FIXED
2 years ago
8 months ago

People

(Reporter: s.h.h.n.j.k, Assigned: baku)

Tracking

({csectype-sop, sec-moderate})

unspecified
mozilla59
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty +
qe-verify -

Firefox Tracking Flags

(firefox-esr5258+ wontfix, firefox57 wontfix, firefox58+ wontfix, firefox59+ fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main59+] deadline: Chrome aiming to fix in M-64)

Attachments

(2 attachments, 1 obsolete attachment)

Reporter

Description

2 years ago
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/62.0.3202.94 Safari/537.36

Steps to reproduce:

1. Go to http://test.shhnjk.com/2.html
2. Click on open another window button.
3. Click on stop button on one tab


Actual results:

Both tabs stopped counting (SharedWorker established cross-origin).


Expected results:

In https://html.spec.whatwg.org/multipage/workers.html#sharedworker, step 11.2's note of "When the SharedWorker(scriptURL, options) constructor is invoked:"  says

data: URLs create a worker with an opaque origin. Both the constructor origin and constructor url are compared so the same data: URL can be used within an origin to get to the same SharedWorkerGlobalScope object, but cannot be used to bypass the same origin restriction.

This is not the case in Firefox and cross-origin site can create SharedWorker using data URL SharedWorker script.
Reporter

Comment 1

2 years ago
This affects Chrome too. I provided their bug ID if you want to coordinate with them. Chrome assigned Security_Severity-High, and already working on fix. They are aiming to fix this on M-63.
Flags: sec-bounty?
This is technically a same-origin policy violation and so chrome's "high" severity makes sense, but the opportunities for abuse don't seem to justify that rating. An attacker would have to find a site that uses a data: shared worker instead of a URL from their site, but that was not dynamic so they could use exactly the same one, and then hope the worker was doing some sort of shared state that leaked useful info. Since it's a data URL it wouldn't be able to fetch any non-public data from the site so it could have secrets if the initiating page passed them in.
Group: firefox-core-security → dom-core-security
Status: UNCONFIRMED → NEW
Component: Untriaged → DOM: Workers
Ever confirmed: true
Product: Firefox → Core
Whiteboard: deadline: Chrome aiming to fix in M-63
We probably need to treat data URLs as opaque here to fix.  Andrea, what do you think?
Flags: needinfo?(amarchesini)
I meant to link to the code for GenerateSharedWorkerKey here:

https://searchfox.org/mozilla-central/source/dom/workers/RuntimeService.cpp#260

That should probably take the PrincipalInfo instead of just OriginAttributes so it can do something smarter.
Assignee

Updated

2 years ago
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
Assignee

Comment 5

2 years ago
Posted patch sharedWorker.patch (obsolete) — Splinter Review
Attachment #8931221 - Flags: review?(bkelly)
Priority: -- → P2
Comment on attachment 8931221 [details] [diff] [review]
sharedWorker.patch

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

::: dom/workers/RuntimeService.cpp
@@ +2485,5 @@
> +    if (mDomainMap.Get(aLoadInfo->mDomain, &domainInfo)) {
> +      for (const SharedWorkerInfo& data : domainInfo->mSharedWorkerInfos) {
> +        if (data.mScriptSpec == scriptSpec &&
> +            data.mName == aName &&
> +            aLoadInfo->mPrincipal->Subsumes(data.mWorkerPrivate->GetPrincipal())) {

I'm not sure a single subsumes is correct here.  It lets a more privileged window attach to a shared worker created by a less privileged window.  This is dangerous because then the lesser window can influence the more powerful window.

Can we maybe do two subsumes checks here?  From new window to shared worker principal and also in the reverse direction?

::: dom/workers/RuntimeService.h
@@ +32,5 @@
>      nsCString mScriptSpec;
>      nsString mName;
>  
> +    SharedWorkerInfo()
> +      : mWorkerPrivate(nullptr)

This seems a bit dangerous to me.  Could you instead make the array nsTArray<UniquePtr<SharedWorkerInfo>> to avoid needing this?
Attachment #8931221 - Flags: review?(bkelly) → review-
Assignee

Comment 7

2 years ago
Attachment #8931221 - Attachment is obsolete: true
Attachment #8932115 - Flags: review?(bkelly)
Comment on attachment 8932115 [details] [diff] [review]
sharedWorker.patch

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

Thanks!

Can you add a test in a follow-on patch or bug?  It would be nice to ensure this doesn't regress in the future.

::: dom/workers/RuntimeService.cpp
@@ +1585,5 @@
> +      for (const UniquePtr<SharedWorkerInfo>& data : domainInfo->mSharedWorkerInfos) {
> +         if (data->mScriptSpec == sharedWorkerScriptSpec &&
> +             data->mName == aWorkerPrivate->WorkerName() &&
> +            // We want to be sure that the window's principal subsumes the
> +            // SharedWorker's principal and vice versa.

nit: indenting
Attachment #8932115 - Flags: review?(bkelly) → review+
Assignee

Updated

2 years ago
Blocks: 1420932
https://hg.mozilla.org/mozilla-central/rev/8417a124b2f4
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
I assume this needs at least a Beta approval request. Do we need this on ESR52 as well?
Flags: needinfo?(amarchesini)
Assignee

Comment 11

2 years ago
Comment on attachment 8932115 [details] [diff] [review]
sharedWorker.patch

Approval Request Comment
[Feature/Bug causing the regression]: SharedWorker
[User impact if declined]: data URL could have access to shared SharedWorkers. This is against the spec.
[Is this code covered by automated tests?]: no. It will be done in a follow up.
[Has the fix been verified in Nightly?]: locally
[Needs manual test from QE? If yes, steps to reproduce]: there is a test in the description of the bug
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: low
[Why is the change risky/not risky?]: Instead of using a key, this patch uses nsIPrincipal to know if a page can have access to a SharedWorker
[String changes made/needed]: none

would be nice to have this patch for 52.
Flags: needinfo?(amarchesini)
Attachment #8932115 - Flags: approval-mozilla-esr52?
Attachment #8932115 - Flags: approval-mozilla-beta?
Attachment #8932115 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Reporter

Comment 13

2 years ago
This indeed fixed the bug, but you need to take another look. What this did is to never allow connecting Shared Worker using data URL, even if same-origin. This will break the site using data URL.

PoC
https://vuln.shhnjk.com/2.html

Click on open new another window (which will open the same page). Shared Worker connects on Firefox 57 stable, but not in Nightly.


FYI, Chrome moved target to M-64.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Do you think we can do a quick follow-up here or should we back out?
Flags: needinfo?(amarchesini)
Assignee

Comment 15

2 years ago
I'll work on this today. If it takes too long, we can revoke the patch.
Flags: needinfo?(amarchesini)
Assignee

Comment 16

2 years ago
Posted patch part 2Splinter Review
The spec talks about comparing the 'constructor origin', not the worker principal. This patch introduces a mLoadingPrincipal in WorkerLoadInfo and cleans up the replacing of mPrincipal from the channel.

mLoadingPrincipal is taken from the parent and it's null only for ServiceWorkers.
Attachment #8932806 - Flags: review?(bkelly)
updating chrome timeline per comment 13
Whiteboard: deadline: Chrome aiming to fix in M-63 → deadline: Chrome aiming to fix in M-64
Comment on attachment 8932806 [details] [diff] [review]
part 2

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

Ok, the spec does specifically talk about supporting this.

I'm a little bit nervous about these changes.  The clients code needs the real, final principal set very early.  Its hard for me to tell from the patch if thats still the case.  It would be really nice to see a try run showing it doesn't his any of the clients assertions.

Also, we obviously don't have a test for this feature.  We should add a WPT test case for it.

Since this is fixing a feature regression and is not directly exposing any security problems, can we move it to a non-secure bug?  That way we could include the test and try runs, etc.

::: dom/workers/WorkerPrivate.cpp
@@ +4883,5 @@
>              rv = tmpPrincipal->GetBaseDomain(loadInfo.mDomain);
>              NS_ENSURE_SUCCESS(rv, rv);
>            } else {
>              // No unsandboxed ancestor, use our GUID.
> +            rv = loadInfo.mLoadingPrincipal->GetBaseDomain(loadInfo.mDomain);

Should the domain for a data URL worker be its parent's domain?  Is this exposed to script?

::: dom/workers/WorkerPrivate.h
@@ +1114,5 @@
>                const nsAString& aScriptURL, bool aIsChromeWorker,
>                LoadGroupBehavior aLoadGroupBehavior, WorkerType aWorkerType,
>                WorkerLoadInfo* aLoadInfo);
>  
> +  // We passed principal must be the Worker principal in case of a ServiceWorker

nit: s/We/The
Attachment #8932806 - Flags: review?(bkelly)
Assignee

Comment 19

2 years ago
> Since this is fixing a feature regression and is not directly exposing any
> security problems, can we move it to a non-secure bug?  That way we could
> include the test and try runs, etc.

I'm OK with adding a test and removing the sec flag.
(In reply to Andrea Marchesini [:baku] from comment #19)
> > Since this is fixing a feature regression and is not directly exposing any
> > security problems, can we move it to a non-secure bug?  That way we could
> > include the test and try runs, etc.
> 
> I'm OK with adding a test and removing the sec flag.

Well, not removing the sec flag here.  The original comment 0 here is still a security issue in release.

But filing a new bug for fixing the sharing of data URL SharedWorkers by same-origin parents.
Assignee

Comment 22

2 years ago
ben, if you don't mind, I would land this patch as part of this bug, and I'll add a test in a separate bug together with a test for comment 0.
Flags: needinfo?(bkelly)
Honestly I don't understand the rush.  Yes we regressed something here, but it is a corner case in a somewhat lightly used feature.  As long as we have a fix for the regression uplifted in the next week or two it seems fine.

Also, I'm really hesitant to rush another patch through here since we clearly don't have adequate tests for this stuff at the moment.

I think it would be more prudent to mark this bug FIXED and open a new non-secure bug to fix the functinal regression.  Then we can land tests together with it cleanly.  This would give me more confidence that we're not making the problem worse.

I also looked at the patch again.  One thing I'd like to add is some documentation about how mLoadingPrincipal relates to mPrincipal.  When is one set and not the other, etc.  Also, it would be nice to have assertions that mLoadingPrincipal relates to mPrincipal in a way we expect.  For example, I think they should be equal unless the script is a data URL in which case mPrincipal becomes a null principal.
Flags: needinfo?(bkelly)

Comment 24

2 years ago
> I think they should be equal unless the script is a data URL in which case mPrincipal becomes a null principal.

That's correct. At least until we add sandboxing to workers.

Andrea, I was thinking that if you don't like loadingPrincipal, another name could be initialPrincipal or creatingPrincipal.
Assignee

Updated

2 years ago
Blocks: 1423507
Assignee

Comment 25

2 years ago
I filed a follow up. Let's mark this as resolved and let's continue the work on the other bug.
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Group: dom-core-security → core-security-release
Flags: sec-bounty? → sec-bounty+
Comment on attachment 8932115 [details] [diff] [review]
sharedWorker.patch

Sec-mod, has stabilized in 58 for ~6 weeks, ESR52+
Attachment #8932115 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Wait, there is a functional regression from this patch that is fixed in another bug.  If you uplift this to ESR you should uplift that as well.  Let me find the related bug.
See bug 1423507.  It is only in FF59 currently.  Also there was some assertion fallout from that bug, so I'm not sure we should uplift it.  For example bug 1424336 and bug 1428447.

Give this is only a moderate privacy risk and there were problems from this fix I personally would be in favor of just waiting for ESR60.
Ritu, please see comment 27 and comment 28.  Thanks.
Flags: needinfo?(rkothari)
Attachment #8932115 - Flags: approval-mozilla-esr52+
Thanks Ben.  Should we back this out of 58 then, so it rolls out with the other fixes in 59?
Flags: needinfo?(rkothari) → needinfo?(bkelly)
(In reply to Julien Cristau [:jcristau] from comment #30)
> Thanks Ben.  Should we back this out of 58 then, so it rolls out with the
> other fixes in 59?

So, ultimately we do need to uplift this bug and these follow-on fixes to ESR:

bug 1423507
bug 1424336
bug 1426162
bug 1428447

Currently only this bug is uplifted to beta and none of those follow-on bugs.

So the question is, do we want to:

1. Uplift all of those bugs to beta and ESR now before the imminent merge.
2. Backout this bug from beta and uplift them all to ESR after the merge for the next release cycle.

Its hard for me to make the call which way to go.  Since its sec-moderate I think (2) would be ok, but maybe we should attempt (1).

Danial, Julien, what do you think?  Can we uplift to beta today before the first merge and then uplift to ESR the following week?
Flags: needinfo?(jcristau)
Flags: needinfo?(dveditz)
Flags: needinfo?(bkelly)
Since it’s sec-moderate and it’s very late to beta16. I think we can back it out and let it ride the train.
Flags: needinfo?(jcristau)
Flags: needinfo?(dveditz)
Comment on attachment 8932115 [details] [diff] [review]
sharedWorker.patch

clearing flag since this was backed out
Attachment #8932115 - Flags: approval-mozilla-beta+
Flags: qe-verify-
Whiteboard: deadline: Chrome aiming to fix in M-64 → [post-critsmash-triage] deadline: Chrome aiming to fix in M-64
Chrome 64 has shipped. I assume there are no issues disclosing this now?
Flags: needinfo?(s.h.h.n.j.k)
Whiteboard: [post-critsmash-triage] deadline: Chrome aiming to fix in M-64 → [post-critsmash-triage][adv-main59+] deadline: Chrome aiming to fix in M-64
Reporter

Comment 36

Last year
yes. Chrome already fixed the bug in 64.
Flags: needinfo?(s.h.h.n.j.k)
Summary: Cross-origin Shared Worker → Cross-origin Shared Worker using data: url
Alias: CVE-2018-5136
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.