Closed Bug 1490561 Opened 11 months ago Closed 11 months ago

heap-use-after-free in [@ mozilla::ScrollFrameHelper::AsyncSmoothMSDScroll::~AsyncSmoothMSDScroll]

Categories

(Core :: Layout, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox-esr60 63+ fixed
firefox62 --- wontfix
firefox63 + fixed
firefox64 + fixed

People

(Reporter: tsmith, Assigned: mats)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [adv-main63+][adv-esr60.3+])

Attachments

(5 files)

The test case is being reduced and I will attach it once reduction is complete.

==19355==ERROR: AddressSanitizer: heap-use-after-free on address 0x611001050d08 at pc 0x7f7cbf1b3ad7 bp 0x7fffae2a1470 sp 0x7fffae2a1468
READ of size 8 at 0x611001050d08 thread T0
    #0 0x7f7cbf1b3ad6 in PresContext src/obj-firefox/dist/include/mozilla/ComputedStyle.h:89:55
    #1 0x7f7cbf1b3ad6 in RefreshDriver src/layout/generic/nsGfxScrollFrame.cpp:1878
    #2 0x7f7cbf1b3ad6 in RemoveObserver src/layout/generic/nsGfxScrollFrame.cpp:1864
    #3 0x7f7cbf1b3ad6 in mozilla::ScrollFrameHelper::AsyncSmoothMSDScroll::~AsyncSmoothMSDScroll() src/layout/generic/nsGfxScrollFrame.cpp:1872
    #4 0x7f7cbf1b385d in mozilla::ScrollFrameHelper::AsyncSmoothMSDScroll::Release() src/layout/generic/nsGfxScrollFrame.cpp:1775:3
    #5 0x7f7cbeb9d455 in Release src/obj-firefox/dist/include/mozilla/RefPtr.h:42:11
    #6 0x7f7cbeb9d455 in Release src/obj-firefox/dist/include/mozilla/RefPtr.h:407
    #7 0x7f7cbeb9d455 in ~RefPtr src/obj-firefox/dist/include/mozilla/RefPtr.h:80
    #8 0x7f7cbeb9d455 in nsRefreshDriver::Tick(mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:1881
    #9 0x7f7cbebaf182 in TickDriver src/layout/base/nsRefreshDriver.cpp:325:13
    #10 0x7f7cbebaf182 in mozilla::RefreshDriverTimer::TickRefreshDrivers(mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) src/layout/base/nsRefreshDriver.cpp:300
    #11 0x7f7cbebaeca1 in mozilla::RefreshDriverTimer::Tick(mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:317:5
    #12 0x7f7cbebb1f51 in RunRefreshDrivers src/layout/base/nsRefreshDriver.cpp:756:5
    #13 0x7f7cbebb1f51 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:672
    #14 0x7f7cbebac849 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::ParentProcessVsyncNotifier::Run() src/layout/base/nsRefreshDriver.cpp:513:20
    #15 0x7f7cb47f21a0 in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1161:14
    #16 0x7f7cb47faf45 in NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:519:10
    #17 0x7f7cb5a0c9de in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:97:21
    #18 0x7f7cb590d72c in RunInternal src/ipc/chromium/src/base/message_loop.cc:325:10
    #19 0x7f7cb590d72c in RunHandler src/ipc/chromium/src/base/message_loop.cc:318
    #20 0x7f7cb590d72c in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:298
    #21 0x7f7cbe4c5006 in nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:158:27
    #22 0x7f7cc268620b in nsAppStartup::Run() src/toolkit/components/startup/nsAppStartup.cpp:290:30
    #23 0x7f7cc294d3c2 in XREMain::XRE_mainRun() src/toolkit/xre/nsAppRunner.cpp:4826:22
    #24 0x7f7cc29504ed in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) src/toolkit/xre/nsAppRunner.cpp:4971:8
    #25 0x7f7cc2951c9e in XRE_main(int, char**, mozilla::BootstrapConfig const&) src/toolkit/xre/nsAppRunner.cpp:5063:21
    #26 0x5563601d09bc in do_main src/browser/app/nsBrowserApp.cpp:233:22
    #27 0x5563601d09bc in main src/browser/app/nsBrowserApp.cpp:315
    #28 0x7f7cd6a6d82f in __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/../csu/libc-start.c:291
    #29 0x5563600fff4c in _start (/home/ubuntu/firefox/firefox+0x2cf4c)

