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

Copy-and-paste or drag-and-drop into designMode document allows XSS

RESOLVED FIXED in mozilla2.0b1

Status

()

Core
Editor
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: Paul Stone, Assigned: Ehsan)

Tracking

(Depends on: 2 bugs, {testcase, verified1.9.1, verified1.9.2})

Trunk
mozilla2.0b1
testcase, verified1.9.1, verified1.9.2
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking1.9.2 .9+, status1.9.2 .9-fixed, blocking1.9.1 .12+, status1.9.1 .12-fixed)

Details

(Whiteboard: [sg:moderate][needs bug 572642 for branch] cross-browser issue)

Attachments

(11 attachments, 11 obsolete attachments)

1.15 KB, text/html
Details
420 bytes, text/html
Details
PoC
7.31 KB, text/html
Details
665 bytes, text/html
Details
560 bytes, text/html
Details
608 bytes, text/html
Details
19.91 KB, patch
Robert Sayre
: review+
Details | Diff | Splinter Review
14.86 KB, patch
Details | Diff | Splinter Review
20.19 KB, patch
Details | Diff | Splinter Review
43.05 KB, patch
Details | Diff | Splinter Review
43.64 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

8 years ago
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
(Reporter)

Comment 1

8 years ago
Created attachment 404261 [details]
XSS payload
(Reporter)

Comment 2

8 years ago
Created attachment 404262 [details]
Simple Testcase

This demonstrates the basic bug.
(Reporter)

Comment 3

8 years ago
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.
(Reporter)

Comment 4

8 years ago
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.
(Reporter)

Comment 5

8 years ago
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.
Whiteboard: [sg:investigate] cross-browser issue
(Reporter)

Updated

8 years ago
(Reporter)

Comment 6

7 years ago
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [sg:investigate] cross-browser issue → [sg:moderate] cross-browser issue

Comment 8

7 years ago
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.

Updated

7 years ago
Summary: Drag and Drop into designMode document allows XSS → Copy-and-paste or drag-and-drop into designMode document allows XSS
(Assignee)

Updated

7 years ago
Keywords: testcase
OS: Windows XP → All
Hardware: x86 → All
Version: unspecified → Trunk
(Assignee)

Comment 9

7 years ago
I have a patch in the works.
Assignee: nobody → ehsan.akhgari
Status: NEW → ASSIGNED
(Assignee)

Comment 10

7 years ago
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.
Attachment #433093 - Flags: review?(peterv)
(Reporter)

Comment 11

7 years ago
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.
(Reporter)

Comment 12

7 years ago
Created attachment 433286 [details]
contentEditable XSS (with <script> tag)

Oops, uploaded wrong file.
Attachment #433285 - Attachment is obsolete: true
(Reporter)

Comment 13

7 years ago
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.
(Reporter)

Comment 14

7 years ago
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.
(Assignee)

Comment 15

7 years ago
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.
Attachment #433093 - Attachment is obsolete: true
Attachment #433093 - Flags: review?(peterv)

Comment 16

7 years ago
It seems to me that mitigating this attack requires dealing with pasted CSS as well as HTML and script.
(Assignee)

Comment 17

7 years ago
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.
Attachment #433611 - Attachment is obsolete: true
Attachment #433670 - Flags: superreview?(roc)
Attachment #433670 - Flags: review?
(Assignee)

Updated

7 years ago
Attachment #433670 - Flags: review? → review?(sayrer)
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

