914017 - (CVE-2013-5604) Stack-buffer-overflow in txXPathNodeUtils::getBaseURI

Status: RESOLVED FIXED

(Core :: XSLT, defect)

Description

[Abhishek Arya]

Attachment: Testcase

==12693==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7fff3757e180 at pc 0x7ffc253f35bf bp 0x7fff3757dbe0 sp 0x7fff3757dbd8
READ of size 8 at 0x7fff3757e180 thread T0
    #0 0x7ffc253f35be in txXPathNodeUtils::getBaseURI(txXPathNode const&, nsAString_internal&) content/xslt/src/xpath/txMozillaXPathTreeWalker.cpp:569
    #1 0x7ffc25419cc0 in txLoadedDocumentsHash::~txLoadedDocumentsHash() content/xslt/src/xslt/txExecutionState.cpp:39
    #2 0x7ffc2541aa77 in txExecutionState::~txExecutionState() content/xslt/src/xslt/txExecutionState.cpp:94
    #3 0x7ffc2545273c in txMozillaXSLTProcessor::TransformToDoc(nsIDOMDocument**, bool) content/xslt/src/xslt/txMozillaXSLTProcessor.cpp:679
    #4 0x7ffc2804cfc1 in NS_InvokeByIndex xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:162
    #5 0x7ffc26242ece in Invoke js/xpconnect/src/XPCWrappedNative.cpp:2809
    #6 0x7ffc26242ece in CallMethodHelper js/xpconnect/src/XPCWrappedNative.cpp:2149
    #7 0x7ffc26242ece in XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) js/xpconnect/src/XPCWrappedNative.cpp:2115
    #8 0x7ffc2625506b in XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1310
    #9 0x7ffc2984ee0a in native js/src/jscntxtinlines.h:219
    #10 0x7ffc2984ee0a in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) js/src/vm/Interpreter.cpp:489
    #11 0x7ffc29842069 in Interpret(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:2459
    #12 0x7ffc298309f1 in js::RunScript(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:446
    #13 0x7ffc2985108b in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::ExecuteType, js::AbstractFramePtr, JS::Value*) js/src/vm/Interpreter.cpp:630
    #14 0x7ffc298513e6 in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) js/src/vm/Interpreter.cpp:666
    #15 0x7ffc29a54c6d in JS::Evaluate(JSContext*, JS::Handle<JSObject*>, JS::CompileOptions, unsigned short const*, unsigned long, JS::Value*) js/src/jsapi.cpp:5120
    #16 0x7ffc256b72ef in nsJSUtils::EvaluateString(JSContext*, nsAString_internal const&, JS::Handle<JSObject*>, JS::CompileOptions&, nsJSUtils::EvaluateOptions&, JS::Value*) dom/base/nsJSUtils.cpp:268
    #17 0x7ffc256a5357 in nsJSContext::EvaluateString(nsAString_internal const&, JS::Handle<JSObject*>, JS::CompileOptions&, bool, JS::Value*) dom/base/nsJSEnvironment.cpp:993
    #18 0x7ffc25662dfc in nsGlobalWindow::RunTimeoutHandler(nsTimeout*, nsIScriptContext*) dom/base/nsGlobalWindow.cpp:10473
    #19 0x7ffc256476d2 in nsGlobalWindow::RunTimeout(nsTimeout*) dom/base/nsGlobalWindow.cpp:10717
    #20 0x7ffc2566203c in nsGlobalWindow::TimerCallback(nsITimer*, void*) dom/base/nsGlobalWindow.cpp:10964
    #21 0x7ffc2800f9e1 in nsTimerImpl::Fire() xpcom/threads/nsTimerImpl.cpp:546
    #22 0x7ffc28010086 in nsTimerEvent::Run() xpcom/threads/nsTimerImpl.cpp:630
    #23 0x7ffc28005b39 in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:622
    #24 0x7ffc27f2d571 in NS_ProcessNextEvent(nsIThread*, bool) objdir-ff-asan/xpcom/build/nsThreadUtils.cpp:238
    #25 0x7ffc26d38880 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:116
    #26 0x7ffc28125253 in RunInternal ipc/chromium/src/base/message_loop.cc:220
    #27 0x7ffc28125253 in RunHandler ipc/chromium/src/base/message_loop.cc:213
    #28 0x7ffc28125253 in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:187
    #29 0x7ffc26b2201c in nsBaseAppShell::Run() widget/xpwidgets/nsBaseAppShell.cpp:161
    #30 0x7ffc265101ce in nsAppStartup::Run() toolkit/components/startup/nsAppStartup.cpp:269
    #31 0x7ffc2392f300 in XREMain::XRE_mainRun() toolkit/xre/nsAppRunner.cpp:3869
    #32 0x7ffc2393023a in XREMain::XRE_main(int, char**, nsXREAppData const*) toolkit/xre/nsAppRunner.cpp:3937
    #33 0x7ffc2393116b in XRE_main toolkit/xre/nsAppRunner.cpp:4139
    #34 0x45524d in do_main browser/app/nsBrowserApp.cpp:275
    #35 0x45524d in main browser/app/nsBrowserApp.cpp:635
    #36 0x7ffc32a9676c in
    #37 0x45483c in _start