0x611001050d08 is located 8 bytes inside of 256-byte region [0x611001050d00,0x611001050e00)
freed by thread T0 here:
    #0 0x5563601a0382 in __interceptor_free /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:68:3
    #1 0x7f7cc4cd3902 in _$LT$servo_arc..Arc$LT$T$GT$$u20$as$u20$core..ops..drop..Drop$GT$::drop::h2fd3191269910b81 src/servo/components/servo_arc/lib.rs:391
    #2 0x7f7cc4cd3902 in core::ptr::drop_in_place::h752eae435b8da090 /checkout/src/libcore/ptr.rs:59
    #3 0x7f7cc4cd3902 in style::gecko::arc_types::Servo_ComputedStyle_Release::_$u7b$$u7b$closure$u7d$$u7d$::h87673abbeabaf7e0 src/servo/components/style/gecko/arc_types.rs:152
    #4 0x7f7cc4cd3902 in _$LT$servo_arc..ArcBorrow$LT$$u27$a$C$$u20$T$GT$$GT$::with_arc::h8819abff15dbea85 src/servo/components/servo_arc/lib.rs:964
    #5 0x7f7cc4cd3902 in Servo_ComputedStyle_Release src/servo/components/style/gecko/arc_types.rs:151
    #6 0x7f7cbef35ff6 in nsFrame::DestroyFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&) src/layout/generic/nsFrame.cpp:835:9
    #7 0x7f7cbf0534f9 in nsFrameList::DestroyFramesFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&) src/layout/generic/nsFrameList.cpp:59:12
    #8 0x7f7cbeecbf01 in DestroyFrames src/layout/generic/nsAbsoluteContainingBlock.cpp:359:19
    #9 0x7f7cbeecbf01 in DestroyAbsoluteFrames src/layout/generic/nsContainerFrame.cpp:193
    #10 0x7f7cbeecbf01 in nsBlockFrame::DestroyFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&) src/layout/generic/nsBlockFrame.cpp:323
    #11 0x7f7cbf17be4a in nsLineBox::DeleteLineList(nsPresContext*, nsLineList&, nsIFrame*, nsFrameList*, mozilla::layout::PostFrameDestroyData&) src/layout/generic/nsLineBox.cpp:401:14
    #12 0x7f7cbeecbfaa in nsBlockFrame::DestroyFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&) src/layout/generic/nsBlockFrame.cpp:327:3
    #13 0x7f7cbf17be4a in nsLineBox::DeleteLineList(nsPresContext*, nsLineList&, nsIFrame*, nsFrameList*, mozilla::layout::PostFrameDestroyData&) src/layout/generic/nsLineBox.cpp:401:14
    #14 0x7f7cbeecbfaa in nsBlockFrame::DestroyFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&) src/layout/generic/nsBlockFrame.cpp:327:3
    #15 0x7f7cbf0534f9 in nsFrameList::DestroyFramesFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&) src/layout/generic/nsFrameList.cpp:59:12
    #16 0x7f7cbeecd060 in nsContainerFrame::DestroyFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&) src/layout/generic/nsContainerFrame.cpp:230:11
    #17 0x7f7cbf0534f9 in nsFrameList::DestroyFramesFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&) src/layout/generic/nsFrameList.cpp:59:12
    #18 0x7f7cbeecd060 in nsContainerFrame::DestroyFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&) src/layout/generic/nsContainerFrame.cpp:230:11
    #19 0x7f7cbf0534f9 in nsFrameList::DestroyFramesFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&) src/layout/generic/nsFrameList.cpp:59:12
    #20 0x7f7cbeecd060 in nsContainerFrame::DestroyFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&) src/layout/generic/nsContainerFrame.cpp:230:11
    #21 0x7f7cbed1217c in Destroy src/layout/generic/nsIFrame.h:679:5
    #22 0x7f7cbed1217c in Destroy src/layout/base/nsFrameManager.cpp:58
    #23 0x7f7cbed1217c in nsCSSFrameConstructor::WillDestroyFrameTree() src/layout/base/nsCSSFrameConstructor.cpp:8200
    #24 0x7f7cbebfb1e1 in mozilla::PresShell::Destroy() src/layout/base/PresShell.cpp:1374:22
    #25 0x7f7cbed4c501 in nsDocumentViewer::DestroyPresShell() src/layout/base/nsDocumentViewer.cpp:4648:15
    #26 0x7f7cbed37076 in nsDocumentViewer::Destroy() src/layout/base/nsDocumentViewer.cpp:1897:5
    #27 0x7f7cbed4f4eb in nsDocumentViewer::Show() src/layout/base/nsDocumentViewer.cpp:2233:17

