User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.9.1.3) Gecko/20090824 Firefox/3.5.3 (.NET CLR 3.5.30729) Build Identifier: An IFRAME with its src attribute set to a data URI can be dragged into a designMode area. The data URI renders an HTML link that, if clicked, will execute some JavaScript. If a selection containing such an IFRAME is dragged between two different domains, the JavaScript will execute in the context of the domain where it is dropped. If an attacker can get a user to perform a drag and drop operation, it is possible to perform XSS on sites that use the designMode feature. Webkit and Internet Explorer are affected by similar bugs, which I have reported in the relevant places. Reproducible: Always Steps to Reproduce: See testcase

This demonstrates the basic bug.

This targets the rich text editor at http://www.google.com/profiles/me/editprofile?hl=en&edit=a (you need to be logged to a Google account for this to work). The drag and drop is performed by getting the user to move a scrollbar.

I think the way to fix this is to disallow javascript and data URIs for iframes that are the direct children of designMode documents. I have also found that contentEditable areas are also vulnerable to this attack, and are easier to exploit. All that is required is to drag and drop a selection containing a script tag into a contenteditable region, and it will be executed - there is no need for the additional click. I suspect that the fix for contentEditable will be more tricky, so it may require a separate bug.

For some reason, when dragging and dropping into a contentEditable region inside an iframe, a plain <script> tag doesn't work. This example uses an iframe with a javascript URI.

Any update on this (or bug 519928)? It's been 3 months and neither bug has been confirmed.

Confirming. Chrome apparently implemented a fix for this bug recently.

Just FYI, we are more concerned about copy-and-paste. While drag-and-drop is not a likely form of interaction, users often copy and paste snippets of news articles or other contents into HTML-enabled webmail or online document editing tools.

I have a patch in the works.

The way that this fix works is by emptying the src attribute of iframe's when they are using a data: or javascript: URI. AFAICT, this is the same way that Chrome fixes this issue. And the fix works for both contenteditable elements and designMode=on documents.

I'm not sure what went wrong when I wrote comment 5, but here is a testcase that does XSS in a contentEditable area using a <script> tag, instead of an iframe.

Oops, uploaded wrong file.

This version uses contentEditable=false to create a clickable link that navigates to a data URI.

I won't upload another testcase, but I should point out that on* event handler attributes also fire in contentEditable areas. I tried a variant of the designMode testcase (see comment 2) using an <object> tag instead of an <iframe>. This didn't work because object elements don't load in designMode. However, depending on how bug 519928 is fixed, it might be worth sanitising those too. I also thought of various other tricks, for example using inline SVG or XUL. However, those both require XHTML documents, and designMode/contentEditable don't currently work with pasted or dropped XHTML content (bug 503672). With the HTML5 parser, you can have SVG inside HTML, but again this doesn't currently get transferred with DnD/copy paste.

This patch adds handling for script elements. I still need to handle some other cases, so this is not ready for review yet.

It seems to me that mitigating this attack requires dealing with pasted CSS as well as HTML and script.

Reusing the HTML paranoid document fragment sink is the way to go here. This is a WIP patch which uses that approach. None of the test cases attached to this bug (also including possible testcases with onxxx event handling attributes) will lead to code execution using this patch. This is not yet complete, mostly because we don't have any support for CSS yet. I will create separate patches based on this one to handle that issue.

Can you document what the aAllContent parameter does? Also, is there documentation anywhere of what the paranoid document fragment sink actually allows? It looks like a whitelist, but what exactly is whitelisted and how seriously will that affect user HTML pasting?

