906301 - (CVE-2013-1736) Memory corruption in nsGfxScrollFrameInner::IsLTR()

Status: RESOLVED FIXED

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

Description

[Nils]

Attachment: crash.zip

The attached testcase crashes Firefox stable and nightly. The testcase is attached in a zip file as it requires multiple files to trigger.

The debugger output on windows:

eax=0101020d ebx=00000000 ecx=00020000 edx=0000000f esi=0018e260 edi=0018e1d4
eip=645f875d esp=0018e148 ebp=0a154aa0 iopl=0         nv up ei pl nz na po nc
cs=001b  ss=0023  ds=0023  es=0023  fs=003b  gs=0000             efl=00210202
xul!nsGfxScrollFrameInner::IsLTR+0x1d:
645f875d 803801          cmp     byte ptr [eax],1           ds:0023:0101020d=??

xul!nsGfxScrollFrameInner::IsLTR+0x1d
xul!nsHTMLScrollFrame::ReflowContents+0x145
xul!nsHTMLScrollFrame::Reflow+0x210
xul!nsContainerFrame::ReflowChild+0x5a
xul!ViewportFrame::Reflow+0x1e8
xul!PresShell::DoReflow+0x418
xul!PresShell::ProcessReflowCommands+0x166
xul!PresShell::FlushPendingNotifications+0x3ef
xul!nsRefreshDriver::Tick+0x29f
xul!mozilla::RefreshDriverTimer::Tick+0x102
xul!nsTimerImpl::Fire+0xe7
xul!nsThread::ProcessNextEvent+0x362
xul!NS_ProcessNextEvent+0x2e
xul!mozilla::ipc::MessagePump::Run+0x46
xul!MessageLoop::RunHandler+0x51
xul!MessageLoop::Run+0x19
xul!nsBaseAppShell::Run+0x2c
xul!nsAppShell::Run+0x14
xul!XREMain::XRE_mainRun+0x3a9
xul!XREMain::XRE_main+0xdc


ASAN on Linux doesn't give us any further information:

