Closed Bug 805287 (CVE-2012-5840) Opened 12 years ago Closed 12 years ago

Heap-use-after-free in nsTextEditorState::PrepareEditor

Categories

(Core :: DOM: Editor, defect)

x86_64
All
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla19
Tracking Status
firefox16 --- wontfix
firefox17 + fixed
firefox18 + fixed
firefox19 --- fixed
firefox-esr10 17+ fixed

People

(Reporter: inferno, Assigned: ayg)

Details

(4 keywords, Whiteboard: [asan][adv-track-main17+][adv-track-esr17+])

Attachments

(2 files)

Attached file Testcase
Reproduces on trunk.

=================================================================
==20748== ERROR: AddressSanitizer heap-use-after-free on address 0x7fec4cb70a60 at pc 0x7fec675d1dda bp 0x7fffdec77c10 sp 0x7fffdec77c08
READ of size 8 at 0x7fec4cb70a60 thread T0
    #0 0x7fec675d1dd9 in nsCOMPtr<nsIEditor>::operator=(nsCOMPtr<nsIEditor> const&) ../../../../dist/include/nsCOMPtr.h:614
    #1 0x7fec675c7820 in nsTextEditorState::PrepareEditor(nsAString_internal const*) content/html/content/src/nsTextEditorState.cpp:1210
    #2 0x7fec675e5be8 in PrepareEditorEvent::Run() content/html/content/src/nsTextEditorState.cpp:1029
    #3 0x7fec6623b72c in nsContentUtils::RemoveScriptBlocker() content/base/src/nsContentUtils.cpp:5015
    #4 0x7fec646d13de in ~nsAutoScriptBlocker ../../../../dist/include/nsContentUtils.h:2332
    #5 0x7fec646bcb26 in ~nsAutoScriptBlocker ../../../../dist/include/nsContentUtils.h:2331
    #6 0x7fec64ae1ce2 in PresShell::FlushPendingNotifications(mozFlushType) layout/base/nsPresShell.cpp:3857
    #7 0x7fec6522c622 in nsHideViewer::Run() layout/generic/nsSubDocumentFrame.cpp:775
    #8 0x7fec6623b72c in nsContentUtils::RemoveScriptBlocker() content/base/src/nsContentUtils.cpp:5015
    #9 0x7fec646d13de in ~nsAutoScriptBlocker ../../../../dist/include/nsContentUtils.h:2332
    #10 0x7fec646bcb26 in ~nsAutoScriptBlocker ../../../../dist/include/nsContentUtils.h:2331
    #11 0x7fec6646a7c2 in nsDocument::AdoptNode(nsIDOMNode*, nsIDOMNode**) content/base/src/nsDocument.cpp:5957
    #12 0x7fec6bec6081 in nsIDOMDocument_AdoptNode(JSContext*, unsigned int, JS::Value*) objdir-ff-asan-sym/js/xpconnect/src/dom_quickstubs.cpp:1391
    #13 0x7fec77ff04e9 in js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) js/src/jscntxtinlines.h:364
    #14 0x7fec77fa00e1 in js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) js/src/jsinterp.cpp:2369
    #15 0x7fec77efef8e in js::RunScript(JSContext*, JS::Handle<JSScript*>, js::StackFrame*) js/src/jsinterp.cpp:324
    #16 0x7fec77ffdced in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::ExecuteType, js::StackFrame*, JS::Value*) js/src/jsinterp.cpp:510
    #17 0x7fec77fff97b in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) js/src/jsinterp.cpp:547
    #18 0x7fec7771447d in JS::Evaluate(JSContext*, JS::Handle<JSObject*>, JS::CompileOptions, unsigned short const*, unsigned long, JS::Value*) js/src/jsapi.cpp:5703
    #19 0x7fec689cdf10 in nsJSContext::EvaluateString(nsAString_internal const&, JSObject*, nsIPrincipal*, nsIPrincipal*, char const*, unsigned int, JSVersion, nsAString_internal*, bool*) dom/base/nsJSEnvironment.cpp:1499
    #20 0x7fec68b8fb06 in nsGlobalWindow::RunTimeoutHandler(nsTimeout*, nsIScriptContext*) dom/base/nsGlobalWindow.cpp:9740
    #21 0x7fec68b43d84 in nsGlobalWindow::RunTimeout(nsTimeout*) dom/base/nsGlobalWindow.cpp:9999
    #22 0x7fec68b8d9b8 in nsGlobalWindow::TimerCallback(nsITimer*, void*) dom/base/nsGlobalWindow.cpp:10266
    #23 0x7fec70b33762 in nsTimerImpl::Fire() xpcom/threads/nsTimerImpl.cpp:472
    #24 0x7fec70b34c6a in nsTimerEvent::Run() xpcom/threads/nsTimerImpl.cpp:555
    #25 0x7fec70af70c6 in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:620
    #26 0x7fec7077746b in NS_ProcessNextEvent_P(nsIThread*, bool) objdir-ff-asan-sym/xpcom/build/nsThreadUtils.cpp:220
    #27 0x7fec6f04e446 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:82
    #28 0x7fec70dd83e1 in MessageLoop::RunInternal() ipc/chromium/src/base/message_loop.cc:215
    #29 0x7fec70dd8216 in MessageLoop::RunHandler() ipc/chromium/src/base/message_loop.cc:208
    #30 0x7fec70dd80fb in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:182
    #31 0x7fec6e463c8a in nsBaseAppShell::Run() widget/xpwidgets/nsBaseAppShell.cpp:163
    #32 0x7fec6cff4ab4 in nsAppStartup::Run() toolkit/components/startup/nsAppStartup.cpp:290
    #33 0x7fec628d83e8 in XREMain::XRE_mainRun() toolkit/xre/nsAppRunner.cpp:3799
    #34 0x7fec628de2cb in XREMain::XRE_main(int, char**, nsXREAppData const*) toolkit/xre/nsAppRunner.cpp:3866
    #35 0x7fec628e11c4 in XRE_main toolkit/xre/nsAppRunner.cpp:3941
    #36 0x40c055 in do_main(int, char**) browser/app/nsBrowserApp.cpp:174
    #37 0x409757 in main browser/app/nsBrowserApp.cpp:279
    #38 0x7fec81f6376c in ?? ??:0
