851781 - (CVE-2013-1681) Heap-use-after-free in nsContentUtils::RemoveScriptBlocker

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Abhishek Arya]

Attachment: Testcase

==18530== ERROR: AddressSanitizer: heap-use-after-free on address 0x601401063130 at pc 0x7fbd90d3aeb9 bp 0x7fffb2289fd0 sp 0x7fffb2289fc8
READ of size 8 at 0x601401063130 thread T0
    #0 0x7fbd90d3aeb8 in nsTArray_Impl<nsCOMPtr<nsIRunnable>, nsTArrayInfallibleAllocator>::RemoveElementsAt(unsigned int, unsigned int) ../../../dist/include/nsCOMPtr.h:409
    #1 0x7fbd90db4b42 in nsDocument::EndUpdate(unsigned int) content/base/src/nsDocument.cpp:4211
    #2 0x7fbd914a8ce8 in nsHTMLDocument::EndUpdate(unsigned int) content/html/document/src/nsHTMLDocument.cpp:2535
    #3 0x7fbd90d37177 in nsContentUtils::SetNodeTextContent(nsIContent*, nsAString_internal const&, bool) content/base/src/mozAutoDocUpdate.h:38
    #4 0x7fbd90f5ae6a in mozilla::dom::FragmentOrElement::SetTextContentInternal(nsAString_internal const&, mozilla::ErrorResult&) content/base/src/FragmentOrElement.cpp:938
    #5 0x7fbd933e1f91 in mozilla::dom::NodeBinding::set_textContent(JSContext*, JS::Handle<JSObject*>, nsINode*, JS::Value*) ../../dist/include/nsINode.h:1088
    #6 0x7fbd933e08e8 in mozilla::dom::NodeBinding::genericSetter(JSContext*, unsigned int, JS::Value*) objdir-ff-asan/dom/bindings/NodeBinding.cpp:1501
    #7 0x7fbd94b50e33 in js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) js/src/jscntxtinlines.h:327
    #8 0x7fbd94b51db0 in js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::Value*) js/src/jsinterp.h:135
    #9 0x7fbd94b528e1 in js::InvokeGetterOrSetter(JSContext*, JSObject*, JS::Value const&, unsigned int, JS::Value*, JS::Value*) js/src/jsinterp.cpp:503
    #10 0x7fbd94baac61 in js::Shape::set(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, bool, JS::MutableHandle<JS::Value>) js/src/vm/Shape-inl.h:309
    #11 0x7fbd94bb2ad1 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:4114
    #12 0x7fbd94b58923 in js::SetPropertyOperation(JSContext*, unsigned char*, JS::Handle<JS::Value>, JS::Handle<JS::Value>) js/src/jsobjinlines.h:89
    #13 0x7fbd94b3b04b in js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) js/src/jsinterp.cpp:2252
    #14 0x7fbd94b320d2 in js::RunScript(JSContext*, js::StackFrame*) js/src/jsinterp.cpp:340
    #15 0x7fbd94b52f3d in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::ExecuteType, js::AbstractFramePtr, JS::Value*) js/src/jsinterp.cpp:530
    #16 0x7fbd94b533a8 in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) js/src/jsinterp.cpp:569
    #17 0x7fbd94a0b5b3 in JS::Evaluate(JSContext*, JS::Handle<JSObject*>, JS::CompileOptions, unsigned short const*, unsigned long, JS::Value*) js/src/jsapi.cpp:5528
    #18 0x7fbd91692e68 in nsJSContext::EvaluateString(nsAString_internal const&, JSObject&, JS::CompileOptions&, bool, JS::Value*) dom/base/nsJSEnvironment.cpp:1290
    #19 0x7fbd9171b8cb in nsGlobalWindow::RunTimeoutHandler(nsTimeout*, nsIScriptContext*) dom/base/nsGlobalWindow.cpp:9957
    #20 0x7fbd91704024 in nsGlobalWindow::RunTimeout(nsTimeout*) dom/base/nsGlobalWindow.cpp:10209
    #21 0x7fbd9171ab78 in nsGlobalWindow::TimerCallback(nsITimer*, void*) dom/base/nsGlobalWindow.cpp:10478
    #22 0x7fbd937df6ac in nsTimerImpl::Fire() xpcom/threads/nsTimerImpl.cpp:496
    #23 0x7fbd937dfd20 in nsTimerEvent::Run() xpcom/threads/nsTimerImpl.cpp:587
    #24 0x7fbd937d56d3 in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:627
    #25 0x7fbd9371c610 in NS_ProcessNextEvent_P(nsIThread*, bool) objdir-ff-asan/xpcom/build/nsThreadUtils.cpp:238
    #26 0x7fbd92e6bc3c in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:82
    #27 0x7fbd9385ab49 in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:216
    #28 0x7fbd92b8b1cc in nsBaseAppShell::Run() widget/xpwidgets/nsBaseAppShell.cpp:163
    #29 0x7fbd9266dd1a in nsAppStartup::Run() toolkit/components/startup/nsAppStartup.cpp:288
    #30 0x7fbd8fdfb405 in XREMain::XRE_mainRun() toolkit/xre/nsAppRunner.cpp:3880
    #31 0x7fbd8fdfc236 in XREMain::XRE_main(int, char**, nsXREAppData const*) toolkit/xre/nsAppRunner.cpp:3947
    #32 0x7fbd8fdfd0d9 in XRE_main toolkit/xre/nsAppRunner.cpp:4150
    #33 0x425556 in main browser/app/nsBrowserApp.cpp:228
    #34 0x7fbd985fe76c in
    #35 0x424864 in
