Closed Bug 795708 (CVE-2012-4213) Opened 12 years ago Closed 12 years ago

Heap-use-after-free in nsEditor::FindNextLeafNode

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 --- unaffected

People

(Reporter: inferno, Assigned: ayg)

References

Details

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

Attachments

(2 files)

Attached file Testcase
Reproduces on trunk

=================================================================
==10788== ERROR: AddressSanitizer heap-use-after-free on address 0x7f236f299db8 at pc 0x7f239927cf45 bp 0x7fff229632d0 sp 0x7fff229632c8
READ of size 8 at 0x7f236f299db8 thread T0
    #0 0x7f239927cf44 in nsINode::GetPreviousSibling() const src/../../dist/include/nsINode.h:1142
    #1 0x7f239e1112e2 in nsEditor::FindNextLeafNode(nsINode*, bool, bool) src/editor/libeditor/base/nsEditor.cpp:3319
    #2 0x7f239e1108dc in nsEditor::FindNode(nsINode*, bool, bool, bool) src/editor/libeditor/base/nsEditor.cpp:3401
    #3 0x7f239e110cc1 in nsEditor::FindNode(nsINode*, bool, bool, bool) src/editor/libeditor/base/nsEditor.cpp:3411
    #4 0x7f239e10ca79 in nsEditor::GetPriorNode(nsINode*, bool, bool) src/editor/libeditor/base/nsEditor.cpp:3301
    #5 0x7f239e10c106 in nsEditor::GetPriorNode(nsINode*, int, bool, bool) src/editor/libeditor/base/nsEditor.cpp:3190
    #6 0x7f239e649789 in nsHTMLEditor::GetPriorHTMLNode(nsINode*, int, bool) src/editor/libeditor/html/nsHTMLEditor.cpp:4104
    #7 0x7f239e7c3182 in nsHTMLEditRules::GetPromotedPoint(nsHTMLEditRules::RulesEndpoint, nsIDOMNode*, int, EditAction, nsCOMPtr<nsIDOMNode>*, int*) src/editor/libeditor/html/nsHTMLEditRules.cpp:5277
    #8 0x7f239e6d078e in nsHTMLEditRules::PromoteRange(nsIDOMRange*, EditAction) src/editor/libeditor/html/nsHTMLEditRules.cpp:5493
    #9 0x7f239e6cac26 in nsHTMLEditRules::AfterEditInner(EditAction, short) src/editor/libeditor/html/nsHTMLEditRules.cpp:430
    #10 0x7f239e6c8df6 in nsHTMLEditRules::AfterEdit(EditAction, short) src/editor/libeditor/html/nsHTMLEditRules.cpp:376
    #11 0x7f239e63798c in nsHTMLEditor::EndOperation() src/editor/libeditor/html/nsHTMLEditor.cpp:3513
    #12 0x7f239e06d372 in ~nsAutoRules src/editor/libeditor/base/nsEditorUtils.h:94
    #13 0x7f239e04afc6 in ~nsAutoRules src/editor/libeditor/base/nsEditorUtils.h:91
    #14 0x7f239e610e37 in nsHTMLEditor::Align(nsAString_internal const&) src/editor/libeditor/html/nsHTMLEditor.cpp:2264
    #15 0x7f239e610fee in non-virtual thunk to nsHTMLEditor::Align(nsAString_internal const&) src/gfx/cairo/cairo/src/cairo-surface-subsurface.c:0
    #16 0x7f23a1b430b0 in nsAlignCommand::SetState(nsIEditor*, nsString&) src/editor/composer/src/nsComposerCommands.cpp:970
    #17 0x7f23a1b37113 in nsMultiStateCommand::DoCommandParams(char const*, nsICommandParams*, nsISupports*) src/editor/composer/src/nsComposerCommands.cpp:599
    #18 0x7f23a13912e2 in nsControllerCommandTable::DoCommandParams(char const*, nsICommandParams*, nsISupports*) src/embedding/components/commandhandler/src/nsControllerCommandTable.cpp:175
    #19 0x7f23a136257d in nsBaseCommandController::DoCommandWithParams(char const*, nsICommandParams*) src/embedding/components/commandhandler/src/nsBaseCommandController.cpp:153
    #20 0x7f23a1362796 in non-virtual thunk to nsBaseCommandController::DoCommandWithParams(char const*, nsICommandParams*) src/gfx/cairo/cairo/src/cairo-surface-subsurface.c:0
    #21 0x7f23a137abd3 in nsCommandManager::DoCommand(char const*, nsICommandParams*, nsIDOMWindow*) src/embedding/components/commandhandler/src/nsCommandManager.cpp:234
    #22 0x7f239c901752 in nsHTMLDocument::ExecCommand(nsAString_internal const&, bool, nsAString_internal const&, bool*) src/content/html/document/src/nsHTMLDocument.cpp:3235
    #23 0x7f239c902c9d in non-virtual thunk to nsHTMLDocument::ExecCommand(nsAString_internal const&, bool, nsAString_internal const&, bool*) src/gfx/cairo/cairo/src/cairo-surface-subsurface.c:0
    #24 0x7f23a4ebb543 in NS_InvokeByIndex_P src/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:162
    #25 0x7f23a00bcfa8 in CallMethodHelper::Invoke() src/js/xpconnect/src/XPCWrappedNative.cpp:3110
    #26 0x7f23a011e1a5 in XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) src/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1469
    #27 0x7f23ab687d9f in js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) src/js/src/jscntxtinlines.h:370
    #28 0x7f23ab629b79 in js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) src/js/src/jsinterp.cpp:2460
    #29 0x7f23ab575aee in js::RunScript(JSContext*, JS::Handle<JSScript*>, js::StackFrame*) src/js/src/jsinterp.cpp:324
    #30 0x7f23ab695566 in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::ExecuteType, js::StackFrame*, JS::Value*) src/js/src/jsinterp.cpp:509
    #31 0x7f23ab6972fb in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) src/js/src/jsinterp.cpp:546
    #32 0x7f23aadaa289 in JS::Evaluate(JSContext*, JS::Handle<JSObject*>, JS::CompileOptions, unsigned short const*, unsigned long, JS::Value*) src/js/src/jsapi.cpp:5679
    #33 0x7f239d0c1dee in nsJSContext::EvaluateString(nsAString_internal const&, JSObject*, nsIPrincipal*, nsIPrincipal*, char const*, unsigned int, JSVersion, nsAString_internal*, bool*) src/dom/base/nsJSEnvironment.cpp:1506
    #34 0x7f239d27ab76 in nsGlobalWindow::RunTimeoutHandler(nsTimeout*, nsIScriptContext*) src/dom/base/nsGlobalWindow.cpp:9621
    #35 0x7f239d2326c4 in nsGlobalWindow::RunTimeout(nsTimeout*) src/dom/base/nsGlobalWindow.cpp:9880
    #36 0x7f239d278a28 in nsGlobalWindow::TimerCallback(nsITimer*, void*) src/dom/base/nsGlobalWindow.cpp:10147
    #37 0x7f23a4def972 in nsTimerImpl::Fire() src/xpcom/threads/nsTimerImpl.cpp:473
    #38 0x7f23a4df0e7a in nsTimerEvent::Run() src/xpcom/threads/nsTimerImpl.cpp:556
    #39 0x7f23a4db4580 in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:612
    #40 0x7f23a4a46ecb in NS_ProcessNextEvent_P(nsIThread*, bool) src/objdir-ff-asan-sym/xpcom/build/nsThreadUtils.cpp:220
    #41 0x7f23a34903b6 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:82
    #42 0x7f23a506ce11 in MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:208
    #43 0x7f23a506cc46 in MessageLoop::RunHandler() src/ipc/chromium/src/base/message_loop.cc:201
    #44 0x7f23a506cb2b in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:175
    #45 0x7f23a2937dda in nsBaseAppShell::Run() src/widget/xpwidgets/nsBaseAppShell.cpp:163
    #46 0x7f23a156a9b4 in nsAppStartup::Run() src/toolkit/components/startup/nsAppStartup.cpp:290
    #47 0x7f2397bdda4d in XREMain::XRE_mainRun() src/toolkit/xre/nsAppRunner.cpp:3782
    #48 0x7f2397be38c5 in XREMain::XRE_main(int, char**, nsXREAppData const*) src/toolkit/xre/nsAppRunner.cpp:3848
    #49 0x7f2397be6774 in XRE_main src/toolkit/xre/nsAppRunner.cpp:3923
    #50 0x40d013 in do_main(int, char**) src/browser/app/nsBrowserApp.cpp:174
    #51 0x40a755 in main src/browser/app/nsBrowserApp.cpp:279
    #52 0x7f23b5adbc4c in ?? ??:0
