The default bug view has changed. See this FAQ.
Bug 535806 (CVE-2010-0169)

XUL cache lets HTML and XUL share stylesheets, can allow remote webpages to break browser UI

RESOLVED FIXED

Status

()

Core
XUL
--
minor
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Wladimir Palant, Assigned: bz)

Tracking

({verified1.9.0.18, verified1.9.1, verified1.9.2})

1.9.2 Branch
x86
Windows 7
verified1.9.0.18, verified1.9.1, verified1.9.2
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking1.9.2 .2+, status1.9.2 .2-fixed, status1.9.1 .8-fixed)

Details

(Whiteboard: [sg:low])

Attachments

(3 attachments)

(Reporter)

Description

7 years ago
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.
> 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.
Created attachment 418407 [details] [diff] [review]
Fix (for branches only)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #418407 - Flags: review?(dbaron)
Created attachment 418409 [details] [diff] [review]
Testcase.  Should land on trunk as well
Attachment #418409 - Flags: review?(dbaron)

Comment 4

7 years ago
(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 ;-)
(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?
XHTML5 then, whatever.  :-)
(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?
> 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 on attachment 418407 [details] [diff] [review]
Fix (for branches only)

r=dbaron
Attachment #418407 - Flags: review?(dbaron) → review+
Comment on attachment 418407 [details] [diff] [review]
Fix (for branches only)

Requesting branch approvals.
Attachment #418407 - Flags: approval1.9.2.1?
Attachment #418407 - Flags: approval1.9.1.8?
Attachment #418407 - Flags: approval1.9.0.18?
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
Attachment #418409 - Flags: review?(dbaron) → review+
> 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.
> 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.
I still think they should both have two tabs, to line up with the typical 16-char indent when the tab stop is 8.
> I still think they should both have two tabs

Done.
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
Attachment #418407 - Flags: approval1.9.1.8?
Attachment #418407 - Flags: approval1.9.1.8+
Attachment #418407 - Flags: approval1.9.0.18?
Attachment #418407 - Flags: approval1.9.0.18+
Whiteboard: [sg:low]
status1.9.1: --- → wanted
Whiteboard: [sg:low] → [sg:low][needs 1.9.1/1.9.0 landing]
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
status1.9.1: wanted → .8-fixed
Flags: in-testsuite+
Keywords: fixed1.9.0.18
Resolution: --- → FIXED
Whiteboard: [sg:low][needs 1.9.1/1.9.0 landing] → [sg:low]
Reftest passes on all platforms for 1.9.1. Marking as verified fixed.
Keywords: verified1.9.1
blocking1.9.2: --- → .2+
status1.9.2: --- → wanted
Comment on attachment 418407 [details] [diff] [review]
Fix (for branches only)

Approved for 1.9.2.2, a=dveditz for release-drivers
Attachment #418407 - Flags: approval1.9.2.2? → approval1.9.2.2+
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
status1.9.2: wanted → .2-fixed
Reftest is passing on all platforms for 1.9.0 as well. Marking as fixed for 1.9.0.
Keywords: fixed1.9.0.18 → verified1.9.0.18
Depends on: 546941
Verified for 1.9.2 Reftest is passing on all platforms.
Keywords: verified1.9.2
Alias: CVE-2010-0169
Group: core-security
You need to log in before you can comment on or make changes to this bug.