(In reply to comment #18) > Can you document what the aAllContent parameter does? > > Also, is there documentation anywhere of what the paranoid document fragment > sink actually allows? It looks like a whitelist, but what exactly is > whitelisted and how seriously will that affect user HTML pasting? The whitelist is here: http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsContentSink.cpp#1758

Nice. We should add <video> and <audio> to that list.

And for the HTML5 parser, we should add a bunch of MathML and SVG stuff.

(In reply to comment #17) > This is not yet complete, mostly because we don't have any support for CSS yet. Does CSS still pose an XSS risk now that XBL bindings are disabled for content?

(In reply to comment #20) > Nice. We should add <video> and <audio> to that list. Filed bug 554125 for that, with a patch.

(In reply to comment #21) > And for the HTML5 parser, we should add a bunch of MathML and SVG stuff. Filed bug 554130 for that.

(In reply to comment #22) > (In reply to comment #17) > > This is not yet complete, mostly because we don't have any support for CSS yet. > > Does CSS still pose an XSS risk now that XBL bindings are disabled for content? jst, Smaug, could you please confirm that we're indeed disabling XBL bindings for content?

We have not yet disabled remote XBL/XUL, but we plan to do so on Q2 of 2010.

Right, I was thinking of this: https://developer.mozilla.org/en/Same_origin_policy_for_XBL, which prevents a site loading XBL from another domain. So it does (mostly) prevent XSS using XBL.

(In reply to comment #26) > We have not yet disabled remote XBL/XUL, but we plan to do so on Q2 of 2010. Does it also include data: URI XBL documents? Anyway I think we need to handle it here because this patch would probably need to be backported to branches as well.

(In reply to comment #28) > Does it also include data: URI XBL documents? From https://developer.mozilla.org/en/XBL/XBL_1.0_Reference/Elements#binding: "In addition, data: URLs are only supported from chrome code (in other words, from the application or an extension)."

Comment on attachment 434283 [details] [diff] [review] Part 1: use paranoid document fragment sink Can you document somewhere what the aAllContent parameter does?

(In reply to comment #31) > (From update of attachment 434283 [details] [diff] [review]) > Can you document somewhere what the aAllContent parameter does? I did: public: + /** + * @param aAllContent Whether there is context information available for the fragment. + */ nsHTMLFragmentContentSink(PRBool aAllContent = PR_FALSE); I'm sure that can be improved, though. Sayrer, do you have any suggestions?

This part of the patch makes sure that style elements and tags continue to work in data transfer operations. I've extended the paranoid fragment content sink to accept additional white-list items other than the default ones, and specified overrides for style and attribute tags. The next (and last) part of the patches for this bug makes sure that -moz-binding inside the allowed CSS code is ignored. According to my IRC conversation with bz, that is the only unsafe thing in CSS which we need to protect against, so with that, this patch is complete.

Instead of introducing this extension mechanism, how about we introduce an additional mode flag to the sink, say "enable CSS"?

(In reply to comment #34) > Instead of introducing this extension mechanism, how about we introduce an > additional mode flag to the sink, say "enable CSS"? We could do that. The implementation inside the content sink would remain nearly the same, it's only a change in the interface exposed to the outsiders. I'm not sure which one is better though, since the only current use case inside the tree is this code. If you want, I'll switch to that approach.

It seems to me that feeds want CSS stripped and HTML copy/paste does not. In this patch the paranoid content sink strips CSS by default and provides an extension mechanism that HTML copy/paste can use to retain CSS. I think it would be better to offer an explicit "retain CSS" interface so the details of how that's done (whitelist "style" elements and "style" attributes) are handled by the content sink instead of the copy/paste code. Not only is it then reusable, I think it's just the right place for that knowledge to be. Whitelisting <style> elements is a little bit scary actually since it allows a pasted <style> element to probe the structure of the document by writing complex selectors and sending messages by loading images in rules using those selectors. Fortunately, we don't currently support any selectors that match based on text content, although we do for attribute values... Still, I can't quite muster an objection.

what about CSS that executes code in other browsers? Even IE8 will still run expressions in IE7 mode. http://blogs.msdn.com/ie/archive/2008/10/16/ending-expressions.aspx

Good point ... maybe we should parse the CSS, pass it through a whitelist filter (which would contain almost everything we support, I guess), and serialize?

(In reply to comment #37) > what about CSS that executes code in other browsers? Even IE8 will still run > expressions in IE7 mode. > > http://blogs.msdn.com/ie/archive/2008/10/16/ending-expressions.aspx As I said in comment 33, the only CSS property which we support that can lead to code execution is -moz-binding. I have another patch in the works to blacklist only that property. There are also CSS properties which take url() values, and the URIs passed to them might be javascript: URIs, but they execute the js code in a sandbox separate from the originating document, so they're safe to allow here. (In reply to comment #38) > Good point ... maybe we should parse the CSS, pass it through a whitelist > filter (which would contain almost everything we support, I guess), and > serialize? Taking the approach in comment 36 into consideration, I'll probably move the CSS filtering code to the paranoid fragment sink as well so that it knows to only allow safe CSS (since we're still paranoid!).

This doesn't work for <style> elements, because they seem to be parsed asynchronously, and by the time that the CSS parser is invoked on them, the property filter has gone away. I don't know the code well enough to know how to proceed from here, therefore I'm requesting feedback from dbaron. I have a sort of dirty solution in mind (scan the style element contents and style attribute values inside the content sink and remove -moz-bindings from there) but I really don't want to go there.

Does the sanitizer already forbid <link rel="stylesheet">?

(In reply to comment #41) > Does the sanitizer already forbid <link rel="stylesheet">? Nothing that belongs in the <head> is in the whitelist.

(In reply to comment #41) > Does the sanitizer already forbid <link rel="stylesheet">? It blocks all link elements.

Then why is <style> in the whitelist?

(In reply to comment #44) > Then why is <style> in the whitelist? It's not currently... Ehsan is enabling it for designMode uses.

Ah, ok. <style> elements are not parsed asynchronously. They're just parsed when inserted into the document. @style attributes are parsed at various points and can get reparsed from source in some cases, as I recall. So any setup which allows @style but doesn't change the actual attribute value is not secure. Ditto for a setup that allows <style> but doesn't change its textContent.

(In reply to comment #39) > As I said in comment 33, the only CSS property which we support that can lead > to code execution is -moz-binding. I think what Rob's concerned about is the situation where a user copies content from evil site X into site Y with Firefox, and then another user views site Y with IE.

(In reply to comment #47) > I think what Rob's concerned about is the situation where a user copies content > from evil site X into site Y with Firefox, and then another user views site Y > with IE. If we're running the CSS through our parser, than anything we don't support will get dropped.

Comment on attachment 434602 [details] [diff] [review] Part 3: Sanitize -moz-binding from styles >+ class PropertyFilterSetter { >+ public: >+ explicit PropertyFilterSetter(nsIDocument* aDoc) >+ : mLoader(aDoc->CSSLoader()) { >+ NS_ASSERTION(mLoader, "The document should have a CSS loader"); >+ mLoader->SetPropertyFilter(&mFilter); Maybe this should assert that the property filter on the loader is currently null? >+ } >+ ~PropertyFilterSetter() { >+ mLoader->SetPropertyFilter(nsnull); >+ } The idea here seems fine to me; I think bzbarsky should probably be the code reviewer since he knows more about the loader/parser setup than I do.

oh, and PropertyFilterSetter could use the macros from bug 531460 if that lands first, although I'm not sure if it's worth it for something with one user.

(In reply to comment #46) > Ah, ok. > > <style> elements are not parsed asynchronously. They're just parsed when > inserted into the document. Well, what I saw in the debugger votes against this, because when I got my breakpoint in mozilla::css::Loader::ParseSheet, none of the fragment parsing code was on the stack. IIRC there was a call from the HTML parser which seemed that it's resuming parsing, and below that was the event loop code, so maybe what I saw was the parser being interrupted? The thing is, that happened after my PropertyFilterSetter was destroyed, which means that there is no property filter any more. > @style attributes are parsed at various points and can get reparsed from source > in some cases, as I recall. Oh, that probably means that I should look into the attribute value (and modify that if needed). And if I do that, I might as well modify the <style> text node contents too. > So any setup which allows @style but doesn't change the actual attribute value > is not secure. Ditto for a setup that allows <style> but doesn't change its > textContent. OK. Now, here comes the dirty part. Can I just look for "-moz-binding", and change it to something like "-xxx-binding", which is dropped by the CSS parser and doesn't generate a warning on the error console (because the parser assumes that it's a vendor specific thing?) The reason I want to do that is that I don't want to write a duplicate CSS mini-parser to parse the entire declaration and remove it. The downside is that it's a dirty hack and I won't be proud of it.

(In reply to comment #49) > The idea here seems fine to me; I think bzbarsky should probably be the code > reviewer since he knows more about the loader/parser setup than I do. According to comment 46, I don't think that this is a good solution.

(In reply to comment #47) > > I think what Rob's concerned about is the situation where a user copies content > from evil site X into site Y with Firefox, and then another user views site Y > with IE. Yes, that's right. In that case, the web site should have validated the input, so we could claim "not our fault", but it seems better not to have any such thing coming in via Firefox.

We won't have that problem, see comment 48.

This patch addresses comment 34.

Comment on attachment 434602 [details] [diff] [review] Part 3: Sanitize -moz-binding from styles This patch also needs to be updated.

Comment on attachment 435339 [details] [diff] [review] Part 2: Allow style attributes and tags > // bail if it's a script or style, or we're already inside one of those This comment needs to be updated. Also, there should be tests verifying that dangerous CSS is being properly filtered.

> because when I got my breakpoint in mozilla::css::Loader::ParseSheet, none of > the fragment parsing code was on the stack. Sure. That's because the insertion into the document doesn't happen until after all the parsing is done and we have a fragment. So it's not quite "asynchronous" in the sense that you don't know when it will happen; it will happen once someone takes the DocumentFragment and puts it somewhere. For the innerHTML case we even know exactly when _that_ will happen. Might not help you much, though. > Now, here comes the dirty part. Can I just look for "-moz-binding", and > change it to something like "-xxx-binding", which is dropped by the CSS parser Given CSS character escapes, not really. Or rather, your "look for" step might get somewhat complicated. > I don't want to write a duplicate CSS mini-parser Indeed. Is there a reason our normal CSS parser can't be used here?

(In reply to comment #57) > (From update of attachment 435339 [details] [diff] [review]) > > // bail if it's a script or style, or we're already inside one of those > > This comment needs to be updated. Done. > Also, there should be tests verifying that dangerous CSS is being properly > filtered. Sure. That will be part of the 3rd patch on this bug.

This is the third part of the patch, which sanitizes the CSS inside style attributes and elements. I've used nsCSSParser to do the parsing, and I actually modify the textual value of the style attributes and textnodes under style elements. The code to sanitize style elements is scary, but I'm not sure if it's possible to make it simpler. I'll also have to add a few more tests, but I thought I'd ask for bz's feedback before that to make sure that I'm moving in the right direction here.

Comment on attachment 435386 [details] [diff] [review] Part 2: Allow style attributes and tags >diff --git a/content/html/document/src/nsHTMLFragmentContentSink.cpp b/content/html/document/src/nsHTMLFragmentContentSink.cpp >-NS_IMPL_ISUPPORTS_INHERITED0(nsHTMLParanoidFragmentSink, >- nsHTMLFragmentContentSink) >+NS_IMPL_ADDREF_INHERITED(nsHTMLParanoidFragmentSink, nsHTMLFragmentContentSink) >+NS_IMPL_RELEASE_INHERITED(nsHTMLParanoidFragmentSink, nsHTMLFragmentContentSink) >+NS_IMPL_QUERY_INTERFACE_INHERITED1(nsHTMLParanoidFragmentSink, >+ nsHTMLFragmentContentSink, >+ nsIParanoidFragmentContentSink) Not NS_IMPL_ISUPPORTS_INHERITED1? >+ NS_IMETHOD AllowStyles() = 0; Since the implementation can't fail: s/NS_IMETHOD/virtual void/.

Updated part 2 per peterv's review comments.

(In reply to comment #60) > Created an attachment (id=436069) [details] > Part 3: Sanitize CSS styles bz: ping?

Sorry for the lag here... Generally, this approach seems ok. Questions/comments: 1) Do we want to be using mTargetDocument->NodePrincipal() instead of the element's principal both places? 2) The |else { if () .. }| pattern should use "else if". 3) Setting mInStyle to false if mInStyle but name != nsGkAtoms::style seems odd. 4) You need to keep @namespace rules. So you need to look at nsICSSRule types, not the DOM ones. In fact, you can just iterate over the non-DOM css style sheet; that will save grief over the GetCssRules security checks. 5) If you were to have an nsICSSStyleRule anyway, you could share the moz-binding-clearing code, perhaps. 6) No need to reinvent nsICSSStyleRule::GetCssText.

(In reply to comment #65) > Sorry for the lag here... > > Generally, this approach seems ok. > > Questions/comments: > > 1) Do we want to be using mTargetDocument->NodePrincipal() instead of the > element's principal both places? I'm not sure. What are the implications of doing so? > 2) The |else { if () .. }| pattern should use "else if". > 3) Setting mInStyle to false if mInStyle but name != nsGkAtoms::style seems > odd. Hmm, yes, I can see the case where a broken HTML includes </foo> before </style>, you're right. I'll correct this. > 4) You need to keep @namespace rules. So you need to look at nsICSSRule > types, not the DOM ones. In fact, you can just iterate over the non-DOM > css style sheet; that will save grief over the GetCssRules security checks. OK, I'll investigate that. > 5) If you were to have an nsICSSStyleRule anyway, you could share the > moz-binding-clearing code, perhaps. OK, good point. > 6) No need to reinvent nsICSSStyleRule::GetCssText. What do you mean?

I addressed all of bz's comments in comment 65, except for 1 and 6 which I was not sure about. This is otherwise ready for review, I think.

> I'm not sure. What are the implications of doing so? If the element has mTargetDocument as the owner document, then at the moment none whatsoever, since it will have the same principal. Longer-term, it's a matter of whether we want the actor (which is the element) or the thing that will be acted on (sorta the document, I guess...) > What do you mean? nsICSSStyleRule has a method that hands back its selector string followed by the decl serialize in between curlies: GetCssText. Is there a reason to not use it here?

(In reply to comment #68) > > I'm not sure. What are the implications of doing so? > > If the element has mTargetDocument as the owner document, then at the moment > none whatsoever, since it will have the same principal. Longer-term, it's a > matter of whether we want the actor (which is the element) or the thing that > will be acted on (sorta the document, I guess...) I'm not sure what the correct thing to do here is, really. I can easily switch to using the node principal, so please r- if you want me to do so. > > What do you mean? > > nsICSSStyleRule has a method that hands back its selector string followed by > the decl serialize in between curlies: GetCssText. Is there a reason to not > use it here? You're right. Switched to using GetCssText here.

Comment on attachment 450953 [details] [diff] [review] Part 3: Sanitize CSS styles This pattern: PRBool styleValid = PR_FALSE; rv = ...; if (NS_SUCEEDED(rv)) { ... styleValid = PR_TRUE; } if (!styleValid) { .... } could just drop the styleValid thing and use an else clause. I'd still like to see that if inside else in AddAttribute changed to an "else if" so you don't reindent the <a> case. We're pretty sure that markup like this: <style> <style> </style> </style> doesn't give this code conniptions? Add a test to that effect? > + nsRefPtr<nsICSSRule> rule = nsnull; Don't need the "= nsnull" bit. Please file a bug on the crappy GetType signature on nsICSSRule? You still need a case for namespace rules in the rule type switch. And a test that would catch that.

(In reply to comment #70) > We're pretty sure that markup like this: > > <style> > <style> > </style> > </style> > > doesn't give this code conniptions? Add a test to that effect? What happens with such code is that the first </style> is considered as the closing of the style tag, and that's how I handle it inside a code as well. Added a test for this. Thanks for bringing this to my attention. > Please file a bug on the crappy GetType signature on nsICSSRule? Filed bug 571946. > You still need a case for namespace rules in the rule type switch. And a test > that would catch that. Yes, sorry for missing that. I've included the fix + the tests in this patch. Also, I addressed the rest of comment 70.

To my great surprise, bugzilla's interdiff can actually handle this! <https://bugzilla.mozilla.org/attachment.cgi?oldid=450953&action=interdiff&newid=451108&headers=1>

Comment on attachment 451108 [details] [diff] [review] Part 3: Sanitize CSS styles r=bzbarsky

we're wrapping up 1.9.2.6/1.9.1.11, how worried about regressions should we be? Leaning toward leaving it for the next set of releases.

(In reply to comment #76) > we're wrapping up 1.9.2.6/1.9.1.11, how worried about regressions should we be? > Leaning toward leaving it for the next set of releases. The patch is well tested, but it's not urgent enough to be worth the risk of taking on branches right now. Let's leave it for the next dot-release.

In order to take this on the branches, we would also need the patch in bug 572642.

Comment on attachment 451119 [details] [diff] [review] Folded patch > // {A47E9526-6E48-4574-9D6C-3164E271F74E} > #define NS_HTMLPARANOIDFRAGMENTSINK_CID \ > { 0xa47e9526, 0x6e48, 0x4574, { 0x9d, 0x6c, 0x31, 0x64, 0xe2, 0x71, 0xf7, 0x4e } } > >+// {A47EF526-6E48-4574-9D60-3164E271F75E} >+#define NS_HTMLPARANOIDFRAGMENTSINK2_CID \ >+{ 0xa47ef526, 0x6e48, 0x4574, { 0x9d, 0x60, 0x31, 0x64, 0xe2, 0x71, 0xf7, 0x5e } } Please don't just change three bits and think that's cool. Use a fresh uuid, please. http://quotes.burntelectrons.org/3303 > // {2D78BBF0-E26C-482B-92B3-78A7B2AFC8F7} > #define NS_XHTMLPARANOIDFRAGMENTSINK_CID \ > { 0x2d78bbf0, 0xe26c, 0x482b, { 0x92, 0xb3, 0x78, 0xa7, 0xb2, 0xaf, 0xc8, 0xf7} } > >+// {AD78BBF0-E261-482B-32B3-78A7B2AFC8F7} >+#define NS_XHTMLPARANOIDFRAGMENTSINK2_CID \ >+{ 0xad78bbf0, 0xe261, 0x482b, { 0x32, 0xb3, 0x78, 0xa7, 0xb2, 0xaf, 0xc8, 0xf7} } Same. >+#define NS_I_PARANOID_FRAGMENT_CONTENT_SINK_IID \ >+ { 0x59ec77f5, 0x9e9b, 0x4040, \ >+ { 0xbd, 0xe7, 0x4e, 0xd0, 0x13, 0xa6, 0x21, 0x74 } } Not sure if this one is affected or not, but mentioning it, too.

(In reply to comment #79) > (From update of attachment 451119 [details] [diff] [review]) > > // {A47E9526-6E48-4574-9D6C-3164E271F74E} > > #define NS_HTMLPARANOIDFRAGMENTSINK_CID \ > > { 0xa47e9526, 0x6e48, 0x4574, { 0x9d, 0x6c, 0x31, 0x64, 0xe2, 0x71, 0xf7, 0x4e } } > > > >+// {A47EF526-6E48-4574-9D60-3164E271F75E} > >+#define NS_HTMLPARANOIDFRAGMENTSINK2_CID \ > >+{ 0xa47ef526, 0x6e48, 0x4574, { 0x9d, 0x60, 0x31, 0x64, 0xe2, 0x71, 0xf7, 0x5e } } > > Please don't just change three bits and think that's cool. Use a fresh uuid, > please. > > http://quotes.burntelectrons.org/3303 > > > // {2D78BBF0-E26C-482B-92B3-78A7B2AFC8F7} > > #define NS_XHTMLPARANOIDFRAGMENTSINK_CID \ > > { 0x2d78bbf0, 0xe26c, 0x482b, { 0x92, 0xb3, 0x78, 0xa7, 0xb2, 0xaf, 0xc8, 0xf7} } > > > >+// {AD78BBF0-E261-482B-32B3-78A7B2AFC8F7} > >+#define NS_XHTMLPARANOIDFRAGMENTSINK2_CID \ > >+{ 0xad78bbf0, 0xe261, 0x482b, { 0x32, 0xb3, 0x78, 0xa7, 0xb2, 0xaf, 0xc8, 0xf7} } > > Same. http://hg.mozilla.org/mozilla-central/rev/2f83edbbeef0 > >+#define NS_I_PARANOID_FRAGMENT_CONTENT_SINK_IID \ > >+ { 0x59ec77f5, 0x9e9b, 0x4040, \ > >+ { 0xbd, 0xe7, 0x4e, 0xd0, 0x13, 0xa6, 0x21, 0x74 } } > > Not sure if this one is affected or not, but mentioning it, too. Nope, that was good, but it's changed now anyway.

Comment on attachment 451119 [details] [diff] [review] Folded patch Approved for 1.9.2.9 and 1.9.1.12, a=dveditz for release-drivers

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/7deefe83af86 http://hg.mozilla.org/releases/mozilla-1.9.1/rev/cac76333f1b2

Backed out because of build bustage failures: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/4300e79948ec http://hg.mozilla.org/releases/mozilla-1.9.1/rev/17fb6acf28b7 So, nsCSSParser is not available on branch. Boris, is there an easy replacement for me to use, which doesn't require me to rewrite everything?

nsICSSParser should work; just ask the relevant document's CSSLoader for one.

Here's a branch specific version of the patch using nsICSSParser and friends. I've verified locally that it compiles and passes the tests.

Comment on attachment 460705 [details] [diff] [review] Branch patch Adding a review request to dbaron as well, in hopes that I can try to get this landed today...

Comment on attachment 460705 [details] [diff] [review] Branch patch r=me

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/02c4c024cf25 http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e65ddc3d5e62

Verified fixed for 1.9.1 and 1.9.2 using the various attached testcases and comparing with pre-fix behavior. Verified for 1.9.1 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.12pre) Gecko/20100809 Shiretoko/3.5.12pre ( .NET CLR 3.5.30729). Verified for 1.9.2 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.9pre) Gecko/20100809 Namoroka/3.6.9pre ( .NET CLR 3.5.30729).