Closed Bug 1237080 Opened 4 years ago Closed 3 years ago

DOMParser needs to be smarter about creating a principal when one is not passed in.

Categories

(Core :: DOM: Security, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla50
Iteration:
49.2 - May 23
Tracking Status
firefox50 --- fixed

People

(Reporter: huseby, Assigned: huseby)

References

(Blocks 1 open bug)

Details

(Whiteboard: [OA][domsecurity-active][uplift49-])

Attachments

(1 file, 2 obsolete files)

in the file dom/base/DOMParser.cpp the DOMParser::Init function takes an optional principal as a parameter.  if one is not passed in, it calls GetSimpleCodebasePrincipal which is wrong. Here's the code:

> 344     rv =
> 345       secMan->GetSimpleCodebasePrincipal(mDocumentURI,
> 346                                          getter_AddRefs(mPrincipal));

i think the correct solution is to take the document pointer that is also passed in and ask its docshell parent what the correct user context id is.  then we need to populate an origin attributes object form the mDocumentURI and then slam the user context ID to match the one we got from the docshell.
Whiteboard: [OA]
Assignee: huseby → nobody
Status: ASSIGNED → NEW
Component: Security → DOM: Security
Product: Firefox → Core
Whiteboard: [OA] → [OA][domsecurity-backlog]
Assignee: nobody → huseby
Status: NEW → ASSIGNED
Priority: -- → P1
Jonas, I'd like to meet with you to talk about this one.  It appears to be a chicken/egg problem that I think you can help straighten out for me.
Flags: needinfo?(jonas)
I also asked some related question to Jonas in https://bugzilla.mozilla.org/show_bug.cgi?id=1259871#c12
it's also related to DOMparser as it's also in the commit made by him.
Iteration: --- → 49.2 - May 23
Attached patch Bug 1237080 WIP (obsolete) — Splinter Review
This just changes the assert the right place to try to catch any calls of DOMParser::Init without a principal.  The latest try push is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=01bdfb09c5b9

I've been going through previous try pushes to make sure nothing calls this function without a valid principal.  The idea is that if we always have a principal, then we no longer need to create one in ::Init and can change the invariants for the ::Init method.
Blocks: 1276412
Attached patch Bug_1237080.patch (obsolete) — Splinter Review
Jonas and I walked through the code paths and in the case where we're creating a simple codebase principal with default origin attributes only happens in chrome scripted situations (e.g. add-ons).  we don't want to break existing add-ons, but we need to start moving towards requiring a principal so that the add-ons and other chrome code is properly sand boxing any caching/requests from parsing a stream into a DOM.
Attachment #8757086 - Attachment is obsolete: true
Attachment #8760016 - Flags: review?(garrett.f.robinson+mozilla)
Attachment #8760016 - Flags: feedback?(jonas)
are either of you still reviewing patches?
Flags: needinfo?(mozbugs)
Flags: needinfo?(garrett.f.robinson+mozilla)
Whiteboard: [OA][domsecurity-backlog] → [OA][domsecurity-active]
nvrmnd, found the right reviewer.
Flags: needinfo?(mozbugs)
Flags: needinfo?(garrett.f.robinson+mozilla)
Comment on attachment 8760016 [details] [diff] [review]
Bug_1237080.patch

baku, you're listed as a peer on DOM and there is no other more specific module that owns the DOMParser.  Will you please review this?  Thanks!

--dave
Attachment #8760016 - Flags: review?(garrett.f.robinson+mozilla) → review?(amarchesini)
Comment on attachment 8760016 [details] [diff] [review]
Bug_1237080.patch

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

::: dom/base/DOMParser.cpp
@@ +326,4 @@
>    NS_ENSURE_ARG(principal || documentURI);
>  
>    mDocumentURI = documentURI;
>    

remove this too.

@@ +342,5 @@
>    if (!mPrincipal) {
> +    // BUG 1237080 -- in this case we're getting a chrome privilege scripted
> +    // DOMParser object creation without an explicit principal set.  This is
> +    // now deprecated.
> +    

remove empty spaces.
Attachment #8760016 - Flags: review?(amarchesini) → review+
already r+ by :baku, cleans up the review nitpicks.
Attachment #8760016 - Attachment is obsolete: true
Attachment #8760016 - Flags: feedback?(jonas)
Attachment #8760947 - Flags: review+
Keywords: checkin-needed
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/79fba69d9631
adds deprecation msg for chrome scripted DOMParser without principal. r=baku
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/79fba69d9631
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Whiteboard: [OA][domsecurity-active] → [OA][domsecurity-active][uplift49?]
Whiteboard: [OA][domsecurity-active][uplift49?] → [OA][domsecurity-active][uplift49-]
You need to log in before you can comment on or make changes to this bug.