Closed Bug 779918 Opened 7 years ago Closed 7 years ago

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

Categories

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

14 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla21
Tracking Status
firefox19 --- affected
firefox20 --- affected
firefox21 --- fixed
firefox-esr17 - wontfix
b2g18 - wontfix

People

(Reporter: noyearzero, Assigned: geekboy)

References

(Blocks 1 open bug)

Details

(Keywords: sec-moderate)

Attachments

(1 file, 3 obsolete files)

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:password@www.host.com/"


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:password@www.host.com/" != "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.
dupe of bug 709991 ?
Component: Untriaged → Networking: HTTP
Product: Firefox → Core
No.  X-Frame-Options is neither CORS nor HTTP.

That said, "http://username:password@www.host.com/" 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?
Component: Networking: HTTP → Document Navigation
http://test:test@www.ohioconnect.net/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.
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.
Status: UNCONFIRMED → NEW
Component: Document Navigation → DOM: Core & HTML
Ever confirmed: true
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.
Blocks: 737064
Blocks: CSP
Assignee: nobody → imelven
Group: core-security
tracking-b2g18: --- → ?
Keywords: sec-moderate
Blocks: 832558
Assignee: imelven → nobody
Attached patch proposed patch (obsolete) — Splinter Review
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.
Assignee: nobody → sstamm
Status: NEW → ASSIGNED
Attachment #705488 - Flags: review?(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.  ;)
Attachment #705488 - Flags: review?(bzbarsky) → review-
(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?
Flags: needinfo?(bzbarsky)
Hmm.  So the test seems to test a document at "http://self.com/bar" that has a subframe at "http://username:password@self.com/foo" and frame-ancestors 'self', right?

The original report in this bug has those the other way around: the parent is at "http://username:password@self.com/foo" and the child is at "http://self.com/bar".

Worth testing it both directions, I think.
Flags: needinfo?(bzbarsky)
Attached patch proposed patch (obsolete) — Splinter Review
Fixed my in-a-hurry mistake (clearing userPass, not prePath), added inverse tests.  Ian, can you take a look at this?
Attachment #705488 - Attachment is obsolete: true
Attachment #707760 - Flags: review?(imelven)
OS: Windows XP → All
Priority: -- → P1
Hardware: x86 → All
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 ?
Attachment #707760 - Flags: review?(imelven)
As this is sec-moderate marking it wontfix for esr/b2g.
(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.
(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.

:)
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).
(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 !
Attached patch proposed patch (obsolete) — Splinter Review
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.
Attachment #707760 - Attachment is obsolete: true
Attachment #709214 - Flags: review?(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
Attachment #709214 - Flags: review?(imelven) → review+
Attached patch fixSplinter Review
Thanks Ian.  Made the change and carrying over r=imelven.
Attachment #709214 - Attachment is obsolete: true
Attachment #709257 - Flags: review+
Inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/3b9a168fc0b7
Group: core-security
Target Milestone: --- → mozilla21
https://hg.mozilla.org/mozilla-central/rev/3b9a168fc0b7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.