397427 - (CVE-2008-0593) [FIX]Stylesheet href property shows redirected URL unlike other browsers

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Daniel Veditz [:dveditz]]

Attachment: Simple testcase

Received the following via mail:
--------------------------------

The issue is that Firefox will follow 302 redirects on <LINK
REL="stylesheet" HREF="..."> requests, *and* then allow the target URL
to be read by accessing element.sheet.href property.

This sounds like a non-issue, but has profound implications for a
number of contemporary websites, most notably:

1) Many webpages use SSO mechanisms and pass temporary authentication
tokens in URLs to set cookies for the target domain, then safely
consume the token. By pointing an already authenticated user to
'sso.example.com/auth?cont=fun.example.com' and reading back
'fun.example.com/auth_ok?secret_token' target URL, user accounts can
be compromised on a large number of high-profile sites.

2) Many webpages redirect an already authenticated user from main page
(fun.example.com) by appending URL information that may temporarily
contain sensitive data, such as session IDs, mailbox names, user
names, and the like
(fun.example.com/home/john.doe/hot_movies;jsessionid=foo). These would
be disclosed to third parties, severely violating user's privacy,
especially if done at a large scale.

Because the problem affects a number of sites and is hard to fix on
server side, we would prefer not to have to explicitly explain these
attack scenarios in a Bugzilla entry (and without that additional
explanation, the impact is not very obvious).

(We have no problem with the existence of a fix being communicated to
users once it is available, naturally; Martin Straka should be
credited as the original reporter if you wish to do so).

----------------------------------------------------
The above did not come with a PoC, but the attached file shows the difference between the link href and the sheet href. Mozilla.com's support page isn't a real stylesheet, but it's a handy redirected page.

Comment 1

[Daniel Veditz [:dveditz]]

The DOM 2 spec is unhelpfully vague about the meaning of the sheet's href property. We have chosen to show the redirected URI much as we do for HTML pages loaded at the top-level and in iframes.

We use the redirected URI as the codebase principal used to determine if the loading page can access the CSSRules and other content of the sheet. We could extend that blocking to the sheet's href property if it's on a different domain. That would throw an exception however, and code written for other browsers might not expect that.

IE and Opera simply copy the href value from the link element.

Comment 2

[Boris Zbarsky [:bzbarsky]]

What do IE and Opera do for @import-ed sheets if that URI redirects?

Comment 3

[Boris Zbarsky [:bzbarsky]]

And I guess I don't understand why this is not a problem for iframes?  Or is it that for iframes after such a redirect we don't allow reading the URI?

We should really bring this up in whatever working group is working on standardizing this (webapi)?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Put another way, I'm happy to implement the IE/Opera behavior if someone can describe it and if we propose it for standardization.  I'm frankly also happy to just implement nsIScriptObjectPrincipal on stylesheets.  That would have some issues though, so I'd probably rather do the former.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Personally I think we're doing the right thing here. I would imagine that a website that relies on the redirected uri to be secret is already on thin ice.

For example, what happens if you in such a stylesheet have a rule like

