Closed
Bug 802905
Opened 12 years ago
Closed 12 years ago
Create custom Content Type for CSP Reports
Categories
(Core :: Security, defect)
Tracking
()
RESOLVED
FIXED
mozilla20
People
(Reporter: tanvi, Assigned: tanvi)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 5 obsolete files)
(deleted),
patch
|
tanvi
:
review+
|
Details | Diff | Splinter Review |
CSP Reports are classified as TYPE_OTHER and this is causing bugs in identifying mixed content in Bug 782654. Since CSP Reports don't have a Requesting Location (they are "requested" by the browser), we get a null requesting location and block the content (since we don't know if it is coming from a secured resource). If I can explicitly call out this type as TYPE_CSP_REPORT, then I can handle this use case and prevent nsMixedContentBlocker.cpp from blocking csp reports. +++ This bug was initially created as a clone of Bug #802833 +++ Spawning this bug off from bug 799346. Replace TYPE_OTHER with more specific Content Types. The uses of TYPE_OTHER are for CSP Reports and SVG Animations and Effects.
Assignee | ||
Comment 1•12 years ago
|
||
Comment on attachment 672622 [details] [diff] [review] Create TYPE_CSP_REPORT >diff --git a/content/base/src/contentSecurityPolicy.js b/content/base/src/contentSecurityPolicy.js >--- a/content/base/src/contentSecurityPolicy.js >+++ b/content/base/src/contentSecurityPolicy.js >@@ -320,18 +321,19 @@ ContentSecurityPolicy.prototype = { > chan.requestMethod = "POST"; > } catch(e) {} // throws only if chan is not an nsIHttpChannel. > > // check with the content policy service to see if we're allowed to > // send this request. > try { > var contentPolicy = Cc["@mozilla.org/layout/content-policy;1"] > .getService(Ci.nsIContentPolicy); >- if (contentPolicy.shouldLoad(Ci.nsIContentPolicy.TYPE_OTHER, >- chan.URI, null, null, null, null) >+ if (contentPolicy.shouldLoad(Ci.nsIContentPolicy.TYPE_CSP_REPORT, >+ chan.URI, this._request, >+ null, null, null) // XXX: should pass principal here > != Ci.nsIContentPolicy.ACCEPT) { Note from Sid: You might want to store a reference to the principal during scanRequestData(), then use it here.
Assignee | ||
Comment 2•12 years ago
|
||
Not sure why we need to add the principal in shouldLoad as part of this bug. So I'm not addressing that here. I've re-attached the patch (without the XXX) and requesting am review from Sid.
Attachment #672622 -
Attachment is obsolete: true
Attachment #673462 -
Flags: review?(sstamm)
Comment 3•12 years ago
|
||
My understanding is that the principal argument is optional only because we haven't yet went back through all the code to add the principal to each call of ShouldLoad. But, when it is possible to pass the principal, we should do so. Unfortunately, the patch that added the aRequestPrincipal didn't add the documentation that states what the aRequestPrincipal is supposed to be. I assume that it is supposed to be the principal that corresponds to the aRequestOrigin. In this case, the aRequestOrigin is the URL of the document, so I'm guessing that the request origin should be the document principal. Looking at http://hg.mozilla.org/mozilla-central/filelog/85172b4eb961/content/base/public/nsIContentPolicy.idl I see that the bug that added the principal was bug 767134, which was fixed by our dear friend devd. We should ask devd to document this new parameter. In general, when we pass a document origin to ShouldLoad/ShouldProcess, we should first get the document principal and then get URI from the principal. Should aContext also be the nsIDocument? When the aContext is a DOM node, then the principal should be the node principal, I think. Now, another question is whether the mixed content blocker should be using the principal or the aRequestOrigin, when both are available. bz?
Comment 4•12 years ago
|
||
The principal argument is optional because well-behaved extensions that do loads on behalf of web pages should be doing content policy checks and we didn't want to break them all when we added the argument. Code that ships as part of Firefox should absolutely be passing in a principal. My apologies for the missing documentation. I should have caught that in review... Your analysis is correct: if the load is on behalf of a document, then the principal passed in should be the principal of that document. > When the aContext is a DOM node, then the principal should be the node principal, I > think. Indeed. > Now, another question is whether the mixed content blocker should be using the > principal or the aRequestOrigin, when both are available. That depends on what it's planning to do with it. I'd err on the side of using the principal, generally, unless there's a strong reason not to somehow...
Comment 6•12 years ago
|
||
Comment on attachment 673462 [details] [diff] [review] Create TYPE_CSP_REPORT Review of attachment 673462 [details] [diff] [review]: ----------------------------------------------------------------- As per the comment 4, please pass the principal into shouldLoad. r=me with that. You'll need sr from someone for the interface changes.
Attachment #673462 -
Flags: superreview?(bzbarsky)
Attachment #673462 -
Flags: review?(sstamm)
Attachment #673462 -
Flags: review+
Comment 7•12 years ago
|
||
(In reply to Tanvi Vyas from comment #5) > https://tbpl.mozilla.org/?tree=Try&rev=361f86c505fa Are the failed mochitest-1 and xpcshell tests expected?
Comment 8•12 years ago
|
||
Comment on attachment 673462 [details] [diff] [review] Create TYPE_CSP_REPORT sr=me, though there needs to be more here that actually updates things like the comments say to do!
Attachment #673462 -
Flags: superreview?(bzbarsky) → superreview+
Assignee | ||
Updated•12 years ago
|
Comment 9•12 years ago
|
||
Comment on attachment 673462 [details] [diff] [review] Create TYPE_CSP_REPORT Review of attachment 673462 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/public/nsIContentPolicy.idl @@ +127,5 @@ > > /** > * Indicates a video or audio load. > */ > + const nsContentPolicyType TYPE_MEDIA = 15; kill trailing whitespace. @@ +142,5 @@ > + > + /* When adding new content types, please update nsContentBlocker, > + * NS_CP_ContentTypeName, contentScurityPolicy.js, all nsIContentPolicy > + * implementations, and other things that are not listed here that are > + * related to nsIContentPolicy. */ This is the comment bz was referring to. There are things I forgot to update to support the new type. ::: content/base/src/contentSecurityPolicy.js @@ +72,5 @@ > > + /* CSP cannot block CSP reports */ > + csp._MAPPINGS[cp.TYPE_CSP_REPORT] = null; > + > + csp._MAPPINGS[cp.TYPE_ANIMATION_OR_EFFECT] = cspr_sd.DEFAULT_SRC; Reference to TYPE_ANIMATION_OR_EFFECT needs to be removed. Also, need to make sure that CSP handles unknown content types like TYPE_OTHER, to match the required behavior described in the comments for TYPE_OTHER that were added to this patch.
Assignee | ||
Comment 10•12 years ago
|
||
(In reply to Sid Stamm [:geekboy] from comment #6) > As per the comment 4, please pass the principal into shouldLoad. r=me with > that. You'll need sr from someone for the interface changes. Done. I added a var at the top of contentSecurityPolicy.js called requestingContext. (In reply to Brian Smith (:bsmith) from comment #9) > @@ +142,5 @@ > > + > > + /* When adding new content types, please update nsContentBlocker, > > + * NS_CP_ContentTypeName, contentScurityPolicy.js, all nsIContentPolicy > > + * implementations, and other things that are not listed here that are > > + * related to nsIContentPolicy. */ > > This is the comment bz was referring to. There are things I forgot to update > to support the new type. > I went through and updated places where I needed to add TYPE_CSP_REPORT (nsContentPolicyUtils.h, nsIContentPolicy.idl, contentSecurityPolicy.js, and nsContentBlocker.cpp). CSPService, nsNoDataProtocolContentPolicy, nsWebBrowserContentPolicy, and nsDataDocumentContentPolicy didn't seem to need any updates. I will update nsMixedContentBlocker as part of bug 782654. > ::: content/base/src/contentSecurityPolicy.js > @@ +72,5 @@ > > > > + /* CSP cannot block CSP reports */ > > + csp._MAPPINGS[cp.TYPE_CSP_REPORT] = null; > > + > > + csp._MAPPINGS[cp.TYPE_ANIMATION_OR_EFFECT] = cspr_sd.DEFAULT_SRC; > > Reference to TYPE_ANIMATION_OR_EFFECT needs to be removed. Done. > Also, need to make sure that CSP handles unknown content types like > TYPE_OTHER, to match the required behavior described in the comments for > TYPE_OTHER that were added to this patch. Brian, I'm not sure what you are referring to here. I removed a bunch of whitespace in contentSecurityPolicy.js and nsIContentPolicy.idl Carrying over the r+ from Sid and sr+ from BZ? Will mention it to Sid tomorrow to see if he wants to take a second look at it. I've pushed the updated patch to try and will take a look at the results tomorrow: https://tbpl.mozilla.org/?tree=Try&rev=b0b4769709c5
Attachment #673462 -
Attachment is obsolete: true
Attachment #679532 -
Flags: superreview+
Attachment #679532 -
Flags: review+
Comment 11•12 years ago
|
||
(In reply to Tanvi Vyas from comment #10) > Brian wrote: > > Also, need to make sure that CSP handles unknown content types like > > TYPE_OTHER, to match the required behavior described in the comments for > > TYPE_OTHER that were added to this patch. > > Brian, I'm not sure what you are referring to here. In the new documentation in nsIContentPolicy, it says that content policy implementations must treat unknown types the same as they treat TYPE_OTHER. So, let's take an arbitrary unknown content type like 55. csp._MAPPINGS[55] is null, which means CSP will not block the content. But, instead, CSP should handle the content just like it does for csp._MAPPINGS[TYPE_OTHER], which maps to cspr_sd.DEFAULT_SRC. (I think the best way to resolve this is to have null mean DEFAULT_SRC, and have a new explicit value for "CSP cannot block this" instead of using null to mean that. But, there are other ways of doing it.)
Assignee | ||
Comment 12•12 years ago
|
||
Patch rebased on current mozilla-central and a new push to try. The last couple have showed a number of tests failing; if this updated patch shows the same, then I'll start debugging - https://tbpl.mozilla.org/?tree=Try&rev=555e69f212a5. Comment 11 seems like a separate bug on how content policies should handle unknown types. Carrying over reviews.
Attachment #679761 -
Flags: superreview+
Attachment #679761 -
Flags: review+
Assignee | ||
Comment 13•12 years ago
|
||
> Comment 11 seems like a separate bug on how content policies should handle > unknown types. Created bug 809983 for this.
Updated•12 years ago
|
Attachment #679532 -
Attachment is obsolete: true
Updated•12 years ago
|
Product: Firefox → Core
Assignee | ||
Comment 14•12 years ago
|
||
I have updated a patch a bit to pass the correct requestingLocation (as an nsIURI instead of a string). I am attempting to pass the principal: //store a reference to the principal, that can later be used in shouldLoad this._requestingContext = aChannel.owner.QueryInterface(Ci.nsIPrincipal); And then passing it to shouldLoad -> if (contentPolicy.shouldLoad(Ci.nsIContentPolicy.TYPE_CSP_REPORT, chan.URI, this._requestOrigin, null, null, null, this._requestingContext) But aChannel.owner is null. How do I get the principal to pass it into shouldLoad?
Assignee | ||
Comment 16•12 years ago
|
||
Thanks BZ. Got the principal and pushing to try. Carrying over r+'s. https://tbpl.mozilla.org/?tree=Try&rev=fc8d7f3b4cd3
Attachment #679761 -
Attachment is obsolete: true
Attachment #683210 -
Attachment is obsolete: true
Attachment #683224 -
Flags: review+
Assignee | ||
Comment 17•12 years ago
|
||
I have two try runs (one based on a tip from a few days ago, and one based on a tip from yesterday). They look pretty good - https://tbpl.mozilla.org/?tree=Try&rev=fc8d7f3b4cd3 https://tbpl.mozilla.org/?tree=Try&rev=9dc1958a5518 The patch has been reviewed by Sid and BZ, so I think we are good to go for checkin.
Assignee: nobody → tanvi
Keywords: checkin-needed
Comment 18•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f76932d28c5 Should this have tests?
Flags: in-testsuite?
Keywords: checkin-needed
Comment 19•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0f76932d28c5
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in
before you can comment on or make changes to this bug.
Description
•