1201747 - nsContentUtils::StorageAllowedFor* accesses the subject principal

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Cătălin Badea (:catalinb)]

When creating a service worker for intercepting navigation requests it is possible
that we don't have the correct subject principal on the stack, which we then use to validate the sw's principal for storage access.

Comment 1

[(no longer active)]

So did we agree on a plan here?

Comment 2

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

It seems like we need a nsContentUtils::IsStorageAllowedForServiceWorker(nsIPrincipal* aPrincipal) that does not compare against SubjectPrincipal(). This is because the StorageAllowedForWindow/Principal() method is written under the assumption that the window or principal that is passed is created by script from a document, so that a valid subject principal either of the document itself, or some other document is on the stack, against which subsumption is checked. If the subject principal is the system principal, it is always allowed.

Service Workers are the first case where chrome often starts a worker that must run with restricted privileges.

Comment 3

[Cătălin Badea (:catalinb)]

Can we just check that the sw is from a trusted origin and it's not running in private mode?

Comment 4

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

If the actual storage mechanisms do further checks when they are actually invoked, then comment 3 sounds fine. Jan, what principal will IDB, etc. use for enforcing permissions? The service worker's GetPrincipal() will return the 'correct' principal, is that enough? I assume when calls come from JS within a worker, the SubjectPrincipal() is set correctly (only relevant if IDB/Quota uses SubjectPrincipal())

Comment 5

[Jan Varga [:janv]]

AFAIK, IDB/Quota doesn't use SubjectPrincipal() at all. 
IndexedDB for workers is created in WorkerGlobalScope::GetIndexedDB() and it uses mWorkerPrivate->GetPrincipalInfo() for passing to IDBFactory::CreateForWorker()

So if WorkerPrivate::GetPrincipalInfo() has correct principal, IndexedDB will use it.

However, the only place where AllowedFor... is called in IDB is IDBFactory::CreateForWindow and IDBFactory::CreateForMainThreadJS and I'm not sure these have something to do with workers.

Comment 6

[Cătălin Badea (:catalinb)]

Callsites for the check:
https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerManager.cpp?case=true&from=ServiceWorkerManager.cpp#4104

And for CreateServiceWorkerForWindow, we call into WorkerPrivate::GetLoadInfo which gets to:
https://dxr.mozilla.org/mozilla-central/source/dom/workers/WorkerPrivate.cpp#4400

We use this check to set a flag on the worker loadinfo, which is then used to allow creating the cache dom object and I *think* it's also used for indexdb.

Comment 7

[Bobby Holley (:bholley)]

This is a regression from bug 1184973. That method has absolutely no business accessing the subject principal. :-(

Writing a patch to rip it out now. Awesome life-saving catch Catalin!

Comment 8

[Bobby Holley (:bholley)]

Attachment: Don't inspect the subject principal in StorageAllowedForPrincipal. v1

Comment 9

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=604943f4e8d8

Comment 10

[Bobby Holley (:bholley)]

[Tracking Requested - why for this release]: Security issue. Affects trunk only right now, but given the upcoming branch I want to make sure it doesn't ride to aurora.

Comment 11

[Bobby Holley (:bholley)]

Mystor, do you have the cycles to investigate the try failures in comment 9?

Comment 12

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8663225 [details] [diff] [review]
Don't inspect the subject principal in StorageAllowedForPrincipal. v1

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

It makes sense that the function should not access the current subject principal, but this change will break the storageAccess tests, which will need to be updated for the new expected behavior.

It might also make sense to re-introduce these checks at the call sites where checking the subject principal makes sense to do, such as in DOMStorage.

(I would be very surprised if this patch passes tests right now)

Comment 13

[Nika Layzell [:nika] (ni? for response)]

(In reply to Bobby Holley (:bholley) from comment #11)
> Mystor, do you have the cycles to investigate the try failures in comment 9?

The M(4) try failures are due to failures in test_storagePermissions... which tested to make sure that chrome code was allowed to access storage for pages which normally wouldn't have storage enabled. This should be fairly straightforward to fix - just modify the checks with specialpowers.wrap to assume failure in the same way as the checks which don't use specialpowers.

The other failures are because of the implementation of getURI on nsSystemPrincipal (https://dxr.mozilla.org/mozilla-central/source/caps/nsSystemPrincipal.cpp?offset=200#57-62), basically, I assumed that I got a non-null URI from the principal if the call succeeded, but that isn't true for system principals (which were previously caught earlier and handled seperately, because if aPrincipal was the system principal, then either SubjectPrincipal() was the system principal (and thus the routine had exited already), or SubjectPrincipal() didn't subsume (and the routine had failed already)). A null-check before trying to check if the URI is an about: URI should do the trick there.

Comment 14

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ac53af5cc97

Comment 15

[Bobby Holley (:bholley)]

Attachment: Don't inspect the subject principal in StorageAllowedForPrincipal. v2

Comment 16

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8663944 [details] [diff] [review]
Don't inspect the subject principal in StorageAllowedForPrincipal. v2

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

LGTM - thanks :)

Comment 17

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1056ac371c04

Comment 18

[Bobby Holley (:bholley)]

Comment on attachment 8663944 [details] [diff] [review]
Don't inspect the subject principal in StorageAllowedForPrincipal. v2

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

Requesting Aurora approval. This is a security regression from bug 1184973. Relatively low risk uplift. No String/UUID changes.

Comment 19

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

Bobby, can you go back and set sec-approval? and answer the template questions?

Comment 20

[Bobby Holley (:bholley)]

Comment on attachment 8663944 [details] [diff] [review]
Don't inspect the subject principal in StorageAllowedForPrincipal. v2

I am skeptical of the value that this adds.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Not easily.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

No.

Which older supported branches are affected by this flaw?

43 only.

If not all supported branches, which bug introduced the flaw?

bug 1184973.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

the backport is the patch. The uplift is only necessary because we branched on monday.

How likely is this patch to cause regressions; how much testing does it need?

Not likely.

Comment 21

[Bobby Holley (:bholley)]

Ryan, now that you've moved on to other things, is somebody else handling security uplifts?

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Yes, Tomcat and KWierso are handling uplifts now. Both have access to sec bugs, so things should Just Work as they did before.

Comment 23

[Bobby Holley (:bholley)]

Great!

Comment 24

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

(In reply to Bobby Holley (:bholley) from comment #20)
> Comment on attachment 8663944 [details] [diff] [review]
> Don't inspect the subject principal in StorageAllowedForPrincipal. v2
> 
> I am skeptical of the value that this adds.

Depending on what winds up in the template answers, it sometimes helps people like me, Dan, and Release Management make decisions.

Comment 25

[Andrew McCreight [:mccr8]]

This didn't get marked when it was merged, but it looks like it was merged on 9-22.

https://hg.mozilla.org/mozilla-central/rev/1056ac371c04

Comment 26

[Carsten Book [:Tomcat]]

https://treeherder.mozilla.org/#/jobs?repo=mozilla-aurora&revision=0fbd79c7d541