div { background: url(#a); }

and then call getComputedStyle(myDiv, "")?

Comment 6

[Boris Zbarsky [:bzbarsky]]

The idea here, as I undestand it, is that we have three sites:  evil.example.com, sso.example.com, and fun.example.com. evil.example.com includes a link to a stylesheet URI like 'sso.example.com/auth?cont=fun.example.com'.  Then it reads the sheet URI, which post-redirect is 'fun.example.com/auth_ok?secret_token'.  It doesn't have any control over the sheet (which is not even valid CSS in this case, being just the fun.example.com website).

I agree that this is not a particularly secure setup on the face of it, but I can't think of obvious holes in it other than this one...  so far.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Note that the case I described can be addressed on sso.example.com by doing a referrer check, from what I can see.  The second case listed in comment 0 is more a matter of "don't do that", but if sites are doing it and it's safe in all other UAs we may just need to make it safe in Gecko too.

Also note that it's pure luck that the URI scripts get compiled with as the "filename" with is the pre-redirect URI and not the post-redirect one.  Otherwise this same issue would pop up with onerror handlers too.  Quite frankly, trying to catch all places where a post-redirect URI (which is, after all the real URI of the resource) might accidentally be exposed to script seems like a losing battle to me.  Especially because there's nothing a priori insecure about it except for assumptions certain sites make about such post-redirect URIs not being available to scripts...

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #3)
> And I guess I don't understand why this is not a problem for iframes?  Or is it
> that for iframes after such a redirect we don't allow reading the URI?

Yeah, one thing I suggested to dveditz when we discussed this in person before he filed the bug was that maybe we should expand the cross-site scripting check to cover reading the URI of the sheet.  (That said, if nobody else has an XSS check there, it could be a problem.)

Comment 9

[Daniel Veditz [:dveditz]]

(In reply to comment #2)
> What do IE and Opera do for @import-ed sheets if that URI redirects?

Consistent with what they do for directly linked sheets. Given a sheet whose only rule is an import of a redirected sheet

Gecko:
  sheet.cssRules[0].href             as written in the sheet ("sheet302.css")
  sheet.cssRules[0].styleSheet.href  redirected URI

Opera:
  sheet.cssRules[0].href             as written but expanded to absolute
  sheet.cssRules[0].styleSheet.href  ditto (http://localhost/test/sheet302.css)

IE7:
  sheet.imports[0].href              as written ("sheet302.css")

Comment 10

[Boris Zbarsky [:bzbarsky]]

Daniel, do you have a testcase for comment 9 by any chance?

Comment 11

[Daniel Veditz [:dveditz]]

Attachment: simple import test

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #6)
> The idea here, as I undestand it, is that we have three sites: 
> evil.example.com, sso.example.com, and fun.example.com. evil.example.com
> includes a link to a stylesheet URI like
> 'sso.example.com/auth?cont=fun.example.com'.  Then it reads the sheet URI,
> which post-redirect is 'fun.example.com/auth_ok?secret_token'.  It doesn't 
> have any control over the sheet (which is not even valid CSS in this case, 
> being just the fun.example.com website).

What if you use the CSSOM to insert a rule into the stylesheet like the one I described? Or if the stylesheet contains rules like:

div { background-image: attr(background, url); }

Though no UA currently support that.

In any case, I'm fine with limiting support to the .href property if it's coming from a different origin than the loading page. Or we could maybe even disallow access to the entire stylesheet object in that case?

Comment 13

[Boris Zbarsky [:bzbarsky]]

> What if you use the CSSOM to insert a rule into the stylesheet

In Gecko, you can only do that with sheets that you control, effectively.  See bug 310165 comment 19.

But yes, if the result is actually a stylesheet, you might be able to get info out of it.  I agree that's a problem these servers need to solve with their authentication setup, and a potential security hole.  For them.

In the interests of interop, I actually think we should do what Opera/IE do as far as .href goes.  Esp. because it's easy and pretty much guaranteed to not break pages.  Patch coming up.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

David, would you review the CSS parts of this?  

Christian, would you look over the nsNetUtil change and its callers?

Daniel, thanks for the testcases!

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 282356 [details] [diff] [review]
Proposed fix

Oh, and the key changes here, of course, are in nsCSSStyleSheet::GetHref and in nsCSSLoader.  The rest is cosmetic.

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

So does anyone know if it's possible to use the CSSOM to reach into the stylesheet and insert a rule like the one in comment 5? If so that's a security problem. If the problem is with the site or the browser is a matter of debate.

Comment 17

[Hixie (not reading bugmail)]

Actually it seems to me that the hole sicking points out (pointing a <link rel=stylesheet> at a redirecting page, and then poking a rule into it that uses a relative URI, and then getting the computed style of an element using that rule) is a real hole. It doesn't even have to be a stylesheet on the other end, since we ignore Content-Type headers in quirks mode. (Yet another security hole caused by content type sniffing, woo.)

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Don't we have XSS restrictions on the rules in a style sheet?

(I was hoping the way to fix this would be adding XSS restrictions on the location, too, but oh well...)

Comment 19

[Hixie (not reading bugmail)]

Yeah, for the mutation issue it seems like we need restrictions that prevent writing (at least) to a stylesheet cross-origin, if we don't already.

Comment 20

[Boris Zbarsky [:bzbarsky]]

> So does anyone know if it's possible to use the CSSOM

That's exactly what you asked in comment 12.  I answered it in comment 13.  Ian, you probably want to read that part too.

I'm open to an XSS restriction on the location, but it has a higher chance of breaking sites due to unexpected security exceptions...

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 282356 [details] [diff] [review]
Proposed fix

>-  // If the channel's original URI is "chrome:", we want that, since
>-  // the observer code in nsXULPrototypeCache depends on chrome stylesheets
>-  // having a chrome URI.  (Whether or not chrome stylesheets come through
>-  // this codepath seems nondeterministic.)
>-  // Otherwise we want the potentially-HTTP-redirected URI.

It seems to me that you want to keep some form of this comment (assuming it's still relevant at all).  I'm assuming that you're still enforcing it, since the way we handle chrome: URIs doesn't set LOAD_REPLACE, right?

Looks fine otherwise.

r+sr=dbaron

Comment 22

[Michal Zalewski]

Hi guys,

Noticed no activity for a while; what's the plan on this one?

Comment 23

[Boris Zbarsky [:bzbarsky]]

It's waiting for approval to land on trunk as well as review from Christian.

After this lands on trunk, I will probably try to port this patch to branch.

Comment 24

[Damon Sicore (:damons)]

Comment on attachment 282356 [details] [diff] [review]
Proposed fix

Approved to land for beta.  Please land ASAP.

Comment 25

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 282356 [details] [diff] [review]
Proposed fix

r=biesi on the nsNetUtil change

Comment 26

[Boris Zbarsky [:bzbarsky]]

Checked in on trunk.  I'll try to work on branch patch, I guess...

Comment 27

[Boris Zbarsky [:bzbarsky]]

Attachment: Branch patch

This includes the fix for bug 74281 (but not any of the other cleanup I did on trunk), but I could take that out too, if desired.

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 285964 [details] [diff] [review]
Branch patch

>       baseURI = aLinkingContent->GetBaseURI();
>       sheetURI = aLinkingContent->GetDocument()->GetDocumentURI();
>-    }
>+      originalURI = nsnull;

I'm thinking it's probably better to stick to originalURI = sheetURI for the branch, and not take that fix.

> NS_IMETHODIMP
> nsCSSStyleSheet::GetHref(nsAString& aHref)
> {
>-  nsCAutoString str;
>-
>-  // XXXldb The DOM spec says that this should be null for inline style sheets.
>-  if (mInner && mInner->mSheetURI) {
>-    mInner->mSheetURI->GetSpec(str);
>+  if (mInner->mOriginalSheetURI) {

Maybe we should leave the mInner null check?  We probably ought to figure out a better allocation failure strategy for mInner for the trunk...

>@@ -655,17 +655,19 @@ CSSParserImpl::Parse(nsIUnicharInputStre

Seems like this is a somewhat dangerous entry point -- we probably want to document this better on the trunk, and check the callers to make sure that the URI's they're passing in are appropriate for this use.

r+sr=dbaron

Comment 29

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #28)
> I'm thinking it's probably better to stick to originalURI = sheetURI for the
> branch, and not take that fix.

...although I don't feel at all strongly about it.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

> I'm thinking it's probably better to stick to originalURI = sheetURI

Done.

> Maybe we should leave the mInner null check?

Done.

> Seems like this is a somewhat dangerous entry point -- we probably want to
> document this better on the trunk, and check the callers to make sure that the
> URI's they're passing in are appropriate for this use.

The only caller of nsICSSParser::Parse() is the CSS loader...  It's doing the right thing (in fact it hands the parser a sheet to parse into).  On trunk, I think we should remove the ability to not pass a sheet in to the CSS parser, and remove the out param on Parse().  Seem ok?

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Sounds good.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Filed bug 404315 on Parse() improvement.

Comment 33

[Daniel Veditz [:dveditz]]

Comment on attachment 289039 [details] [diff] [review]
Updated to comments

approved for 1.8.1.12, a=dveditz for release-drivers

Comment 34

[Boris Zbarsky [:bzbarsky]]

Fixed on branch.

Comment 35

[Al Billings [:abillings - ex-MoCo]]

Verified on branch with Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.12pre) Gecko/2008011803 BonEcho/2.0.0.12pre. Both the of the test cases work properly now.

Comment 36

[Alexander Sack]

distro patches block 1.8.0.15

Comment 37

[Alexander Sack]

Comment on attachment 289039 [details] [diff] [review]
Updated to comments

looks like we ship this unmodified in distros already.

caillon, please sign off for 1.8.0.15.

Comment 38

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 289039 [details] [diff] [review]
Updated to comments

a=caillon for 1.8.0.15

Comment 39

[Christopher Aillon (sabbatical, not receiving bugmail)]

committed this to the branch