previously allocated by thread T0 here:
    #0 0x5563601a06c3 in malloc /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:88:3
    #1 0x7f7cc4b9f96e in alloc_system::platform::_$LT$impl$u20$core..alloc..GlobalAlloc$u20$for$u20$alloc_system..System$GT$::alloc::h5a1f0db41e296502 /checkout/src/liballoc_system/lib.rs:137
    #2 0x7f7cc4b9f96e in __rdl_alloc /checkout/src/libstd/alloc.rs:157
    #3 0x7f7cc4b9f96e in alloc::alloc::alloc::hb6317adf94d93093 /checkout/src/liballoc/alloc.rs:62
    #4 0x7f7cc4b9f96e in alloc::alloc::exchange_malloc::h911d16fc4c40eb97 /checkout/src/liballoc/alloc.rs:157
    #5 0x7f7cc4b9f96e in _$LT$alloc..boxed..Box$LT$T$GT$$GT$::new::h9945beb157c6fd75 /checkout/src/liballoc/boxed.rs:96
    #6 0x7f7cc4b9f96e in _$LT$servo_arc..Arc$LT$T$GT$$GT$::new::h26c84de38f94541b src/servo/components/servo_arc/lib.rs:187
    #7 0x7f7cc4b9f96e in style::gecko_properties::_$LT$impl$u20$style..gecko_bindings..structs..root..ServoComputedData$GT$::to_outer_helper::h3f3b0a02d613b37c src/obj-firefox/toolkit/library/x86_64-unknown-linux-gnu/release/build/style-90ab97c350cba8c9/out/gecko_properties.rs:463
    #8 0x7f7cc4b9f96e in style::gecko_properties::_$LT$impl$u20$style..gecko_bindings..structs..root..ServoComputedData$GT$::to_outer::h38e90bf3fff4a892 src/obj-firefox/toolkit/library/x86_64-unknown-linux-gnu/release/build/style-90ab97c350cba8c9/out/gecko_properties.rs:453
    #9 0x7f7cc4b9f96e in style::gecko_properties::ComputedValues::new::h243a4760ea51eeca src/obj-firefox/toolkit/library/x86_64-unknown-linux-gnu/release/build/style-90ab97c350cba8c9/out/gecko_properties.rs:219
    #10 0x7f7cc4b9f96e in style::properties::StyleBuilder::build::h3bb2a9130f68b8db src/obj-firefox/toolkit/library/x86_64-unknown-linux-gnu/release/build/style-90ab97c350cba8c9/out/properties.rs:82243
Keywords: sec-high
Hmm, alloc and free stacks make sense, the crashing stack makes no sense, it does not access any style related stuff. So I suspect it may be another thing being allocated earlier. But we'll see.
n-i for testcase
Flags: needinfo?(twsmith)
Setting to P1 for now because of sec-high and to get off triage list. We can lower once we have more information.
Priority: -- → P1
Flags: needinfo?(twsmith)
Fwiw, the line number in stack frame #3 above seems to indicate this
build has the fix for bug 1484559 in it.

I suspect that we crash in:
  nsRefreshDriver* RefreshDriver(ScrollFrameHelper* aCallee) {
    return aCallee->mOuter->PresContext()->RefreshDriver();
  }

We know aCallee can't be null since RemoveObserver() checks that.
So mOuter or mOuter->mComputedStyle is bogus for some reason?
But if the ScrollFrameHelper associated with this
AsyncSmoothMSDScroll object is destroyed then it should have
called RemoveObserver() already (and mCallee should be null
after that).
I should have a test case soon (tonight? tomorrow). It has taken multiple attempts and multiple days of running to get this test case reduced because it is so inconsistent.
Attached file testcase.html
It can take a couple minutes to trigger the issue
Flags: in-testsuite?
Keywords: testcase
Attached file prefs.js
Thanks Tyson, I can reproduce it in rr.
Assignee: nobody → mats
Attached file stacks
We have a registered mAsyncSmoothMSDScroll and during its callback
we null that out in CompleteAsyncScroll before calling ScrollToImpl.
(this is the first stack)
Note that nulling out mAsyncSmoothMSDScroll doesn't destroy it,
because nsRefreshDriver::Tick holds a strong ref on the stack
while its WillRefresh callback is running:
https://searchfox.org/mozilla-central/rev/ce57be88b8aa2ad03ace1b9684cd6c361be5109f/layout/base/nsRefreshDriver.cpp#1874-1875

While running ScrollToImpl we destroy the scrollframe associated
with this AsyncSmoothMSDScroll; this is the second stack.
ScrollFrameHelper::Destroy has code that reset the observer and
its pointer to this frame:
https://searchfox.org/mozilla-central/rev/ce57be88b8aa2ad03ace1b9684cd6c361be5109f/layout/generic/nsGfxScrollFrame.cpp#4910-4915
Unfortunately though, since we nulled out mAsyncSmoothMSDScroll
this code doesn't do anything.

So then we leave the scope in nsRefreshDriver::Tick which has
the last ref to the AsyncSmoothMSDScroll instance so we run
its destructor:
https://searchfox.org/mozilla-central/rev/ce57be88b8aa2ad03ace1b9684cd6c361be5109f/layout/generic/nsGfxScrollFrame.cpp#1752,1862-1867,1871-1872
which uses mCallee (the destroyed frame) to get to
the RefreshDriver to deregister itself.

