Closed Bug 382754 Opened 17 years ago Closed 17 years ago

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


(Core :: DOM: Events, defect)

1.8 Branch
Not set





(Reporter: vladimir.sukhoy, Assigned: vladimir.sukhoy)



(Keywords: fixed1.8.0.13, fixed1.8.1.5, Whiteboard: [sg:critical?] keep private until 355548 is fixed; meta, fixes other sg:critical bugs;)


(2 files, 1 obsolete file)

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.
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.
Flags: wanted1.8.1.x?
bz has some reasons though, see bug 355548 comment 41 .
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.
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).
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.
Assignee: nobody → vladimir.sukhoy
Attached patch No mutation if aNotify is false. (obsolete) — Splinter Review
Added revised aNotify patch. The issue with native-anonymous content is indeed separate, testcase in bug 382568 still crashes (perhaps, another metabug for that?).
Attachment #266968 - Flags: review?(jonas)
...bugzilla newbie... sorry (
@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..
I'm probably not going to be able to look at this for at least several weeks, unfortunately.  :(
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.
Attachment #266968 - Flags: review?(jonas) → review-
Ok, here we go.
Attachment #266968 - Attachment is obsolete: true
Attachment #268044 - Flags: review?(jonas)
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.
Ahm, ok. I'm also editing so that other people wont repeat my stupid mistakes..
Attachment #268044 - Attachment description: No mutation if aNotify is false rev2. → No mutation if aNotify is false rev2. -w
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.
Attachment #268044 - Flags: superreview+
Attachment #268044 - Flags: review?(jonas)
Attachment #268044 - Flags: review+
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.
Attachment #268044 - Flags: approval1.8.1.5?
Blocks: 378963
Flags: wanted1.8.1.x?
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.5+
Flags: blocking1.8.0.13+
Whiteboard: meta, fixes other sg:critical bugs
I don't have access to bug 378963. Is it related to the issue with unsafe DOM events?
yes, it's basically a dup of this one
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?
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.
No longer depends on: 382568, 382700
Adjusted the blocking chain to reflect the current idea of what fixes what here.
Blocks: 355548
No longer depends on: 355548
Comment on attachment 268044 [details] [diff] [review]
No mutation if aNotify is false rev2. -w

approved for, a=dveditz for release-drivers
Attachment #268044 - Flags: approval1.8.1.5? → approval1.8.1.5+
Who's landing this?
I mailed Jonas on July 1st about committing the patch..
The best place to find someone to check it in is to go on #developers.
Attachment #268044 - Flags: approval1.8.0.13?
Comment on attachment 268044 [details] [diff] [review]
No mutation if aNotify is false rev2. -w

approved for, a=dveditz
Attachment #268044 - Flags: approval1.8.0.13? → approval1.8.0.13+
Checked in on branches.
Closed: 17 years ago
Resolution: --- → FIXED
Whiteboard: meta, fixes other sg:critical bugs → [sg:critical?] meta, fixes other sg:critical bugs
Whiteboard: [sg:critical?] meta, fixes other sg:critical bugs → [sg:critical?] meta, fixes other sg:critical bugs; keep private until 355548 is fixed
Whiteboard: [sg:critical?] meta, fixes other sg:critical bugs; keep private until 355548 is fixed → [sg:critical?] keep private until 355548 is fixed; meta, fixes other sg:critical bugs;
Flags: in-testsuite?
Group: core-security
You need to log in before you can comment on or make changes to this bug.