Heap-use-after-free in mozilla::RecomputeDirectionality

RESOLVED FIXED in Firefox 20

Status

()

Core
Layout: Text
--
critical
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Abhishek Arya, Assigned: smontagu)

Tracking

(6 keywords)

Trunk
mozilla20
x86_64
All
crash, csectype-uaf, regression, reproducible, sec-critical, testcase
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty +
in-testsuite +

Firefox Tracking Flags

(firefox19 unaffected, firefox20+ fixed, firefox-esr10 unaffected, firefox-esr17 unaffected, b2g18 unaffected)

Details

(Whiteboard: [asan][adv-main20-])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Created attachment 685475 [details]
Testcase

Reproduces on trunk, looks like regression from http://hg.mozilla.org/mozilla-central/rev/7f2e295e4981

==518== ERROR: AddressSanitizer: heap-use-after-free on address 0x7fe0d36b0a6c at pc 0x7fe0fb4e9640 bp 0x7fff7754b430 sp 0x7fff7754b428
READ of size 4 at 0x7fe0d36b0a6c thread T0
    #0 0x7fe0fb4e963f in nsINode::GetBoolFlag(nsINode::BooleanFlag) const src/content/base/public/nsINode.h:1334
    #1 0x7fe0fd312d5a in nsINode::HasTextNodeDirectionalityMap() const src/../../../dist/include/nsINode.h:1417
    #2 0x7fe0fd30f424 in mozilla::nsTextNodeDirectionalityMap::RemoveElementFromMap(nsINode*, mozilla::dom::Element*) src/content/base/src/DirectionalityUtils.cpp:510
    #3 0x7fe0fd30eaea in mozilla::RecomputeDirectionality(mozilla::dom::Element*, bool) src/content/base/src/DirectionalityUtils.cpp:551
    #4 0x7fe0fe864870 in nsGenericHTMLElement::AfterSetAttr(int, nsIAtom*, nsAttrValue const*, bool) src/content/html/content/src/nsGenericHTMLElement.cpp:1760
    #5 0x7fe0fd8c6951 in mozilla::dom::Element::SetAttrAndNotify(int, nsIAtom*, nsIAtom*, nsAttrValue const&, nsAttrValue&, unsigned char, bool, bool, bool) src/content/base/src/Element.cpp:1883
    #6 0x7fe0fd8c3ab7 in mozilla::dom::Element::SetAttr(int, nsIAtom*, nsIAtom*, nsAString_internal const&, bool) src/content/base/src/Element.cpp:1775
    #7 0x7fe0fe86703d in nsGenericHTMLElement::SetAttr(int, nsIAtom*, nsIAtom*, nsAString_internal const&, bool) src/content/html/content/src/nsGenericHTMLElement.cpp:1838
    #8 0x7fe0fd8ac9e1 in mozilla::dom::Element::SetAttribute(nsAString_internal const&, nsAString_internal const&, mozilla::ErrorResult&) src/content/base/src/Element.cpp:747
    #9 0x7fe1035f0c13 in nsIDOMElement_SetAttribute(JSContext*, unsigned int, JS::Value*) src/objdir-ff-asan-sym/js/xpconnect/src/dom_quickstubs.cpp:1992
    #10 0x7fe10f7de089 in js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) src/js/src/jscntxtinlines.h:364
    #11 0x7fe10f7de089 in js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) src/js/src/jsinterp.cpp:369
    #12 0x7fe10f78d875 in js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) src/js/src/jsinterp.cpp:2334
    #13 0x7fe10f6ed4ee in js::RunScript(JSContext*, JS::Handle<JSScript*>, js::StackFrame*) src/js/src/jsinterp.cpp:326
    #14 0x7fe10f7eb68d in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::ExecuteType, js::StackFrame*, JS::Value*) src/js/src/jsinterp.cpp:515
    #15 0x7fe10f7ed34b in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) src/js/src/jsinterp.cpp:552
    #16 0x7fe10ef3cbed in JS::Evaluate(JSContext*, JS::Handle<JSObject*>, JS::CompileOptions, unsigned short const*, unsigned long, JS::Value*) src/js/src/jsapi.cpp:5612
    #17 0x7fe0ffeb6fcb in nsJSContext::EvaluateString(nsAString_internal const&, JSObject*, nsIPrincipal*, nsIPrincipal*, char const*, unsigned int, JSVersion, nsAString_internal*, bool*) src/dom/base/nsJSEnvironment.cpp:1520
    #18 0x7fe100078071 in nsGlobalWindow::RunTimeoutHandler(nsTimeout*, nsIScriptContext*) src/dom/base/nsGlobalWindow.cpp:9686
    #19 0x7fe10002cb3e in nsGlobalWindow::RunTimeout(nsTimeout*) src/dom/base/nsGlobalWindow.cpp:9945
    #20 0x7fe100076186 in nsGlobalWindow::TimerCallback(nsITimer*, void*) src/dom/base/nsGlobalWindow.cpp:10214
    #21 0x7fe1083561a8 in nsTimerImpl::Fire() src/xpcom/threads/nsTimerImpl.cpp:482
    #22 0x7fe108357631 in nsTimerEvent::Run() src/xpcom/threads/nsTimerImpl.cpp:565
    #23 0x7fe10831a32e in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:627
    #24 0x7fe107f8ff7f in NS_ProcessNextEvent_P(nsIThread*, bool) src/objdir-ff-asan-sym/xpcom/build/nsThreadUtils.cpp:221
    #25 0x7fe10667907d in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:117
    #26 0x7fe108602e2e in MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:215
    #27 0x7fe108602c75 in MessageLoop::RunHandler() src/ipc/chromium/src/base/message_loop.cc:208
    #28 0x7fe108602b5b in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:182
    #29 0x7fe105a8c414 in nsBaseAppShell::Run() src/widget/xpwidgets/nsBaseAppShell.cpp:163
    #30 0x7fe1045ef232 in nsAppStartup::Run() src/toolkit/components/startup/nsAppStartup.cpp:290
    #31 0x7fe0f9b28bd4 in XREMain::XRE_mainRun() src/toolkit/xre/nsAppRunner.cpp:3823
    #32 0x7fe0f9b2e8a9 in XREMain::XRE_main(int, char**, nsXREAppData const*) src/toolkit/xre/nsAppRunner.cpp:3890
    #33 0x7fe0f9b31620 in XRE_main src/toolkit/xre/nsAppRunner.cpp:4084
    #34 0x40c146 in do_main(int, char**) src/browser/app/nsBrowserApp.cpp:174
    #35 0x409980 in main src/browser/app/nsBrowserApp.cpp:279
    #36 0x7fe119da476c in
