802905 - Create custom Content Type for CSP Reports

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Tanvi Vyas[:tanvi]]

Attachment: Create TYPE_CSP_REPORT

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.

Comment 1

[Tanvi Vyas[:tanvi]]

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.

Comment 2

[Tanvi Vyas[:tanvi]]

Attachment: Create TYPE_CSP_REPORT

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.

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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

[Boris Zbarsky [:bzbarsky]]

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 5

[Tanvi Vyas[:tanvi]]

https://tbpl.mozilla.org/?tree=Try&rev=361f86c505fa

Comment 6

[Sid Stamm [:geekboy or :sstamm]]

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.

Comment 7

[Sid Stamm [:geekboy or :sstamm]]

(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

[Boris Zbarsky [:bzbarsky]]

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!

Comment 9

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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.

Comment 10

[Tanvi Vyas[:tanvi]]

Attachment: Create TYPE_CSP_REPORT

(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

Comment 11

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(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.)

Comment 12

[Tanvi Vyas[:tanvi]]

Attachment: Create TYPE_CSP_REPORT

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.

Comment 13

[Tanvi Vyas[:tanvi]]

> Comment 11 seems like a separate bug on how content policies should handle
> unknown types.
Created bug 809983 for this.

Comment 14

[Tanvi Vyas[:tanvi]]

Attachment: Create TYPE_CSP_REPORT

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?

Comment 15

[Boris Zbarsky [:bzbarsky]]

nsIScriptSecurityManager.getChannelPrincipal.

Comment 16

[Tanvi Vyas[:tanvi]]

Attachment: Create TYPE_CSP_REPORT

Thanks BZ.  Got the principal and pushing to try.  Carrying over r+'s.

https://tbpl.mozilla.org/?tree=Try&rev=fc8d7f3b4cd3

Comment 17

[Tanvi Vyas[:tanvi]]

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.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0f76932d28c5

Should this have tests?

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0f76932d28c5

Comment 20

[Mihai Morar, (:MihaiMorar)]

How can QA verify this fix?