Last Comment Bug 520189 - (CVE-2010-2769) Copy-and-paste or drag-and-drop into designMode document allows XSS
(CVE-2010-2769)
: Copy-and-paste or drag-and-drop into designMode document allows XSS
Status: RESOLVED FIXED
[sg:moderate][needs bug 572642 for br...
: testcase, verified1.9.1, verified1.9.2
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla2.0b1
Assigned To: :Ehsan Akhgari
:
Mentors:
: 560725 (view as bug list)
Depends on: 606117 613912 572290 572637 572642 580442 592601 596300 596797 597190 597784 598090 598105 604332 613130 632326 647682
Blocks: 519928 568395
  Show dependency treegraph
 
Reported: 2009-10-02 09:28 PDT by Paul Stone
Modified: 2011-04-07 07:24 PDT (History)
21 users (show)
ehsan: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.9+
.9-fixed
.12+
.12-fixed


Attachments
XSS payload (1.15 KB, text/html)
2009-10-02 09:28 PDT, Paul Stone
no flags Details
Simple Testcase (420 bytes, text/html)
2009-10-02 09:29 PDT, Paul Stone
no flags Details
PoC (7.31 KB, text/html)
2009-10-02 09:35 PDT, Paul Stone
no flags Details
contentEditable XSS (665 bytes, text/html)
2009-10-05 01:47 PDT, Paul Stone
no flags Details
Patch (v1) (10.70 KB, patch)
2010-03-17 10:37 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
contentEditable XSS (with <script> tag) (7.31 KB, text/html)
2010-03-18 02:11 PDT, Paul Stone
no flags Details
contentEditable XSS (with <script> tag) (560 bytes, text/html)
2010-03-18 02:12 PDT, Paul Stone
no flags Details
contentEditable XSS (with clickable link) (608 bytes, text/html)
2010-03-18 02:45 PDT, Paul Stone
no flags Details
Patch (v2) (16.58 KB, patch)
2010-03-19 12:36 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
Part 1: use paranoid document fragment sink (19.22 KB, patch)
2010-03-19 17:15 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
Part 1: use paranoid document fragment sink (19.91 KB, patch)
2010-03-23 11:20 PDT, :Ehsan Akhgari
sayrer: review+
roc: superreview+
Details | Diff | Splinter Review
Part 2: Allow style attributes and tags (16.88 KB, patch)
2010-03-23 16:55 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
Part 3: Sanitize -moz-binding from styles (13.03 KB, patch)
2010-03-24 11:33 PDT, :Ehsan Akhgari
dbaron: feedback+
Details | Diff | Splinter Review
Part 2: Allow style attributes and tags (14.83 KB, patch)
2010-03-26 18:32 PDT, :Ehsan Akhgari
sayrer: review+
Details | Diff | Splinter Review
Part 2: Allow style attributes and tags (14.98 KB, patch)
2010-03-27 08:43 PDT, :Ehsan Akhgari
peterv: review+
roc: superreview+
Details | Diff | Splinter Review
Part 3: Sanitize CSS styles (15.28 KB, patch)
2010-03-30 16:42 PDT, :Ehsan Akhgari
bzbarsky: feedback+
Details | Diff | Splinter Review
Part 2: Allow style attributes and tags (14.86 KB, patch)
2010-04-09 14:13 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
Part 3: Sanitize CSS styles (17.09 KB, patch)
2010-06-11 17:51 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
Part 3: Sanitize CSS styles (16.88 KB, patch)
2010-06-13 15:04 PDT, :Ehsan Akhgari
bzbarsky: review-
Details | Diff | Splinter Review
Part 3: Sanitize CSS styles (20.19 KB, patch)
2010-06-14 13:20 PDT, :Ehsan Akhgari
bzbarsky: review+
Details | Diff | Splinter Review
Folded patch (43.05 KB, patch)
2010-06-14 13:51 PDT, :Ehsan Akhgari
dveditz: approval1.9.2.9+
dveditz: approval1.9.1.12+
Details | Diff | Splinter Review
Branch patch (43.64 KB, patch)
2010-07-27 17:01 PDT, :Ehsan Akhgari
bzbarsky: review+
Details | Diff | Splinter Review

Description Paul Stone 2009-10-02 09:28:09 PDT
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
Comment 1 Paul Stone 2009-10-02 09:28:34 PDT
Created attachment 404261 [details]
XSS payload
Comment 2 Paul Stone 2009-10-02 09:29:26 PDT
Created attachment 404262 [details]
Simple Testcase

This demonstrates the basic bug.
Comment 3 Paul Stone 2009-10-02 09:35:10 PDT
Created attachment 404265 [details]
PoC

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.
Comment 4 Paul Stone 2009-10-05 01:24:59 PDT
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.
Comment 5 Paul Stone 2009-10-05 01:47:06 PDT
Created attachment 404572 [details]
contentEditable XSS

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.
Comment 6 Paul Stone 2010-01-12 06:43:55 PST
Any update on this (or bug 519928)? It's been 3 months and neither bug has been confirmed.
Comment 7 Brandon Sterne (:bsterne) 2010-03-12 11:14:08 PST
Confirming.  Chrome apparently implemented a fix for this bug recently.
Comment 8 Michal Zalewski 2010-03-12 11:17:49 PST
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.
Comment 9 :Ehsan Akhgari 2010-03-16 14:31:26 PDT
I have a patch in the works.
Comment 10 :Ehsan Akhgari 2010-03-17 10:37:31 PDT
Created attachment 433093 [details] [diff] [review]
Patch (v1)

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.
Comment 11 Paul Stone 2010-03-18 02:11:56 PDT
Created attachment 433285 [details]
contentEditable XSS (with <script> tag)

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.
Comment 12 Paul Stone 2010-03-18 02:12:48 PDT
Created attachment 433286 [details]
contentEditable XSS (with <script> tag)

Oops, uploaded wrong file.
Comment 13 Paul Stone 2010-03-18 02:45:46 PDT
Created attachment 433291 [details]
contentEditable XSS (with clickable link)

This version uses contentEditable=false to create a clickable link that navigates to a data URI.
Comment 14 Paul Stone 2010-03-18 03:30:21 PDT
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.
Comment 15 :Ehsan Akhgari 2010-03-19 12:36:31 PDT
Created attachment 433611 [details] [diff] [review]
Patch (v2)

This patch adds handling for script elements.  I still need to handle some other cases, so this is not ready for review yet.
Comment 16 Robert Sayre 2010-03-19 15:41:10 PDT
It seems to me that mitigating this attack requires dealing with pasted CSS as well as HTML and script.
Comment 17 :Ehsan Akhgari 2010-03-19 17:15:16 PDT
Created attachment 433670 [details] [diff] [review]
Part 1: use paranoid document fragment sink

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.
Comment 18 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-03-20 02:14:38 PDT
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?
Comment 19 Robert Sayre 2010-03-20 09:58:58 PDT
(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
Comment 20 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-03-20 13:26:35 PDT
Nice. We should add <video> and <audio> to that list.
Comment 21 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-03-20 13:27:10 PDT
And for the HTML5 parser, we should add a bunch of MathML and SVG stuff.
Comment 22 Paul Stone 2010-03-21 07:05:11 PDT
(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?
Comment 23 :Ehsan Akhgari 2010-03-22 12:33:04 PDT
(In reply to comment #20)
> Nice. We should add <video> and <audio> to that list.

Filed bug 554125 for that, with a patch.
Comment 24 :Ehsan Akhgari 2010-03-22 13:11:15 PDT
(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.
Comment 25 :Ehsan Akhgari 2010-03-22 15:17:28 PDT
(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?
Comment 26 Johnny Stenback (:jst, jst@mozilla.com) 2010-03-22 16:13:03 PDT
We have not yet disabled remote XBL/XUL, but we plan to do so on Q2 of 2010.
Comment 27 Paul Stone 2010-03-23 02:43:16 PDT
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.
Comment 28 :Ehsan Akhgari 2010-03-23 10:28:13 PDT
(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.
Comment 29 Paul Stone 2010-03-23 10:31:51 PDT
(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 30 :Ehsan Akhgari 2010-03-23 11:20:34 PDT
Created attachment 434283 [details] [diff] [review]
Part 1: use paranoid document fragment sink
Comment 31 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-03-23 11:36:20 PDT
Comment on attachment 434283 [details] [diff] [review]
Part 1: use paranoid document fragment sink

Can you document somewhere what the aAllContent parameter does?
Comment 32 :Ehsan Akhgari 2010-03-23 11:39:24 PDT
(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?
Comment 33 :Ehsan Akhgari 2010-03-23 16:55:55 PDT
Created attachment 434415 [details] [diff] [review]
Part 2: Allow style attributes and tags

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.
Comment 34 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-03-23 17:08:11 PDT
Instead of introducing this extension mechanism, how about we introduce an additional mode flag to the sink, say "enable CSS"?
Comment 35 :Ehsan Akhgari 2010-03-23 21:44:49 PDT
(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.
Comment 36 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-03-23 22:17:00 PDT
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.
Comment 37 Robert Sayre 2010-03-23 22:18:42 PDT
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
Comment 38 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-03-23 22:23:13 PDT
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?
Comment 39 :Ehsan Akhgari 2010-03-24 08:13:40 PDT
(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!).
Comment 40 :Ehsan Akhgari 2010-03-24 11:33:27 PDT
Created attachment 434602 [details] [diff] [review]
Part 3: Sanitize -moz-binding from styles

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.
Comment 41 Boris Zbarsky [:bz] 2010-03-24 11:37:04 PDT
Does the sanitizer already forbid <link rel="stylesheet">?
Comment 42 Robert Sayre 2010-03-24 12:55:10 PDT
(In reply to comment #41)
> Does the sanitizer already forbid <link rel="stylesheet">?

Nothing that belongs in the <head> is in the whitelist.
Comment 43 :Ehsan Akhgari 2010-03-24 12:56:37 PDT
(In reply to comment #41)
> Does the sanitizer already forbid <link rel="stylesheet">?

It blocks all link elements.
Comment 44 Boris Zbarsky [:bz] 2010-03-24 13:15:53 PDT
Then why is <style> in the whitelist?
Comment 45 Robert Sayre 2010-03-24 13:21:30 PDT
(In reply to comment #44)
> Then why is <style> in the whitelist?

It's not currently... Ehsan is enabling it for designMode uses.
Comment 46 Boris Zbarsky [:bz] 2010-03-24 13:26:41 PDT
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.
Comment 47 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-03-24 14:37:51 PDT
(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.
Comment 48 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2010-03-24 17:32:23 PDT
(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 49 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2010-03-24 17:35:38 PDT
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.
Comment 50 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2010-03-24 17:40:48 PDT
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.
Comment 51 :Ehsan Akhgari 2010-03-25 16:26:01 PDT
(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.
Comment 52 :Ehsan Akhgari 2010-03-25 16:31:26 PDT
(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.
Comment 53 Robert Sayre 2010-03-26 13:42:22 PDT
(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.
Comment 54 :Ehsan Akhgari 2010-03-26 16:25:05 PDT
We won't have that problem, see comment 48.
Comment 55 :Ehsan Akhgari 2010-03-26 18:32:06 PDT
Created attachment 435339 [details] [diff] [review]
Part 2: Allow style attributes and tags

This patch addresses comment 34.
Comment 56 :Ehsan Akhgari 2010-03-26 18:32:55 PDT
Comment on attachment 434602 [details] [diff] [review]
Part 3: Sanitize -moz-binding from styles

This patch also needs to be updated.
Comment 57 Robert Sayre 2010-03-26 18:49:46 PDT
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.
Comment 58 Boris Zbarsky [:bz] 2010-03-26 22:33:44 PDT
> 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?
Comment 59 :Ehsan Akhgari 2010-03-27 08:43:35 PDT
Created attachment 435386 [details] [diff] [review]
Part 2: Allow style attributes and tags

(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.
Comment 60 :Ehsan Akhgari 2010-03-30 16:42:30 PDT
Created attachment 436069 [details] [diff] [review]
Part 3: Sanitize CSS styles

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 61 Peter Van der Beken [:peterv] - away till Aug 1st 2010-04-09 12:11:31 PDT
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/.
Comment 62 :Ehsan Akhgari 2010-04-09 14:13:37 PDT
Created attachment 438169 [details] [diff] [review]
Part 2: Allow style attributes and tags

Updated part 2 per peterv's review comments.
Comment 63 :Ehsan Akhgari 2010-04-09 14:18:31 PDT
(In reply to comment #60)
> Created an attachment (id=436069) [details]
> Part 3: Sanitize CSS styles

bz: ping?
Comment 64 :Ehsan Akhgari 2010-04-25 16:25:40 PDT
*** Bug 560725 has been marked as a duplicate of this bug. ***
Comment 65 Boris Zbarsky [:bz] 2010-06-10 22:09:45 PDT
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.
Comment 66 :Ehsan Akhgari 2010-06-11 14:40:08 PDT
(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?
Comment 67 :Ehsan Akhgari 2010-06-11 17:51:45 PDT
Created attachment 450794 [details] [diff] [review]
Part 3: Sanitize CSS styles

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.
Comment 68 Boris Zbarsky [:bz] 2010-06-11 20:45:04 PDT
> 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?
Comment 69 :Ehsan Akhgari 2010-06-13 15:04:25 PDT
Created attachment 450953 [details] [diff] [review]
Part 3: Sanitize CSS styles

(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 70 Boris Zbarsky [:bz] 2010-06-14 11:54:57 PDT
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.
Comment 71 :Ehsan Akhgari 2010-06-14 13:20:40 PDT
Created attachment 451108 [details] [diff] [review]
Part 3: Sanitize CSS styles

(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.
Comment 72 :Ehsan Akhgari 2010-06-14 13:21:36 PDT
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 73 Boris Zbarsky [:bz] 2010-06-14 13:44:07 PDT
Comment on attachment 451108 [details] [diff] [review]
Part 3: Sanitize CSS styles

r=bzbarsky
Comment 74 :Ehsan Akhgari 2010-06-14 13:51:35 PDT
Created attachment 451119 [details] [diff] [review]
Folded patch
Comment 76 Daniel Veditz [:dveditz] 2010-06-16 10:49:09 PDT
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.
Comment 77 :Ehsan Akhgari 2010-06-16 10:58:17 PDT
(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.
Comment 78 :Ehsan Akhgari 2010-06-23 12:13:12 PDT
In order to take this on the branches, we would also need the patch in bug 572642.
Comment 79 Reed Loden [:reed] (use needinfo?) 2010-06-23 12:32:35 PDT
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.
Comment 80 :Ehsan Akhgari 2010-06-23 13:04:25 PDT
(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 81 Daniel Veditz [:dveditz] 2010-07-23 11:02:19 PDT
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
Comment 83 :Ehsan Akhgari 2010-07-23 14:19:40 PDT
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?
Comment 84 Boris Zbarsky [:bz] 2010-07-23 19:30:43 PDT
nsICSSParser should work; just ask the relevant document's CSSLoader for one.
Comment 85 :Ehsan Akhgari 2010-07-27 17:01:14 PDT
Created attachment 460705 [details] [diff] [review]
Branch patch

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 86 :Ehsan Akhgari 2010-07-28 06:46:12 PDT
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 87 Boris Zbarsky [:bz] 2010-07-28 09:43:26 PDT
Comment on attachment 460705 [details] [diff] [review]
Branch patch

r=me
Comment 89 Al Billings [:abillings] 2010-08-09 17:53:23 PDT
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).

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