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

RESOLVED FIXED in mozilla37

Status

()

Core
DOM: Security
P3
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Matthew Gertner, Assigned: ckerschb)

Tracking

(Blocks: 1 bug)

unspecified
mozilla37
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 years ago
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).
Assignee: nobody → grobinson
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.
Blocks: 493857
Component: Image Blocking → DOM: Security
... something *similar* in CSP.
(Assignee)

Updated

4 years ago
Assignee: garrett.f.robinson+mozilla → mozilla
Priority: -- → P3
(Assignee)

Comment 4

4 years ago
Created attachment 8526371 [details] [diff] [review]
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
Attachment #8526371 - Flags: review?(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;
 }
Attachment #8526371 - Flags: review?(sstamm) → review+
(Assignee)

Comment 6

4 years ago
Created attachment 8529199 [details] [diff] [review]
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.
Attachment #8526371 - Attachment is obsolete: true
Attachment #8529199 - Flags: review?(sstamm)
(Assignee)

Updated

4 years ago
Status: NEW → ASSIGNED
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?
Attachment #8529199 - Flags: review?(sstamm) → review+
(Assignee)

Comment 8

4 years ago
(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.
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.
(Assignee)

Comment 10

4 years ago
Created attachment 8531113 [details] [diff] [review]
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
Attachment #8529199 - Attachment is obsolete: true
Attachment #8531113 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/80b2348e3ab6
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.