0x7fe0d36b0a6c is located 44 bytes inside of 120-byte region [0x7fe0d36b0a40,0x7fe0d36b0ab8)
freed by thread T0 here:
    #0 0x4c37d0 in __interceptor_free
    #1 0x7fe117932405 in moz_free src/memory/mozalloc/mozalloc.cpp:48
    #2 0x7fe0fdbc3c1d in operator delete(void*) src/../../../dist/include/mozilla/mozalloc.h:224
    #3 0x7fe0fdbc3c1d in nsTextNode::~nsTextNode() src/content/base/src/nsTextNode.cpp:123
    #4 0x7fe0fda92462 in nsNodeUtils::LastRelease(nsINode*) src/content/base/src/nsNodeUtils.cpp:257
    #5 0x7fe0fd912887 in nsGenericDOMDataNode::Release() src/content/base/src/nsGenericDOMDataNode.cpp:115
    #6 0x7fe0fdbc4107 in nsTextNode::Release() src/content/base/src/nsTextNode.cpp:127
    #7 0x7fe0f9af2c3b in nsCOMPtr_base::~nsCOMPtr_base() src/objdir-ff-asan-sym/media/webrtc/signaling/signaling_ecc/../../../../dist/include/nsCOMPtr.h:410
    #8 0x7fe0fb79c048 in nsCOMPtr<nsIContent>::~nsCOMPtr() src/../../dist/include/nsCOMPtr.h:449
    #9 0x7fe0fb79bd35 in nsCOMPtr<nsIContent>::~nsCOMPtr() src/../../dist/include/nsCOMPtr.h:449
    #10 0x7fe0fdd54118 in mozilla::dom::FragmentOrElement::RemoveChildAt(unsigned int, bool) src/content/base/src/FragmentOrElement.cpp:890
    #11 0x7fe0fe858020 in nsGenericHTMLElement::SetInnerHTML(nsAString_internal const&, mozilla::ErrorResult&) src/content/html/content/src/nsGenericHTMLElement.cpp:1206
    #12 0x7fe103a5a4f6 in nsIDOMHTMLElement_SetInnerHTML(JSContext*, JS::Handle<JSObject*>, JS::Handle<long>, int, JS::MutableHandle<JS::Value>) src/objdir-ff-asan-sym/js/xpconnect/src/dom_quickstubs.cpp:7938
    #13 0x7fe10fabc165 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>) src/js/src/jscntxtinlines.h:448
    #14 0x7fe10fabc165 in js::Shape::set(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, bool, JS::MutableHandle<JS::Value>) src/js/src/jsscopeinlines.h:333
    #15 0x7fe10faf275d in js::baseops::SetPropertyHelper(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, JS::Handle<long>, unsigned int, JS::MutableHandle<JS::Value>, int) src/js/src/jsobj.cpp:4584
    #16 0x7fe10f81cc61 in js::SetPropertyOperation(JSContext*, unsigned char*, JS::Handle<JS::Value>, JS::Handle<JS::Value>) src/js/src/jsinterpinlines.h:364
