Closed Bug 1601024 Opened 4 years ago Closed 4 years ago

heap-use-after-free in [@ GetOwningEventTarget]

Categories

(Core :: DOM: Workers, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla74
Tracking Status
firefox-esr68 --- unaffected
firefox72 --- wontfix
firefox73 + fixed
firefox74 + fixed

People

(Reporter: tsmith, Assigned: perry)

References

(Blocks 2 open bugs, Regression)

Details

(5 keywords, Whiteboard: [testcase reduction blocked by bug 1588357][adv-main73+r][post-critsmash-triage])

Crash Data

Attachments

(1 file)

Report from m-c 20191202-9420b5dc27e0

We've hit this a few times but the reported test cases trigger bug 1588357 so not sure if this is a dupe. If so that makes bug 1588357 a s-s bug.

==18881==ERROR: AddressSanitizer: heap-use-after-free on address 0x6110005bb548 at pc 0x7ff8f3f95a74 bp 0x7fff9bf357b0 sp 0x7fff9bf357a8
READ of size 8 at 0x6110005bb548 thread T0 (Web Content)
    #0 0x7ff8f3f95a73 in get /src/obj-firefox/dist/include/nsCOMPtr.h:823:48
    #1 0x7ff8f3f95a73 in operator nsISerialEventTarget * /src/obj-firefox/dist/include/nsCOMPtr.h:831:33
    #2 0x7ff8f3f95a73 in GetOwningEventTarget /src/dom/workers/remoteworkers/RemoteWorkerChild.cpp:274:10
    #3 0x7ff8f3f95a73 in mozilla::dom::RemoteWorkerChild::ErrorPropagationOnMainThread(mozilla::dom::WorkerErrorReport const*, bool) /src/dom/workers/remoteworkers/RemoteWorkerChild.cpp:675:3
    #4 0x7ff8f3f3b395 in mozilla::dom::(anonymous namespace)::ReportErrorRunnable::WorkerRun(JSContext*, mozilla::dom::WorkerPrivate*) /src/dom/workers/WorkerError.cpp:91:20
    #5 0x7ff8f3f6fa1e in mozilla::dom::WorkerRunnable::Run() /src/dom/workers/WorkerRunnable.cpp:369:12
    #6 0x7ff8eb908748 in mozilla::ThrottledEventQueue::Inner::ExecuteRunnable() /src/xpcom/threads/ThrottledEventQueue.cpp:252:22
    #7 0x7ff8eb8ff43f in mozilla::ThrottledEventQueue::Inner::Executor::Run() /src/xpcom/threads/ThrottledEventQueue.cpp:80:15
    #8 0x7ff8eb8e74ea in nsThread::ProcessNextEvent(bool, bool*) /src/xpcom/threads/nsThread.cpp:1250:14
    #9 0x7ff8eb8ee991 in NS_ProcessNextEvent(nsIThread*, bool) /src/xpcom/threads/nsThreadUtils.cpp:486:10
    #10 0x7ff8ecb083ef in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /src/ipc/glue/MessagePump.cpp:88:21
    #11 0x7ff8eca12622 in RunInternal /src/ipc/chromium/src/base/message_loop.cc:315:10
    #12 0x7ff8eca12622 in RunHandler /src/ipc/chromium/src/base/message_loop.cc:308:3
    #13 0x7ff8eca12622 in MessageLoop::Run() /src/ipc/chromium/src/base/message_loop.cc:290:3
    #14 0x7ff8f46e8f58 in nsBaseAppShell::Run() /src/widget/nsBaseAppShell.cpp:137:27
    #15 0x7ff8f8753216 in XRE_RunAppShell() /src/toolkit/xre/nsEmbedFunctions.cpp:934:20
    #16 0x7ff8eca12622 in RunInternal /src/ipc/chromium/src/base/message_loop.cc:315:10
    #17 0x7ff8eca12622 in RunHandler /src/ipc/chromium/src/base/message_loop.cc:308:3
    #18 0x7ff8eca12622 in MessageLoop::Run() /src/ipc/chromium/src/base/message_loop.cc:290:3
    #19 0x7ff8f8752a64 in XRE_InitChildProcess(int, char**, XREChildData const*) /src/toolkit/xre/nsEmbedFunctions.cpp:769:34
    #20 0x562d82361c5c in content_process_main /src/browser/app/../../ipc/contentproc/plugin-container.cpp:56:28
    #21 0x562d82361c5c in main /src/browser/app/nsBrowserApp.cpp:272:18
    #22 0x7ff90ea49b96 in __libc_start_main /build/glibc-OTsEL5/glibc-2.27/csu/../csu/libc-start.c:310
    #23 0x562d822b706c in _start (/home/worker/builds/m-c-20191202220401-fuzzing-asan-opt/firefox+0x5506c)

0x6110005bb548 is located 200 bytes inside of 240-byte region [0x6110005bb480,0x6110005bb570)
freed by thread T14 (Worker Launcher) here:
    #0 0x562d8232ed3d in free /builds/worker/fetches/llvm-project/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:123:3
    #1 0x7ff8eca6ae1f in Release /src/obj-firefox/dist/include/mozilla/RefCounted.h:201:7
    #2 0x7ff8eca6ae1f in Release /src/obj-firefox/dist/include/mozilla/ThreadSafeWeakPtr.h:213:23
    #3 0x7ff8eca6ae1f in Release /src/obj-firefox/dist/include/mozilla/RefPtr.h:48:40
    #4 0x7ff8eca6ae1f in Release /src/obj-firefox/dist/include/mozilla/RefPtr.h:373:36
    #5 0x7ff8eca6ae1f in ~RefPtr /src/obj-firefox/dist/include/mozilla/RefPtr.h:79:7
    #6 0x7ff8eca6ae1f in mozilla::ipc::BackgroundChildImpl::DeallocPRemoteWorkerChild(mozilla::dom::PRemoteWorkerChild*) /src/ipc/glue/BackgroundChildImpl.cpp:344:1
    #7 0x7ff8ecb0f083 in mozilla::ipc::ActorLifecycleProxy::~ActorLifecycleProxy() /src/ipc/glue/ProtocolUtils.cpp:253:11
    #8 0x7ff8ed28f167 in Release /src/obj-firefox/dist/include/mozilla/ipc/ProtocolUtils.h:914:3
    #9 0x7ff8ed28f167 in Release /src/obj-firefox/dist/include/mozilla/RefPtr.h:48:40
    #10 0x7ff8ed28f167 in Release /src/obj-firefox/dist/include/mozilla/RefPtr.h:373:36
    #11 0x7ff8ed28f167 in ~RefPtr /src/obj-firefox/dist/include/mozilla/RefPtr.h:79:7
    #12 0x7ff8ed28f167 in mozilla::ipc::PBackgroundChild::OnMessageReceived(IPC::Message const&) /src/obj-firefox/ipc/ipdl/PBackgroundChild.cpp:5877:5
    #13 0x7ff8ecaff236 in mozilla::ipc::MessageChannel::DispatchAsyncMessage(mozilla::ipc::ActorLifecycleProxy*, IPC::Message const&) /src/ipc/glue/MessageChannel.cpp:2208:25
    #14 0x7ff8ecafa251 in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) /src/ipc/glue/MessageChannel.cpp:2130:9
    #15 0x7ff8ecafc7c1 in mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) /src/ipc/glue/MessageChannel.cpp:1972:3
    #16 0x7ff8ecafd687 in mozilla::ipc::MessageChannel::MessageTask::Run() /src/ipc/glue/MessageChannel.cpp:2003:13
    #17 0x7ff8eb8e74ea in nsThread::ProcessNextEvent(bool, bool*) /src/xpcom/threads/nsThread.cpp:1250:14
    #18 0x7ff8eb8ee991 in NS_ProcessNextEvent(nsIThread*, bool) /src/xpcom/threads/nsThreadUtils.cpp:486:10
    #19 0x7ff8ecb09d94 in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) /src/ipc/glue/MessagePump.cpp:333:5
    #20 0x7ff8eca12622 in RunInternal /src/ipc/chromium/src/base/message_loop.cc:315:10
    #21 0x7ff8eca12622 in RunHandler /src/ipc/chromium/src/base/message_loop.cc:308:3
    #22 0x7ff8eca12622 in MessageLoop::Run() /src/ipc/chromium/src/base/message_loop.cc:290:3
    #23 0x7ff8eb8e0f71 in nsThread::ThreadFunc(void*) /src/xpcom/threads/nsThread.cpp:458:11
    #24 0x7ff90ff23ec5 in _pt_root /src/nsprpub/pr/src/pthreads/ptthread.c:201:5
    #25 0x7ff90fb6b6da in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x76da)

previously allocated by thread T14 (Worker Launcher) here:
    #0 0x562d8232efbd in malloc /builds/worker/fetches/llvm-project/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:145:3
    #1 0x562d823647fd in moz_xmalloc /src/memory/mozalloc/mozalloc.cpp:52:15
    #2 0x7ff8eca6abfb in operator new /src/obj-firefox/dist/include/mozilla/cxxalloc.h:33:10
    #3 0x7ff8eca6abfb in mozilla::ipc::BackgroundChildImpl::AllocPRemoteWorkerChild(mozilla::dom::RemoteWorkerData const&) /src/ipc/glue/BackgroundChildImpl.cpp:328:42
    #4 0x7ff8ed29184c in mozilla::ipc::PBackgroundChild::OnMessageReceived(IPC::Message const&) /src/obj-firefox/ipc/ipdl/PBackgroundChild.cpp:6245:49
    #5 0x7ff8ecaff236 in mozilla::ipc::MessageChannel::DispatchAsyncMessage(mozilla::ipc::ActorLifecycleProxy*, IPC::Message const&) /src/ipc/glue/MessageChannel.cpp:2208:25
    #6 0x7ff8ecafa251 in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) /src/ipc/glue/MessageChannel.cpp:2130:9
    #7 0x7ff8ecafc7c1 in mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) /src/ipc/glue/MessageChannel.cpp:1972:3
    #8 0x7ff8ecafd687 in mozilla::ipc::MessageChannel::MessageTask::Run() /src/ipc/glue/MessageChannel.cpp:2003:13
    #9 0x7ff8eb8e74ea in nsThread::ProcessNextEvent(bool, bool*) /src/xpcom/threads/nsThread.cpp:1250:14
    #10 0x7ff8eb8ee991 in NS_ProcessNextEvent(nsIThread*, bool) /src/xpcom/threads/nsThreadUtils.cpp:486:10
    #11 0x7ff8ecb09d94 in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) /src/ipc/glue/MessagePump.cpp:333:5
    #12 0x7ff8eca12622 in RunInternal /src/ipc/chromium/src/base/message_loop.cc:315:10
    #13 0x7ff8eca12622 in RunHandler /src/ipc/chromium/src/base/message_loop.cc:308:3
    #14 0x7ff8eca12622 in MessageLoop::Run() /src/ipc/chromium/src/base/message_loop.cc:290:3
    #15 0x7ff8eb8e0f71 in nsThread::ThreadFunc(void*) /src/xpcom/threads/nsThread.cpp:458:11
    #16 0x7ff90ff23ec5 in _pt_root /src/nsprpub/pr/src/pthreads/ptthread.c:201:5
    #17 0x7ff90fb6b6da in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x76da)

Thread T14 (Worker Launcher) created by T0 (Web Content) here:
    #0 0x562d8231974a in pthread_create /builds/worker/fetches/llvm-project/llvm/projects/compiler-rt/lib/asan/asan_interceptors.cc:209:3
    #1 0x7ff90ff15b99 in _PR_CreateThread /src/nsprpub/pr/src/pthreads/ptthread.c:458:14
    #2 0x7ff90feff1ee in PR_CreateThread /src/nsprpub/pr/src/pthreads/ptthread.c:533:12
    #3 0x7ff8eb8e33d6 in nsThread::Init(nsTSubstring<char> const&) /src/xpcom/threads/nsThread.cpp:673:8
    #4 0x7ff8eb8edb01 in nsThreadManager::NewNamedThread(nsTSubstring<char> const&, unsigned int, nsIThread**) /src/xpcom/threads/nsThreadManager.cpp:550:12
    #5 0x7ff8eb8f1af3 in NS_NewNamedThread(nsTSubstring<char> const&, nsIThread**, nsIRunnable*, unsigned int) /src/xpcom/threads/nsThreadUtils.cpp:139:57
    #6 0x7ff8f3fa46fd in NS_NewNamedThread<16> /src/obj-firefox/dist/include/nsThreadUtils.h:71:10
    #7 0x7ff8f3fa46fd in mozilla::dom::RemoteWorkerService::InitializeOnMainThread() /src/dom/workers/remoteworkers/RemoteWorkerService.cpp:82:17
    #8 0x7ff8f3fa4128 in mozilla::dom::RemoteWorkerService::Initialize() /src/dom/workers/remoteworkers/RemoteWorkerService.cpp:49:28
    #9 0x7ff8f3d1f192 in mozilla::dom::ContentChild::InitXPCOM(mozilla::dom::XPCOMInitData const&, mozilla::dom::ipc::StructuredCloneData const&) /src/dom/ipc/ContentChild.cpp:1353:3
    #10 0x7ff8f3d1edd8 in mozilla::dom::ContentChild::RecvSetXPCOMProcessAttributes(mozilla::dom::XPCOMInitData const&, mozilla::dom::ipc::StructuredCloneData const&, nsTArray<LookAndFeelInt>&&, nsTArray<mozilla::dom::SystemFontListEntry>&&, mozilla::Maybe<base::FileDescriptor> const&, unsigned long const&) /src/dom/ipc/ContentChild.cpp:628:3
    #11 0x7ff8eceb129f in mozilla::dom::PContentChild::OnMessageReceived(IPC::Message const&) /src/obj-firefox/ipc/ipdl/PContentChild.cpp:10564:56
    #12 0x7ff8ecaff236 in mozilla::ipc::MessageChannel::DispatchAsyncMessage(mozilla::ipc::ActorLifecycleProxy*, IPC::Message const&) /src/ipc/glue/MessageChannel.cpp:2208:25
    #13 0x7ff8ecafa251 in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) /src/ipc/glue/MessageChannel.cpp:2130:9
    #14 0x7ff8ecafc7c1 in mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) /src/ipc/glue/MessageChannel.cpp:1972:3
    #15 0x7ff8ecafd687 in mozilla::ipc::MessageChannel::MessageTask::Run() /src/ipc/glue/MessageChannel.cpp:2003:13
    #16 0x7ff8eb8e74ea in nsThread::ProcessNextEvent(bool, bool*) /src/xpcom/threads/nsThread.cpp:1250:14
    #17 0x7ff8eb8ee991 in NS_ProcessNextEvent(nsIThread*, bool) /src/xpcom/threads/nsThreadUtils.cpp:486:10
    #18 0x7ff8ecb083ef in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /src/ipc/glue/MessagePump.cpp:88:21
    #19 0x7ff8eca12622 in RunInternal /src/ipc/chromium/src/base/message_loop.cc:315:10
    #20 0x7ff8eca12622 in RunHandler /src/ipc/chromium/src/base/message_loop.cc:308:3
    #21 0x7ff8eca12622 in MessageLoop::Run() /src/ipc/chromium/src/base/message_loop.cc:290:3
    #22 0x7ff8f46e8f58 in nsBaseAppShell::Run() /src/widget/nsBaseAppShell.cpp:137:27
    #23 0x7ff8f8753216 in XRE_RunAppShell() /src/toolkit/xre/nsEmbedFunctions.cpp:934:20
    #24 0x7ff8eca12622 in RunInternal /src/ipc/chromium/src/base/message_loop.cc:315:10
    #25 0x7ff8eca12622 in RunHandler /src/ipc/chromium/src/base/message_loop.cc:308:3
    #26 0x7ff8eca12622 in MessageLoop::Run() /src/ipc/chromium/src/base/message_loop.cc:290:3
    #27 0x7ff8f8752a64 in XRE_InitChildProcess(int, char**, XREChildData const*) /src/toolkit/xre/nsEmbedFunctions.cpp:769:34
    #28 0x562d82361c5c in content_process_main /src/browser/app/../../ipc/contentproc/plugin-container.cpp:56:28
    #29 0x562d82361c5c in main /src/browser/app/nsBrowserApp.cpp:272:18
    #30 0x7ff90ea49b96 in __libc_start_main /build/glibc-OTsEL5/glibc-2.27/csu/../csu/libc-start.c:310

Not a dupe of bug 1588357. That's largely just a bad assertion that's going to freak out anytime the fuzzer decides to call register() multiple times (or add in an unregister). That said, based on some other bugs, it's possible that removing the assertion would instantly reveal a bunch of life-cycle issues that were why the assertion was put there in the first place. (I think bug 1472573 for example may be an indication of such an underlying issue.) And that life-cycle issue could be in the same family as this one.

Priority: -- → P1
Whiteboard: testcase reduction blocked by bug 1588357
Assignee: nobody → ytausky

:tsmith, could you please upload the test cases you mentioned?

Flags: needinfo?(twsmith)

(In reply to Yaron Tausky [:ytausky] from comment #3)

:tsmith, could you please upload the test cases you mentioned?

The test case mentioned does not reproduce this issue. It only reproduces bug 1588357. My guess is that once bug 1588357 is fixed it will be reducible. At the moment the unreduced fuzz test case is far to big to be in anyway useful.

Flags: needinfo?(twsmith)
Depends on: 1588357

Unassigning myself before going away for 3 weeks, someone from the team will pick it up.

Assignee: ytausky → nobody
Assignee: nobody → perry

I can't reproduce with the testcase from bug 1588357. tsmith, do you know if the unreduced testcase is still able to reproduce this?

Flags: needinfo?(twsmith)

(In reply to Perry Jiang [:perry] from comment #6)

I can't reproduce with the testcase from bug 1588357. tsmith, do you know if the unreduced testcase is still able to reproduce this?

No, of the 7 instances reported by the fuzzer none of the reproduce the issue with the latest build. FWIW it was first seen with 20191122-2a76c7273dd2 and last seen with 20191202-9420b5dc27e0 so it was last seen about 15 days ago.

Flags: needinfo?(twsmith)

Wait I take that back! I managed to reproduce with 20191217-83fc8cf83221 but the test is still very unreliable. I will let the reducer run over night to see if I can get something.

:tsmith, any luck in reducing ?

Flags: needinfo?(twsmith)
See Also: → 1606364

(In reply to Jens Stutte [:jstutte] (OOO until 2020-01-06) from comment #9)

:tsmith, any luck in reducing ?

No, but the fuzzers do seem to be hitting it. Unfortunately all the testcases that are reported are too unreliable to reduce atm.

Flags: needinfo?(twsmith)
Crash Signature: [@ mozilla::dom::RemoteWorkerChild::ErrorPropagationOnMainThread]

As :mccr8 suggested, this looks very similar to bug 1606364. Maybe the ASAN stack helps to learn something more?

Flags: needinfo?(perry)

The UAF'ed object appears to be RemoteWorkerChild, which inherits from SupportsThreadSafeWeakPtr<T>, which inherits from AtomicRefCounted<T>.

AtomicRefCounted<T>::Release does an atomic decrement, but the decrement and delete if the refcount reaches 0 is not atomic, so maybe it's the case that AddRef was called between the "final" Release and subsequent delete.

This is possible if the ordering is

  1. Thread A calls ThreadSafeWeakReference::getRefPtr (which occurs when trying to convert a ThreadSafeWeakPtr to a RefPtr) and gets to mLock.ReadLock() (but does not call the RefPtr constructor yet).
  2. Thread B calls SupportsThreadSafeWeakPtr::Release with refcount=1, which calls ThreadSafeWeakReference::tryDetach. tryDetach does nothing because a read lock is already acquired from 1). After calling tryDetach, AtomicRefCounted<T>::Release is called, still with refcount=1, and the object is deleted.
  3. Thread A calls RefPtr<T>'s constructor, AddRef-ing the just-deleted object, and a UAF follows.
Flags: needinfo?(perry)

Comment on attachment 9121106 [details]
Bug 1601024 - ThreadSafeWeakReference::tryDetach must acquire its lock r?froydnj

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: An exploit would necessarily involve a race condition under a very specific ordering of events. The race condition also involves two non-worker threads, and an exploit would likely originate from a worker thread, which adds extra difficulty (there are currently no synchronization available to JS for the involved threads). So I would say that constructing an exploit would be very difficult and definitely involves luck.
  • 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?: none
  • If not all supported branches, which bug introduced the flaw?: Bug 1231213
  • Do you have backports for the affected branches?: No
  • If not, how different, hard to create, and risky will they be?: n/a
  • How likely is this patch to cause regressions; how much testing does it need?: Highly unlikely; it makes cross-thread refcounting logic acquire a lock when it originally might not have. Testing will probably have to come from real usage because it's a pretty edge case race condition.
Attachment #9121106 - Flags: sec-approval?
Keywords: regression

Comment on attachment 9121106 [details]
Bug 1601024 - ThreadSafeWeakReference::tryDetach must acquire its lock r?froydnj

Approved to land and request uplift.

Attachment #9121106 - Flags: sec-approval? → sec-approval+
Group: dom-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla74

Please nominate this for Beta approval when you get a chance. It grafts cleanly as-landed.

Flags: needinfo?(perry)

Comment on attachment 9121106 [details]
Bug 1601024 - ThreadSafeWeakReference::tryDetach must acquire its lock r?froydnj

Beta/Release Uplift Approval Request

  • User impact if declined: null pointer de-reference, which will crash or, IIRC, cause undefined behavior.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Fixes a clear race condition that previously led to the use of de-allocated objects.
  • String changes made/needed:
Flags: needinfo?(perry)
Attachment #9121106 - Flags: approval-mozilla-beta?

Comment on attachment 9121106 [details]
Bug 1601024 - ThreadSafeWeakReference::tryDetach must acquire its lock r?froydnj

Fixes a sec issue, approved for 73.0b9.

Attachment #9121106 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Whiteboard: testcase reduction blocked by bug 1588357 → [testcase reduction blocked by bug 1588357][adv-main73+r]
Flags: qe-verify-
Whiteboard: [testcase reduction blocked by bug 1588357][adv-main73+r] → [testcase reduction blocked by bug 1588357][adv-main73+r][postcritsmash-triage]
Whiteboard: [testcase reduction blocked by bug 1588357][adv-main73+r][postcritsmash-triage] → [testcase reduction blocked by bug 1588357][adv-main73+r][post-critsmash-triage]
Group: core-security-release
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: