Last Comment Bug 915210 - (CVE-2013-5599) ASAN heap-use-after-free in nsIPresShell::GetPresContext() with canvas, onresize and mozTextStyle
: ASAN heap-use-after-free in nsIPresShell::GetPresContext() with canvas, onres...
: csectype-uaf, sec-critical
Product: Core
Classification: Components
Component: Canvas: 2D (show other bugs)
: Trunk
: x86_64 Linux
-- normal (vote)
: mozilla27
Assigned To: Olli Pettay [:smaug] (pto-ish for couple of days)
: Milan Sreckovic [:milan]
Depends on:
  Show dependency treegraph
Reported: 2013-09-11 08:16 PDT by Nils
Modified: 2015-02-25 20:16 PST (History)
14 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch (981 bytes, patch)
2013-09-11 08:47 PDT, Olli Pettay [:smaug] (pto-ish for couple of days)
bzbarsky: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
akeybl: approval‑mozilla‑esr17+
akeybl: approval‑mozilla‑esr24+
akeybl: approval‑mozilla‑b2g18+
Details | Diff | Splinter Review

Description User image Nils 2013-09-11 08:16:50 PDT
The following testcase crashes the firefox ASAN build:

function start() {
o14.src="data:text/html,<body id='e0'></body>";
window.setTimeout('startrly()', 100);
function startrly() {
function cb_frameresize_25_1() {
<body onload="start()">
<div id="s"></div>
Comment 1 User image Nils 2013-09-11 08:17:52 PDT
The asan output after the crash:

==44288==ERROR: AddressSanitizer: heap-use-after-free on address 0x6160001a7990 at pc 0x7f135250880d bp 0x7fff00d7e250 sp 0x7fff00d7e248
READ of size 8 at 0x6160001a7990 thread T0
    #0 0x7f135250880c in nsIPresShell::GetPresContext() const /layout/style/../base/nsIPresShell.h:286:0
    #1 0x7f13525083b5 in nsComputedDOMStyle::GetStyleContextForElementNoFlush(mozilla::dom::Element*, nsIAtom*, nsIPresShell*, nsComputedDOMStyle::StyleType) /layout/style/nsComputedDOMStyle.cpp:332:54
    #1 0x7f13525083b5 in nsComputedDOMStyle::GetStyleContextForElementNoFlush(mozilla::dom::Element*, nsIAtom*, nsIPresShell*, nsComputedDOMStyle::StyleType) /layout/style/nsComputedDOMStyle.cpp:332:54
    #3 0x7f1352b32822 in mozilla::dom::CanvasRenderingContext2D::SetFont(nsAString_internal const&, mozilla::ErrorResult&) /content/canvas/src/CanvasRenderingContext2D.cpp:2072:0
    #4 0x7f1352b36edb in mozilla::dom::CanvasRenderingContext2D::GetCurrentFontStyle() /content/canvas/src/CanvasRenderingContext2D.cpp:2724:0
    #5 0x7f1354ff7af6 in mozilla::dom::CanvasRenderingContext2D::GetFont() /obj-firefox/dom/bindings/../../dist/include/mozilla/dom/CanvasRenderingContext2D.h:569:0
    #6 0x7f1354ff3c66 in mozilla::dom::CanvasRenderingContext2DBinding::genericGetter(JSContext*, unsigned int, JS::Value*) /obj-firefox/dom/bindings/CanvasRenderingContext2DBinding.cpp:4197:0
    #7 0x7f13574b51fa in JSFunction::native() const /js/src/jscntxtinlines.h:219:0
    #8 0x7f13574b6218 in js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) /js/src/vm/Interpreter.cpp:539:0
    #9 0x7f13574b706c in js::InvokeGetterOrSetter(JSContext*, JSObject*, JS::Value, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) /js/src/vm/Interpreter.cpp:610:23
    #10 0x7f1357834a53 in js::Shape::get(JSContext*, JS::Handle<JSObject*>, JSObject*, JSObject*, JS::MutableHandle<JS::Value>) /js/src/vm/Shape-inl.h:256:0
    #11 0x7f1357824b63 in _ZL15NativeGetInlineILN2js7AllowGCE1EEbP9JSContextNS0_11MaybeRootedIP8JSObjectXT_EE10HandleTypeES8_S8_NS4_IPNS0_5ShapeEXT_EE10HandleTypeEjNS4_IN2JS5ValueEXT_EE17MutableHandleTypeE /js/src/jsobj.cpp:4070:21
    #12 0x7f13574c722d in GetPropertyOperation(JSContext*, js::StackFrame*, JS::Handle<JSScript*>, unsigned char*, JS::MutableHandle<JS::Value>, JS::MutableHandle<JS::Value>) /js/src/vm/Interpreter.cpp:292:0
    #13 0x7f13574a3122 in Interpret(JSContext*, js::RunState&) /js/src/vm/Interpreter.cpp:2298:0
    #14 0x7f13574969b1 in js::RunScript(JSContext*, js::RunState&) /js/src/vm/Interpreter.cpp:446:0
    #15 0x7f13574b74eb in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::ExecuteType, js::AbstractFramePtr, JS::Value*) /js/src/vm/Interpreter.cpp:630:0
    #16 0x7f13574b7846 in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) /js/src/vm/Interpreter.cpp:666:0
    #17 0x7f13576bcf9d in JS::Evaluate(JSContext*, JS::Handle<JSObject*>, JS::CompileOptions, unsigned short const*, unsigned long, JS::Value*) /js/src/jsapi.cpp:5120:0
    #18 0x7f135331870a in nsJSUtils::EvaluateString(JSContext*, nsAString_internal const&, JS::Handle<JSObject*>, JS::CompileOptions&, nsJSUtils::EvaluateOptions&, JS::Value*) /dom/base/nsJSUtils.cpp:268:0
    #19 0x7f1353306607 in nsJSContext::EvaluateString(nsAString_internal const&, JS::Handle<JSObject*>, JS::CompileOptions&, bool, JS::Value*) /dom/base/nsJSEnvironment.cpp:993:0
    #20 0x7f13532c487c in nsGlobalWindow::RunTimeoutHandler(nsTimeout*, nsIScriptContext*) /dom/base/nsGlobalWindow.cpp:10473:0
    #21 0x7f13532a8f72 in nsGlobalWindow::RunTimeout(nsTimeout*) /dom/base/nsGlobalWindow.cpp:10717:0
    #22 0x7f13532c3abc in nsGlobalWindow::TimerCallback(nsITimer*, void*) /dom/base/nsGlobalWindow.cpp:10964:0
    #23 0x7f1355c4bda0 in nsTimerImpl::Fire() /xpcom/threads/nsTimerImpl.cpp:546:0
    #24 0x7f1355c4c456 in nsTimerEvent::Run() /xpcom/threads/nsTimerImpl.cpp:630:0
    #25 0x7f1355c41a69 in nsThread::ProcessNextEvent(bool, bool*) /xpcom/threads/nsThread.cpp:622:0
    #26 0x7f1355b685b1 in NS_ProcessNextEvent(nsIThread*, bool) /obj-firefox/xpcom/build/nsThreadUtils.cpp:238:0
    #27 0x7f13549a29e0 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /ipc/glue/MessagePump.cpp:116:0
    #28 0x7f1355d66b53 in MessageLoop::RunInternal() /ipc/chromium/src/base/
    #29 0x7f135478915c in nsBaseAppShell::Run() /widget/xpwidgets/nsBaseAppShell.cpp:161:0
    #30 0x7f135416fc5e in nsAppStartup::Run() /toolkit/components/startup/nsAppStartup.cpp:269:0
    #31 0x7f135155ac00 in XREMain::XRE_mainRun() /toolkit/xre/nsAppRunner.cpp:3869:0
    #32 0x7f135155bb55 in XREMain::XRE_main(int, char**, nsXREAppData const*) /toolkit/xre/nsAppRunner.cpp:3937:0
    #33 0x7f135155ca8b in XRE_main /toolkit/xre/nsAppRunner.cpp:4139:0
    #34 0x459c8d in do_main(int, char**, nsIFile*) /browser/app/nsBrowserApp.cpp:275:0
    #35 0x7f1360aa9ea4 (/lib/x86_64-linux-gnu/
    #36 0x45910c in ?? ??:0:0
0x6160001a7990 is located 16 bytes inside of 600-byte region [0x6160001a7980,0x6160001a7bd8)
freed by thread T0 here:
    #0 0x446015 in __interceptor_free /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/
    #1 0x7f1352121db8 in PresShell::Release() /layout/base/nsPresShell.cpp:712:0
previously allocated by thread T0 here:
    #0 0x446155 in malloc /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/
    #1 0x7f135c0ae5c8 in moz_xmalloc /memory/mozalloc/mozalloc.cpp:54:0
    #2 0x7f13520aa8c3 in nsDocumentViewer::InitPresentationStuff(bool) /layout/base/nsDocumentViewer.cpp:661:0
    #3 0x7f13520aa1a8 in nsDocumentViewer::InitInternal(nsIWidget*, nsISupports*, nsIntRect const&, bool, bool, bool) /layout/base/nsDocumentViewer.cpp:907:0
    #4 0x7f13520a92f0 in nsDocumentViewer::Init(nsIWidget*, nsIntRect const&) /layout/base/nsDocumentViewer.cpp:643:0
    #5 0x7f1353f84917 in nsDocShell::Embed(nsIContentViewer*, char const*, nsISupports*) /docshell/base/nsDocShell.cpp:6357:0
    #6 0x7f1353f96e8d in nsDocShell::CreateContentViewer(char const*, nsIRequest*, nsIStreamListener**) /docshell/base/nsDocShell.cpp:8133:0
    #7 0x7f1353f3af5e in nsDSURIContentListener::DoContent(char const*, bool, nsIRequest*, nsIStreamListener**, bool*) /docshell/base/nsDSURIContentListener.cpp:122:0
    #8 0x7f1353fde272 in nsDocumentOpenInfo::TryContentListener(nsIURIContentListener*, nsIChannel*) /uriloader/base/nsURILoader.cpp:680:0
    #9 0x7f1353fdbc1c in nsDocumentOpenInfo::DispatchContent(nsIRequest*, nsISupports*) /uriloader/base/nsURILoader.cpp:382:0
    #10 0x7f1353fdb38f in nsDocumentOpenInfo::OnStartRequest(nsIRequest*, nsISupports*) /uriloader/base/nsURILoader.cpp:258:0
    #11 0x7f13518113b2 in nsBaseChannel::OnStartRequest(nsIRequest*, nsISupports*) /netwerk/base/src/nsBaseChannel.cpp:720:0
Comment 2 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2013-09-11 08:26:28 PDT
There are at least two bugs, nsComputedDOMStyle::GetStyleContextForElement uses raw presshell pointer after flush and so does CanvasRenderingContext2D::SetFont
Comment 3 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2013-09-11 08:47:46 PDT
Created attachment 803085 [details] [diff] [review]

Actually this should be enough. We return early in canvas code if
this code doesn't end up succeeding.
Comment 4 User image Boris Zbarsky [:bz] (still a bit busy) 2013-09-11 08:51:56 PDT
Comment on attachment 803085 [details] [diff] [review]

Comment 5 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2013-09-11 09:17:29 PDT
Comment on attachment 803085 [details] [diff] [review]

[Approval Request Comment]
Bug caused by (feature/regressing bug #): at least 4 years old regression
User impact if declined: crashes
Testing completed (on m-c, etc.): NA 
Risk to taking this patch (and alternatives if risky): Should be super-safe 
String or IDL/UUID changes made by this patch: NA
Comment 6 User image Daniel Veditz [:dveditz] 2013-09-11 11:49:08 PDT
Clearly the bug must not be that easy to stumble across if we haven't in 4 years of testing, including a year of fuzz testing on ASan builds, but the trivial patch is a big clue: adding a reference right before a flush. Not sure I'm comfortable exposing this 7 weeks before we can ship it, and I wouldn't count on getting a ride-along on a 24.0.1 because we really really don't want to have to do a 24.0.1
Comment 7 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2013-09-11 12:51:09 PDT
I think it should be ok to land this very late during the next cycle.
Comment 8 User image Jesse Ruderman 2013-09-11 14:15:39 PDT
It makes me uncomfortable that the line 'o131.mozTextStyle' causes the iframe's onresize event to fire, synchronously.  Is that a bug too?
Comment 9 User image Jesse Ruderman 2013-09-11 14:18:24 PDT
(and why is the scroll() call needed to make it happen synchronously?)
Comment 10 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2013-09-11 14:27:59 PDT
FlushPendingNotifications explicitly fires pending resize events, very much on purpose. Though
perhaps these days we could fire it at the end of microtask.
Comment 11 User image Lukas Blakk [:lsblakk] use ?needinfo 2013-09-18 12:36:34 PDT
How late in the cycle are we talking?  2nd to last week of beta 25?
Comment 12 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2013-09-19 13:21:41 PDT
That sounds good. The patch should be super safe.
Comment 13 User image Ryan VanderMeulen [:RyanVM] 2013-10-14 12:12:52 PDT

Will land on the release branches today after it's green on inbound.
Comment 14 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2013-10-14 13:31:05 PDT

Argh, sorry, I didn't see that message about m-i, and I landed to m-c.
I hope I didn't ruin merging.
Comment 17 User image Wes Kocher (:KWierso) 2013-10-14 18:56:03 PDT
Comment 18 User image Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2013-10-15 13:41:26 PDT
Matt, can you please verify this is fixed across all patched branches?
Comment 19 User image Matt Wobensmith [:mwobensmith][:matt:] 2013-10-15 15:41:43 PDT
Confirmed crash on pre-patch ASan build of 24esr.
Verified fix on ASan builds of 24esr, 25, 26 and 27, 2013-10-15.

I'm having build issues with 17esr at the moment, and we'll have to verify B2G later.
Comment 20 User image Matt Wobensmith [:mwobensmith][:matt:] 2013-10-23 12:27:57 PDT
Verified 17esr, 2013-10-23.

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