ASAN:SIGSEGV
=================================================================
==7971==ERROR: AddressSanitizer: SEGV on unknown address 0x0000dfff8000 (pc 0x7f58fb6e7943 sp 0x7fff44347e00 bp 0x7fff44347eb0 T0)
AddressSanitizer can not provide additional info.
    #0 0x7f58fb6e7942 in nsGfxScrollFrameInner::IsLTR() const /builds/slave/m-cen-l64-asan-000000000000000/build/layout/generic/nsGfxScrollFrame.cpp:3302:0
    #1 0x7f58fb6ca4c3 in nsGfxScrollFrameInner::GetScrolledRectInternal(nsRect const&, nsSize const&) const /builds/slave/m-cen-l64-asan-000000000000000/build/layout/generic/nsGfxScrollFrame.cpp:3983:0
    #2 0x7f58fb6ca4c3 in nsHTMLScrollFrame::ReflowContents(ScrollReflowState*, nsHTMLReflowMetrics const&) /builds/slave/m-cen-l64-asan-000000000000000/build/layout/generic/nsGfxScrollFrame.cpp:574:0
    #3 0x7f58fb6cc40a in nsHTMLScrollFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) /builds/slave/m-cen-l64-asan-000000000000000/build/layout/generic/nsGfxScrollFrame.cpp:783:0
    #4 0x7f58fb64dad3 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, int, int, unsigned int, unsigned int&, nsOverflowContinuationTracker*) /builds/slave/m-cen-l64-asan-000000000000000/build/layout/generic/nsContainerFrame.cpp:970:0
    #5 0x7f58fb7f8177 in ViewportFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) /builds/slave/m-cen-l64-asan-000000000000000/build/layout/generic/nsViewportFrame.cpp:225:0
    #6 0x7f58fb53d539 in PresShell::DoReflow(nsIFrame*, bool) /builds/slave/m-cen-l64-asan-000000000000000/build/layout/base/nsPresShell.cpp:7850:0
    #7 0x7f58fb54eda8 in PresShell::ProcessReflowCommands(bool) /builds/slave/m-cen-l64-asan-000000000000000/build/layout/base/nsPresShell.cpp:7991:0
    #8 0x7f58fb54e66f in PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) /builds/slave/m-cen-l64-asan-000000000000000/build/layout/base/nsPresShell.cpp:3890:0
    #9 0x7f58fb582704 in nsRefreshDriver::Tick(long, mozilla::TimeStamp) /builds/slave/m-cen-l64-asan-000000000000000/build/layout/base/nsRefreshDriver.cpp:1191:0
    #10 0x7f58fb5889f0 in mozilla::RefreshDriverTimer::TickDriver(nsRefreshDriver*, long, mozilla::TimeStamp) /builds/slave/m-cen-l64-asan-000000000000000/build/layout/base/nsRefreshDriver.cpp:171:0
    #11 0x7f58fb5889f0 in mozilla::RefreshDriverTimer::Tick() /builds/slave/m-cen-l64-asan-000000000000000/build/layout/base/nsRefreshDriver.cpp:163:0
    #12 0x7f58ff1ace1a in nsTimerImpl::Fire() /builds/slave/m-cen-l64-asan-000000000000000/build/xpcom/threads/nsTimerImpl.cpp:544:0
    #13 0x7f58ff1ad4a6 in nsTimerEvent::Run() /builds/slave/m-cen-l64-asan-000000000000000/build/xpcom/threads/nsTimerImpl.cpp:628:0
    #14 0x7f58ff1a2829 in nsThread::ProcessNextEvent(bool, bool*) /builds/slave/m-cen-l64-asan-000000000000000/build/xpcom/threads/nsThread.cpp:622:0
    #15 0x7f58ff0cc8c6 in NS_ProcessNextEvent(nsIThread*, bool) /builds/slave/m-cen-l64-asan-000000000000000/build/obj-firefox/xpcom/build/nsThreadUtils.cpp:238:0
    #16 0x7f58fdf5fc51 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/slave/m-cen-l64-asan-000000000000000/build/ipc/glue/MessagePump.cpp:81:0
    #17 0x7f58ff2ca6c3 in MessageLoop::RunInternal() /builds/slave/m-cen-l64-asan-000000000000000/build/ipc/chromium/src/base/message_loop.cc:220:0
    #18 0x7f58ff2ca6c3 in MessageLoop::RunHandler() /builds/slave/m-cen-l64-asan-000000000000000/build/ipc/chromium/src/base/message_loop.cc:213:0
    #19 0x7f58ff2ca6c3 in MessageLoop::Run() /builds/slave/m-cen-l64-asan-000000000000000/build/ipc/chromium/src/base/message_loop.cc:187:0
    #20 0x7f58fdd4e6cc in nsBaseAppShell::Run() /builds/slave/m-cen-l64-asan-000000000000000/build/widget/xpwidgets/nsBaseAppShell.cpp:163:0
    #21 0x7f58fd74663e in nsAppStartup::Run() /builds/slave/m-cen-l64-asan-000000000000000/build/toolkit/components/startup/nsAppStartup.cpp:269:0
    #22 0x7f58fa95c0f0 in XREMain::XRE_mainRun() /builds/slave/m-cen-l64-asan-000000000000000/build/toolkit/xre/nsAppRunner.cpp:3858:0
    #23 0x7f58fa95d055 in XREMain::XRE_main(int, char**, nsXREAppData const*) /builds/slave/m-cen-l64-asan-000000000000000/build/toolkit/xre/nsAppRunner.cpp:3926:0
    #24 0x7f58fa95df8b in XRE_main /builds/slave/m-cen-l64-asan-000000000000000/build/toolkit/xre/nsAppRunner.cpp:4128:0
    #25 0x459c8d in do_main(int, char**, nsIFile*) /builds/slave/m-cen-l64-asan-000000000000000/build/browser/app/nsBrowserApp.cpp:275:0
    #26 0x459c8d in main /builds/slave/m-cen-l64-asan-000000000000000/build/browser/app/nsBrowserApp.cpp:635:0
    #27 0x7f5909567ea4 in ?? ??:0
    #28 0x45910c in _start ??:?
==7971==ABORTING

Comment 1

[Mats Palmgren (inactive)]

In a Linux64 debug build I get these before crashing (SEGV):

###!!! ASSERTION: Inserting node that already has parent: '!aKid->GetParentNode()', file nsINode.cpp, line 1351
###!!! ASSERTION: Already have a document.  Unbind first!: '!GetCurrentDoc()', file Element.cpp, line 922
###!!! ASSERTION: Shouldn't be here!: '!(IsNodeOfType(eCONTENT) && IsInDoc())', file nsINode.h, line 1464
###!!! ASSERTION: Bound to wrong document: 'aDocument == GetCurrentDoc()', file Element.cpp, line 1090

