Bug 397427 (CVE-2008-0593)

[FIX]Stylesheet href property shows redirected URL unlike other browsers

RESOLVED FIXED

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
10 years ago
7 years ago

People

(Reporter: dveditz, Assigned: bz)

Tracking

({fixed1.8.0.15, privacy, verified1.8.1.12})

Trunk
fixed1.8.0.15, privacy, verified1.8.1.12
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
blocking1.8.1.12 +
wanted1.8.1.x +
blocking1.8.0.next +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:low?] severity depends on website [dbaron-1.9:RsCe])

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

10 years ago
Created attachment 282197 [details]
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.
(Reporter)

Comment 1

10 years ago
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.
What do IE and Opera do for @import-ed sheets if that URI redirects?
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)?
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.
Flags: blocking1.9?
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, "")?
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.
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...
(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.)
(Reporter)

Updated

10 years ago
Whiteboard: [sg:investigate]
(Reporter)

Comment 9

10 years ago
(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")
Daniel, do you have a testcase for comment 9 by any chance?
(Reporter)

Comment 11

10 years ago
Created attachment 282305 [details]
simple import test
(Reporter)

Updated

10 years ago
Flags: wanted1.8.1.x+
(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?
> 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.
Created attachment 282356 [details] [diff] [review]
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!
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #282356 - Flags: superreview?(dbaron)
Attachment #282356 - Flags: review?(dbaron)
Attachment #282356 - Flags: review?(cbiesinger)
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.
Summary: Stylesheet href property shows redirected URL unlike other browsers → [FIX]Stylesheet href property shows redirected URL unlike other browsers
Flags: blocking1.9? → blocking1.9+
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.
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.)
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...)
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.
> 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...
Whiteboard: [sg:investigate] → [sg:investigate][dbaron-1.9:RsCe]
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
Attachment #282356 - Flags: superreview?(dbaron)
Attachment #282356 - Flags: superreview+
Attachment #282356 - Flags: review?(dbaron)
Attachment #282356 - Flags: review+
Attachment #282356 - Flags: approval1.9?

Comment 22

10 years ago
Hi guys,

Noticed no activity for a while; what's the plan on this one?
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 on attachment 282356 [details] [diff] [review]
Proposed fix

Approved to land for beta.  Please land ASAP.
Attachment #282356 - Flags: approval1.9? → approval1.9+
Comment on attachment 282356 [details] [diff] [review]
Proposed fix

r=biesi on the nsNetUtil change
Attachment #282356 - Flags: review?(cbiesinger) → review+
Checked in on trunk.  I'll try to work on branch patch, I guess...
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Blocks: 74281
Created attachment 285964 [details] [diff] [review]
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.
Attachment #285964 - Flags: superreview?(dbaron)
Attachment #285964 - Flags: review?(dbaron)
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
Attachment #285964 - Flags: superreview?(dbaron)
Attachment #285964 - Flags: superreview+
Attachment #285964 - Flags: review?(dbaron)
Attachment #285964 - Flags: review+
(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.
Created attachment 289039 [details] [diff] [review]
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?
Attachment #285964 - Attachment is obsolete: true
Attachment #289039 - Flags: approval1.8.1.11?
Attachment #289039 - Flags: approval1.8.1.10?
Sounds good.
(Reporter)

Updated

10 years ago
Flags: blocking1.8.1.11?
Filed bug 404315 on Parse() improvement.
(Reporter)

Updated

10 years ago
Attachment #289039 - Flags: approval1.8.1.10?
(Reporter)

Updated

10 years ago
Flags: blocking1.8.1.12? → blocking1.8.1.12+
Keywords: privacy
Whiteboard: [sg:investigate][dbaron-1.9:RsCe] → [sg:low?] severity depends on website [dbaron-1.9:RsCe]
(Reporter)

Comment 33

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

approved for 1.8.1.12, a=dveditz for release-drivers
Attachment #289039 - Flags: approval1.8.1.12? → approval1.8.1.12+
Fixed on branch.
Keywords: fixed1.8.1.12
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.
Keywords: fixed1.8.1.12 → verified1.8.1.12
Depends on: 405818
(Reporter)

Updated

9 years ago
Group: security
Depends on: 416896

Comment 36

9 years ago
distro patches block 1.8.0.15
Flags: blocking1.8.0.15+

Comment 37

9 years ago
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.
Attachment #289039 - Flags: approval1.8.0.15?
Comment on attachment 289039 [details] [diff] [review]
Updated to comments

a=caillon for 1.8.0.15
Attachment #289039 - Flags: approval1.8.0.15? → approval1.8.0.15+
committed this to the branch
Keywords: fixed1.8.0.15
Depends on: 443231
Alias: CVE-2008-0593
You need to log in before you can comment on or make changes to this bug.