heap-use-after-free in [@ mozilla::dom::TabParent::SendRealDragEvent]

RESOLVED FIXED in Firefox -esr52

Status

()

defect
P1
normal
RESOLVED FIXED
2 years ago
8 months ago

People

(Reporter: tsmith, Assigned: stone)

Tracking

(4 keywords)

Trunk
mozilla58
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr5257+ fixed, firefox56 wontfix, firefox57+ fixed, firefox58+ fixed)

Details

(Whiteboard: [no-nag][adv-main57+][adv-esr52.5+][post-critsmash-triage])

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
I came across this while trying to reduce a test case. Unfortunately I am unable to reproduce this. I will attach a test case if I am able to get one. Hopefully the log is helpful.

==15228==ERROR: AddressSanitizer: heap-use-after-free on address 0x61a000104272 at pc 0x7f3cc6b4a8b6 bp 0x7ffd4ac24b10 sp 0x7ffd4ac24b08
READ of size 1 at 0x61a000104272 thread T0
    #0 0x7f3cc6b4a8b5 in mozilla::dom::TabParent::SendRealDragEvent(mozilla::WidgetDragEvent&, unsigned int, unsigned int) src/dom/ipc/TabParent.cpp:1122:7
    #1 0x7f3cc579c0ed in mozilla::EventStateManager::DispatchCrossProcessEvent(mozilla::WidgetEvent*, nsFrameLoader*, nsEventStatus*) src/dom/events/EventStateManager.cpp:1270:27
    #2 0x7f3cc579cdb6 in mozilla::EventStateManager::HandleCrossProcessEvent(mozilla::WidgetEvent*, nsEventStatus*) src/dom/events/EventStateManager.cpp:1368:19
    #3 0x7f3cc57a7cff in mozilla::EventStateManager::PostHandleEvent(nsPresContext*, mozilla::WidgetEvent*, nsIFrame*, nsEventStatus*) src/dom/events/EventStateManager.cpp:2906:37
    #4 0x7f3cc7977ac3 in mozilla::PresShell::HandleEventInternal(mozilla::WidgetEvent*, nsEventStatus*, bool) src/layout/base/PresShell.cpp:8147:23
    #5 0x7f3cc79791d7 in mozilla::PresShell::HandlePositionedEvent(nsIFrame*, mozilla::WidgetGUIEvent*, nsEventStatus*) src/layout/base/PresShell.cpp:7915:10
    #6 0x7f3cc79746df in mozilla::PresShell::HandleEvent(nsIFrame*, mozilla::WidgetGUIEvent*, bool, nsEventStatus*, nsIContent**) src/layout/base/PresShell.cpp:7703:12
    #7 0x7f3cc71b576e in nsViewManager::DispatchEvent(mozilla::WidgetGUIEvent*, nsView*, nsEventStatus*) src/view/nsViewManager.cpp:805:14
    #8 0x7f3cc71b4f93 in nsView::HandleEvent(mozilla::WidgetGUIEvent*, bool) src/view/nsView.cpp:1146:9
    #9 0x7f3cc726f4d9 in nsWindow::DispatchEvent(mozilla::WidgetGUIEvent*, nsEventStatus&) src/widget/gtk/nsWindow.cpp:571:27
    #10 0x7f3cc71ce09b in nsBaseWidget::ProcessUntransformedAPZEvent(mozilla::WidgetInputEvent*, mozilla::layers::ScrollableLayerGuid const&, unsigned long, nsEventStatus) src/widget/nsBaseWidget.cpp:1034:3
    #11 0x7f3cc71ced75 in nsBaseWidget::DispatchInputEvent(mozilla::WidgetInputEvent*) src/widget/nsBaseWidget.cpp:1175:14
    #12 0x7f3cc728c712 in nsWindow::DispatchDragEvent(mozilla::EventMessage, mozilla::gfx::IntPointTyped<mozilla::LayoutDevicePixel> const&, unsigned int) src/widget/gtk/nsWindow.cpp:3398:5
    #13 0x7f3cc72f0ed9 in DispatchMotionEvents src/widget/gtk/nsDragService.cpp:2097:20
    #14 0x7f3cc72f0ed9 in nsDragService::RunScheduledTask() src/widget/gtk/nsDragService.cpp:1997
    #15 0x7f3cc72f07ee in nsDragService::TaskDispatchCallback(void*) src/widget/gtk/nsDragService.cpp:1914:25
    #16 0x7f3cd65fd049 in g_main_context_dispatch (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x4a049)
    #17 0x7f3cd65fd3ef  (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x4a3ef)
    #18 0x7f3cd65fd49b in g_main_context_iteration (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x4a49b)
    #19 0x7f3cc72d142e in nsAppShell::ProcessNextNativeEvent(bool) src/widget/gtk/nsAppShell.cpp:278:12
    #20 0x7f3cc7235136 in DoProcessNextNativeEvent src/widget/nsBaseAppShell.cpp:138:17
    #21 0x7f3cc7235136 in nsBaseAppShell::OnProcessNextEvent(nsIThreadInternal*, bool) src/widget/nsBaseAppShell.cpp:271
    #22 0x7f3cc7235a3f in non-virtual thunk to nsBaseAppShell::OnProcessNextEvent(nsIThreadInternal*, bool) src/widget/nsBaseAppShell.cpp:233:17
    #23 0x7f3cc0fc5c68 in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1354:10
    #24 0x7f3cc0fd3678 in NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:472:10
    #25 0x7f3cc1d80221 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:96:21
    #26 0x7f3cc1cde3a0 in RunInternal src/ipc/chromium/src/base/message_loop.cc:318:10
    #27 0x7f3cc1cde3a0 in RunHandler src/ipc/chromium/src/base/message_loop.cc:311
    #28 0x7f3cc1cde3a0 in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:291
    #29 0x7f3cc72349ef in nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:156:27
    #30 0x7f3ccb2b1431 in nsAppStartup::Run() src/toolkit/components/startup/nsAppStartup.cpp:283:30
    #31 0x7f3ccb481674 in XREMain::XRE_mainRun() src/toolkit/xre/nsAppRunner.cpp:4569:22
    #32 0x7f3ccb4831e0 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) src/toolkit/xre/nsAppRunner.cpp:4749:8
    #33 0x7f3ccb484531 in XRE_main(int, char**, mozilla::BootstrapConfig const&) src/toolkit/xre/nsAppRunner.cpp:4844:21
    #34 0x4eb613 in do_main src/browser/app/nsBrowserApp.cpp:237:22
    #35 0x4eb613 in main src/browser/app/nsBrowserApp.cpp:310
    #36 0x7f3cddb5c82f in __libc_start_main /build/glibc-bfm8X4/glibc-2.23/csu/../csu/libc-start.c:291
    #37 0x41d168 in _start /m-c-1497454412-asan-opt/firefox+0x41d168)