0x7fec4cb70a60 is located 32 bytes inside of 128-byte region [0x7fec4cb70a40,0x7fec4cb70ac0)
freed by thread T0 here:
    #0 0x4c3d50 in __interceptor_free ??:?
    #1 0x7fec7fce0406 in moz_free memory/mozalloc/mozalloc.cpp:48
    #2 0x7fec6796dc86 in operator delete(void*) ../../../../dist/include/mozilla/mozalloc.h:224
    #3 0x7fec6797cf99 in nsHTMLInputElement::HandleTypeChange(unsigned char) content/html/content/src/nsHTMLInputElement.cpp:2628
    #4 0x7fec679b1fd4 in nsHTMLInputElement::ParseAttribute(int, nsIAtom*, nsAString_internal const&, nsAttrValue&) content/html/content/src/nsHTMLInputElement.cpp:2758
    #5 0x7fec6666d1d9 in nsGenericElement::SetAttr(int, nsIAtom*, nsIAtom*, nsAString_internal const&, bool) content/base/src/nsGenericElement.cpp:1954
    #6 0x7fec67538abd in nsGenericHTMLElement::SetAttr(int, nsIAtom*, nsIAtom*, nsAString_internal const&, bool) content/html/content/src/nsGenericHTMLElement.cpp:2002
    #7 0x7fec6751c823 in nsGenericHTMLElement::SetAttr(int, nsIAtom*, nsAString_internal const&, bool) content/html/document/src/../../content/src/nsGenericHTMLElement.h:245
    #8 0x7fec6751ec27 in nsGenericHTMLElement::SetAttrHelper(nsIAtom*, nsAString_internal const&) content/html/content/src/nsGenericHTMLElement.cpp:2871
    #9 0x7fec6798c820 in nsHTMLInputElement::SetType(nsAString_internal const&) content/html/content/src/nsHTMLInputElement.cpp:905
    #10 0x7fec6798c982 in non-virtual thunk to nsHTMLInputElement::SetType(nsAString_internal const&) :?
    #11 0x7fec6c4e71c1 in nsIDOMHTMLInputElement_SetType(JSContext*, JS::Handle<JSObject*>, JS::Handle<long>, int, JS::MutableHandle<JS::Value>) objdir-ff-asan-sym/js/xpconnect/src/dom_quickstubs.cpp:13483
    #12 0x7fec782d5d65 in js::CallJSPropertyOpSetter(JSContext*, int (*)(JSContext*, JS::Handle<JSObject*>, JS::Handle<long>, int, JS::MutableHandle<JS::Value>), JS::Handle<JSObject*>, JS::Handle<long>, int, JS::MutableHandle<JS::Value>) js/src/jscntxtinlines.h:450
    #13 0x7fec7830c326 in js::baseops::SetPropertyHelper(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, JS::Handle<long>, unsigned int, JS::MutableHandle<JS::Value>, int) js/src/jsobj.cpp:4613
    #14 0x7fec78037376 in js::SetPropertyOperation(JSContext*, unsigned char*, JS::Handle<JS::Value>, JS::Handle<JS::Value>) js/src/jsinterpinlines.h:362