7 years ago
(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.
(Reporter)

Comment 22

7 years ago
(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?
(Assignee)

Comment 23

7 years ago
(In reply to comment #20)
> Nice. We should add <video> and <audio> to that list.

Filed bug 554125 for that, with a patch.
(Assignee)

Comment 24

7 years ago
(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.
(Assignee)

Comment 25

7 years ago
(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.
(Reporter)

Comment 27

7 years ago
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.
(Assignee)

Comment 28

7 years ago
(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.
(Reporter)

Comment 29

7 years ago
(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)."
(Assignee)

Comment 30

7 years ago
Created attachment 434283 [details] [diff] [review]
Part 1: use paranoid document fragment sink
Attachment #433670 - Attachment is obsolete: true
Attachment #434283 - Flags: superreview?(roc)
Attachment #434283 - Flags: review?(sayrer)
Attachment #433670 - Flags: superreview?(roc)
Attachment #433670 - Flags: review?(sayrer)
Comment on attachment 434283 [details] [diff] [review]
Part 1: use paranoid document fragment sink

Can you document somewhere what the aAllContent parameter does?
Attachment #434283 - Flags: superreview?(roc) → superreview+
(Assignee)

Comment 32

7 years ago
(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?
(Assignee)

Comment 33

7 years ago
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.
Attachment #434415 - Flags: superreview?(roc)
Attachment #434415 - Flags: review?(sayrer)
Attachment #434415 - Flags: review?(peterv)
Instead of introducing this extension mechanism, how about we introduce an additional mode flag to the sink, say "enable CSS"?
(Assignee)

Comment 35

7 years ago
(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.

Comment 37

7 years ago
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?
(Assignee)

Comment 39

7 years ago
(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!).
(Assignee)

Comment 40

7 years ago
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.
Attachment #434602 - Flags: feedback?(dbaron)
Does the sanitizer already forbid <link rel="stylesheet">?

Comment 42

7 years ago
(In reply to comment #41)
> Does the sanitizer already forbid <link rel="stylesheet">?

Nothing that belongs in the <head> is in the whitelist.
(Assignee)

Comment 43

7 years ago
(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?

Comment 45

7 years ago
(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.
Attachment #434602 - Flags: feedback?(dbaron) → feedback+
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.
(Assignee)

Comment 51

7 years ago
(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.
(Assignee)

Comment 52

7 years ago
(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

7 years ago
(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.
(Assignee)

Comment 54

7 years ago
We won't have that problem, see comment 48.
(Assignee)

Comment 55

7 years ago
Created attachment 435339 [details] [diff] [review]
Part 2: Allow style attributes and tags

This patch addresses comment 34.
Attachment #434415 - Attachment is obsolete: true
Attachment #435339 - Flags: superreview?(roc)
Attachment #435339 - Flags: review?(sayrer)
Attachment #435339 - Flags: review?(peterv)
Attachment #434415 - Flags: superreview?(roc)
Attachment #434415 - Flags: review?(sayrer)
Attachment #434415 - Flags: review?(peterv)
(Assignee)

Comment 56

7 years ago
Comment on attachment 434602 [details] [diff] [review]
Part 3: Sanitize -moz-binding from styles

This patch also needs to be updated.
Attachment #434602 - Attachment is obsolete: true

Updated

7 years ago
Attachment #434283 - Flags: review?(sayrer) → review+

Comment 57

7 years ago
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.
Attachment #435339 - Flags: review?(sayrer) → review+
> 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?
(Assignee)

Comment 59

7 years ago
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.
Attachment #435386 - Flags: superreview?(roc)
Attachment #435386 - Flags: review?(peterv)
(Assignee)

Updated

7 years ago
Attachment #435339 - Attachment is obsolete: true
Attachment #435339 - Flags: superreview?(roc)
Attachment #435339 - Flags: review?(peterv)
Attachment #435386 - Flags: superreview?(roc) → superreview+
(Assignee)

Comment 60

7 years ago
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.
Attachment #436069 - Flags: feedback?(bzbarsky)
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/.
Attachment #435386 - Flags: review?(peterv) → review+
(Assignee)

Comment 62

7 years ago
Created attachment 438169 [details] [diff] [review]
Part 2: Allow style attributes and tags

Updated part 2 per peterv's review comments.
Attachment #435386 - Attachment is obsolete: true
(Assignee)

Comment 63

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

bz: ping?
(Assignee)

Updated

7 years ago
Blocks: 519928
(Assignee)

Updated

7 years ago
Duplicate of this bug: 560725
Blocks: 568395
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.
Attachment #436069 - Flags: feedback?(bzbarsky) → feedback+
(Assignee)

Comment 66

7 years ago
(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?
(Assignee)

Comment 67

7 years ago
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.
Attachment #436069 - Attachment is obsolete: true
Attachment #450794 - Flags: review?(bzbarsky)
> 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?
(Assignee)

Comment 69

7 years ago
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.
Attachment #450794 - Attachment is obsolete: true
Attachment #450953 - Flags: review?(bzbarsky)
Attachment #450794 - Flags: review?(bzbarsky)
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.
Attachment #450953 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 71

7 years ago
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.
Attachment #450953 - Attachment is obsolete: true
Attachment #451108 - Flags: review?(bzbarsky)
(Assignee)

Comment 72

7 years ago
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
Attachment #451108 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 74

7 years ago
Created attachment 451119 [details] [diff] [review]
Folded patch
(Assignee)

Comment 75

7 years ago
http://hg.mozilla.org/mozilla-central/rev/208d7f999037
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a6
(Assignee)

Updated

7 years ago
Attachment #451119 - Flags: approval1.9.2.6?
Attachment #451119 - Flags: approval1.9.1.11?
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
status1.9.1: --- → wanted
status1.9.2: --- → wanted
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.
blocking1.9.1: ? → needed
blocking1.9.2: ? → needed
(Assignee)

Comment 77

7 years ago
(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.
Depends on: 572637
(Assignee)

Updated

7 years ago
No longer depends on: 572637
Attachment #451119 - Flags: approval1.9.2.6? → approval1.9.2.7?
Attachment #451119 - Flags: approval1.9.1.11? → approval1.9.1.12?
(Assignee)

Updated

7 years ago
Depends on: 572642
(Assignee)

Comment 78

7 years ago
In order to take this on the branches, we would also need the patch in bug 572642.
Whiteboard: [sg:moderate] cross-browser issue → [sg:moderate][needs bug 572642 for branch] cross-browser issue
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.
(Assignee)

Comment 80

7 years ago
(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.
(Assignee)

Updated

7 years ago
Depends on: 572637
Depends on: 580442
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
Attachment #451119 - Flags: approval1.9.2.8?
Attachment #451119 - Flags: approval1.9.2.8+
Attachment #451119 - Flags: approval1.9.1.12?
Attachment #451119 - Flags: approval1.9.1.12+
(Assignee)

Comment 82

7 years ago
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/7deefe83af86
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/cac76333f1b2
status1.9.1: wanted → .12-fixed
status1.9.2: wanted → .9-fixed
(Assignee)

Comment 83

7 years ago
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?
status1.9.1: .12-fixed → ?
status1.9.2: .9-fixed → ?
nsICSSParser should work; just ask the relevant document's CSSLoader for one.

Updated

7 years ago
blocking1.9.1: needed → .12+
blocking1.9.2: needed → .9+
status1.9.1: ? → wanted
status1.9.2: ? → wanted
(Assignee)

Comment 85

7 years ago
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.
Attachment #460705 - Flags: review?(bzbarsky)
(Assignee)

Comment 86

7 years ago
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...
Attachment #460705 - Flags: review?(dbaron)
Comment on attachment 460705 [details] [diff] [review]
Branch patch

r=me
Attachment #460705 - Flags: review?(dbaron)
Attachment #460705 - Flags: review?(bzbarsky)
Attachment #460705 - Flags: review+
(Assignee)

Comment 88

7 years ago
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/02c4c024cf25
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e65ddc3d5e62
status1.9.1: wanted → .12-fixed
status1.9.2: wanted → .9-fixed
(Assignee)

Updated

7 years ago
Depends on: 572290
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).
Keywords: verified1.9.1, verified1.9.2
Alias: CVE-2010-2769
(Assignee)

Updated

7 years ago
Depends on: 592601
Depends on: 596797
Depends on: 597190
Depends on: 596300
(Assignee)

Updated

7 years ago
Depends on: 598090
(Assignee)

Updated

7 years ago
Depends on: 598105
(Assignee)

Updated

7 years ago
Depends on: 597784
(Assignee)

Updated

7 years ago
Blocks: 604332
(Assignee)

Updated

7 years ago
No longer blocks: 604332
Depends on: 604332
Group: core-security

Updated

7 years ago
Depends on: 606117
Depends on: 613130
(Assignee)

Updated

6 years ago
Depends on: 632326
(Assignee)

Updated

6 years ago
Depends on: 613912

Updated

6 years ago
Depends on: 647682
You need to log in before you can comment on or make changes to this bug.