Closed Bug 1201747 Opened 4 years ago Closed 4 years ago

nsContentUtils::StorageAllowedFor* accesses the subject principal

Categories

(Core :: DOM: Service Workers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox41 --- unaffected
firefox42 --- unaffected
firefox43 + fixed
firefox44 --- fixed
firefox-esr38 --- unaffected
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- unaffected
b2g-v2.1S --- unaffected
b2g-v2.2 --- unaffected
b2g-v2.2r --- unaffected
b2g-master --- fixed

People

(Reporter: catalinb, Assigned: bholley)

References

(Depends on 1 open bug)

Details

(Keywords: regression, sec-critical)

Attachments

(1 file, 1 obsolete file)

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.
So did we agree on a plan here?
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.
Can we just check that the sw is from a trusted origin and it's not running in private mode?
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())
Flags: needinfo?(Jan.Varga)
Flags: needinfo?(bobbyholley)
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.
Flags: needinfo?(Jan.Varga)
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.
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!
Flags: needinfo?(bobbyholley)
[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.
Blocks: 1184973
Keywords: regression
Assignee: nobody → bobbyholley
Summary: Wrong SubjectPrincipal() when creating a service worker for intercepting navigations → nsContentUtils::StorageAllowedFor* accesses the subject principal
Group: core-security
Status: NEW → ASSIGNED
Mystor, do you have the cycles to investigate the try failures in comment 9?
Flags: needinfo?(michael)
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)
Attachment #8663225 - Flags: review?(michael)
(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.
Flags: needinfo?(michael)
Attachment #8663225 - Attachment is obsolete: true
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 :)
Attachment #8663944 - Flags: review?(michael) → review+
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.
Attachment #8663944 - Flags: approval-mozilla-aurora?
Bobby, can you go back and set sec-approval? and answer the template questions?
Flags: needinfo?(bobbyholley)
Attachment #8663944 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Flags: needinfo?(bobbyholley)
Attachment #8663944 - Flags: sec-approval?
Ryan, now that you've moved on to other things, is somebody else handling security uplifts?
Flags: needinfo?(ryanvm)
Yes, Tomcat and KWierso are handling uplifts now. Both have access to sec bugs, so things should Just Work as they did before.
Flags: needinfo?(ryanvm)
Great!
(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.
Attachment #8663944 - Flags: sec-approval? → sec-approval+
Group: core-security → dom-core-security
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
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Group: dom-core-security → core-security-release
Group: core-security-release
Depends on: 1234021
Depends on: 1232955
No longer depends on: 1232955
Depends on: 1245595
Depends on: 1271257
You need to log in before you can comment on or make changes to this bug.