Tyson Smith [:tsmith] Reporter
	Description • 6 years ago

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

	Emilio Cobos Álvarez (:emilio)
	Comment 1 • 6 years ago

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.

	Mats Palmgren (inactive) Assignee
	Comment 2 • 6 years ago

n-i for testcase

	Sean Voisen (:svoisen)
	Comment 3 • 6 years ago

Setting to P1 for now because of sec-high and to get off triage list. We can lower once we have more information.

	Mats Palmgren (inactive) Assignee
	Comment 4 • 6 years ago

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).

	Tyson Smith [:tsmith] Reporter
	Comment 5 • 6 years ago

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.

	Tyson Smith [:tsmith] Reporter
	Comment 6 • 6 years ago

It can take a couple minutes to trigger the issue

	Mats Palmgren (inactive) Assignee
	Comment 8 • 6 years ago

Thanks Tyson, I can reproduce it in rr.

	Mats Palmgren (inactive) Assignee
	Comment 9 • 6 years ago

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.

	Mats Palmgren (inactive) Assignee
	Comment 10 • 6 years ago

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.

	Mats Palmgren (inactive) Assignee
	Comment 11 • 6 years ago

(I'm guessing this (or bug 1484559) is also the root cause of bug 1107353.)

	Emilio Cobos Álvarez (:emilio)
	Comment 12 • 6 years ago

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.

	Mats Palmgren (inactive) Assignee
	Comment 13 • 6 years ago

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...

	Mats Palmgren (inactive) Assignee
	Comment 14 • 6 years ago

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.

	Emilio Cobos Álvarez (:emilio)
	Comment 15 • 6 years ago

(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.

	Al Billings [:abillings - ex-MoCo]
	Comment 16 • 6 years ago

Comment on attachment 9012655 [details] [diff] [review]
fix

sec-approval+ for trunk. We'll want patches to backport this.

	Mats Palmgren (inactive) Assignee
	Comment 17 • 6 years ago

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

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 18 • 6 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e879c661c84d27e657d10425229232f9798c7cd
https://hg.mozilla.org/mozilla-central/rev/4e879c661c84

	Ryan VanderMeulen [:RyanVM]
	Comment 19 • 6 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 20 • 6 years ago
uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/b4edf6d224f7

	Mats Palmgren (inactive) Assignee
	Comment 21 • 6 years ago

[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:

	Ryan VanderMeulen [:RyanVM]
	Comment 22 • 6 years ago

Comment on attachment 9012655 [details] [diff] [review]
fix

Fixes a sec-high, approved for ESR 60.3.

	Ryan VanderMeulen [:RyanVM]
	Comment 23 • 6 years ago
uplift

https://hg.mozilla.org/releases/mozilla-esr60/rev/1749661dfd28