0x61a000104272 is located 1010 bytes inside of 1160-byte region [0x61a000103e80,0x61a000104308)
freed by thread T0 here:
    #0 0x4bb69b in __interceptor_free /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:47:3
    #1 0x7f3cc6b3d7e5 in Release src/dom/ipc/TabParent.cpp:133:1
    #2 0x7f3cc6b3d7e5 in non-virtual thunk to mozilla::dom::TabParent::Release() src/dom/ipc/TabParent.cpp:133
    #3 0x7f3cc0e5c841 in ~nsCOMPtr_base src/xpcom/base/nsCOMPtr.h:294:7
    #4 0x7f3cc0e5c841 in PopLast src/obj-firefox/dist/include/mozilla/SegmentedVector.h:98
    #5 0x7f3cc0e5c841 in mozilla::SegmentedVector<nsCOMPtr<nsISupports>, 4096ul, mozilla::MallocAllocPolicy>::PopLastN(unsigned int) src/obj-firefox/dist/include/mozilla/SegmentedVector.h:265
    #6 0x7f3cc0e4c55e in mozilla::dom::DeferredFinalizerImpl<nsISupports>::DeferredFinalize(unsigned int, void*) src/obj-firefox/dist/include/mozilla/dom/BindingUtils.h:2805:15
    #7 0x7f3cc0e4d116 in mozilla::IncrementalFinalizeRunnable::ReleaseNow(bool) src/xpcom/base/CycleCollectedJSRuntime.cpp:1322:18
    #8 0x7f3cc0e4d5b0 in mozilla::IncrementalFinalizeRunnable::Run() src/xpcom/base/CycleCollectedJSRuntime.cpp:1356:3
    #9 0x7f3cc0fc6798 in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1416:14
    #10 0x7f3cc0fbf6e6 in NS_ProcessPendingEvents(nsIThread*, unsigned int) src/xpcom/threads/nsThreadUtils.cpp:414:19
    #11 0x7f3cc7234788 in nsBaseAppShell::NativeEventCallback() src/widget/nsBaseAppShell.cpp:97:3
    #12 0x7f3cc72d0574 in nsAppShell::EventProcessorCallback(_GIOChannel*, GIOCondition, void*) src/widget/gtk/nsAppShell.cpp:127:11
    #13 0x7f3cd65fd049 in g_main_context_dispatch (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x4a049)