previously allocated by thread T0 here:
    #0 0x4c3e10 in malloc ??:?
    #1 0x7fec7fce055a in moz_xmalloc memory/mozalloc/mozalloc.cpp:54
    #2 0x7fec6796c7aa in operator new(unsigned long) ../../../../dist/include/mozilla/mozalloc.h:200
    #3 0x7fec6796b520 in NS_NewHTMLInputElement(already_AddRefed<nsINodeInfo>, mozilla::dom::FromParser) content/html/content/src/nsHTMLInputElement.cpp:538
    #4 0x7fec68144d2f in CreateHTMLElement(unsigned int, already_AddRefed<nsINodeInfo>, mozilla::dom::FromParser) content/html/document/src/nsHTMLContentSink.cpp:497
    #5 0x7fec6814558a in NS_NewHTMLElement(nsIContent**, already_AddRefed<nsINodeInfo>, mozilla::dom::FromParser) content/html/document/src/nsHTMLContentSink.cpp:480
    #6 0x7fec667460f1 in NS_NewElement(nsIContent**, already_AddRefed<nsINodeInfo>, mozilla::dom::FromParser) content/base/src/nsNameSpaceManager.cpp:201
    #7 0x7fec69ddd666 in nsHtml5TreeOperation::Perform(nsHtml5TreeOpExecutor*, nsIContent**) parser/html/nsHtml5TreeOperation.cpp:345
    #8 0x7fec69dfe22b in nsHtml5TreeOpExecutor::RunFlushLoop() parser/html/nsHtml5TreeOpExecutor.cpp:564
Shadow byte and word:
  0x1ffd8996e14c: fd
  0x1ffd8996e148: fd fd fd fd fd fd fd fd
More shadow bytes:
  0x1ffd8996e128: 00 00 00 00 00 00 00 00
  0x1ffd8996e130: 00 00 00 00 00 00 00 00
  0x1ffd8996e138: fa fa fa fa fa fa fa fa
  0x1ffd8996e140: fa fa fa fa fa fa fa fa
=>0x1ffd8996e148: fd fd fd fd fd fd fd fd
  0x1ffd8996e150: fd fd fd fd fd fd fd fd
  0x1ffd8996e158: fa fa fa fa fa fa fa fa
  0x1ffd8996e160: fa fa fa fa fa fa fa fa
  0x1ffd8996e168: 00 00 00 00 00 00 00 00
Stats: 239M malloced (227M for red zones) by 420825 calls
Stats: 43M realloced by 17471 calls
Stats: 201M freed by 199819 calls
Stats: 163M really freed by 165412 calls
Stats: 248M (63708 full pages) mmaped in 474 calls
  mmaps   by size class: 7:200655; 8:45034; 9:12276; 10:4599; 11:8670; 12:1152; 13:704; 14:544; 15:144; 16:616; 17:452; 18:112; 19:35; 20:21;
  mallocs by size class: 7:289609; 8:78837; 9:21511; 10:7139; 11:15529; 12:2157; 13:1652; 14:1478; 15:316; 16:1063; 17:1326; 18:146; 19:40; 20:22;
  frees   by size class: 7:111546; 8:49507; 9:15454; 10:4185; 11:12566; 12:1338; 13:1217; 14:1311; 15:263; 16:1015; 17:1310; 18:51; 19:37; 20:19;
  rfrees  by size class: 7:94468; 8:41302; 9:9580; 10:2979; 11:11409; 12:1056; 13:983; 14:1225; 15:217; 16:860; 17:1278; 18:49; 19:5; 20:1;
Stats: malloc large: 2913 small slow: 4736
==20748== ABORTING
Severity: normal → critical
Component: General → DOM: Core & HTML
Keywords: crash, testcase
Product: Firefox → Core
Whiteboard: [asan]
This looks like editor to me, at least just guessing from the name of the class.
Component: DOM: Core & HTML → Editor
Guessing sg-crit based on UAF...
Assignee: nobody → ayg
When I run the test case, it looks like nsTextControlFrame::UpdateValueDisplay() calls rootNode->RemoveChildAt() at line 1373, which destroys the nsTextEditorState somehow.  Specifically, this code:

  if (aBeforeEditorInit && value.IsEmpty()) {
    rootNode->RemoveChildAt(0, true);
    return NS_OK;
  }

