779918 - authorization credentials are treated as part of the host name thus causing "Blocked by Content Security Policy" on framed sites

Status: RESOLVED FIXED

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

Description

[Ryan]

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:14.0) Gecko/20100101 Firefox/14.0.1
Build ID: 20120713134347

Steps to reproduce:

Visited a like like "http://username:[e-mail]/"


Actual results:

that site used frames with a source of "/frame.php".  Firefox then renders all non-absolute links to a root that does not include the authorization credentials.  thus the frame is calling "http://www.host.com/frame.php".  This causes the X-Frame-Options: SAMEORIGIN header to fail since "http://username:[e-mail]/" != "http://www.host.com/"


Expected results:

Firefox should remove the auth credentials from both the page source and frame source when doing this test since they don't effect the host domain.

Comment 1

[Matthias Versen [:Matti]]

dupe of bug 709991 ?

Comment 2

[Boris Zbarsky [:bzbarsky]]

No.  X-Frame-Options is neither CORS nor HTTP.

That said, "http://username:[e-mail]/" and "http://www.host.com/" are in fact same-origin from the point of view of our code; it's not doing a pure string compare.  Ryan, do you have a link to an actual page showing the problem?

Comment 3

[Ryan]

http://test:[e-mail]/bug_779918/

After setting up this test case, I realized it wasn't the X-Frame-Options header.  Instead it is the header including at least in the frame's header:

x-content-security-policy: allow 'self'; frame-ancestors 'self'

The problem still does seem to be with the auth credentials because after you load it for the first time then reload the page without them it works fine.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Ryan, thank you (and sorry for the lag here).

This looks like a bug in the CSP code to me.

