382754 - Unsafe DOM mutation events fired in presentation code on 1.8 and 1.8.0

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Vlad Sukhoy]

Content/Layout on 1.8 and 1.8.0 fire DOM mutation events on any call to tree manipulation methods, that is nsGenericElement::SetAttr, nsGenericElement::UnsetAttr, nsGenericElement::AppendChildTo, nsGenericElement::RemoveChildAt even when aNotify is false.

When these methods are used from within frame code in many instances the JavaScript mutation events are triggered while presentation tree is in an inconsistent state, it is unlikely that the frame code developers were aware of such complications at the time of writing, therefore each call site of the above within layout is a potential security risk. In fact, bug 382568, bug 382681, bug 382700 were all found after a surface screening of some of call sites of the above within frame code.

See the linked bugs for some specific examples of the issue.

Also see the discussion in bug 355548.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

In bug 90983 I made us not send these events when aNotify is false. I don't see a reason we couldn't do this on branch as well.

Comment 2

[Vlad Sukhoy]

bz has some reasons though, see bug 355548 comment 41 .

Comment 3

[Boris Zbarsky [:bzbarsky]]

If we don't break any of the branch code and any of the various Mozilla-based apps and binary extensions we promised compat with on branch, go for it.  Someone needs to do that testing, though.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Not all SetAttr calls in layout are unsafe. Worst are those which 
happen during layout, but most others, if handled carefully unfortunately this isn't always the case), are ok.
If the DOM mutation happens in native anonymous, we could stop
propagating those mutation events to non-anon-content (like we do
in trunk).

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

If we go down that route then for native-anonymous content we could simply avoid firing the event at all. I.e. we wouldn't fire if aNotify is false and it's a native-anonymous node.

Comment 6

[Vlad Sukhoy]

Attachment: No mutation if aNotify is false.

Added revised aNotify patch. The issue with native-anonymous content is indeed separate, testcase in bug 382568 still crashes (perhaps, another metabug for that?).

Comment 7

[Vlad Sukhoy]

...bugzilla newbie... sorry (

Comment 8

[Vlad Sukhoy]

@Boris/Jonas - I'm wondering if you guys could take a glance at that patch for aNotify i.e. attachment 266968 [details] [diff] [review].. The issue is still around and then there is native-anonymous stuff.. Firefox 2 will be around for a while and it is vulnerable to this kind of thing. If the patch is ok then someone is needed to test all sorts of stuff that could be broken due to it, if the patch is not ok then I'll write another..

Comment 9

[Boris Zbarsky [:bzbarsky]]

I'm probably not going to be able to look at this for at least several weeks, unfortunately.  :(

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 266968 [details] [diff] [review]
No mutation if aNotify is false.

>Index: base/src/nsGenericElement.cpp
>@@ -2827,41 +2827,40 @@ doInsertChildAt(nsIContent* aKid, PRUint
>   
>   // The kid may have removed its parent from the document, so recheck that
>   // that's still in the document before proceeding.  Also, the kid may have
>   // just removed itself, in which case we don't really want to fire
>   // ContentAppended or a mutation event.
>   // XXXbz What if the kid just moved us in the document?  Scripts suck.  We
>   // really need to stop running them while we're in the middle of modifying
>   // the DOM....
>-  if (aDocument && aKid->GetCurrentDoc() == aDocument &&
>+  // XXX bug 382754: not fire mutation events if aNotify is false.
>+  if (aNotify && aDocument && aKid->GetCurrentDoc() == aDocument &&
>       (!aParent || aKid->GetParent() == aParent)) {
>-    if (aNotify) {
>+    if (aParent) {
>       // Note that we always want to call ContentInserted when things ar
>       // as kids to documents
>-      if (aParent && isAppend) {
>+      if (isAppend) {
>         aDocument->ContentAppended(aParent, aIndex);
>       } else {
>         aDocument->ContentInserted(aParent, aKid, aIndex);
>       }

This looks wrong, you'll only call ContentInserted if aParent is non-null, that wasn't the case in the old code.

Also, please attach a patch diffed with -w to ease review of the patch.

Comment 11

[Vlad Sukhoy]

Ok..

Comment 12

[Vlad Sukhoy]

Attachment: No mutation if aNotify is false rev2. -w

Ok, here we go.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Sorry, I wasn't clear enough, when attaching a -w patch, always attach a version without it so that the reviewer can check that whitespace is done correctly.

Comment 14

[Vlad Sukhoy]

Ahm, ok. I'm also editing http://developer.mozilla.org/en/docs/Creating_a_patch so that other people wont repeat my stupid mistakes..

Comment 15

[Vlad Sukhoy]

Attachment: No mutation if aNotify is false rev2.

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 268044 [details] [diff] [review]
No mutation if aNotify is false rev2. -w

Looks good.

For future reference though, for a branch patch like this it might be worth creating the simplest possible patch, even though that doesn't necessarily produce the neatest code. I would probably simply have added |aNotify| arguments before every |HasMutationListeners| call rather than to try to consolidate if-statements and the like.

r/sr=me with the patch as it is though.

Comment 17

[Vlad Sukhoy]

Comment on attachment 268044 [details] [diff] [review]
No mutation if aNotify is false rev2. -w

Now, per comment 3 this should be tested with at least most common plugin/websites... bug 355548 comment 50 implies that there is a prior experience of doing this kind of thing, well, it seems that whatever was done can be repeated.

Comment 18

[Vlad Sukhoy]

I don't have access to bug 378963. Is it related to the issue with unsafe DOM events?

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

yes, it's basically a dup of this one

Comment 20

[Daniel Veditz [:dveditz]]

Does this patch *fix* the "depends on" bugs (355548, 382568, 382681, 382700)? if so it would be better marked as "blocking" those. If not, in what way does this patch depend on those currently-unfixed bugs?

Comment 21

[Vlad Sukhoy]

For bug 355548 it does indeed fix the testcase on the bug, i.e. attachment 266332 [details] no longer crashes branches. I suspect there is still some way to crash MathML frame style reresolution code there and also on trunk, in particular - see bug 378146 for an example of debug-only crash on trunk, but this patch does fix the mutation event vector there.
For others, disabling native-anonymous content as it is done in attachment 267939 [details] [diff] [review] should do.

Note that there are more vulnerable call-sites with aNotify=false in MathML code, see e.g. bug 355548 comment 40. This patch fixes those as well. Also there suspicious call-sites with aNotify=false within XUL.

Comment 22

[Vlad Sukhoy]

Adjusted the blocking chain to reflect the current idea of what fixes what here.

Comment 23

[Daniel Veditz [:dveditz]]

Comment on attachment 268044 [details] [diff] [review]
No mutation if aNotify is false rev2. -w

approved for 1.8.1.5, a=dveditz for release-drivers

Comment 24

[Damon Sicore (:damons)]

Who's landing this?

Comment 25

[Vlad Sukhoy]

I mailed Jonas on July 1st about committing the patch..

Comment 26

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The best place to find someone to check it in is to go on irc.mozilla.org #developers.

Comment 27

[Daniel Veditz [:dveditz]]

Comment on attachment 268044 [details] [diff] [review]
No mutation if aNotify is false rev2. -w

approved for 1.8.0.13, a=dveditz

Comment 28

[Vlad Sukhoy]

Checked in on branches.