That doesn't match the stack trace given here, not sure why.  This is *not* the same as bug 795804, although the stack trace looks similar, because AFAICT the state object is only freed during the course of the PrepareEditor call.  Otherwise the fix for that would have fixed this.

Ehsan, what should I do here?  Since the class isn't refcounted, I can't make it just hold a reference to itself or its owning object or whatever -- it's calling a method that can explicitly delete it, or so it seems in debugging so far.  (I don't properly understand what all the code here is doing by a long shot.)  The only thing that occurs to me other than making it refcounted is to make it hold a WeakPtr to itself and then check it at the point where we know it might have been deleted, but that seems awfully silly, not to mention error-prone.  Is there something better to do?

Actually, what all these security bugs really tell me is that our whole script blocker model is completely broken, and that delaying execution of fairly arbitrary large chunks of code is going to create all sorts of horrible bugs.  But that's a little beyond the scope of this bug.  :)
Status: NEW → ASSIGNED
(In reply to :Aryeh Gregor from comment #3)
> When I run the test case, it looks like
> nsTextControlFrame::UpdateValueDisplay() calls rootNode->RemoveChildAt() at
> line 1373, which destroys the nsTextEditorState somehow.  Specifically, this
> code:
> 
>   if (aBeforeEditorInit && value.IsEmpty()) {
>     rootNode->RemoveChildAt(0, true);
>     return NS_OK;
>   }

If you before that call to RemoveChildAt, and set a breakpoint on ~nsTextEditorState when you hit that, you should be able to get a stack trace of what destroys that object.  Can you please post that stack trace here?

> That doesn't match the stack trace given here, not sure why.  This is *not*
> the same as bug 795804, although the stack trace looks similar, because
> AFAICT the state object is only freed during the course of the PrepareEditor
> call.  Otherwise the fix for that would have fixed this.

That's fair.

> Ehsan, what should I do here?  Since the class isn't refcounted, I can't
> make it just hold a reference to itself or its owning object or whatever --
> it's calling a method that can explicitly delete it, or so it seems in
> debugging so far.

The right fix really depends on what causes the nsTextEditorState object to be deleted.

> (I don't properly understand what all the code here is
> doing by a long shot.)

Please feel free to ask questions.  I'll be more than happy to explain this to you.  :-)

>  The only thing that occurs to me other than making
> it refcounted is to make it hold a WeakPtr to itself and then check it at
> the point where we know it might have been deleted, but that seems awfully
> silly, not to mention error-prone.  Is there something better to do?

Well, if the object is dying because its content node is, then maybe we should hold a kungfuDeathGrip to that...

> Actually, what all these security bugs really tell me is that our whole
> script blocker model is completely broken, and that delaying execution of
> fairly arbitrary large chunks of code is going to create all sorts of
> horrible bugs.  But that's a little beyond the scope of this bug.  :)

Yeah, unfortunately.  :(  The root cause of many of these types of bugs is that things that lead to a layout flush can run arbitrary script, and that means that pretty much anything can die underneath us..  But unfortunately that's part of the web.  Mutation events are perhaps the worst example of these types of problem.
(In reply to Ehsan Akhgari [:ehsan] from comment #4)
> If you before that call to RemoveChildAt, and set a breakpoint on
> ~nsTextEditorState when you hit that, you should be able to get a stack
> trace of what destroys that object.  Can you please post that stack trace
> here?

It's the same as the "freed by thread T0 here:" trace from comment #0 (obviously except the first couple of frames), but it has a lot more entries.  Here it is in case you want it:

#0  nsTextEditorState::~nsTextEditorState (this=0x9dfd3b50, __in_chrg=<optimized out>)
    at /mnt/ssd/checkouts/central/content/html/content/src/nsTextEditorState.cpp:943
#1  0xb3e52f53 in nsHTMLInputElement::FreeData (this=0x9d553040)
    at /mnt/ssd/checkouts/central/content/html/content/src/nsHTMLInputElement.cpp:591
#2  0xb3e59c85 in nsHTMLInputElement::HandleTypeChange (this=0x9d553040, aNewType=130 '\202')
    at /mnt/ssd/checkouts/central/content/html/content/src/nsHTMLInputElement.cpp:2628
#3  0xb3e5a135 in nsHTMLInputElement::ParseAttribute (this=0x9d553040, aNamespaceID=0, 
    aAttribute=0xae172ac0, aValue=..., aResult=...)
    at /mnt/ssd/checkouts/central/content/html/content/src/nsHTMLInputElement.cpp:2758
#4  0xb3c53660 in nsGenericElement::SetAttr (this=0x9d553040, aNamespaceID=0, aName=0xae172ac0, 
    aPrefix=0x0, aValue=..., aNotify=true)
    at /mnt/ssd/checkouts/central/content/base/src/nsGenericElement.cpp:1954
#5  0xb3dfa3a9 in nsGenericHTMLElement::SetAttr (this=0x9d553040, aNameSpaceID=0, aName=0xae172ac0, 
    aPrefix=0x0, aValue=..., aNotify=true)
    at /mnt/ssd/checkouts/central/content/html/content/src/nsGenericHTMLElement.cpp:2003