previously allocated by thread T0 here:
    #0 0x4c3890 in malloc
    #1 0x7fe117932541 in moz_xmalloc src/memory/mozalloc/mozalloc.cpp:54
    #2 0x7fe0fdbc301f in operator new(unsigned long) src/../../../dist/include/mozilla/mozalloc.h:200
    #3 0x7fe0fdbc301f in NS_NewTextNode(nsIContent**, nsNodeInfoManager*) src/content/base/src/nsTextNode.cpp:105
    #4 0x7fe1013802bd in nsHtml5TreeOperation::AppendText(unsigned short const*, unsigned int, nsIContent*, nsHtml5TreeOpExecutor*) src/parser/html/nsHtml5TreeOperation.cpp:164
    #5 0x7fe10138b506 in nsHtml5TreeOperation::Perform(nsHtml5TreeOpExecutor*, nsIContent**) src/parser/html/nsHtml5TreeOperation.cpp:457
    #6 0x7fe1013a9502 in nsHtml5TreeOpExecutor::RunFlushLoop() src/parser/html/nsHtml5TreeOpExecutor.cpp:565
    #7 0x7fe1013e7a99 in nsHtml5ExecutorFlusher::Run() src/parser/html/nsHtml5StreamParser.cpp:129
    #8 0x7fe10831a32e in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:627
    #9 0x7fe107f8ff7f in NS_ProcessNextEvent_P(nsIThread*, bool) src/objdir-ff-asan-sym/xpcom/build/nsThreadUtils.cpp:221
Shadow byte and word:
  0x1ffc1a6d614d: fd
  0x1ffc1a6d6148: fd fd fd fd fd fd fd fd
More shadow bytes:
  0x1ffc1a6d6128: 00 00 00 00 00 00 00 00
  0x1ffc1a6d6130: 00 00 00 00 00 00 00 fb
  0x1ffc1a6d6138: fa fa fa fa fa fa fa fa
  0x1ffc1a6d6140: fa fa fa fa fa fa fa fa
=>0x1ffc1a6d6148: fd fd fd fd fd fd fd fd
  0x1ffc1a6d6150: fd fd fd fd fd fd fd fd
  0x1ffc1a6d6158: fa fa fa fa fa fa fa fa
  0x1ffc1a6d6160: fa fa fa fa fa fa fa fa
  0x1ffc1a6d6168: fd fd fd fd fd fd fd fd
Stats: 206M malloced (192M for red zones) by 303102 calls
Stats: 33M realloced by 19469 calls
Stats: 174M freed by 173854 calls
Stats: 135M really freed by 146679 calls
Stats: 214M (54832 full pages) mmaped in 405 calls
  mmaps   by size class: 7:147420; 8:34799; 9:14322; 10:5110; 11:6120; 12:1152; 13:768; 14:544; 15:192; 16:576; 17:452; 18:28; 19:34; 20:21;
  mallocs by size class: 7:204386; 8:50002; 9:23156; 10:7924; 11:9868; 12:2106; 13:1629; 14:1423; 15:297; 16:898; 17:1315; 18:38; 19:38; 20:22;
  frees   by size class: 7:118172; 8:20912; 9:16539; 10:4725; 11:7658; 12:1215; 13:1162; 14:1266; 15:189; 16:633; 17:1300; 18:28; 19:36; 20:19;
  rfrees  by size class: 7:104746; 8:16540; 9:10096; 10:3345; 11:6949; 12:988; 13:899; 14:1174; 15:145; 16:526; 17:1255; 18:11; 19:4; 20:1;
Stats: malloc large: 2608 small slow: 3842
==518== ABORTING

Updated

5 years ago
Blocks: 548206
Severity: normal → critical
Component: General → Layout: Text
Keywords: crash, reproducible, testcase
Product: Firefox → Core
Whiteboard: [asan]

