535806 - (CVE-2010-0169) XUL cache lets HTML and XUL share stylesheets, can allow remote webpages to break browser UI

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Wladimir Palant]

Attachment: 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 [:bzbarsky]]

> 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 [:bzbarsky]]

Attachment: Fix (for branches only)

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase. Should land on trunk as well

Comment 4

[[e-mail]]

(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)]

(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]]

XHTML5 then, whatever.  :-)

Comment 7

[Zack Weinberg (:zwol)]

(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 [:bzbarsky]]

> 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-4, no longer working on Mozilla)]

Comment on attachment 418407 [details] [diff] [review]
Fix (for branches only)

r=dbaron

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 418407 [details] [diff] [review]
Fix (for branches only)

Requesting branch approvals.

Comment 11

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

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 [:bzbarsky]]

> 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 [:bzbarsky]]

> 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-4, no longer working on Mozilla)]

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 [:bzbarsky]]

> I still think they should both have two tabs

Done.

Comment 16

[Daniel Veditz [:dveditz]]

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 17

[Boris Zbarsky [:bzbarsky]]

Pushed test on m-c:
  http://hg.mozilla.org/mozilla-central/rev/9206cc10a0d1

Pushed patch and test on 1.9.1:
  http://hg.mozilla.org/releases/mozilla-1.9.1/rev/548254810b01
  http://hg.mozilla.org/releases/mozilla-1.9.1/rev/fe83149b378f

Checked into CVS.

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Reftest passes on all platforms for 1.9.1. Marking as verified fixed.

Comment 19

[Daniel Veditz [:dveditz]]

Comment on attachment 418407 [details] [diff] [review]
Fix (for branches only)

Approved for 1.9.2.2, a=dveditz for release-drivers

Comment 20

[Boris Zbarsky [:bzbarsky]]

Pushed patch and test on 1.9.2:
  http://hg.mozilla.org/releases/mozilla-1.9.2/rev/e396ed6836ce
  http://hg.mozilla.org/releases/mozilla-1.9.2/rev/ebba043bb505

Comment 21

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

Reftest is passing on all platforms for 1.9.0 as well. Marking as fixed for 1.9.0.

Comment 22

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

Verified for 1.9.2 Reftest is passing on all platforms.