1419166 - (CVE-2018-5136) Cross-origin Shared Worker using data: url

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect, P2)

Description

[Jun (Request needinfo for comment, I ignore other updates)]

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.

Comment 1

[Jun (Request needinfo for comment, I ignore other updates)]

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.

Comment 2

[Daniel Veditz [:dveditz]]

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.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

We probably need to treat data URLs as opaque here to fix.  Andrea, what do you think?

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 5

[Andrea Marchesini [:baku]]

Attachment: sharedWorker.patch

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

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?

Comment 7

[Andrea Marchesini [:baku]]

Attachment: sharedWorker.patch

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/8417a124b2f4

Comment 10

[Ryan VanderMeulen [:RyanVM]]

I assume this needs at least a Beta approval request. Do we need this on ESR52 as well?

Comment 11

[Andrea Marchesini [:baku]]

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.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a34fb2beb5f3

Comment 13

[Jun (Request needinfo for comment, I ignore other updates)]

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.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Do you think we can do a quick follow-up here or should we back out?

Comment 15

[Andrea Marchesini [:baku]]

I'll work on this today. If it takes too long, we can revoke the patch.

Comment 16

[Andrea Marchesini [:baku]]

Attachment: part 2

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.

Comment 17

[Julien Cristau [:jcristau]]

updating chrome timeline per comment 13

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 19

[Andrea Marchesini [:baku]]

> 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.

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

(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.

Comment 21

[Andrea Marchesini [:baku]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5aaeed68934fa8e7c97177303eb97157c32ba35e&selectedJob=148584877

green on try.

Comment 22

[Andrea Marchesini [:baku]]

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.

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 24

[Anne (:annevk)]

> 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.

Comment 25

[Andrea Marchesini [:baku]]

I filed a follow up. Let's mark this as resolved and let's continue the work on the other bug.

Comment 26

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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

Sec-mod, has stabilized in 58 for ~6 weeks, ESR52+

Comment 27

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 28

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 29

[Ben Kelly [:bkelly, not reviewing]]

Ritu, please see comment 27 and comment 28.  Thanks.

Comment 30

[Julien Cristau [:jcristau]]

Thanks Ben.  Should we back this out of 58 then, so it rolls out with the other fixes in 59?

Comment 31

[Ben Kelly [:bkelly, not reviewing]]

(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?

Comment 32

[Gerry Chang [:gchang]]

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.

Comment 33

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out as requested: https://hg.mozilla.org/releases/mozilla-beta/rev/15c6742655c07b097d9b8dd3529eea0d62388645

Comment 34

[Julien Cristau [:jcristau]]

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

clearing flag since this was backed out

Comment 35

[Al Billings [:abillings - ex-MoCo]]

Chrome 64 has shipped. I assume there are no issues disclosing this now?

Comment 36

[Jun (Request needinfo for comment, I ignore other updates)]

yes. Chrome already fixed the bug in 64.