0x7f236f299db8 is located 56 bytes inside of 120-byte region [0x7f236f299d80,0x7f236f299df8)
freed by thread T0 here:
    #0 0x4c4af0 in free ??:0
    #1 0x7f23b2967586 in moz_free src/memory/mozalloc/mozalloc.cpp:51
    #2 0x7f239b3afccd in operator delete(void*) src/../../../dist/include/mozilla/mozalloc.h:224
    #3 0x7f239b28424d in nsNodeUtils::LastRelease(nsINode*) src/content/base/src/nsNodeUtils.cpp:260
    #4 0x7f239b138dfd in nsGenericDOMDataNode::Release() src/content/base/src/nsGenericDOMDataNode.cpp:113
    #5 0x7f239b3b00d7 in nsTextNode::Release() src/content/base/src/nsTextNode.cpp:125
    #6 0x7f239b3b018b in non-virtual thunk to nsTextNode::Release() src/gfx/cairo/cairo/src/cairo-surface-subsurface.c:0
    #7 0x7f2397ba18db in ~nsCOMPtr_base src/../../dist/include/nsCOMPtr.h:408
    #8 0x7f2399415899 in nsCOMPtr<nsIDOMNode>::~nsCOMPtr() src/../../../dist/include/nsCOMPtr.h:447
    #9 0x7f2399402ab6 in nsCOMPtr<nsIDOMNode>::~nsCOMPtr() src/../../../dist/include/nsCOMPtr.h:447
    #10 0x7f239e64ff9d in nsHTMLEditor::IsVisTextNode(nsIContent*, bool*, bool) src/editor/libeditor/html/nsHTMLEditor.cpp:4408
    #11 0x7f239e64e47a in nsHTMLEditor::IsTextInDirtyFrameVisible(nsIContent*) src/editor/libeditor/html/nsHTMLEditor.cpp:4348
    #12 0x7f239e11704e in nsEditor::IsEditable(nsIContent*) src/editor/libeditor/base/nsEditor.cpp:3742
    #13 0x7f239e67039c in nsHTMLEditor::IsEditable(nsIContent*) src/editor/libeditor/html/nsHTMLEditor.cpp:5459
    #14 0x7f239e110a91 in nsEditor::FindNode(nsINode*, bool, bool, bool) src/editor/libeditor/base/nsEditor.cpp:3407
    #15 0x7f239e10ca79 in nsEditor::GetPriorNode(nsINode*, bool, bool) src/editor/libeditor/base/nsEditor.cpp:3301
    #16 0x7f239e10c106 in nsEditor::GetPriorNode(nsINode*, int, bool, bool) src/editor/libeditor/base/nsEditor.cpp:3190
    #17 0x7f239e649789 in nsHTMLEditor::GetPriorHTMLNode(nsINode*, int, bool) src/editor/libeditor/html/nsHTMLEditor.cpp:4104
    #18 0x7f239e7c3182 in nsHTMLEditRules::GetPromotedPoint(nsHTMLEditRules::RulesEndpoint, nsIDOMNode*, int, EditAction, nsCOMPtr<nsIDOMNode>*, int*) src/editor/libeditor/html/nsHTMLEditRules.cpp:5277
    #19 0x7f239e6d078e in nsHTMLEditRules::PromoteRange(nsIDOMRange*, EditAction) src/editor/libeditor/html/nsHTMLEditRules.cpp:5493
    #20 0x7f239e6cac26 in nsHTMLEditRules::AfterEditInner(EditAction, short) src/editor/libeditor/html/nsHTMLEditRules.cpp:430
    #21 0x7f239e6c8df6 in nsHTMLEditRules::AfterEdit(EditAction, short) src/editor/libeditor/html/nsHTMLEditRules.cpp:376
    #22 0x7f239e63798c in nsHTMLEditor::EndOperation() src/editor/libeditor/html/nsHTMLEditor.cpp:3513
    #23 0x7f239e06d372 in ~nsAutoRules src/editor/libeditor/base/nsEditorUtils.h:94
    #24 0x7f239e04afc6 in ~nsAutoRules src/editor/libeditor/base/nsEditorUtils.h:91
    #25 0x7f239e610e37 in nsHTMLEditor::Align(nsAString_internal const&) src/editor/libeditor/html/nsHTMLEditor.cpp:2264
    #26 0x7f239e610fee in non-virtual thunk to nsHTMLEditor::Align(nsAString_internal const&) src/gfx/cairo/cairo/src/cairo-surface-subsurface.c:0
    #27 0x7f23a1b430b0 in nsAlignCommand::SetState(nsIEditor*, nsString&) src/editor/composer/src/nsComposerCommands.cpp:970
    #28 0x7f23a1b37113 in nsMultiStateCommand::DoCommandParams(char const*, nsICommandParams*, nsISupports*) src/editor/composer/src/nsComposerCommands.cpp:599
    #29 0x7f23a13912e2 in nsControllerCommandTable::DoCommandParams(char const*, nsICommandParams*, nsISupports*) src/embedding/components/commandhandler/src/nsControllerCommandTable.cpp:175