#6  0xb3df375b in nsGenericHTMLElement::SetAttr (this=0x9d553040, aNameSpaceID=0, aName=0xae172ac0, 
    aValue=..., aNotify=true)
    at /mnt/ssd/checkouts/central/content/html/content/src/nsGenericHTMLElement.h:245
#7  0xb3dfc341 in nsGenericHTMLElement::SetAttrHelper (this=0x9d553040, aAttr=0xae172ac0, aValue=...)
    at /mnt/ssd/checkouts/central/content/html/content/src/nsGenericHTMLElement.cpp:2871
#8  0xb3e5531c in nsHTMLInputElement::SetType (this=0x9d553040, aValue=...)
    at /mnt/ssd/checkouts/central/content/html/content/src/nsHTMLInputElement.cpp:905
#9  0xb46ca75c in nsIDOMHTMLInputElement_SetType (cx=0xac1fa540, obj=..., id=..., strict=0, vp_=...)
    at /mnt/ssd/checkouts/central/obj/js/xpconnect/src/dom_quickstubs.cpp:13546
#10 0xb5ba00b3 in js::CallJSPropertyOpSetter (cx=0xac1fa540, 
    op=0xb46ca620 <nsIDOMHTMLInputElement_SetType(JSContext*, JSHandleObject, JSHandleId, JSBool, JSMutableHandleValue)>, obj=..., id=..., strict=0, vp=...)
    at /mnt/ssd/checkouts/central/js/src/jscntxtinlines.h:450
#11 0xb5ba1380 in js::Shape::set (this=0xac23a568, cx=0xac1fa540, obj=..., receiver=..., strict=false, 
    vp=...) at /mnt/ssd/checkouts/central/js/src/jsscopeinlines.h:333
#12 0xb5bb5cce in js::baseops::SetPropertyHelper (cx=0xac1fa540, obj=..., receiver=..., id=..., 
    defineHow=1, vp=..., strict=0) at /mnt/ssd/checkouts/central/js/src/jsobj.cpp:4613
#13 0xb5b7271f in js::SetPropertyOperation (cx=0xac1fa540, pc=0x9dfd3b2f "6", lval=..., rval=...)
    at /mnt/ssd/checkouts/central/js/src/jsinterpinlines.h:362
#14 0xb5b7e100 in js::Interpret (cx=0xac1fa540, entryFrame=0xad4ff0e0, interpMode=js::JSINTERP_NORMAL)
    at /mnt/ssd/checkouts/central/js/src/jsinterp.cpp:2279
#15 0xb5b7679b in js::RunScript (cx=0xac1fa540, script=..., fp=0xad4ff0e0)
    at /mnt/ssd/checkouts/central/js/src/jsinterp.cpp:324
#16 0xb5b76d86 in js::InvokeKernel (cx=0xac1fa540, args=..., construct=js::NO_CONSTRUCT)
    at /mnt/ssd/checkouts/central/js/src/jsinterp.cpp:379
#17 0xb5ab1651 in js::Invoke (cx=0xac1fa540, args=..., construct=js::NO_CONSTRUCT)
    at /mnt/ssd/checkouts/central/js/src/jsinterp.h:109
#18 0xb5b7702f in js::Invoke (cx=0xac1fa540, thisv=..., fval=..., argc=1, argv=0xbfffa7b0, 
    rval=0xbfffa740) at /mnt/ssd/checkouts/central/js/src/jsinterp.cpp:412
#19 0xb5aa0cb6 in JS_CallFunctionValue (cx=0xac1fa540, objArg=0xac21f040, fval=..., argc=1, 
    argv=0xbfffa7b0, rval=0xbfffa740) at /mnt/ssd/checkouts/central/js/src/jsapi.cpp:5884