0x601401063130 is located 16 bytes inside of 32-byte region [0x601401063120,0x601401063140)
freed by thread T0 here:
    #0 0x4188f7 in __interceptor_realloc
    #1 0x7fbd9963e4ae in moz_xrealloc memory/mozalloc/mozalloc.cpp:86
previously allocated by thread T0 here:
    #0 0x4188f7 in __interceptor_realloc
    #1 0x7fbd9963e4ae in moz_xrealloc memory/mozalloc/mozalloc.cpp:86
Shadow bytes around the buggy address:
  0x0c03002045d0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c03002045e0: fa fa fa fa fa fa fa fa 00 00 00 fa fa fa fa fa
  0x0c03002045f0: fa fa fa fa fa fa fa fa fa fa fa fa fd fd fd fa
  0x0c0300204600: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c0300204610: 00 00 00 00 fa fa fa fa fa fa fa fa fa fa fa fa
=>0x0c0300204620: fa fa fa fa fd fd[fd]fd fa fa fa fa fa fa fa fa
  0x0c0300204630: fa fa fa fa fa fa fa fa 00 00 04 fa fa fa fa fa
  0x0c0300204640: fa fa fa fa fa fa fa fa fa fa fa fa fd fd fd fd
  0x0c0300204650: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c0300204660: fd fd fd fd fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c0300204670: fa fa fa fa fd fd fd fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:     fa
  Heap righ redzone:     fb
  Freed Heap region:     fd
  Stack left redzone:    f1
  Stack mid redzone:     f2
  Stack right redzone:   f3
  Stack partial redzone: f4
  Stack after return:    f5
  Stack use after scope: f8
  Global redzone:        f9
  Global init order:     f6
  Poisoned by user:      f7
  ASan internal:         fe
==18530== ABORTING

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Haven't managed to reproduce this on linux, non-asan build.

Comment 2

[Daniel Veditz [:dveditz]]

Matt: please check whether this bug happens on ESR-17 (in case we have to back-port a fix), Firefox 19 (in case we have to issue an advisory), and if not in 19 see if it's in 20 or 21.

Comment 3

[Matt Wobensmith [:mwobensmith][:matt:]]

I'm not able to get a crash on any build I've tried - 17.0.4 ESR, 19, 20, 21 or 22.

These are all ASan builds on Mac from the past week, with the exception of 19, which was a release build.

Abhishek, can you see if this still reproduces, or if there's a missing step here? I see mention of a dependent file ("BLOCKQUOTE.html") in the source of your test case. Is that important? 

Thanks.

Comment 4

[Abhishek Arya]

Attachment: Prefs.js and domfuzz dir

Clobbered my build dir, synced to trunk, testcase still reproduces. A little unreliable, but easier to reproduce with multiple instances. Here is the python script i use [also find enclosed as attachment the prefs.js and domfuzz dirs]. Btw, i am using clang r176408 for asan

import os
import time

thread_num = 15

for i in range(0, thread_num):
  profile_dir = '/tmp/firefox_profile_%d' % i
  os.system('mkdir %s' % profile_dir)
  os.system('cp /mnt/moz/prefs.js %s/' % profile_dir)
  os.system('mkdir %s/extensions' % profile_dir)
  os.system('cp -R /mnt/moz/[e-mail] %s/extensions' % profile_dir)
  extension_config_file_contents = "[ExtensionDirs]\r\nExtension0=%s/extensions/[e-mail]\r\n\r\n[ThemeDirs]\r\n" % profile_dir
  extension_config_file = os.path.join(profile_dir, "extensions.ini")
  f = open(extension_config_file, "w")
  f.write(extension_config_file_contents)
  f.close()
  cmd = 'ASAN_OPTIONS=redzone=64:alloc_dealloc_mismatch=0:strict_memcmp=0 /path-to-firefox/objdir-ff-asan/dist/bin/firefox-bin -no-remote -private -silent -setDefaultBrowser -profile "%s" "/mnt/moz/test.html" 2>&1 | tee /tmp/%d.log &' % (profile_dir, i)
  os.system(cmd)
  time.sleep(0.3)