previously allocated by thread T0 here:
    #0 0x4c4bb0 in __interceptor_malloc ??:0
    #1 0x7f23b29676da in moz_xmalloc src/memory/mozalloc/mozalloc.cpp:57
    #2 0x7f239b3af0d0 in operator new(unsigned long) src/../../../dist/include/mozilla/mozalloc.h:200
    #3 0x7f239e412cc7 in nsHtml5TreeOperation::AppendText(unsigned short const*, unsigned int, nsIContent*, nsHtml5TreeOpExecutor*) src/parser/html/nsHtml5TreeOperation.cpp:163
    #4 0x7f239e41de8a in nsHtml5TreeOperation::Perform(nsHtml5TreeOpExecutor*, nsIContent**) src/parser/html/nsHtml5TreeOperation.cpp:444
    #5 0x7f239e43ab75 in nsHtml5TreeOpExecutor::RunFlushLoop() src/parser/html/nsHtml5TreeOpExecutor.cpp:564
    #6 0x7f239e477259 in nsHtml5ExecutorFlusher::Run() src/parser/html/nsHtml5StreamParser.cpp:127
    #7 0x7f23a4db4580 in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:612
    #8 0x7f23a4a46ecb in NS_ProcessNextEvent_P(nsIThread*, bool) src/objdir-ff-asan-sym/xpcom/build/nsThreadUtils.cpp:220
    #9 0x7f23a34903b6 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:82
    #10 0x7f23a506ce11 in MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:208
    #11 0x7f23a506cc46 in MessageLoop::RunHandler() src/ipc/chromium/src/base/message_loop.cc:201
    #12 0x7f23a506cb2b in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:175
    #13 0x7f23a2937dda in nsBaseAppShell::Run() src/widget/xpwidgets/nsBaseAppShell.cpp:163
    #14 0x7f23a156a9b4 in nsAppStartup::Run() src/toolkit/components/startup/nsAppStartup.cpp:290
    #15 0x7f2397bdda4d in XREMain::XRE_mainRun() src/toolkit/xre/nsAppRunner.cpp:3782
    #16 0x7f2397be38c5 in XREMain::XRE_main(int, char**, nsXREAppData const*) src/toolkit/xre/nsAppRunner.cpp:3848
    #17 0x7f2397be6774 in XRE_main src/toolkit/xre/nsAppRunner.cpp:3923
    #18 0x40d013 in do_main(int, char**) src/browser/app/nsBrowserApp.cpp:174
    #19 0x40a755 in main src/browser/app/nsBrowserApp.cpp:279
    #20 0x7f23b5adbc4c in ?? ??:0
Shadow byte and word:
  0x1fe46de533b7: fd
  0x1fe46de533b0: fd fd fd fd fd fd fd fd
More shadow bytes:
  0x1fe46de53390: 00 00 00 00 00 00 00 00
  0x1fe46de53398: 06 fb fb fb fb fb fb fb
  0x1fe46de533a0: fa fa fa fa fa fa fa fa
  0x1fe46de533a8: fa fa fa fa fa fa fa fa
=>0x1fe46de533b0: fd fd fd fd fd fd fd fd
  0x1fe46de533b8: fd fd fd fd fd fd fd fd
  0x1fe46de533c0: fa fa fa fa fa fa fa fa
  0x1fe46de533c8: fa fa fa fa fa fa fa fa
  0x1fe46de533d0: 00 00 00 00 00 00 00 00
Stats: 260M malloced (302M for red zones) by 541116 calls
Stats: 44M realloced by 25176 calls
Stats: 227M freed by 312858 calls
Stats: 92M really freed by 204843 calls
Stats: 484M (123992 full pages) mmaped in 121 calls
  mmaps   by size class: 8:311277; 9:32764; 10:12285; 11:14329; 12:3072; 13:1536; 14:1536; 15:256; 16:1024; 17:1248; 18:144; 19:40; 20:20;
  mallocs by size class: 8:466783; 9:36490; 10:11191; 11:16795; 12:2871; 13:1975; 14:1888; 15:396; 16:1190; 17:1317; 18:159; 19:41; 20:20;
  frees   by size class: 8:255470; 9:27519; 10:7990; 11:13506; 12:2059; 13:1677; 14:1698; 15:352; 16:1127; 17:1300; 18:105; 19:38; 20:17;
  rfrees  by size class: 8:182058; 9:9100; 10:1831; 11:8659; 12:596; 13:527; 14:531; 15:179; 16:934; 17:399; 18:24; 19:4; 20:1;
Stats: malloc large: 1537 small slow: 2600
==10788== ABORTING
Component: General → Editor
Product: Firefox → Core
Severity: normal → critical
Whiteboard: [asan]
Assignee: nobody → ayg
Sorry for taking so long to get to this -- I was away for the last eight days or so.  The test doesn't seem to do anything bad on trunk (as of an hour or two ago), even if I refresh a bunch of times.  Can you confirm that this will actually crash a trunk build right now?  Or should I be looking for something else?  I can't easily tell from the stack traces what's going on, particularly since the line numbers don't seem to match trunk anymore either.
1. It does not reproduce on trunk anymore, but before it used to reproduce easily with just one firefox instance.
2. The crash happens on line http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/base/nsEditor.cpp#3319 which i remember was the same line when i filed the bug.
3. You can use builds from https://people.mozilla.com/~choller/firefox/asan/ to see when it stopped reproducing. it is like 10 days since the bug is filed and i see builds till 9/30 there.
Yeah, let's try to bisect it to see what fixed it, and whether that was expected.  This might have been just wallpapered by something else, so we shouldn't necessarily assume that it's fixed unless we know for sure.
I can't use ASAN builds on my desktop because it's 32-bit Linux, and the builds seem to be only 64-bit.  I'll try on my laptop.  I tried with some nightlies from <http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/>, and couldn't reproduce on any of them: 2012-09-24-mozilla-central-debug, 2012-09-29-03-06-24-mozilla-central, 2012-09-29-mozilla-central-debug.  The stack traces here don't make a lot of sense to me, so I probably need a local reproduction to track it down.
I can reproduce with the ASAN builds, but not with a regular debug nightly of the same night.  20121006-mozilla-central-linux64-debug-2da1f2bde40e+asan does not exhibit the problem, but 20121004-mozilla-central-linux64-debug-4cb8f88213f5+asan does.  Linux m-c builds for 2012-10-05 are not available, so that's the smallest range I can get using ASAN builds.  So this is the range:

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=4cb8f88213f5&tochange=2da1f2bde40e

Ehsan, what should I do here?  I don't see a way to debug the problem without building locally, but it seems like I need to build with ASAN to detect the problem, which looks to be something of an ordeal.  I'm not totally sure I can, because my machine is 32 bits with PAE, rather than 64 bits as recommended.  Do you think this is a good use of a few hours of my time, or can you think of a better way to approach this?

In particular, is there any way to detect the bug in a non-ASAN build, such as by adding a bit of extra code somewhere?  That would make things a lot easier.
Building with ASAN should not be very hard.  There's some initial setup in terms of building your own clang, and after that it's mostly a mozconfig change.  Have you tried the instructions here? <https://developer.mozilla.org/en-US/docs/Building_Firefox_with_Address_Sanitizer>  I'm not aware of anything which prevents you from doing 32-bit ASAN builds.

It seems like ASAN builds are the only way we can reproduce this...
It reproduces easily every time on my older windows build (i think 1-2 weeks old). As i was debugging, candidate raw node looks get freed in the IsEditable call and is later used in FindNode.

if (!aEditableNode || IsEditable(candidate)) {
    return candidate;
  }

  return FindNode(candidate, aGoForward, aEditableNode, bNoBlockCrossing);

Specifically, something in this part of the code is freeing it [nsHTMLEditor::IsVisTextNode].

    if (aNode->TextIsOnlyWhitespace())
    {
      nsCOMPtr<nsIDOMNode> node = do_QueryInterface(aNode);
      nsWSRunObject wsRunObj(this, node, 0);
      nsCOMPtr<nsIDOMNode> visNode;
      int32_t outVisOffset=0;
      WSType visType;
      wsRunObj.NextVisibleNode(node, 0, address_of(visNode),
                               &outVisOffset, &visType);
      if (visType == WSType::normalWS || visType == WSType::text) {
        *outIsEmptyNode = (node != visNode);
      }
    }
Put a alert(1) in the event handler and see it getting fired here

  if (content && content->IsElement()) {
    elementStyle = nsComputedDOMStyle::GetStyleContextForElement(content->AsElement(),
                                                                 nullptr,
                                                                 ps);

That event handler blows away the nodes.

>	xul.dll!nsEditor::IsPreformatted(nsIDOMNode * aNode, bool * aResult)  Line 4047	C++
 	xul.dll!nsWSRunObject::GetRuns()  Line 914	C++
 	xul.dll!nsWSRunObject::nsWSRunObject(nsHTMLEditor * aEd, nsIDOMNode * aNode, int aOffset)  Line 61	C++
 	xul.dll!nsHTMLEditor::IsVisTextNode(nsIContent * aNode, bool * outIsEmptyNode, bool aSafeToAskFrames)  Line 4400	C++
 	xul.dll!nsHTMLEditor::IsTextInDirtyFrameVisible(nsIContent * aNode)  Line 4348 + 0x12 bytes	C++
 	xul.dll!nsEditor::IsEditable(nsIContent * aNode)  Line 3742 + 0x14 bytes	C++
 	xul.dll!nsHTMLEditor::IsEditable(nsIContent * aNode)  Line 5459 + 0xc bytes	C++
 	xul.dll!nsEditor::FindNode(nsINode * aCurrentNode, bool aGoForward, bool aEditableNode, bool bNoBlockCrossing)  Line 3407 + 0x1c bytes	C++
 	xul.dll!nsEditor::GetPriorNode(nsINode * aCurrentNode, bool aEditableNode, bool aNoBlockCrossing)  Line 3302	C++
 	xul.dll!nsEditor::GetPriorNode(nsINode * aParentNode, int aOffset, bool aEditableNode, bool aNoBlockCrossing)  Line 3190 + 0x16 bytes	C++
 	xul.dll!nsHTMLEditor::GetPriorHTMLNode(nsINode * aParent, int aOffset, bool aNoBlockCrossing)  Line 4105	C++
 	xul.dll!nsHTMLEditRules::GetPromotedPoint(nsHTMLEditRules::RulesEndpoint aWhere, nsIDOMNode * aNode, int aOffset, EditAction actionID, nsCOMPtr<nsIDOMNode> * outNode, int * outOffset)  Line 5277 + 0x1a bytes	C++
 	xul.dll!nsHTMLEditRules::PromoteRange(nsIDOMRange * inRange, EditAction inOperationType)  Line 5495	C++
 	xul.dll!nsHTMLEditRules::AfterEditInner(EditAction action, short aDirection)  Line 430 + 0x1e bytes	C++
 	xul.dll!nsHTMLEditRules::AfterEdit(EditAction action, short aDirection)  Line 376 + 0x11 bytes	C++
 	xul.dll!nsHTMLEditor::EndOperation()  Line 3513 + 0x46 bytes	C++
 	xul.dll!nsAutoRules::~nsAutoRules()  Line 96	C++
 	xul.dll!nsHTMLEditor::Align(const nsAString_internal & aAlignType)  Line 2263 + 0x26 bytes	C++
 	xul.dll!nsAlignCommand::SetState(nsIEditor * aEditor, nsString & newState)  Line 970 + 0x20 bytes	C++
 	xul.dll!nsMultiStateCommand::DoCommandParams(const char * aCommandName, nsICommandParams * aParams, nsISupports * refCon)  Line 599 + 0x1d bytes	C++
 	xul.dll!nsControllerCommandTable::DoCommandParams(const char * aCommandName, nsICommandParams * aParams, nsISupports * aCommandRefCon)  Line 175 + 0x25 bytes	C++
 	xul.dll!nsBaseCommandController::DoCommandWithParams(const char * aCommand, nsICommandParams * aParams)  Line 153 + 0x28 bytes	C++
 	xul.dll!nsCommandManager::DoCommand(const char * aCommandName, nsICommandParams * aCommandParams, nsIDOMWindow * aTargetWindow)  Line 234 + 0x21 bytes	C++
 	xul.dll!nsHTMLDocument::ExecCommand(const nsAString_internal & commandID, bool doShowUI, const nsAString_internal & value, bool * _retval)  Line 3235 + 0x41 bytes	C++
 	xul.dll!NS_InvokeByIndex_P(nsISupports * that, unsigned int methodIndex, unsigned int paramCount, nsXPTCVariant * params)  Line 71	C++
 	xul.dll!CallMethodHelper::Invoke()  Line 3110 + 0x1c bytes	C++
 	xul.dll!CallMethodHelper::Call()  Line 2444 + 0x8 bytes	C++
 	xul.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx, XPCWrappedNative::CallMode mode)  Line 2410 + 0x16 bytes	C++
 	xul.dll!XPC_WN_CallMethod(JSContext * cx, unsigned int argc, JS::Value * vp)  Line 1469 + 0xb bytes	C++
 	mozjs.dll!js::CallJSNative(JSContext * cx, int (JSContext *, unsigned int, JS::Value *)* native, const JS::CallArgs & args)  Line 370 + 0x19 bytes	C++
 	mozjs.dll!js::InvokeKernel(JSContext * cx, JS::CallArgs args, js::MaybeConstruct construct)  Line 367 + 0x1d bytes	C++
 	mozjs.dll!js::Interpret(JSContext * cx, js::StackFrame * entryFrame, js::InterpMode interpMode)  Line 2460 + 0x2a bytes	C++
 	mozjs.dll!js::RunScript(JSContext * cx, JS::Handle<JSScript *> script, js::StackFrame * fp)  Line 324 + 0xf bytes	C++
 	mozjs.dll!js::ExecuteKernel(JSContext * cx, JS::Handle<JSScript *> script, JSObject & scopeChain, const JS::Value & thisv, js::ExecuteType type, js::StackFrame * evalInFrame, JS::Value * result)  Line 509 + 0x16 bytes	C++
 	mozjs.dll!js::Execute(JSContext * cx, JS::Handle<JSScript *> script, JSObject & scopeChainArg, JS::Value * rval)  Line 547 + 0x22 bytes	C++
 	mozjs.dll!JS::Evaluate(JSContext * cx, JS::Handle<JSObject *> obj, JS::CompileOptions options, const wchar_t * chars, unsigned int length, JS::Value * rval)  Line 5679 + 0x24 bytes	C++
 	xul.dll!nsJSContext::EvaluateString(const nsAString_internal & aScript, JSObject * aScopeObject, nsIPrincipal * aPrincipal, nsIPrincipal * aOriginPrincipal, const char * aURL, unsigned int aLineNo, JSVersion aVersion, nsAString_internal * aRetValue, bool * aIsUndefined)  Line 1506 + 0x55 bytes	C++
 	xul.dll!nsGlobalWindow::RunTimeoutHandler(nsTimeout * aTimeout, nsIScriptContext * aScx)  Line 9649 + 0x59 bytes	C++
 	xul.dll!nsGlobalWindow::RunTimeout(nsTimeout * aTimeout)  Line 9906 + 0x1e bytes	C++
 	xul.dll!nsGlobalWindow::TimerCallback(nsITimer * aTimer, void * aClosure)  Line 10174	C++
 	xul.dll!nsTimerImpl::Fire()  Line 473 + 0xe bytes	C++
 	xul.dll!nsTimerEvent::Run()  Line 558	C++
 	xul.dll!nsThread::ProcessNextEvent(bool mayWait, bool * result)  Line 612 + 0x19 bytes	C++
 	xul.dll!NS_ProcessNextEvent_P(nsIThread * thread, bool mayWait)  Line 220 + 0x17 bytes	C++
 	xul.dll!mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate * aDelegate)  Line 82 + 0xe bytes	C++
 	xul.dll!MessageLoop::RunInternal()  Line 209	C++
 	xul.dll!MessageLoop::RunHandler()  Line 202	C++
 	xul.dll!MessageLoop::Run()  Line 176	C++
 	xul.dll!nsBaseAppShell::Run()  Line 165	C++
 	xul.dll!nsAppShell::Run()  Line 232 + 0x9 bytes	C++
 	xul.dll!nsAppStartup::Run()  Line 290 + 0x1c bytes	C++
 	xul.dll!XREMain::XRE_mainRun()  Line 3782 + 0x22 bytes	C++
 	xul.dll!XREMain::XRE_main(int argc, char * * argv, const nsXREAppData * aAppData)  Line 3848 + 0x8 bytes	C++
 	xul.dll!XRE_main(int argc, char * * argv, const nsXREAppData * aAppData, unsigned int aFlags)  Line 3923 + 0x17 bytes	C++
 	firefox.exe!do_main(int argc, char * * argv)  Line 174 + 0x15 bytes	C++
 	firefox.exe!NS_internal_main(int argc, char * * argv)  Line 279 + 0xd bytes	C++
 	firefox.exe!wmain(int argc, wchar_t * * argv)  Line 105 + 0xd bytes	C++
 	firefox.exe!__tmainCRTStartup()  Line 552 + 0x19 bytes	C
 	firefox.exe!wmainCRTStartup()  Line 371	C
 	kernel32.dll!@BaseThreadInitThunk@12()  + 0x12 bytes	
 	ntdll.dll!___RtlUserThreadStart@8()  + 0x27 bytes	
 	ntdll.dll!__RtlUserThreadStart@8()  + 0x1b bytes
