Closed Bug 1186312 Opened 4 years ago Closed 4 years ago

Cache API should not accept system principals for child processes

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: bkelly, Assigned: dimi)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Currently Cache API allows child processes to be opened for the system principal.  There are no known uses cases for this currently.  We should prevent this for now to avoid the case where the system principal is spoofed.

Note, IDB does allow child processes to use the system principal, but that is necessary for a couple specific databases on b2g.  We should not replicate this if its not necessary.

Also, this will still allow things like about:serviceworkers to function.  Utilities like this are ultimately opening CacheStorage on behalf of a real origin, not the system principal.
Dimi, do you have any interest in implementing this one since you've been looking at PrincipalVerifier recently?  I think its basically adding an "if system principal and aContentParent actor exists" check.  If you hit that condition then we know there is a remote child saying it has the system principal and we should return failure.
Flags: needinfo?(dlee)
(In reply to Ben Kelly [:bkelly] from comment #1)
> Dimi, do you have any interest in implementing this one since you've been
> looking at PrincipalVerifier recently?  I think its basically adding an "if
> system principal and aContentParent actor exists" check.  If you hit that
> condition then we know there is a remote child saying it has the system
> principal and we should return failure.

No problem, i will take it
Assignee: nobody → dlee
Flags: needinfo?(dlee)
Attached patch patch v1Splinter Review
Hi Ben,
Could you suggest a good way to verify this ? thanks!
Attachment #8637794 - Flags: review?(bkelly)
Comment on attachment 8637794 [details] [diff] [review]
patch v1

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

Exactly what I was thinking.  Thank you!

Unfortunately I'm not sure we can positively test that it blocks spoofing in our automation tests.  We should at least verify this doesn't break any existing tests or functionality.

In addition to a try build, can you try the update/unregister buttons on about:serviceworkers in e10s desktop?  I don't think those should trigger this, but lets verify.
Attachment #8637794 - Flags: review?(bkelly) → review+
Try result : https://treeherder.mozilla.org/#/jobs?repo=try&revision=2196ddd752fd

I also test update/unregister buttons on about:serviceworkers in e10s desktop as mentioned in Comment 4, it works fine.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/97d2f6256237
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.