This is an UAF where frame-poisoning doesn't help because we
destroyed the entire shell and thus deallocated the frame arena.
Attached patch fixSplinter Review
So rather than just nulling out these members in CompleteAsyncScroll,
we must also call their RemoveObserver() methods, like we do in
ScrollFrameHelper::Destroy.  So I'm adding a RemoveObservers()
method to do that from both places.

(In retrospect, I should've figured this out this could happen
already in bug 1484559 comment 6.)

I'll try to rewrite this code to be less fragile in a follow-up
bug, but since we should probably uplift this patch to branches
I'm trying to keep it simple and not include the refactoring.
Attachment #9012655 - Flags: review?(emilio)
(I'm guessing this (or bug 1484559) is also the root cause of bug 1107353.)
Comment on attachment 9012655 [details] [diff] [review]
fix

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

Thanks, this looks good, though I'd also note that the AccessibleCaretManager::FlushLayout call that ends up destroying the shell and the frame should just not happen (flushing from a reflow observer is very wrong :/).

I should probably take time to fix bug 1445794...

r=me in any case.
Attachment #9012655 - Flags: review?(emilio) → review+
Yup, true.  However, I don't want to exclude the possibility that
we might still have other culprits besides AccessibleCaretManager.
But yes, it's about time we simply stop allowing stuff like this
by adding fatal assertions and then just fix whatever falls out...
Comment on attachment 9012655 [details] [diff] [review]
fix

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

Very hard.

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

Not really, it reveals it's some issue related to scrolling.

Which older supported branches are affected by this flaw?

All, but the bug is (likely) only triggered when the pref
layout.accessiblecaret.enabled = true.  As far as I know, it's
only true on Android by default, it's false on other platforms
by default (but can be enabled in about:config by the user).

If not all supported branches, which bug introduced the flaw?

n/a

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Should be easy to uplift.  Note that the fix for bug 1484559 is also needed (first).

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

The risk for regressions seems low.
Not much testing needed, just do a basic check that scrolling any web page
still works on Android, with some text selected on the page.
Attachment #9012655 - Flags: sec-approval?
(In reply to Mats Palmgren (:mats) from comment #14)
> Which older supported branches are affected by this flaw?
> 
> All, but the bug is (likely) only triggered when the pref
> layout.accessiblecaret.enabled = true.  As far as I know, it's
> only true on Android by default, it's false on other platforms
> by default (but can be enabled in about:config by the user).

I _think_ it's enabled for desktops with touchscreens as well, though TYLin can probably confirm.
Comment on attachment 9012655 [details] [diff] [review]
fix

sec-approval+ for trunk. We'll want patches to backport this.
Attachment #9012655 - Flags: sec-approval? → sec-approval+
Comment on attachment 9012655 [details] [diff] [review]
fix

Approval Request Comment
[Feature/Bug causing the regression]:not a recent regression
[User impact if declined]: possibly exploitable crash
[Is this code covered by automated tests?]:not yet
[Has the fix been verified in Nightly?]:not yet
[Needs manual test from QE? If yes, steps to reproduce]: Yes, on an Android device please. STR in the bug, see test note at end of comment 14.
[List of other uplifts needed for the feature/fix]: bug 1484559
[Is the change risky?]:no
[Why is the change risky/not risky?]:fairly trivial change
[String changes made/needed]:none
Attachment #9012655 - Flags: approval-mozilla-geckoview62?
Attachment #9012655 - Flags: approval-mozilla-esr60?
Attachment #9012655 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e879c661c84d27e657d10425229232f9798c7cd
https://hg.mozilla.org/mozilla-central/rev/4e879c661c84
Group: layout-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Comment on attachment 9012655 [details] [diff] [review]
fix

Approved for 63.0b11. Not approving for GV62 per previous discussions with the owners of that release about sec backports to that branch.
Attachment #9012655 - Flags: approval-mozilla-geckoview62?
Attachment #9012655 - Flags: approval-mozilla-geckoview62-
Attachment #9012655 - Flags: approval-mozilla-beta?
Attachment #9012655 - Flags: approval-mozilla-beta+
Attached patch esr60 patchSplinter Review
[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: 

User impact if declined: 

Fix Landed on Version: 

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): 

String or UUID changes made by this patch:
Attachment #9014569 - Flags: approval-mozilla-esr60?
Comment on attachment 9012655 [details] [diff] [review]
fix

Fixes a sec-high, approved for ESR 60.3.
Attachment #9012655 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Attachment #9012655 - Flags: approval-mozilla-esr60+
Attachment #9014569 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Duplicate of this bug: 1497991
Whiteboard: [adv-main63+][adv-esr60.3+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.