if (aNode->TextIsOnlyWhitespace())
    {
      nsCOMPtr<nsIDOMNode> node = do_QueryInterface(aNode);
      ........
    }

since node maintains the ref, as soon as the if ends, you will see aNode (raw pointer) going stale.
Now I started getting consistent segfaults in my regular debug builds on this test-case.  Well, okay.  Stepping through, I find that going into IsVisTextNode, aNode has a refcount of 1.  After the nsWSRunObject constructor, it goes up to 7.  It stays there until the end of the block, and after the end of the block the node is killed.  So the nsWSRunObject destructor is Release()ing more times than the constructor is AddRef()ing.  Now I'll just step through and see why that's happening.
So the basic bug seems to still exist in trunk -- the destructor still releases more times than the constructor addrefs.  I printed out refcounts at various points using List():

IsVisTextNode 1:Text@0x9d5c3ab0 flags=[00c00080] primaryframe=(nil) refcount=4<\u000a\u000a>
IsVisTextNode 2:Text@0x9d5c3ab0 flags=[00c00080] primaryframe=(nil) refcount=5<\u000a\u000a>
Constructor 1: Text@0x9d5c3ab0 flags=[00c00080] primaryframe=(nil) refcount=7<\u000a\u000a>
Constructor 2: Text@0x9d5c3ab0 flags=[00c00080] primaryframe=(nil) refcount=10<\u000a\u000a>
Constructor 3: Text@0x9d5c3ab0 flags=[00000080] primaryframe=(nil) refcount=11<\u000a\u000a>
IsVisTextNode 3:Text@0x9d5c3ab0 flags=[00000080] primaryframe=(nil) refcount=10<\u000a\u000a>
IsVisTextNode 4:Text@0x9d5c3ab0 flags=[00000080] primaryframe=(nil) refcount=10<\u000a\u000a>
Destructor 1: Text@0x9d5c3ab0 flags=[00000080] primaryframe=(nil) refcount=11<\u000a\u000a>
Destructor 2: Text@0x9d5c3ab0 flags=[00000080] primaryframe=(nil) refcount=9<\u000a\u000a>
IsVisTextNode 5:Text@0x9d5c3ab0 flags=[00000080] primaryframe=(nil) refcount=3<\u000a\u000a>