#20 0xb4008869 in nsJSContext::CallEventHandler (this=0x9eaf5dd0, aTarget=0xa17f725c, 
    aScope=0xac21f040, aHandler=0xac22bda0, aargv=0xab391240, arv=0xbfffa9b8)
    at /mnt/ssd/checkouts/central/dom/base/nsJSEnvironment.cpp:1913
#21 0xb41387fe in nsJSEventListener::HandleEvent (this=0x9d534740, aEvent=0x9d530100)
    at /mnt/ssd/checkouts/central/dom/src/events/nsJSEventListener.cpp:212
#22 0xb3d9a370 in nsEventListenerManager::HandleEventSubType (this=0x9d51d600, 
    aListenerStruct=0x9d51d620, aListener=0x9d534740, aDOMEvent=0x9d530100, aCurrentTarget=0xa17f7268, 
    aPhaseFlags=6, aPusher=0xbfffac1c)
    at /mnt/ssd/checkouts/central/content/events/src/nsEventListenerManager.cpp:868
#23 0xb3d9a5c2 in nsEventListenerManager::HandleEventInternal (this=0x9d51d600, 
    aPresContext=0xa1f4d800, aEvent=0xbfffacdc, aDOMEvent=0xbfffac0c, aCurrentTarget=0xa17f7268, 
    aFlags=6, aEventStatus=0xbfffac10, aPusher=0xbfffac1c)
    at /mnt/ssd/checkouts/central/content/events/src/nsEventListenerManager.cpp:941
#24 0xb3dc93ff in nsEventListenerManager::HandleEvent (this=0x9d51d600, aPresContext=0xa1f4d800, 
    aEvent=0xbfffacdc, aDOMEvent=0xbfffac0c, aCurrentTarget=0xa17f7268, aFlags=6, 
    aEventStatus=0xbfffac10, aPusher=0xbfffac1c)
    at /mnt/ssd/checkouts/central/content/events/src/nsEventListenerManager.h:144
#25 0xb3dc98db in nsEventTargetChainItem::HandleEvent (this=0xaa4ec1a0, aVisitor=..., aFlags=6, 
    aMayHaveNewListenerManagers=false, aPusher=0xbfffac1c)
    at /mnt/ssd/checkouts/central/content/events/src/nsEventDispatcher.cpp:184
#26 0xb3dc9df3 in nsEventTargetChainItem::HandleEventTargetChain (this=0xaa4ec140, aVisitor=..., 
    aFlags=6, aCallback=0x0, aMayHaveNewListenerManagers=false, aPusher=0xbfffac1c)
    at /mnt/ssd/checkouts/central/content/events/src/nsEventDispatcher.cpp:316
#27 0xb3dcae8a in nsEventDispatcher::Dispatch (aTarget=0xa17f5e20, aPresContext=0xa1f4d800, 
    aEvent=0xbfffacdc, aDOMEvent=0x0, aEventStatus=0xbfffad18, aCallback=0x0, aTargets=0x0)
    at /mnt/ssd/checkouts/central/content/events/src/nsEventDispatcher.cpp:634
#28 0xb38766a2 in DocumentViewerImpl::LoadComplete (this=0xa36ac350, aStatus=NS_OK)
    at /mnt/ssd/checkouts/central/layout/base/nsDocumentViewer.cpp:1025
#29 0xb475f229 in nsDocShell::EndPageLoad (this=0x9eaee400, aProgress=0x9eaee414, aChannel=0x9eb4a034, 
    aStatus=NS_OK) at /mnt/ssd/checkouts/central/docshell/base/nsDocShell.cpp:6510
#30 0xb475e8df in nsDocShell::OnStateChange (this=0x9eaee400, aProgress=0x9eaee414, 
    aRequest=0x9eb4a034, aStateFlags=131088, aStatus=NS_OK)
    at /mnt/ssd/checkouts/central/docshell/base/nsDocShell.cpp:6338
#31 0xb478f8ba in nsDocLoader::DoFireOnStateChange (this=0x9eaee400, aProgress=0x9eaee414, 
    aRequest=0x9eb4a034, aStateFlags=@0xbfffb200: 131088, aStatus=NS_OK)
    at /mnt/ssd/checkouts/central/uriloader/base/nsDocLoader.cpp:1351
#32 0xb478e5a7 in nsDocLoader::doStopDocumentLoad (this=0x9eaee400, request=0x9eb4a034, aStatus=NS_OK)
    at /mnt/ssd/checkouts/central/uriloader/base/nsDocLoader.cpp:931