Address 0x7fff3757e180 is located in stack of thread T0 at offset 896 in frame
    #0 0x7ffc2545214f in txMozillaXSLTProcessor::TransformToDoc(nsIDOMDocument**, bool) content/xslt/src/xslt/txMozillaXSLTProcessor.cpp:629
  This frame has 4 object(s):
    [32, 40) 'sourceDOMDocument'
    [96, 784) 'es'
    [832, 872) 'handlerFactory'
    [928, 936) 'doc'
HINT: this may be a false positive if your program uses some custom stack unwind mechanism or swapcontext
      (longjmp and C++ exceptions *are* supported)
Shadow bytes around the buggy address:
  0x100066ea7be0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100066ea7bf0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100066ea7c00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100066ea7c10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100066ea7c20: 00 00 f4 f4 f2 f2 f2 f2 00 00 00 00 00 f4 f4 f4
=>0x100066ea7c30:[f2]f2 f2 f2 00 f4 f4 f4 f3 f3 f3 f3 00 00 00 00
  0x100066ea7c40: 00 00 00 00 00 00 00 00 00 00 00 00 f1 f1 f1 f1
  0x100066ea7c50: 00 00 00 00 00 00 f4 f4 f2 f2 f2 f2 00 00 00 00
  0x100066ea7c60: 00 00 00 00 f3 f3 f3 f3 00 00 00 00 00 00 00 00
  0x100066ea7c70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100066ea7c80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
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 right 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
==12693==ABORTING

Comment 1

[Andrew McCreight [:mccr8]]

Could please you take a look at this, Peter?

Comment 2

[Peter Van der Beken [:peterv]]

Attachment: v1

If txExecutionState::init failed before it called txLoadedDocumentsHash::init we left the mSourceDocument uninitialized, and then tried to use it from the destructor. This patch also makes txLoadedDocumentsHash::init infallible because nsTHashtable is too.

Comment 4

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

Please get sec-approval before checking in, assuming this isn't trunk only. How far back does this issue go? Does it affect ESR17?

Comment 5

[Andrew McCreight [:mccr8]]

Peter, is this ready to get sec-approval, land, etc.?  Thanks.

Comment 6

[Peter Van der Beken [:peterv]]

Comment on attachment 810505 [details] [diff] [review]
v1

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Not very easily I think, if we don't check in the testcase.

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

The testcase does, so we should probably hold off on checking it in?

Which older supported branches are affected by this flaw?

All AFAIK.

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?

Not yet, but they won't be hard. Some parts of the patch rely on the fact that certain operations are infallible which they aren't on the branches, so they'd need some additional error checking.

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

Unlikely to cause regressions, it just makes sure we don't access an unitialized pointer from a destructor.

Comment 7

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

Please prepare branch patches (including for ESR17 and 24 unless that isn't feasible) and nominate them.

Also, we need the tests to be a separate patch that can be checked in later. Once these are done, I'll approve. We're close enough to the line that I don't want to approve this for trunk and then get it punted for Beta (aka 25) because of lack of time until release.

Comment 8

[Peter Van der Beken [:peterv]]

Attachment: v1

[Security approval request comment]
See comment 6.

Turns out we don't need separate patches for branches, all of them have the same infallible hashtables.

Comment 9

[Peter Van der Beken [:peterv]]

Attachment: Automated testcase v1

Comment 10

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

Comment on attachment 817781 [details] [diff] [review]
v1

sec-approval+ for trunk. I've approved this for both ESR branches, Aurora, and Beta but it needs to go in as soon as possible since we're running out of beta builds.

Comment 11

[Peter Van der Beken [:peterv]]

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

Comment 12

[Carsten Book [:Tomcat]]

fixed on central https://hg.mozilla.org/mozilla-central/rev/e282e8a9712a

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cdee8d489101
https://hg.mozilla.org/releases/mozilla-beta/rev/e449862b8ddd
https://hg.mozilla.org/releases/mozilla-esr24/rev/108b5be7dd02
https://hg.mozilla.org/releases/mozilla-b2g18/rev/bb1ba0f59b31
https://hg.mozilla.org/releases/mozilla-esr17/rev/1ee27c334366

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/05db0f8744fa

Comment 15

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

Confirmed bug FF27, 2013-09-25.
Verified fixed 17esr, 24esr, 25, 26, 27, 2013-10-21.