Last Comment Bug 382568 - Unsafe DOM mutation events in <textarea>
: Unsafe DOM mutation events in <textarea>
Status: RESOLVED FIXED
[sg:low? null deref?] keep private un...
: crash, fixed1.8.1.5, testcase, verified1.8.0.13
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: 1.8 Branch
: x86 All
: -- critical (vote)
: ---
Assigned To: Olli Pettay [:smaug] (vacation Aug 25-28)
:
Mentors:
Depends on: 382681
Blocks:
  Show dependency treegraph
 
Reported: 2007-05-31 02:47 PDT by Vlad Sukhoy
Modified: 2009-04-24 10:46 PDT (History)
11 users (show)
dveditz: blocking1.8.1.5+
dveditz: blocking1.8.0.13+
bob: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
test case (crashes 1_8 and 1_8_0). (1.23 KB, application/xhtml+xml)
2007-05-31 02:47 PDT, Vlad Sukhoy
no flags Details
WIP (2.75 KB, patch)
2007-05-31 08:12 PDT, Olli Pettay [:smaug] (vacation Aug 25-28)
no flags Details | Diff | Splinter Review
proposed patch (6.64 KB, patch)
2007-05-31 12:58 PDT, Olli Pettay [:smaug] (vacation Aug 25-28)
jonas: review+
jonas: superreview+
Details | Diff | Splinter Review

Description Vlad Sukhoy 2007-05-31 02:47:15 PDT
Created attachment 266725 [details]
test case (crashes 1_8 and 1_8_0).

DOMAttrModified events are fired from the presentation code which triggers JavaScript code within unsafe context.
Comment 1 Vlad Sukhoy 2007-05-31 03:04:53 PDT
See bug 355548 for the discussion on related issue.

In this particular case, nsTextControlFrame::CreateAnonymousContent does attribute manipulation on the content <div> and this fires DOMAttrModified event, which in this context is unsafe.
Comment 2 Olli Pettay [:smaug] (vacation Aug 25-28) 2007-05-31 08:12:39 PDT
Created attachment 266754 [details] [diff] [review]
WIP

Will do some tests, and possibly add a method to nsGenericHTMLFormElement to 
test whether mutation event is coming from native anon.
Comment 3 Olli Pettay [:smaug] (vacation Aug 25-28) 2007-05-31 12:58:50 PDT
Created attachment 266793 [details] [diff] [review]
proposed patch

Handling mutation events the same way in input and textarea.
<select> has also some native anon content, but couldn't get it to crash
the same way as <textarea>. (I did find one crash, but that is null pointer 
thingie).

On trunk mutation events from native anon are always prevented, but that
relies on the new event dispatching code.
Comment 4 Vlad Sukhoy 2007-05-31 13:01:35 PDT
I'm working on a related testcase to crash generic scrollable frame code (the same stuff in nsGfxScrollFrameInner::CreateAnonymousContent()). 
The whole thing of branches firing mutation whenever a frame changes something in content is vulnerable IMO.
Comment 5 Olli Pettay [:smaug] (vacation Aug 25-28) 2007-05-31 13:11:26 PDT
(In reply to comment #4)
> The whole thing of branches firing mutation whenever a frame changes something
> in content is vulnerable IMO.
> 

Yes, it is, though in trunk there shouldn't be any problems if content is native
anonymous. And note, nsGfxScrollFrame(Inner) has changed quite a bit since 1.8.
If you find a new crash, IsEventHandlingDisabled could be moved up to 
nsGenericElement.
Comment 6 Vlad Sukhoy 2007-05-31 14:13:41 PDT
Could not get it to crash in this particular case, but definitely was able to trash the UI thread with stuff, see bug 382681. 
Comment 7 Olli Pettay [:smaug] (vacation Aug 25-28) 2007-06-05 15:53:08 PDT
Comment on attachment 266793 [details] [diff] [review]
proposed patch

I'd take this to 1.8 and if someone can write a testcase where mutation events coming from native anonymous are causing crashes, ::IsEventHandlingDisabled() could be pushed up to nsGenericElement, (or a new patch with similar functionality could be written).
Comment 8 Vlad Sukhoy 2007-06-06 12:19:35 PDT
Why not approval1.8.0.13 as well though?
I'll try to generate another testcase...
Comment 9 Olli Pettay [:smaug] (vacation Aug 25-28) 2007-06-06 13:43:45 PDT
Will there be 1.8.0.13? I thought 1.8.0.12 was the last 1.8.0.x one.
Comment 10 Vlad Sukhoy 2007-06-06 16:22:59 PDT
Indeed. Why do we get those guys in bugzilla then?
Comment 11 Samuel Sidler (old account; do not CC) 2007-06-06 16:28:46 PDT
There will be a 1.8.0.13 for Thunderbird, but [probably] not for Firefox. The flags exist for those bugs, aiui.
Comment 12 Vlad Sukhoy 2007-06-06 17:18:52 PDT
Ok, then the flag should be probably up, and also for those guys who might still use 1.8.0 layout in their code..
Comment 13 Vlad Sukhoy 2007-06-07 14:17:03 PDT
@Smaug: I finally was able to crash scrollable frame code, see attachment 267625 [details]. Is that enough to warrant promotion of attachment 266793 [details] [diff] [review] to nsGenericElement?
Comment 14 Olli Pettay [:smaug] (vacation Aug 25-28) 2007-06-08 04:10:00 PDT
Because of XUL's strange way to flag native anonymous content, that 
may not work after all. I'll do some tests and write a bit different
kind of patch if needed.
Comment 15 Olli Pettay [:smaug] (vacation Aug 25-28) 2007-06-15 11:25:01 PDT
So the patch in Bug 382681 fixes this too.
Comment 16 Olli Pettay [:smaug] (vacation Aug 25-28) 2007-06-25 00:42:54 PDT
Fixed by the patch in bug 382681.
Comment 17 Daniel Veditz [:dveditz] 2007-06-26 12:48:08 PDT
In FF2.0.0.4 the crash I get is a null deref. This testcase doesn't lead to an exploitable situation but that doesn't mean these unsafe events can't be abused in other ways.
Comment 18 Stephen Donner [:stephend] 2007-08-21 15:57:27 PDT
I can verify that we don't crash using Thunderbird version 1.5.0.13 (20070809) (Mac OS X) using the testcase in comment 0; replacing fixed1.5.0.13 with verified1.5.0.13.
Comment 19 Bob Clary [:bc:] 2009-04-24 10:46:50 PDT
crash test landed
http://hg.mozilla.org/mozilla-central/rev/ae37a2531c4b

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