#33 0xb478e134 in nsDocLoader::DocLoaderIsEmpty (this=0x9eaee400, aFlushLayout=true)
    at /mnt/ssd/checkouts/central/uriloader/base/nsDocLoader.cpp:820
#34 0xb478bfc1 in nsDocLoader::ChildDoneWithOnload (this=0x9eaee400, aChild=0x9eb4a800)
    at /mnt/ssd/checkouts/central/uriloader/base/nsDocLoader.h:193
#35 0xb478e162 in nsDocLoader::DocLoaderIsEmpty (this=0x9eb4a800, aFlushLayout=true)
    at /mnt/ssd/checkouts/central/uriloader/base/nsDocLoader.cpp:823
#36 0xb478dc7b in nsDocLoader::OnStopRequest (this=0x9eb4a800, aRequest=0xaab7da80, aCtxt=0x0, 
    aStatus=NS_BINDING_ABORTED) at /mnt/ssd/checkouts/central/uriloader/base/nsDocLoader.cpp:704
#37 0xb34d4a9e in nsLoadGroup::RemoveRequest (this=0x9eae8200, request=0xaab7da80, ctxt=0x0, 
    aStatus=NS_BINDING_ABORTED) at /mnt/ssd/checkouts/central/netwerk/base/src/nsLoadGroup.cpp:698
#38 0xb34d3778 in nsLoadGroup::Cancel (this=0x9eae8200, status=NS_BINDING_ABORTED)
    at /mnt/ssd/checkouts/central/netwerk/base/src/nsLoadGroup.cpp:304
#39 0xb478d0d0 in nsDocLoader::Stop (this=0x9eb4a800)
    at /mnt/ssd/checkouts/central/uriloader/base/nsDocLoader.cpp:328
#40 0xb474660f in nsDocShell::Stop (this=0x9eb4a800)
    at /mnt/ssd/checkouts/central/docshell/base/nsDocShell.h:189
#41 0xb4758641 in nsDocShell::Stop (this=0x9eb4a800, aStopFlags=3)
    at /mnt/ssd/checkouts/central/docshell/base/nsDocShell.cpp:4609
#42 0xb47596eb in nsDocShell::Destroy (this=0x9eb4a800)
    at /mnt/ssd/checkouts/central/docshell/base/nsDocShell.cpp:4878
#43 0xb3c3c731 in nsFrameLoader::Finalize (this=0x9dfe4c90)
    at /mnt/ssd/checkouts/central/content/base/src/nsFrameLoader.cpp:569
#44 0xb3c11a02 in nsDocument::MaybeInitializeFinalizeFrameLoaders (this=0x9eaf0000)
    at /mnt/ssd/checkouts/central/content/base/src/nsDocument.cpp:5422
#45 0xb3c0bff5 in nsDocument::EndUpdate (this=0x9eaf0000, aUpdateType=1)
    at /mnt/ssd/checkouts/central/content/base/src/nsDocument.cpp:3965
#46 0xb3ef2900 in nsHTMLDocument::EndUpdate (this=0x9eaf0000, aUpdateType=1)
    at /mnt/ssd/checkouts/central/content/html/document/src/nsHTMLDocument.cpp:2338
#47 0xb3959c36 in mozAutoDocUpdate::~mozAutoDocUpdate (this=0xbfffb840, __in_chrg=<optimized out>)
    at /mnt/ssd/checkouts/central/layout/generic/../../content/base/src/mozAutoDocUpdate.h:35
#48 0xb3c614d6 in nsINode::doRemoveChildAt (this=0x9dffaf10, aIndex=0, aNotify=true, aKid=0x9dffaf60, 
    aChildArray=...) at /mnt/ssd/checkouts/central/content/base/src/nsINode.cpp:1370
#49 0xb3cd65bb in mozilla::dom::FragmentOrElement::RemoveChildAt (this=0x9dffaf10, aIndex=0, 
    aNotify=true) at /mnt/ssd/checkouts/central/content/base/src/FragmentOrElement.cpp:887
#50 0xb38ff389 in nsTextControlFrame::UpdateValueDisplay (this=0xab201d00, aNotify=true, 
    aBeforeEditorInit=true, aValue=0x0)
    at /mnt/ssd/checkouts/central/layout/forms/nsTextControlFrame.cpp:1373
. . .

> The right fix really depends on what causes the nsTextEditorState object to
> be deleted.
> 
> . . .
> 
> Well, if the object is dying because its content node is, then maybe we
> should hold a kungfuDeathGrip to that...

