Last Comment Bug 914017 - (CVE-2013-5604) Stack-buffer-overflow in txXPathNodeUtils::getBaseURI
(CVE-2013-5604)
: Stack-buffer-overflow in txXPathNodeUtils::getBaseURI
Status: RESOLVED FIXED
[asan][adv-main25+][adv-esr1710+][adv...
: crash, csectype-uninitialized, sec-high, testcase
Product: Core
Classification: Components
Component: XSLT (show other bugs)
: Trunk
: x86_64 All
: -- critical (vote)
: mozilla27
Assigned To: Peter Van der Beken [:peterv]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-09-08 21:45 PDT by Abhishek Arya
Modified: 2015-02-25 20:16 PST (History)
9 users (show)
dveditz: sec‑bounty+
cbook: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified
verified
verified
verified
verified
fixed
fixed
fixed


Attachments
Testcase (996 bytes, text/html)
2013-09-08 21:45 PDT, Abhishek Arya
no flags Details
v1 (4.99 KB, patch)
2013-09-26 04:02 PDT, Peter Van der Beken [:peterv]
jonas: review+
Details | Diff | Review
v1 (3.58 KB, patch)
2013-10-16 05:22 PDT, Peter Van der Beken [:peterv]
peterv: review+
abillings: approval‑mozilla‑aurora+
abillings: approval‑mozilla‑beta+
abillings: approval‑mozilla‑esr17+
abillings: approval‑mozilla‑esr24+
abillings: sec‑approval+
Details | Diff | Review
Automated testcase v1 (597 bytes, patch)
2013-10-16 05:22 PDT, Peter Van der Beken [:peterv]
peterv: review+
Details | Diff | Review

Description Abhishek Arya 2013-09-08 21:45:43 PDT
Created attachment 801365 [details]
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 (PTO-ish through 6-29) [:mccr8] 2013-09-11 10:55:29 PDT
Could please you take a look at this, Peter?
Comment 2 Peter Van der Beken [:peterv] 2013-09-26 04:02:21 PDT
Created attachment 810505 [details] [diff] [review]
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 [On PTO until 6/29] 2013-09-30 15:34:09 PDT
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 (PTO-ish through 6-29) [:mccr8] 2013-10-14 09:47:03 PDT
Peter, is this ready to get sec-approval, land, etc.?  Thanks.
Comment 6 Peter Van der Beken [:peterv] 2013-10-15 08:02:16 PDT
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 [On PTO until 6/29] 2013-10-15 09:42:15 PDT
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] 2013-10-16 05:22:24 PDT
Created attachment 817781 [details] [diff] [review]
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] 2013-10-16 05:22:54 PDT
Created attachment 817782 [details] [diff] [review]
Automated testcase v1
Comment 10 [On PTO until 6/29] 2013-10-16 09:37:27 PDT
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] 2013-10-16 14:13:57 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/e282e8a9712a
Comment 12 Carsten Book [:Tomcat] 2013-10-17 05:10:57 PDT
fixed on central https://hg.mozilla.org/mozilla-central/rev/e282e8a9712a
Comment 15 Matt Wobensmith [:mwobensmith][:matt:] 2013-10-23 11:34:40 PDT
Confirmed bug FF27, 2013-09-25.
Verified fixed 17esr, 24esr, 25, 26, 27, 2013-10-21.

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