time.sleep(120)

Comment 5

[David Bolter [:davidb] (NeedInfo me for attention)]

Can someone from DOM try reproducing again?

Comment 6

[Andrew McCreight [:mccr8]]

Boris, based on the stacks, who do you think might be good to look at this?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Me or Olli or Jonas, if Jonas ever has time, I guess.

So the stack in comment 0 claims that the relevant array manipulation is under nsContentUtils::RemoveScriptBlocker.

This tries to be pretty careful about handling reentry from under Run() and the like, but I wonder whether there's some runnable involved whose destructor ends up triggering scriptblocker operations or something.

Comment 8

[Boris Zbarsky [:bzbarsky]]

So I tried asserting in RemoveScriptBlocker that it's not called while we're under that RemoveElementsAt call, and that assert totally triggers on this testcase:

#0  nsContentUtils::RemoveScriptBlocker () at nsContentUtils.cpp:5044
#1  0x00000001031db969 in nsAutoScriptBlocker::~nsAutoScriptBlocker (this=0x7fff5fbfa938) at nsContentUtils.h:2290
#2  0x00000001031d7025 in nsAutoScriptBlocker::~nsAutoScriptBlocker (this=0x7fff5fbfa938) at nsContentUtils.h:2289
#3  0x0000000103673920 in nsAttrAndChildArray::Clear (this=0x11523bb08) at nsAttrAndChildArray.cpp:668
#4  0x000000010367372f in nsAttrAndChildArray::~nsAttrAndChildArray (this=0x11523bb08) at nsAttrAndChildArray.cpp:100
#5  0x00000001036736f5 in nsAttrAndChildArray::~nsAttrAndChildArray (this=0x11523bb08) at nsAttrAndChildArray.cpp:95
#6  0x0000000103865e46 in mozilla::dom::FragmentOrElement::~FragmentOrElement (this=0x11523baa0) at FragmentOrElement.cpp:634
#7  0x0000000103799925 in mozilla::dom::Element::~Element (this=0x11523baa0) at Element.h:127
#8  0x00000001037cf8b5 in nsStyledElementNotElementCSSInlineStyle::~nsStyledElementNotElementCSSInlineStyle (this=0x11523baa0) at nsStyledElement.h:27
#9  0x00000001037cf895 in nsStyledElement::~nsStyledElement (this=0x11523baa0) at nsStyledElement.h:82
#10 0x00000001044c1b65 in nsXULElement::~nsXULElement (this=0x11523baa0) at nsXULElement.h:336
#11 0x00000001044bdb05 in nsXULElement::~nsXULElement (this=0x11523baa0) at nsXULElement.h:336
#12 0x00000001044bdb29 in nsXULElement::~nsXULElement (this=0x11523baa0) at nsXULElement.h:336
#13 0x00000001037dd227 in nsNodeUtils::LastRelease (aNode=0x11523baa0) at nsNodeUtils.cpp:259
#14 0x000000010386ada3 in mozilla::dom::FragmentOrElement::Release (this=0x11523baa0) at FragmentOrElement.cpp:1716
#15 0x00000001044b1a3f in nsXULElement::Release (this=0x11523baa0) at nsXULElement.cpp:342
#16 0x00000001031895eb in nsCOMPtr<nsIContent>::~nsCOMPtr (this=0x10e7be298) at nsCOMPtr.h:494
#17 0x00000001031894f5 in nsCOMPtr<nsIContent>::~nsCOMPtr (this=0x10e7be298) at nsCOMPtr.h:491
#18 0x00000001036cc339 in AnonymousContentDestroyer::~AnonymousContentDestroyer (this=0x10e7be280) at nsContentUtils.cpp:4568
#19 0x00000001036cc2a5 in AnonymousContentDestroyer::~AnonymousContentDestroyer (this=0x10e7be280) at nsContentUtils.cpp:4568
#20 0x00000001036cc2c9 in AnonymousContentDestroyer::~AnonymousContentDestroyer (this=0x10e7be280) at nsContentUtils.cpp:4568
#21 0x000000010593609f in nsRunnable::Release (this=0x10e7be280) at nsThreadUtils.cpp:31
#22 0x0000000102bb2afb in nsCOMPtr<nsIRunnable>::~nsCOMPtr (this=0x11045a608) at nsCOMPtr.h:494
#23 0x0000000102bb2425 in nsCOMPtr<nsIRunnable>::~nsCOMPtr (this=0x11045a608) at nsCOMPtr.h:491
#24 0x0000000102fd93b5 in nsTArrayElementTraits<nsCOMPtr<nsIRunnable> >::Destruct (e=0x11045a608) at nsTArray.h:439
#25 0x0000000102fd9382 in nsTArray_Impl<nsCOMPtr<nsIRunnable>, nsTArrayInfallibleAllocator>::DestructRange (this=0x10050ee00, start=0, count=3) at nsTArray.h:1304
#26 0x0000000102fd9301 in nsTArray_Impl<nsCOMPtr<nsIRunnable>, nsTArrayInfallibleAllocator>::RemoveElementsAt (this=0x10050ee00, start=0, count=3) at nsTArray.h:1023
#27 0x00000001036bade1 in nsContentUtils::RemoveScriptBlocker () at nsContentUtils.cpp:5069