Ah, of course.  I should have thought of that.  But it doesn't work -- the input element still sticks around, it just changed type to a non-text input, so it deletes the state object in nsHTMLInputElement::FreeData (see stack).  If nothing but the input is supposed to keep a reference to the state, and the input doesn't want to keep a reference because it no longer needs the state, what's the right fix?

> Yeah, unfortunately.  :(  The root cause of many of these types of bugs is
> that things that lead to a layout flush can run arbitrary script, and that
> means that pretty much anything can die underneath us..  But unfortunately
> that's part of the web.  Mutation events are perhaps the worst example of
> these types of problem.

Hopefully we can get rid of mutation events!
Yikes, input type change...  OK, now this all makes sense.  I think the right thing would be to add an auto script blocker in the caller of UpdateValueDisplay to defer the execution of the scripts enough so that we don't end up destroying the editor prematurely.
Critsmash triage: What versions are affected by this? How far back does it go?
I'm guessing that it affects everything based on the fact that it is editor, but please update as appropriate.
Attached patch PatchSplinter Review
Sorry for the slow turnaround on these two.  :(

https://tbpl.mozilla.org/?tree=Try&rev=5cda1d8ed513
Attachment #678104 - Flags: review?(ehsan)
Comment on attachment 678104 [details] [diff] [review]
Patch

Review of attachment 678104 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, but we need to remove the comment before landing.
Attachment #678104 - Flags: sec-approval?
Attachment #678104 - Flags: review?(ehsan)
Attachment #678104 - Flags: review+
The sec-approval? flag was set but no details given. Can someone fill out the template, which should show up when you set the flag to ?

[Security approval request comment]
How easily can the security issue be deduced from the patch?

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Which older supported branches are affected by this flaw?

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

How likely is this patch to cause regressions; how much testing does it need?
(In reply to Al Billings [:abillings] from comment #11)
> The sec-approval? flag was set but no details given. Can someone fill out
> the template, which should show up when you set the flag to ?

Hmm, that was because Bugzilla didn't fill in the form...

> [Security approval request comment]
> How easily can the security issue be deduced from the patch?

It's obvious that the bug is a use-after-free, however the exact way to trigger it is not obvious from the patch.

> Do comments in the patch, the check-in comment, or tests included in the
> patch paint a bulls-eye on the security problem?

We'll land the patch without tests or comments.

> Which older supported branches are affected by this flaw?

All.

> If not all supported branches, which bug introduced the flaw?
> 
> Do you have backports for the affected branches? If not, how different, hard
> to create, and risky will they be?

Same.

> How likely is this patch to cause regressions; how much testing does it need?

Should be very safe.
Comment on attachment 678104 [details] [diff] [review]
Patch

sec-approval+. Let's get it in.
Attachment #678104 - Flags: sec-approval? → sec-approval+
Landed with no code comments and an obvious commit message:

https://hg.mozilla.org/integration/mozilla-inbound/rev/c22bf15ca07c
https://hg.mozilla.org/mozilla-central/rev/c22bf15ca07c
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Please nominate this sg:crit for uplift to FF17 and ESR10 asap.
Comment on attachment 678104 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): It has existed since forever.
User impact if declined: sg-crit
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): Very minimal
String or UUID changes made by this patch: none
Attachment #678104 - Flags: approval-mozilla-esr17?
Attachment #678104 - Flags: approval-mozilla-esr10?
Attachment #678104 - Flags: approval-mozilla-beta?
Attachment #678104 - Flags: approval-mozilla-aurora?
Comment on attachment 678104 [details] [diff] [review]
Patch

ESR17 will be cut from mozilla-release once 17 is on it, so we'll get this fix for free there.  Approving the others.
Attachment #678104 - Flags: approval-mozilla-esr17?
Attachment #678104 - Flags: approval-mozilla-esr17-
Attachment #678104 - Flags: approval-mozilla-esr10?
Attachment #678104 - Flags: approval-mozilla-esr10+
Attachment #678104 - Flags: approval-mozilla-beta?
Attachment #678104 - Flags: approval-mozilla-beta+
Attachment #678104 - Flags: approval-mozilla-aurora?
Attachment #678104 - Flags: approval-mozilla-aurora+
Alias: CVE-2012-5840
Whiteboard: [asan] → [asan][adv-track-main17+][adv-track-esr17+]
Keywords: verifyme
Flags: sec-bounty?
This bug qualifies for a security bug bounty.
Flags: sec-bounty? → sec-bounty+
Group: core-security
mass remove verifyme requests greater than 4 months old
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.