So at the end, the refcount is still one lower than at the start.  It just happens that this makes it go from 4 to 3 instead of from 1 to 0, so a use-after-free is averted in this specific case, but something is still very wrong here!

The problem is, I'm not clear how this could be happening.  I'll try setting conditional breakpoints on AddRef() and Release() and see if I can puzzle it out.
Okay, so after a good hour or so of painstakingly stepping through every AddRef() and Release(), here's the issue.  The constructor for nsWSRunObject calls nsWSRunObject::GetRuns.  This wants to figure out how to break up the whitespace, so it calls nsEditor::IsPreformatted.  This needs style info, so it calls nsComputedDOMStyle::GetStyleContextForElement.  This then calls PresShell::FlushPendingNotifications, which eventually winds up calling JS.  The JS modifies the DOM.

So basically, calling IsEditable() can wind up running script, which can Release().  Thus anything that calls IsEditable() must make sure it has strong references to anything it plans to refer to later on.  It so happens that something randomly changed in trunk that meant that in this case the refcount wound up being nonzero anyway, but we can't depend on that.

It goes back to nsEditor::FindNode doing

  nsIContent* candidate =
    FindNextLeafNode(aCurrentNode, aGoForward, bNoBlockCrossing);

and then passing candidate to a function after calling IsEditable(candidate).  Since it doesn't keep a reference, it blows up.  This is due to bug 635618 -- it rewrote the function to use nsINode*, and changed nsCOMPtr<nsIDOMNode> candidate to nsIContent* candidate in the process.  Boris was probably making silly assumptions like "IsEditable() will not mutate the DOM".  This is why I always use nsCOMPtr even if it looks like it shouldn't be necessary -- who knows what crazy stuff can happen further down on the stack?

