Last Comment Bug 851781 - (CVE-2013-1681) Heap-use-after-free in nsContentUtils::RemoveScriptBlocker
(CVE-2013-1681)
: Heap-use-after-free in nsContentUtils::RemoveScriptBlocker
Status: RESOLVED FIXED
[asan][adv-main21+][adv-esr1706+]
: csectype-uaf, sec-critical
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal (vote)
: mozilla22
Assigned To: Boris Zbarsky [:bz] (Out June 25-July 6)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-03-16 00:10 PDT by Abhishek Arya
Modified: 2014-07-24 16:07 PDT (History)
9 users (show)
abillings: sec‑bounty+
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
+
fixed
+
fixed
21+
fixed
21+
fixed
wontfix
affected


Attachments
Testcase (3.99 KB, text/html)
2013-03-16 00:10 PDT, Abhishek Arya
no flags Details
Prefs.js and domfuzz dir (11.89 KB, application/x-zip-compressed)
2013-03-25 19:10 PDT, Abhishek Arya
no flags Details
Make sure that we don't reenter scriptblocker removal when we don't expect to. (2.23 KB, patch)
2013-03-28 14:04 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
bugs: review+
bajaj.bhavana: approval‑mozilla‑aurora+
bajaj.bhavana: approval‑mozilla‑beta-
lukasblakk+bugs: approval‑mozilla‑esr17+
lukasblakk+bugs: approval‑mozilla‑b2g18+
Details | Diff | Review

Description Abhishek Arya 2013-03-16 00:10:57 PDT
Created attachment 725727 [details]
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] (high review load, please consider other reviewers) 2013-03-18 03:22:03 PDT
Haven't managed to reproduce this on linux, non-asan build.
Comment 2 Daniel Veditz [:dveditz] 2013-03-21 13:26:17 PDT
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:] 2013-03-25 15:45:42 PDT
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 2013-03-25 19:10:24 PDT
Created attachment 729364 [details]
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/domfuzz@squarefree.com %s/extensions' % profile_dir)
  extension_config_file_contents = "[ExtensionDirs]\r\nExtension0=%s/extensions/domfuzz@squarefree.com\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] 2013-03-28 13:10:18 PDT
Can someone from DOM try reproducing again?
Comment 6 Andrew McCreight [:mccr8] 2013-03-28 13:10:55 PDT
Boris, based on the stacks, who do you think might be good to look at this?
Comment 7 Boris Zbarsky [:bz] (Out June 25-July 6) 2013-03-28 13:50:40 PDT
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 [:bz] (Out June 25-July 6) 2013-03-28 13:52:25 PDT
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 [:bz] (Out June 25-July 6) 2013-03-28 13:55:35 PDT
Abhishek, thanks a ton for the testcase and stack!
Comment 10 Boris Zbarsky [:bz] (Out June 25-July 6) 2013-03-28 14:04:40 PDT
Created attachment 730872 [details] [diff] [review]
Make sure that we don't reenter scriptblocker removal when we don't expect to.
Comment 11 Boris Zbarsky [:bz] (Out June 25-July 6) 2013-03-29 21:15:03 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd4322eb7af4
Comment 12 Boris Zbarsky [:bz] (Out June 25-July 6) 2013-03-29 22:23:25 PDT
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] 2013-03-30 06:12:01 PDT
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] 2013-03-30 18:12:46 PDT
https://hg.mozilla.org/mozilla-central/rev/cd4322eb7af4
Comment 15 Boris Zbarsky [:bz] (Out June 25-July 6) 2013-03-30 21:00:09 PDT
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] 2013-03-31 15:04:13 PDT
(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] 2013-03-31 15:06:22 PDT
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] 2013-04-01 07:11:46 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/73965923ad12
Comment 19 Boris Zbarsky [:bz] (Out June 25-July 6) 2013-04-01 07:20:18 PDT
> 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] 2013-04-01 15:11:23 PDT
Is there a reason that this wasn't flagged sec-approval? before going in?
Comment 22 Andrew McCreight [:mccr8] 2013-04-01 15:20:44 PDT
(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] 2013-04-01 16:24:45 PDT
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] 2013-04-09 06:50:39 PDT
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?

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