548949 - CSP doesn't parse policy with hostless schemes properly

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Brandon Sterne (:bsterne)]

We run into problems when we try to clone a host when there isn't a host present for a given source.  Sending the policy:

X-Content-Security-Policy: allow 'self' javascript:

Results in the parsing error:

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "this._host.clone is not a function" {file: "file:///build/csp/ff-debug/dist/bin/modules/CSPUtils.jsm" line: 989}]' when calling method: [nsIContentSecurityPolicy::refinePolicy]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "<unknown>"  data: yes]
************************************************************

Comment 1

[Sid Stamm [:geekboy or :sstamm]]

Attachment: proposed fix

The parser was setting a the host for "allow javascript:" to "*" instead of a CSPHost object, so clone() was failing.  I skimmed through the file for other cloning of a CSPSource's _host and _host assignments to make sure it was being used right.

Comment 3

[Brandon Sterne (:bsterne)]

Comment on attachment 429249 [details] [diff] [review]
proposed fix

This is the right fix, but I'm not a reviewer.  dveditz, do you mind?

Comment 4

[Brandon Sterne (:bsterne)]

dveditz, ping. Can we get r+ here?

Comment 5

[Sid Stamm [:geekboy or :sstamm]]

dveditz: any chance we could get a review on this soon?  The patch is small.

Comment 6

[Brandon Sterne (:bsterne)]

dveditz: re-ping.

Comment 7

[Daniel Veditz [:dveditz]]

Comment on attachment 429249 [details] [diff] [review]
proposed fix

>+    if (this._host && that._host)
>       newSource._host = this._host.intersectWith(that._host);
>+    else if (this._host)
>+      newSource._host = this._host.clone();
>+    else if (that._host)
>+      newSource._host = that._host.clone();

Would it make more sense to add
      else
        newSource._host = CSPHost.fromString("*");

and be sure than to have the scary "probably due to" change in permits()?

>   permits:
>   function(aHost) {
>-    if (!aHost) return false;
>+    // if the host is undefined, it's probably due to a hostless scheme so we
>+    // want to be sure that this source permits all hosts since we'll never
>+    // have a host context.
>+    if (!aHost) aHost = CSPHost.fromString("*");

"probably", but how else might you end up with a null/false/undefined aHost? Can an empty string get passed in here? If so an empty string is now a wildcard. Can someone get a hostname of "0" (string zero) into here? If so it's now a wildcard.

That worries me enough that I think I have to r- this unless you can convince me it can never happen.

Comment 8

[Sid Stamm [:geekboy or :sstamm]]

Actually, what you said makes complete sense, and I don't know why I didn't catch that before.  Here's why:  Based on CSPSource.fromString(), even hostless schemes have "*" as their hosts.  This means the host must always be defined... so yeah, we should add the else clause and throw an error in permits() if !aHost.  An undefined host is a parse error.

FWIW, I don't think "0" (string zero) is false.

Comment 9

[Sid Stamm [:geekboy or :sstamm]]

Actually, wait, it has been a while so I forgot the context a little bit.  

In the permits function, "aHost" is the host we're checking to see if it is an allowed source -- we're not changing the directive's host value.  Setting aHost to "*" means "all hosts", so the directive that's being asked permission must accept all hosts for the check to succeed.  That means effectively that an undefined host passed in will be allowed for any rule that accepts "*" (including hostless sources in a policy), but that seems fair to me.

Consider this example: There's a javascript: uri in an img tag.  This is passed into the CSP object as a whole URI, but when parsed as nsIURI, it doesn't have a host.  That's where the undefined comes from.  By rewriting 'undefined' as "*", we're requiring the directive to accept all hosts, "*", the most lenient rule.  The same process is followed in CSPSource.fromString() when the source has no host.  (e.g., "javascript:" is parsed as "javascript:*:*" allowing any host and any port).  In order for permits() to return true, either the directive must allow all hosts ("*") or it must have had a hostless scheme.

Maybe the comment should be changed to say "If the host is undefined, we assume it's a hostless scheme.  This is safe because our assumption will rewrite the lack of a host to mean that all hosts must be allowed by this directive, and only two sources will match that undefined host: a source allowing all hosts and a hostless source like "javascript:'".

With respect to adding the dangling else clause mentioned above, I don't think it's necessary, but it wouldn't hurt.  There's an else clause in CSPSource.fromString that sets the host to be "*" if there's not one specified in the string.  This means this._host and that._host should always have values and neither of the existing "else if" clauses get hit.  We could probably remove them and if they're not both defined, raise an error.

Comment 10

[Sid Stamm [:geekboy or :sstamm]]

Attachment: proposed fix

Took care of scary comments in CSPUtils.jsm as well as the else clause for CSPSource.intersect() mentioned in the r- comments.

Comment 11

[Sid Stamm [:geekboy or :sstamm]]

Attachment: proposed fix (now with Test!)

Figured it might be good to add a test for this fix.

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 466739 [details] [diff] [review]
proposed fix (now with Test!)

>+    if (this._host && that._host) {
>       newSource._host = this._host.intersectWith(that._host);
>+    } else if (this._host) {
>+      CSPError("intersecting source with undefined host: " + that.toString());
>+      newSource._host = this._host.clone();
>+    } else if (that._host) {
>+      CSPError("intersecting source with undefined host: " + this.toString());
>+      newSource._host = that._host.clone();
>+    } else {
>+      CSPError("intersecting two sources with undefined hosts: " +
>+               this.toString() + " and " + that.toString());
>+      newSource._host = CSPHost.fromString("*");

ObNit: given the locations of the CSPError() calls aren't all the toString()s likely to return "" or "undefined"? Especially in the first two cases you might want to make the strings differ a little so you can tell which case was called. Maybe
 "intersecting this source with undefined host: " and
 "intersecting undefined source with host: " ?

Or maybe print out the ones you _do_ know:
  "intersecting "+this._host.toString()+" with undefined host" and
  "intersecting undefined source with host "+that._host.toString()

CSPError() only spits out in debug mode, right? I don't feel all that strongly about it.

Looks good, r=dveditz

Comment 13

[Sid Stamm [:geekboy or :sstamm]]

> ObNit: given the locations of the CSPError() 
> calls aren't all the toString()s likely to return 
> "" or "undefined"?

Nope! The error is reporting the whole CSPSource; "this" and "that" are CSPSource objects, not CSPHosts.  If this._host is not defined, "this" is still defined, so there will be port or scheme information in this.toString(). Seems more useful to me to print out the object that has the undefined host, since it's likely at least scheme or port will be defined (or the object is a complete waste). An example error would look like "intersecting source with undefined host: htttttp::*" where the host was undefined and the port was "*".

> CSPError() only spits out in debug mode, right?

Yup, once bug 542302 lands.  We shouldn't encounter these errors anyway since currently all the CSPSource objects we use are made via CSPSource.fromString that at the very least sets the undefined host to "*" instead of undefined. 

That's not to say someone's add-on won't build their own CSPSource without using the provided fromString factory, so it's good to at least report the strange situation anyway.

Comment 14

[Brandon Sterne (:bsterne)]

http://hg.mozilla.org/mozilla-central/rev/4d0f2c178e66