Last Comment Bug 535806 - (CVE-2010-0169) XUL cache lets HTML and XUL share stylesheets, can allow remote webpages to break browser UI
(CVE-2010-0169)
: XUL cache lets HTML and XUL share stylesheets, can allow remote webpages to b...
Status: RESOLVED FIXED
[sg:low]
: verified1.9.0.18, verified1.9.1, verified1.9.2
Product: Core
Classification: Components
Component: XUL (show other bugs)
: 1.9.2 Branch
: x86 Windows 7
: -- minor (vote)
: ---
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
Depends on: 546941
Blocks: songbird
  Show dependency treegraph
 
Reported: 2009-12-18 08:44 PST by Wladimir Palant
Modified: 2010-03-24 00:29 PDT (History)
12 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.2+
.2-fixed
.8-fixed


Attachments
Testcase (remote HTML) (100 bytes, text/html)
2009-12-18 08:44 PST, Wladimir Palant
no flags Details
Fix (for branches only) (1.19 KB, patch)
2009-12-18 12:07 PST, Boris Zbarsky [:bz]
dbaron: review+
dveditz: approval1.9.2.2+
dveditz: approval1.9.1.8+
dveditz: approval1.9.0.18+
Details | Diff | Review
Testcase. Should land on trunk as well (7.75 KB, patch)
2009-12-18 12:08 PST, Boris Zbarsky [:bz]
dbaron: review+
Details | Diff | Review

Description Wladimir Palant 2009-12-18 08:44:17 PST
Created attachment 418384 [details]
Testcase (remote HTML)

This came up as a Songbird issue. I am marking it as security sensitive since there seems to be the possibility of abuse by remote webpages.

Stylesheets loaded from chrome land in XUL cache, subsequent requests get a cached copy. Usually this is not a problem. However, if an HTML page requests a stylesheet this stylesheet seems to be converted before it is put into XUL cache, e.g. all attribute names are lowercased. If a XUL document tries to use that stylesheet later it gets the version with lowercased attributes, consequently the selectors no longer match correctly. Here is an example (three files that need to be put into a chrome location):

test.html:
<link rel="stylesheet" href="test.css" type="text/css">
<iframe src="test.xul"></iframe>

test.css:
label {
  color: red;
}
label[someAttr] {
  color: green;
}

test.xul:
<?xml-stylesheet href="test.css" type="text/css"?>
<page xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
  <label someAttr="true" value="This should be green"/>
</page>

If test.html is opened with XUL cache disabled you will see green text, with XUL cache enabled the text will be red because the selectors no longer apply correctly in the XUL document.

This issue can also be triggered by remote webpages. If you used bookmarks UI already, restart your browser. Then open the testcase, it will load a stylesheet used by Firefox' bookmarks UI. Now click the star symbol in location bar to bookmark the current page. Click again to edit the bookmark - try changing the folder and note that the icon in the menulist element doesn't change. This icon is added by CSS styles dependent on a mixed-case attribute name which doesn't work if the stylesheet was previously loaded by an HTML page.

Reproduced in Firefox 3.5.6 and 3.6b5. The issue doesn't occur in Minefield, it doesn't seem to convert attribute names in stylesheets to lowercase.
Comment 1 Boris Zbarsky [:bz] 2009-12-18 10:07:35 PST
> all attribute names are lowercased

That's bug 507762, effectively (or one of its depenencies, as you prefer).

David, Zack, Neil, what do you think of conditioning the prototype cache put in CSSLoaderImpl::DoSheetComplete on mCaseSensitive?  That should prevent this problem without causing any performance issues, I'd think.
Comment 2 Boris Zbarsky [:bz] 2009-12-18 12:07:33 PST
Created attachment 418407 [details] [diff] [review]
Fix (for branches only)
Comment 3 Boris Zbarsky [:bz] 2009-12-18 12:08:06 PST
Created attachment 418409 [details] [diff] [review]
Testcase.  Should land on trunk as well
Comment 4 neil@parkwaycc.co.uk 2009-12-18 12:48:28 PST
(In reply to comment #1)
> David, Zack, Neil, what do you think of conditioning the prototype cache put in
> CSSLoaderImpl::DoSheetComplete on mCaseSensitive?  That should prevent this
> problem without causing any performance issues, I'd think.
Sounds good to me. Chrome should be using XHTML anyway ;-)
Comment 5 Zack Weinberg (:zwol) 2009-12-21 10:45:19 PST
(In reply to comment #4)
> Sounds good to me. Chrome should be using XHTML anyway ;-)