Specifically, permitsAncestry in contentSecurityPolicy.js does this:

      let ancestor = ancestors[i].prePath;
      if (!this._policy.permits(ancestor, cspContext)) {

so it's passing in the prePath of the URI, not the URI object itself.  Then csp_permits() calls cspsd_permits, which calls CSPSource.permits.

The input to CSPSource.permits is not a CSPSource itself (it's a string, per above), so we do:

    if (!(aSource instanceof CSPSource))
      return this.permits(CSPSource.create(aSource));

and create() does this:

  if (typeof aData === 'string')
    return CSPSource.fromString(aData, self, enforceSelfChecks);

which ends up testing the string against R_HOST, which it seems to not match.

In any case, the real question is why we're passing in the prePath instead of the actual nsIURI.

Comment 5

[Sid Stamm [:geekboy or :sstamm]]

Thanks for figuring this one out bz.  Yeah, I don't know why we're passing the prePath (probably because I was an idiot when I wrote that line of code), but it wouldn't hurt to change it to the URI at this point.

I think since it's just a string representing scheme+userpass+host+port I assumed it would be okay, but in all reality the userpass is probably something we need to explicitly look for and ignore in strings.  

We recently changed the parser (introduced in bug 737064) and I think we're reusing some policy-parsing logic to parse request URIs that may get blocked.  That's probably a no-no here, so we should change it and then maybe put something in CSPSource.fromString() to look for user:pass patterns.

Comment 6

[Sid Stamm [:geekboy or :sstamm]]

Attachment: proposed patch

Okay, so this test was a little more complicated than I would have liked, but I think this fixes the issue.  I verified before/after with the URL in comment 3, and the parser/permits bits now ignore userpass.

bz: since you figured out what was wrong here, would you be so kind to take a look at this patch?  I'm curious what you think about the test... think I did it right, but am not completely sure.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 705488 [details] [diff] [review]
proposed patch

This patch makes no sense.  prePath is a readonly attribute, so setting it is a no-op.

That said, presumably someone familiar with this code should review the patch...  Me finding a problem and suggesting a possible solution doesn't make me qualified to review a totally different solution.  ;)

Comment 8

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Boris Zbarsky (:bz) from comment #7)
> Comment on attachment 705488 [details] [diff] [review]
> proposed patch
> 
> This patch makes no sense.  prePath is a readonly attribute, so setting it
> is a no-op.

Aww crap, yeah, I rushed the patch.  Should not be prePath assignments, should be userPass assignments.  The bit that actually fixes the problem uses userPass though, so that explains why the tests pass.

I'm still interested to know what you think of the tests, bz, since you found the problem; do the tests check the right things?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Hmm.  So the test seems to test a document at "http://self.com/bar" that has a subframe at "http://username:[e-mail]/foo" and frame-ancestors 'self', right?

The original report in this bug has those the other way around: the parent is at "http://username:[e-mail]/foo" and the child is at "http://self.com/bar".

Worth testing it both directions, I think.

Comment 10

[Sid Stamm [:geekboy or :sstamm]]

Attachment: proposed patch

Fixed my in-a-hurry mistake (clearing userPass, not prePath), added inverse tests.  Ian, can you take a look at this?

Comment 11

[Ian Melven :imelven]

Comment on attachment 707760 [details] [diff] [review]
proposed patch

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

The tests for this make sense to me. I have a couple of questions.

::: content/base/src/CSPUtils.jsm
@@ +250,5 @@
> +    // clean userpass out of the URI (not used for CSP origin checking, but
> +    // shows up in prePath).
> +    try {
> +      selfUri.userPass = '';
> +    } catch (ex) {} // not all URIs have a userPass and will throw

can we check if the selfUri has a userPass rather than trying and throwing ?
That would be cleaner IMO.

::: content/base/src/contentSecurityPolicy.js
@@ +433,5 @@
>            break;
>          }
> +        // ignore any userpass
> +        let ancestor = it.currentURI.cloneIgnoringRef();
> +        ancestor.userPass = '';

Can we be sure there's a userPass on it.currentURI ?

Comment 12

[bhavana bajaj [:bajaj]]

As this is sec-moderate marking it wontfix for esr/b2g.

Comment 13

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Ian Melven :imelven from comment #11)
> can we check if the selfUri has a userPass rather than trying and throwing ?
> That would be cleaner IMO.

It would, but I think even the getter throws for some protocols:
http://mxr.mozilla.org/mozilla-central/source/webapprt/content/webapp.js#169

> > +        let ancestor = it.currentURI.cloneIgnoringRef();
> > +        ancestor.userPass = '';
> 
> Can we be sure there's a userPass on it.currentURI ?

Nope, which means we need the try block here too.  Good catch.

Comment 14

[Ian Melven :imelven]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #13)
> (In reply to Ian Melven :imelven from comment #11)
> > can we check if the selfUri has a userPass rather than trying and throwing ?
> > That would be cleaner IMO.
> 
> It would, but I think even the getter throws for some protocols:
> http://mxr.mozilla.org/mozilla-central/source/webapprt/content/webapp.js#169

I was thinking of something like https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/Object/hasOwnProperty but it's not a big deal to use try/catch I guess.

> > > +        let ancestor = it.currentURI.cloneIgnoringRef();
> > > +        ancestor.userPass = '';
> > 
> > Can we be sure there's a userPass on it.currentURI ?
> 
> Nope, which means we need the try block here too.  Good catch.

:)

Comment 15

[Boris Zbarsky [:bzbarsky]]

Ian, the object always has the property.  Or rather it never has an own property with that name, but its prototype always does.

But then calling the getter or setter will throw if the underlying C++ object doesn't support hostnames (e.g. if it's a URI for a non-hierarchical scheme).

Comment 16

[Ian Melven :imelven]

(In reply to Boris Zbarsky (:bz) from comment #15)
> Ian, the object always has the property.  Or rather it never has an own
> property with that name, but its prototype always does.
> 
> But then calling the getter or setter will throw if the underlying C++
> object doesn't support hostnames (e.g. if it's a URI for a non-hierarchical
> scheme).

ah ok, thanks for the explanation, Boris !

Comment 17

[Sid Stamm [:geekboy or :sstamm]]

Attachment: proposed patch

Added the appropriate try-block, and made uniform all the comments about the various try-block additions.  I found one other place where userPass was used without a try block, so I added one.

Comment 18

[Ian Melven :imelven]

Comment on attachment 709214 [details] [diff] [review]
proposed patch

in the new test, it doesn't look like selfURI is used (i think you might have used URI(self) instead) 

r=me with that fixed

Comment 19

[Sid Stamm [:geekboy or :sstamm]]

Attachment: fix

Thanks Ian.  Made the change and carrying over r=imelven.

Comment 20

[Sid Stamm [:geekboy or :sstamm]]

Inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/3b9a168fc0b7

Comment 21

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3b9a168fc0b7