1021669 - It should be possible to whitelist a protocol handler with respect to webpage CSP

Status: RESOLVED FIXED

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

Description

[Matthew Gertner]

Currently there is no way to configure a protocol handler so that it is whitelisted when a page's CSP has restrictions that would otherwise block it. This means, for example, that I can't inject a stylesheet into a page with a restrictive CSP using my own protocol handler.

The main problem is that the whitelisted protocols in the CSP service are hardcoded (see http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsCSPService.cpp#88). They should be based on protocol flags instead.

There are other places in the CSP code as well where protocols are hardcoded (e.g. http://mxr.mozilla.org/mozilla-central/source/content/base/src/CSPUtils.jsm#864).

Comment 1

[Tanvi Vyas[:tanvi]]

Mixed Content Blocker's implementation: http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsMixedContentBlocker.cpp#317

And see also https://bugzilla.mozilla.org/show_bug.cgi?id=899099

Comment 2

[Sid Stamm [:geekboy or :sstamm]]

This is totally a bug in CSP, so moving to relevant component.

The good news is that CSPUtils.jsm is going away soon.  We're moving towards nsCSPContext.h/cpp and nsCSPUtils.h/cpp soon.

Thanks for the link, Tanvi.  We should do something in CSP.

Comment 3

[Sid Stamm [:geekboy or :sstamm]]

... something *similar* in CSP.

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1021669.patch

The attached patch uses the protocol flags to whitelist certain protocols which are not subject to CSP.
The three protocols (data: [1], blob: [2], filesystem: [3]) however share the flag "URI_IS_LOCAL_RESOURCE" with other local resources, but those three protocols are subject to CSP and should not be whitelisted, hence we explicitly check for those three protocols before any early return.

[1] http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/file/nsFileProtocolHandler.cpp#148
[2] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsHostObjectProtocolHandler.cpp#456 (blob)
[3] http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/data/nsDataHandler.cpp#52

Comment 5

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8526371 [details] [diff] [review]
bug_1021669.patch

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

Just minor formatting comments.

So for other protocol handlers, the correct way to opt-out of CSP is to set one of the nsIProtocolHandler flags that are whitelisted?  (Just want to make that explicit in the bug.)  Are there any other protocol flags that should be used as indications that CSP shall not be applied?

::: dom/security/nsCSPService.cpp
@@ +57,5 @@
> +  // to make sure those protocols are subject to CSP, see:
> +  // http://www.w3.org/TR/CSP2/#source-list-guid-matching
> +  bool schemeMatch = false;
> +  if (NS_SUCCEEDED(aURI->SchemeIs("data", &schemeMatch)) &&
> +      schemeMatch) {

Nit: no need to wrap this line, same with other if statements below.  You could even collapse the if-return statements onto one line for brevity, especially if you implement my next suggestion.

 rv = aURI->SchemeIs("data", &schemeMatch);
 if (NS_SUCCEEDED(rv) && schemeMatch) { return true; }

@@ +89,5 @@
> +       NS_URIChainHasFlags(aURI,
> +                           nsIProtocolHandler::URI_INHERITS_SECURITY_CONTEXT,
> +                           &schemeMatch)) && schemeMatch) {
> +    return false;
> +  }

Nit: these last two checks look to be formatted a little awkwardly...

What if we restructure a little bit?  Yeah some lines will be > 80 chars, and we'll break style, but it might be easier to read...

rv = NS_URIChainHasFlags(aURI, nsIProtocolHandler::URI_IS_LOCAL_RESOURCE, &match);
if (NS_SUCCEDED(rv) && match) { return false; }

rv = NS_URIChainHasFlags(aURI, nsIProtocolHandler::URI_INHERITS_SECURITY_CONTEXT, &match);
if (NS_SUCCEDED(rv) && match) { return false; }

@@ +129,3 @@
>  
> +  // see if the protocol is subject to CSP
> +  if (!subjectToCSP(aContentLocation)) {

Nit: want to combine these two if statements?

 if (!sCSPEnabled || !subjectToCSP(aContentLocation)) {
   return NS_OK;
 }

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1021669.patch

Incorporated Sid's suggestions, but giving Sid the chance to veto his decision again because I had to make one change which has a lot of impact on the final patch. I had to manually whitelist "about:", because it's not using URI_IS_LOCAL_RESOURCE as a protocolflag. Rather it uses protocolFlags which are not suitable for CSP to whitelist, see:
http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/about/nsAboutProtocolHandler.cpp#55

Still fine with the patch?

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #5)
> So for other protocol handlers, the correct way to opt-out of CSP is to set
> one of the nsIProtocolHandler flags that are whitelisted?  (Just want to
> make that explicit in the bug.)

Expanded the comment before the call to isSubjectToCSP() to make that more explicit.

> Are there any other protocol flags that
> should be used as indications that CSP shall not be applied?

No, I don't think so. MixedContentBlocker for example also whitelists:
* URI_DOES_NOT_RETURN_DATA, and
* URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT.
But both do not apply to CSP because "mailto:" as well as "https" respectively need to be subject to CSP.
There are no other protocols I can think of that shouldn't be subject to CSP.

Comment 7

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8529199 [details] [diff] [review]
bug_1021669.patch

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

> Still fine with the patch?

Sure.  Why not do the :about check with the other scheme checks near the top?  That way we don't have to dig into the protocol flags if we're going to whitelist it anyway.  We want *all* about: schemed URIs to be allowed, right?

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #7)
> Comment on attachment 8529199 [details] [diff] [review]
> bug_1021669.patch
> 
> Review of attachment 8529199 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> > Still fine with the patch?
> 
> Sure.  Why not do the :about check with the other scheme checks near the
> top?  That way we don't have to dig into the protocol flags if we're going
> to whitelist it anyway.

My thinking was, that querying the protocol-handler flags is slightly faster than doing the string comparsion. Probably I should move it up to the top - makes the code easier to read. Performance shouldn't matter that much here.

> We want *all* about: schemed URIs to be allowed,
> right?

Yes, we do for certain.

Comment 9

[Sid Stamm [:geekboy or :sstamm]]

You've already done three other string comparisons, I don't think it will change performance much and it will ensure correctness since we don't want an about: URI with some protocol handlers set to do something surprising.

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1021669.patch

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #9)
> You've already done three other string comparisons, I don't think it will
> change performance much and it will ensure correctness since we don't want
> an about: URI with some protocol handlers set to do something surprising.

You are right, here we go - new patch. Carrying over r+ from sstamm.

Also pushing this to try to make sure we don't get any surprising results:
  https://tbpl.mozilla.org/?tree=Try&rev=6d395133f2ff

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

 https://hg.mozilla.org/integration/mozilla-inbound/rev/80b2348e3ab6

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/80b2348e3ab6