I'll make this use nsCOMPtr, and look for any other patches from that bug that also might have taken away nsCOMPtr.
Blocks: 635618
Attached patch PatchSplinter Review
Bug 635618 removed nsCOMPtr use in a whole bunch more places than this.  I don't *think* any of them would be a problem right now, but it still makes me nervous, because someone could change any of the relevant code at any time and not realize they're introducing a use-after-free.  Would you like a patch to change them all to use nsCOMPtr instead of raw pointers?  I somehow don't think any of this is performance-critical.
Attachment #670328 - Flags: review?(ehsan)
(In reply to :Aryeh Gregor from comment #13)
> Created attachment 670328 [details] [diff] [review]
> Patch
> 
> Bug 635618 removed nsCOMPtr use in a whole bunch more places than this.  I
> don't *think* any of them would be a problem right now, but it still makes
> me nervous, because someone could change any of the relevant code at any
> time and not realize they're introducing a use-after-free.  Would you like a
> patch to change them all to use nsCOMPtr instead of raw pointers?

Yes please!  And thanks for the extensive analysis.

Note that bug 796839 recently touched the code around there, so that may describe some behavior differences between trunk/aurora and older branches.
Attachment #670328 - Flags: review?(ehsan) → review+
Comment on attachment 670328 [details] [diff] [review]
Patch

[Security approval request comment]
How easily can the security issue be deduced from the patch? fairly easily.  This sort of screams refcounting bug.

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

Which older supported branches are affected by this flaw? all except ESR.

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

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

How likely is this patch to cause regressions; how much testing does it need? not likely at all.
Attachment #670328 - Flags: sec-approval?
Thanks for the activity on this bug. Can you advise which versions of Firefox need patching?
(In reply to David Bolter [:davidb] from comment #16)
> Thanks for the activity on this bug. Can you advise which versions of
> Firefox need patching?

All except ESR.
Keywords: csec-uaf
I searched using the following regex:

  find editor/ -name '*.cpp' -exec grep '^ \+nsI[a-zA-Z_]\+ \?\*.*=' {} +

This will find anything that looks like "nsIFoo*" or "nsIFoo *" at the start of the line, if there's an = anywhere later on the line.  Crude, but it largely works.  I found a lot of things that matched it, and fixed all of them, skipping nsIFrame and nsITransaction.  (Later I undid changes to nsITableCellLayout too.)  I used the following command to fix:

  find editor/ -name '*.cpp' -exec sed -i 's/^\( \+\)nsI\(Atom\|BidiKeyboard\|Content\|DocShell\|Document\|DOMElement\|DOMNode\|FocusManager\|LoadContext\|Node\|ParserService\|PresShell\|Principal\|TableCellLayout\|TransactionListener\|URI\|WordBreaker\)\(\* \| \*\)\(.*=\)/\1nsCOMPtr<nsI\2> \4/g' {} +

Then I had to manually fix up places that were now invalid static_casts, one case with "nsIDOMNode *a, *b =" that became "nsCOMPtr<nsIDOMNode> a, *b", etc.  A lot of these places are obviously okay if manually inspected and don't actually need changing, but it would be a lot of error-prone work to figure out which are which.  There were around 160 total places changed.

But this caused problems, like

###!!! ASSERTION: Changing refcount of nsDocument object during Traverse is not permitted!: 'Error', file /mnt/ssd/checkouts/central/content/base/src/nsDocument.cpp, line 1713

So it doesn't seem to be a good idea to change *everything* like this.  Do you have any suggestions as to how I could narrow it down?
Since there's little risk of regression and the patch makes it fairly obvious what kind of problem this is, please wait a week or so and check-in around the midpoint of the cycle.
Whiteboard: [asan] → [asan] wait until Oct 21 to land
(In reply to :Aryeh Gregor from comment #19)
> I searched using the following regex:
> 
>   find editor/ -name '*.cpp' -exec grep '^ \+nsI[a-zA-Z_]\+ \?\*.*=' {} +
> 
> This will find anything that looks like "nsIFoo*" or "nsIFoo *" at the start
> of the line, if there's an = anywhere later on the line.  Crude, but it
> largely works.  I found a lot of things that matched it, and fixed all of
> them, skipping nsIFrame and nsITransaction.  (Later I undid changes to
> nsITableCellLayout too.)  I used the following command to fix:
> 
>   find editor/ -name '*.cpp' -exec sed -i 's/^\(
> \+\)nsI\(Atom\|BidiKeyboard\|Content\|DocShell\|Document\|DOMElement\|DOMNode
> \|FocusManager\|LoadContext\|Node\|ParserService\|PresShell\|Principal\|Table
> CellLayout\|TransactionListener\|URI\|WordBreaker\)\(\* \|
> \*\)\(.*=\)/\1nsCOMPtr<nsI\2> \4/g' {} +
> 
> Then I had to manually fix up places that were now invalid static_casts, one
> case with "nsIDOMNode *a, *b =" that became "nsCOMPtr<nsIDOMNode> a, *b",
> etc.  A lot of these places are obviously okay if manually inspected and
> don't actually need changing, but it would be a lot of error-prone work to
> figure out which are which.  There were around 160 total places changed.
> 
> But this caused problems, like
> 
> ###!!! ASSERTION: Changing refcount of nsDocument object during Traverse is
> not permitted!: 'Error', file
> /mnt/ssd/checkouts/central/content/base/src/nsDocument.cpp, line 1713
> 
> So it doesn't seem to be a good idea to change *everything* like this.  Do
> you have any suggestions as to how I could narrow it down?

Hmm, not besides doing the stuff in the sed command one by one...  :/
(In reply to Daniel Veditz [:dveditz] from comment #20)
> Since there's little risk of regression and the patch makes it fairly
> obvious what kind of problem this is, please wait a week or so and check-in
> around the midpoint of the cycle.

I'll assume that around that time someone will grant the sec-approval flag, at which point we'll remember about landing this patch.  :-)
yup, that's a fair assumption. Leaving the sec-approval in the request state nags us constantly :-)
Whiteboard: [asan] wait until Oct 21 to land → [asan]
Comment on attachment 670328 [details] [diff] [review]
Patch

sec-approval+

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 635618
User impact if declined: potentially exploitable crash
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): none
String or UUID changes made by this patch: none
Attachment #670328 - Flags: sec-approval?
Attachment #670328 - Flags: sec-approval+
Attachment #670328 - Flags: approval-mozilla-beta?
Attachment #670328 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/1d2cb3aeb1ac
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Attachment #670328 - Flags: approval-mozilla-beta?
Attachment #670328 - Flags: approval-mozilla-beta+
Attachment #670328 - Flags: approval-mozilla-aurora?
Attachment #670328 - Flags: approval-mozilla-aurora+
Whiteboard: [asan] → [asan][adv-track-main17+]
Keywords: verifyme
Alias: CVE-2012-4213
Flags: sec-bounty+
Group: core-security
Keywords: regression
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.