previously allocated by thread T0 here:
    #0 0x4bb9ec in malloc /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:64:3
    #1 0x4ecf0d in moz_xmalloc src/memory/mozalloc/mozalloc.cpp:83:17
    #2 0x7f3cc6b65cad in operator new src/obj-firefox/dist/include/mozilla/mozalloc.h:194:12
    #3 0x7f3cc6b65cad in mozilla::dom::nsIContentParent::AllocPBrowserParent(mozilla::dom::IdType<mozilla::dom::TabParent> const&, mozilla::dom::IdType<mozilla::dom::TabParent> const&, mozilla::dom::IPCTabContext const&, unsigned int const&, mozilla::dom::IdType<mozilla::dom::ContentParent> const&, bool const&) src/dom/ipc/nsIContentParent.cpp:194
    #4 0x7f3cc2575023 in mozilla::dom::PContentParent::OnMessageReceived(IPC::Message const&) src/obj-firefox/ipc/ipdl/PContentParent.cpp:3272:21
    #5 0x7f3cc1d7862e in mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) src/ipc/glue/MessageChannel.cpp:2043:25
    #6 0x7f3cc1d75444 in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) src/ipc/glue/MessageChannel.cpp:1969:17
    #7 0x7f3cc1d77094 in mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) src/ipc/glue/MessageChannel.cpp:1838:5
    #8 0x7f3cc1d77678 in mozilla::ipc::MessageChannel::MessageTask::Run() src/ipc/glue/MessageChannel.cpp:1871:15
    #9 0x7f3cc0fc6798 in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1416:14
    #10 0x7f3cc0fd3678 in NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:472:10
    #11 0x7f3cc1d80216 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:124:5
    #12 0x7f3cc1cde3a0 in RunInternal src/ipc/chromium/src/base/message_loop.cc:318:10
    #13 0x7f3cc1cde3a0 in RunHandler src/ipc/chromium/src/base/message_loop.cc:311
    #14 0x7f3cc1cde3a0 in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:291
    #15 0x7f3cc72349ef in nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:156:27
    #16 0x7f3ccb2b1431 in nsAppStartup::Run() src/toolkit/components/startup/nsAppStartup.cpp:283:30
    #17 0x7f3ccb481674 in XREMain::XRE_mainRun() src/toolkit/xre/nsAppRunner.cpp:4569:22
    #18 0x7f3ccb4831e0 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) src/toolkit/xre/nsAppRunner.cpp:4749:8
    #19 0x7f3ccb484531 in XRE_main(int, char**, mozilla::BootstrapConfig const&) src/toolkit/xre/nsAppRunner.cpp:4844:21
    #20 0x4eb613 in do_main src/browser/app/nsBrowserApp.cpp:237:22
    #21 0x4eb613 in main src/browser/app/nsBrowserApp.cpp:310
    #22 0x7f3cddb5c82f in __libc_start_main /build/glibc-bfm8X4/glibc-2.23/csu/../csu/libc-start.c:291

SUMMARY: AddressSanitizer: heap-use-after-free src/dom/ipc/TabParent.cpp:1122:7 in mozilla::dom::TabParent::SendRealDragEvent(mozilla::WidgetDragEvent&, unsigned int, unsigned int)
Shadow bytes around the buggy address:
  0x0c34800187f0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c3480018800: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c3480018810: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c3480018820: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c3480018830: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
=>0x0c3480018840: fd fd fd fd fd fd fd fd fd fd fd fd fd fd[fd]fd
  0x0c3480018850: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c3480018860: fd fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c3480018870: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c3480018880: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c3480018890: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Heap right redzone:      fb
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack partial redzone:   f4
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==15228==ABORTING
Component: DOM → DOM: Events
EventStateManager::DispatchCrossProcessEvent() looks like:
  TabParent* remote = TabParent::GetFrom(aFrameLoader);
  ...
  <call some drag session methods>
  remote->SendRealDragEvent(...)

Maybe some of the drag session methods cause the tab parent to be destroyed?
Hmm, I have no idea. Looks like that the all methods called in the method won't destroy the TabParent. (Although I'm not familiar with D&D event handlers.)
Michael may know a bit about D&D event handlers and could take a quick look here.
Flags: needinfo?(michael)
I took a quick look. It seems like the object was somehow destroyed between http://searchfox.org/mozilla-central/rev/b7e531410ebc1c7d74a78e86a894e69608db318c/dom/events/EventStateManager.cpp#1259 and http://searchfox.org/mozilla-central/rev/b7e531410ebc1c7d74a78e86a894e69608db318c/dom/events/EventStateManager.cpp#1276.