Won't we eventually want to be using HTML5 in there instead?
Comment 6 Jeff Walden [:Waldo] (remove +bmo to email) 2009-12-21 12:01:23 PST
XHTML5 then, whatever.  :-)
Comment 7 Zack Weinberg (:zwol) 2009-12-21 12:09:15 PST
(In reply to comment #6)
> XHTML5 then, whatever.  :-)

You missed the question -- do chrome HTML documents still need to be processed as XML documents once we can use HTML5 throughout, and if so, why?
Comment 8 Boris Zbarsky [:bz] 2009-12-21 18:07:03 PST
> Won't we eventually want to be using HTML5 in there instead?

Quite possibly, but on 1.9.3 and later this bug is a non-issue (and no code changes are planned); all CSS parsing is always case-preserving.  I doubt we'll be trying to use non-XML HTML5 in our chrome on 1.9.2 and before.
Comment 9 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2009-12-22 15:31:57 PST
Comment on attachment 418407 [details] [diff] [review]
Fix (for branches only)

r=dbaron
Comment 10 Boris Zbarsky [:bz] 2009-12-22 15:33:45 PST
Comment on attachment 418407 [details] [diff] [review]
Fix (for branches only)

Requesting branch approvals.
Comment 11 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2009-12-22 15:37:13 PST
Comment on attachment 418409 [details] [diff] [review]
Testcase.  Should land on trunk as well

>+DIRS +=        chrome \
>+       $(NULL)

Both these lines should have two tabs.


(I'd note that we have both regular and browser mochitests in the parent
directory, but I suppose separating chrome tests probably makes sense, 
and we should perhaps do the same for the existing browser tests.)

I presume you built chrome.template.txt from something in the tree?

And also that you checked that the test failed without the patch?

r=dbaron
Comment 12 Boris Zbarsky [:bz] 2009-12-22 15:38:59 PST
> I presume you built chrome.template.txt from something in the tree?

Yeah, from xul.template.txt

> And also that you checked that the test failed without the patch?

Yep.
Comment 13 Boris Zbarsky [:bz] 2009-12-22 16:37:34 PST
> Both these lines should have two tabs.

They actually both have one tab.  It's just that with the diff the number of chars before the tab on the DIRS line is big enough that we end up crossing a tab stop.
Comment 14 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2009-12-28 14:48:23 PST
I still think they should both have two tabs, to line up with the typical 16-char indent when the tab stop is 8.
Comment 15 Boris Zbarsky [:bz] 2009-12-29 11:26:59 PST
> I still think they should both have two tabs

Done.
Comment 16 Daniel Veditz [:dveditz] 2010-01-05 10:48:45 PST
Comment on attachment 418407 [details] [diff] [review]
Fix (for branches only)

Approved for 1.9.1.8 and 1.9.0.18, a=dveditz for release-drivers
Comment 18 Henrik Skupin (:whimboo) 2010-01-28 13:16:36 PST
Reftest passes on all platforms for 1.9.1. Marking as verified fixed.
Comment 19 Daniel Veditz [:dveditz] 2010-02-05 13:29:12 PST
Comment on attachment 418407 [details] [diff] [review]
Fix (for branches only)

Approved for 1.9.2.2, a=dveditz for release-drivers
Comment 21 Al Billings [:abillings] 2010-02-16 10:53:47 PST
Reftest is passing on all platforms for 1.9.0 as well. Marking as fixed for 1.9.0.
Comment 22 Al Billings [:abillings] 2010-03-15 16:41:40 PDT
Verified for 1.9.2 Reftest is passing on all platforms.

Note You need to log in before you can comment on or make changes to this bug.