The stack for the first one is:

nsINode::doInsertChildAt
nsINode::ReplaceOrInsertBefore
PrependChild
nsRange::CutContents
nsRange::ExtractContents
mozilla::dom::RangeBinding::extractContents
mozilla::dom::RangeBinding::genericMethod
js::CallJSNative
js::Invoke
js::Invoke
js::DirectProxyHandler::call
js::CrossCompartmentWrapper::call
js::Proxy::call
proxy_Call
js::CallJSNative
js::Invoke
Interpret
js::RunScript
js::ExecuteKernel
js::Execute
JS::Evaluate
nsJSUtils::EvaluateString
nsJSContext::EvaluateString
nsGlobalWindow::RunTimeoutHandler
...

Comment 2

[Mats Palmgren (inactive)]

Seems like it might be a DOM problem at first glance.

Note: the test requires the "DOM fuzzer" extension.

Comment 3

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

Attachment: patch

Patch to fix the bad DOM assertions, and can't get any related crashes with the
patch.

(Note, looks like we're overly strict in some other cases where we're using
GetParent() and not GetParentNode(), but that just means we're possibly throwing
too easily or end up executing slower code path.)

Comment 4

[Peter Van der Beken [:peterv]]

Comment on attachment 792410 [details] [diff] [review]
patch

Review of attachment 792410 [details] [diff] [review]:
-----------------------------------------------------------------

This looks right, but does it actually fix the crash? If so, can you briefly explain how?

Comment 5

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

We don't end up having same element as a child of two different parents.
nsINode::doInsertChildAt just does aChildArray.InsertChildAt(aKid, aIndex); and if
aKid already has parent...

Comment 6

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

(I'll upload a minimal testcase to show the DOM problem in a minute)

Comment 7

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

Attachment: Smaller testcase, no addons needed

Comment 8

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

Comment on attachment 792410 [details] [diff] [review]
patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
I think it wouldn't be too difficult.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Check-in comment will be about following the DOM specification more strictly

Which older supported branches are affected by this flaw?
All

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
The patch should apply (with some fuzz) to all the branches

How likely is this patch to cause regressions; how much testing does it need?
The patch should be safe, almost like a null check.

Comment 9

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

We need to get a security rating on this before approval. Is this an exploitable crash?

Comment 10

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

As far as I see this is sg-crit. Something goes horribly wrong in the DOM tree.

Comment 11

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

Comment on attachment 792410 [details] [diff] [review]
patch

sec-approval+ for trunk. I've approved it on beta and aurora after it goes into trunk.

Comment 12

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

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

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a7b75c5af99a
https://hg.mozilla.org/releases/mozilla-beta/rev/e0cc79b0d1e4

Comment 15

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

We need to determine if we're taking this on ESR17, Release Management.

Comment 16

[Lukas Blakk [:lsblakk] use ?needinfo]

If it can be taken on ESR 17, it should as we'll have 2 more ESR 17 releases during the transition to ESR 24.  Marking for 24+ and approving the patch for esr17.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr17/rev/423c0d3cd0c0

Comment 18

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

Confirmed crash in FF23, various pre-patch builds of FF24 and FF25.
Verified fixed in ASan builds of FF17esr, FF24, FF25 and FF26, 2013-08-26.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

b2g18 is frozen for non-leo+ blockers. Please request blocking status if you feel that this must land there.

Comment 20

[Alex Keybl [:akeybl]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #19)
> b2g18 is frozen for non-leo+ blockers. Please request blocking status if you
> feel that this must land there.

I'm sorry, I fucked this up. I fed RyanVM bad information. The v1.1 branch will be open until 3/3/2014, as it's being maintained for security until that time.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/05c16432cc04

Comment 22

[Ryan VanderMeulen [:RyanVM]]

And because I fail at life, I forgot that this required a bustage fix on esr17 which I then forgot to fold into the b2g18 landing. Argh, FML.
https://hg.mozilla.org/releases/mozilla-b2g18/rev/b5c75cafd328

Comment 23

[Ryan VanderMeulen [:RyanVM]]

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