So now we're manipulating the sBlockedScriptRunners buffer while we're in the middle of operating on it, and in particular could cause it to realloc while we're under that RemoveElementsAt call.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Abhishek, thanks a ton for the testcase and stack!

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Make sure that we don't reenter scriptblocker removal when we don't expect to.

Comment 11

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cd4322eb7af4

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 730872 [details] [diff] [review]
Make sure that we don't reenter scriptblocker removal when we don't expect to.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Been around for a while
User impact if declined: Possible to cause us to try to call virtual methods on
   deallocated memory, which seems bad.
Testing completed (on m-c, etc.): Seems to be green on inbound, passes manual
   testing.
Risk to taking this patch (and alternatives if risky): Fairly low risk, I believe.
String or IDL/UUID changes made by this patch: None

I realize it's most likely too late for 20.  Just let me know!

Comment 13

[Andrew McCreight [:mccr8]]

As this affects more than trunk, it should have had sec-approval+ before landing.  (It doesn't matter now, though.)

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cd4322eb7af4

Comment 15

[Boris Zbarsky [:bzbarsky]]

I considered that, but if I'm going to land it for 20 we needed it in ASAP, and we're right near the end of a cycle, which is when we normally try to land security fixes that we've held for sec-approval...

Given that, I wasn't sure the typical lag for a sec-approval request to be processed would help anything.  :(

Comment 16

[bhavana bajaj [:bajaj]]

(In reply to Boris Zbarsky (:bz) from comment #12)
> Comment on attachment 730872 [details] [diff] [review]
> Make sure that we don't reenter scriptblocker removal when we don't expect
> to.
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): Been around for a while
> User impact if declined: Possible to cause us to try to call virtual methods
> on
>    deallocated memory, which seems bad.
> Testing completed (on m-c, etc.): Seems to be green on inbound, passes manual
>    testing.
> Risk to taking this patch (and alternatives if risky): Fairly low risk, I
> believe.
> String or IDL/UUID changes made by this patch: None
> 
> I realize it's most likely too late for 20.  Just let me know!

We have wrapped up all our Beta's for Fx 20 and ready for release on Tuesday so Its too late to get this in 20 now :( How long does this this regression go back to ?

Comment 17

[bhavana bajaj [:bajaj]]

Comment on attachment 730872 [details] [diff] [review]
Make sure that we don't reenter scriptblocker removal when we don't expect to.

If this lands tomorrow before the merge(~10 am  PT) this approval should be fine else please renominate with beta approval if needed.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/73965923ad12

Comment 19

[Boris Zbarsky [:bzbarsky]]

> How long does this this regression go back to ?

It's not clear to me that it's a "regression" in any meaningful sense; the code has had this problem for literally years...

Comment 20

[Al Billings [:abillings - ex-MoCo]]

Is there a reason that this wasn't flagged sec-approval? before going in?

Comment 22

[Andrew McCreight [:mccr8]]

(In reply to Al Billings [:abillings] from comment #20)
> Is there a reason that this wasn't flagged sec-approval? before going in?
See comment 15.  But unfortunately it was way too late for getting in 20.

Comment 23

[Al Billings [:abillings - ex-MoCo]]

Yeah, it was without release management approval, which would be unlikely at this point since we'd have to rebuild, etc.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/72ec41538d37
https://hg.mozilla.org/releases/mozilla-esr17/rev/662e9ecb9267

Is this status-firefox20:wontfix or would we consider this as a 20.0.1 ride-along?