Closed Bug 548949 Opened 10 years ago Closed 9 years ago

CSP doesn't parse policy with hostless schemes properly

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: bsterne, Assigned: geekboy)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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]
************************************************************
Attached patch proposed fix (obsolete) — Splinter Review
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.
Blocks: CSP
Duplicate of this bug: 544061
Comment on attachment 429249 [details] [diff] [review]
proposed fix

This is the right fix, but I'm not a reviewer.  dveditz, do you mind?
Attachment #429249 - Flags: review?(dveditz)
dveditz, ping. Can we get r+ here?
dveditz: any chance we could get a review on this soon?  The patch is small.
Status: NEW → ASSIGNED
dveditz: re-ping.
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.
Attachment #429249 - Flags: review?(dveditz) → review-
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.
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.
Attached patch proposed fix (obsolete) — Splinter Review
Took care of scary comments in CSPUtils.jsm as well as the else clause for CSPSource.intersect() mentioned in the r- comments.
Attachment #429249 - Attachment is obsolete: true
Attachment #466728 - Flags: review?(dveditz)
Figured it might be good to add a test for this fix.
Attachment #466728 - Attachment is obsolete: true
Attachment #466739 - Flags: review?(dveditz)
Attachment #466728 - Flags: review?(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
Attachment #466739 - Flags: review?(dveditz) → review+
> 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.
Attachment #466739 - Flags: approval2.0?
Attachment #466739 - Flags: approval2.0? → approval2.0+
http://hg.mozilla.org/mozilla-central/rev/4d0f2c178e66
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Duplicate of this bug: 606039
You need to log in before you can comment on or make changes to this bug.