Looking at all of these functions, none seem to do anything super dramatic other than MaybeInvokeDragSession http://searchfox.org/mozilla-central/rev/b7e531410ebc1c7d74a78e86a894e69608db318c/dom/ipc/ContentParent.cpp#4395. There's a chance that something in that method causes the TabParent object to be torn down.

I really don't know much about how this part of D&D works (I've basically only worked on the DataTransfer interface). Once enndeakin gets back he'll probably be the best person to look at this, if we can wait that long.
Flags: needinfo?(michael)
I'll needinfo David to see if anyone else comes to mind but otherwise let's wait for Neil.
Flags: needinfo?(ddurst)
Priority: -- → P1
David, there's a needinfo pending. Any updates?
Keywords: testcase-wanted
There are no updates. Neil is the right (and possibly only) person for this and he's on leave until early August.
Flags: needinfo?(ddurst)

Comment 8

2 years ago
It's possible that many things could be destroyed within MaybeInvokeDragSession (specifically the call to FillAllExternalData), especially on Linux, as it uses a synchronous event loop to wait for all the drag data.

You could just try keeping a refptr to the TabParent.
Stone, any chance you could take a look at this, perhaps following comment 8 suggestion?
Flags: needinfo?(sshih)
(Assignee)

Comment 10

2 years ago
Flags: needinfo?(sshih)
(Assignee)

Updated

2 years ago
Assignee: nobody → sshih
(Assignee)

Updated

2 years ago
Attachment #8909676 - Flags: review?(bugs)
Attachment #8909676 - Flags: review?(bugs) → review+
(Assignee)

Comment 11

2 years ago
Comment on attachment 8909676 [details] [diff] [review]
bug-1375146-fix-v1.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
I think it's hard. So far we didn't have the exact steps to reproduce the problem.

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

Which older supported branches are affected by this flaw?
All supported branches (They are affected only when e10s enabled)

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
I can prepare patches for all affected branches if necessary.

How likely is this patch to cause regressions; how much testing does it need?
The probability should be low.
Attachment #8909676 - Flags: sec-approval?
sec-approval for trunk checkin on October 9, two weeks into the cycle. Do not check this in before that date, please.

We'll want a beta patch made and nominated for 57 and ESR52 at that time.
Attachment #8909676 - Flags: sec-approval? → sec-approval+
Whiteboard: [checkin on 10/9] → [checkin on 10/9][no-nag]
https://hg.mozilla.org/mozilla-central/rev/d4496b8befbf

Please request Beta and ESR52 approval on this when you get a chance. Note that it grafts cleanly to Beta but will require a rebased patch for ESR52.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Flags: needinfo?(sshih)
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
(Assignee)

Comment 15

2 years ago
[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
This is a sec bug.

User impact if declined: 
We don't know the exact steps to reproduce this problem. It's hard for me to say the specific impacts to end users. But this is a security vulnerability since it's a use-after-free problem.

Fix Landed on Version:
firefox58

Risk to taking this patch (and alternatives if risky): 
I think its low risk to take this patch since it only holds the remote browser when dispatching DnD events.

String or UUID changes made by this patch: 
NA

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Flags: needinfo?(sshih)
Attachment #8916969 - Flags: approval-mozilla-esr52?
(Assignee)

Comment 16

2 years ago
Comment on attachment 8909676 [details] [diff] [review]
bug-1375146-fix-v1.patch

Approval Request Comment
[Feature/Bug causing the regression]:
Bug 936092

[User impact if declined]:
This is a security vulnerability since it's a use-after-free problem.

[Is this code covered by automated tests?]:
No. We don't know the steps to reproduce the bug.

[Has the fix been verified in Nightly?]:
No. We don't know the steps to reproduce the bug. I only verified the DnD behavior is correct.

[Needs manual test from QE? If yes, steps to reproduce]: 
No.

[List of other uplifts needed for the feature/fix]:
NA

[Is the change risky?]:
I think its low risk.

[Why is the change risky/not risky?]:
It only holds the remote browser when dispatching DnD events.

[String changes made/needed]:
NA
Attachment #8909676 - Flags: approval-mozilla-beta?
Comment on attachment 8909676 [details] [diff] [review]
bug-1375146-fix-v1.patch

Sec-high, beta57+
Attachment #8909676 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8916969 [details] [diff] [review]
bug-1375146-fix-esr52.patch

ESR52+
Attachment #8916969 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
Group: dom-core-security → core-security-release
Whiteboard: [no-nag] → [no-nag][adv-main57+][adv-esr52.5+]
Flags: qe-verify-
Whiteboard: [no-nag][adv-main57+][adv-esr52.5+] → [no-nag][adv-main57+][adv-esr52.5+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.