Bug 906301 (CVE-2013-1736)

Memory corruption in nsGfxScrollFrameInner::IsLTR()

RESOLVED FIXED in Firefox 24, Firefox OS v1.1hd

Status

()

Core
DOM
--
critical
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: Nils, Assigned: smaug)

Tracking

({crash, sec-critical, testcase})

Trunk
mozilla26
x86_64
Linux
crash, sec-critical, testcase
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox23 wontfix, firefox24+ verified, firefox25+ verified, firefox26+ verified, firefox-esr1724+ verified, b2g1824+ fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix, b2g-v1.1hd fixed)

Details

(Whiteboard: [asan][adv-main24+][adv-esr1709+])

Attachments

(3 attachments)

(Reporter)

Description

4 years ago
Created attachment 791654 [details]
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
(Reporter)

Updated

4 years ago
Summary: Memory corruption innsGfxScrollFrameInner::IsLTR() → Memory corruption in nsGfxScrollFrameInner::IsLTR()
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
...
Severity: normal → critical
Seems like it might be a DOM problem at first glance.

Note: the test requires the "DOM fuzzer" extension.
Component: Layout → DOM
Keywords: crash, testcase
Whiteboard: [asan]
(Assignee)

Updated

4 years ago
Assignee: nobody → bugs
(Assignee)

Comment 3

4 years ago
Created attachment 792410 [details] [diff] [review]
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.)
Attachment #792410 - Flags: review?(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?
Attachment #792410 - Flags: review?(peterv) → review+
(Assignee)

Comment 5

4 years ago
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...
(Assignee)

Comment 6

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

Comment 7

4 years ago
Created attachment 792745 [details]
Smaller testcase, no addons needed
(Assignee)

Comment 8

4 years ago
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.
Attachment #792410 - Flags: sec-approval?
Attachment #792410 - Flags: approval-mozilla-esr17?
Attachment #792410 - Flags: approval-mozilla-beta?
Attachment #792410 - Flags: approval-mozilla-b2g18?
Attachment #792410 - Flags: approval-mozilla-aurora?
We need to get a security rating on this before approval. Is this an exploitable crash?
Flags: needinfo?(bugs)
(Assignee)

Comment 10

4 years ago
As far as I see this is sg-crit. Something goes horribly wrong in the DOM tree.
Flags: needinfo?(bugs)
Keywords: sec-critical
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.
Attachment #792410 - Flags: sec-approval?
Attachment #792410 - Flags: sec-approval+
Attachment #792410 - Flags: approval-mozilla-beta?
Attachment #792410 - Flags: approval-mozilla-beta+
Attachment #792410 - Flags: approval-mozilla-aurora?
Attachment #792410 - Flags: approval-mozilla-aurora+
status-b2g18: --- → affected
status-firefox23: --- → wontfix
status-firefox24: --- → affected
status-firefox25: --- → affected
status-firefox26: --- → affected
status-firefox-esr17: --- → affected
tracking-b2g18: --- → ?
tracking-firefox24: --- → +
tracking-firefox25: --- → +
tracking-firefox26: --- → +
tracking-firefox-esr17: --- → ?
(Assignee)

Comment 12

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b81ac916f015
https://hg.mozilla.org/mozilla-central/rev/b81ac916f015
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox26: affected → fixed
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
https://hg.mozilla.org/releases/mozilla-aurora/rev/a7b75c5af99a
https://hg.mozilla.org/releases/mozilla-beta/rev/e0cc79b0d1e4
status-firefox24: affected → fixed
status-firefox25: affected → fixed
We need to determine if we're taking this on ESR17, Release Management.
Whiteboard: [asan] → [asan][adv-main24+]
Flags: needinfo?(release-mgmt)
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.
tracking-firefox-esr17: ? → 24+
Flags: needinfo?(release-mgmt)
Attachment #792410 - Flags: approval-mozilla-esr17? → approval-mozilla-esr17+
https://hg.mozilla.org/releases/mozilla-esr17/rev/423c0d3cd0c0
status-firefox-esr17: affected → fixed
Whiteboard: [asan][adv-main24+] → [asan][adv-main24+][adv-esr1709+]
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.
status-firefox24: fixed → verified
status-firefox25: fixed → verified
status-firefox26: fixed → verified
status-firefox-esr17: fixed → verified
Attachment #792410 - Flags: approval-mozilla-b2g18?
b2g18 is frozen for non-leo+ blockers. Please request blocking status if you feel that this must land there.
status-b2g18: affected → wontfix
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → wontfix
status-b2g-v1.1hd: --- → wontfix
(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.
status-b2g18: wontfix → affected
status-b2g-v1.1hd: wontfix → affected
tracking-b2g18: ? → 24+

Updated

4 years ago
Attachment #792410 - Flags: approval-mozilla-b2g18+
https://hg.mozilla.org/releases/mozilla-b2g18/rev/05c16432cc04
status-b2g18: affected → fixed
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
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
status-b2g-v1.1hd: affected → fixed
Alias: CVE-2013-1736
Group: core-security
You need to log in before you can comment on or make changes to this bug.