Comment 1

5 years ago
Could be a dupe of bug 815477.
(Assignee)

Comment 2

5 years ago
Created attachment 685810 [details] [diff] [review]
Patch
Assignee: nobody → smontagu
Attachment #685810 - Flags: review?(peterv)
Could you explain why we're crashing, because I don't understand. It looks like the textNode is dead, but it's still in the dirAutoSetBy property of an element?
(Assignee)

Comment 4

5 years ago
Yes, exactly. As far as I can figure out, it's a race condition where the line "test1.innerHTML =..." causes the dirAutoSetBy property of test2 to change, but the timeOut'd "test2.setAttribute("dir", "&locale.dir;")" triggers a call to RecomputeDirectionality while the dirAutoSetBy is still pointing to the old textnode which has meanwhile been freed.
(Assignee)

Comment 5

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=4392e3725778 together with the patch for bug 815276 and tests for both bugs and bug 815477.
Blocks: 815477
status-firefox19: --- → unaffected
status-firefox20: --- → affected
status-firefox-esr17: --- → unaffected
tracking-firefox20: --- → +
Flags: sec-bounty?
I still don't understand how the fix is right. ResetNodeDirection is called when setting the innerHTML because that removes the textNode child of test1, but WalkDescendantsSetDirectionFromText doesn't find a textNode. Shouldn't we at that point call RemoveElementFromMap on the textNode that's being removed?
Well, not call RemoveElementFromMap but

      rootNode->SetProperty(nsGkAtoms::dirAutoSetBy, aTextNode);
      rootNode->SetHasDirAutoSet();
(Assignee)

Comment 8

5 years ago
(In reply to Peter Van der Beken [:peterv] from comment #6)
> I still don't understand how the fix is right. ResetNodeDirection is called
> when setting the innerHTML because that removes the textNode child of test1,
> but WalkDescendantsSetDirectionFromText doesn't find a textNode. Shouldn't
> we at that point call RemoveElementFromMap on the textNode that's being
> removed?

But we already do, in effect, as a consequence of ResetNodeDirection returning PL_DHASH_REMOVE to EnumerateEntries
But that doesn't do the two lines from comment 7, does it?
(Assignee)

Comment 10

5 years ago
I'm not sure I understand comment 7. If WalkDescendantsSetDirectionFromText returns a text node, we do those two lines in AddEntry, called from AddEntryToMap. If it doesn't return a text node, why would we want to do them?
Woops, what I meant was

        rootNode->ClearHasDirAutoSet();
        rootNode->UnsetProperty(nsGkAtoms::dirAutoSetBy);

That is, I don't see where that happens if the text node that's stored in rootNode's dirAutoSetBy property is replaced with a new text node that doesn't have strong directional characters. Could you point it out?
(Assignee)

Comment 12

5 years ago
Created attachment 690402 [details] [diff] [review]
Patch v.2

Now I see what was missing, sorry to be slow.
Attachment #685810 - Attachment is obsolete: true
Attachment #685810 - Flags: review?(peterv)
Attachment #690402 - Flags: review?(peterv)
(Assignee)

Updated

5 years ago
Blocks: 819014
Keywords: csec-uaf, regression, sec-critical
Comment on attachment 690402 [details] [diff] [review]
Patch v.2

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

We should check in the testcase as a crashtest too.

::: content/base/src/DirectionalityUtils.cpp
@@ +450,5 @@
>        mElements.Remove(aElement);
>  
> +      if (mElements.IsEmpty()) {
> +        aTextNode->ClearHasTextNodeDirectionalityMap();
> +      }

I think I'd actually prefer to not do this (and the change in nsCheapSets) since, unless I'm missing something, it seems unneeded to fix this bug. Just because mElements is empty doesn't mean that there is no map anymore since you don't actually remove the property. I'd like the flag to reflect the real state.
Attachment #690402 - Flags: review?(peterv) → review+

Updated

5 years ago
Duplicate of this bug: 819014
https://hg.mozilla.org/mozilla-central/rev/0804d5dc4403
https://hg.mozilla.org/mozilla-central/rev/1392f08d1000
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-firefox20: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla20

Updated

5 years ago
Flags: sec-bounty? → sec-bounty+
status-firefox-esr10: --- → unaffected
status-b2g18: --- → unaffected
Whiteboard: [asan] → [asan][adv-main20+]
Whiteboard: [asan][adv-main20+] → [asan][adv-main20-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.