AddressSanitizer: heap-use-after-free /builds/worker/workspace/build/src/obj-firefox/dist/include/mtransport/sigslot.h:318:13 in ~lock_block

RESOLVED FIXED in Firefox -esr52

Status

()

P1
critical
Rank:
9
RESOLVED FIXED
a year ago
7 months ago

People

(Reporter: jkratzer, Assigned: mjf)

Tracking

(Blocks: 2 bugs, 4 keywords)

59 Branch
mozilla59
crash, csectype-uaf, sec-high, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?
qe-verify -

Firefox Tracking Flags

(firefox-esr5258+ fixed, firefox57 wontfix, firefox58+ fixed, firefox59+ fixed)

Details

(Whiteboard: [adv-main58+][adv-esr52.6+][post-critsmash-triage])

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

a year ago
Posted file trigger.html
Testcase found while fuzzing mozilla-central rev 6d82e132348f.

==4360==ERROR: AddressSanitizer: heap-use-after-free on address 0x6150002ede48 at pc 0x7f107ecf555f bp 0x7ffe1c4fa1a0 sp 0x7ffe1c4fa198
READ of size 8 at 0x6150002ede48 thread T0 (file:// Content)
    #0 0x7f107ecf555e in ~lock_block /builds/worker/workspace/build/src/obj-firefox/dist/include/mtransport/sigslot.h:318:13
    #1 0x7f107ecf555e in operator() /builds/worker/workspace/build/src/obj-firefox/dist/include/mtransport/sigslot.h:2424
    #2 0x7f107ecf555e in mozilla::PeerConnectionMedia::IceGatheringStateChange_m(mozilla::NrIceCtx*, mozilla::NrIceCtx::GatheringState) /builds/worker/workspace/build/src/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:1359
    #3 0x7f107ed45472 in apply<mozilla::PeerConnectionMedia *, void (mozilla::PeerConnectionMedia::*)(mozilla::NrIceCtx *, mozilla::NrIceCtx::GatheringState), mozilla::NrIceCtx *, mozilla::NrIceCtx::GatheringState, 0, 1> /builds/worker/workspace/build/src/obj-firefox/dist/include/mtransport/runnable_utils.h:104:5
    #4 0x7f107ed45472 in mozilla::runnable_args_memfn<mozilla::PeerConnectionMedia*, void (mozilla::PeerConnectionMedia::*)(mozilla::NrIceCtx*, mozilla::NrIceCtx::GatheringState), mozilla::NrIceCtx*, mozilla::NrIceCtx::GatheringState>::Run() /builds/worker/workspace/build/src/obj-firefox/dist/include/mtransport/runnable_utils.h:174
    #5 0x7f107cf713ee in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1033:14
    #6 0x7f107cf8cd50 in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:508:10
    #7 0x7f107de1760a in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/workspace/build/src/ipc/glue/MessagePump.cpp:97:21
    #8 0x7f107dd6afa9 in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:326:10
    #9 0x7f107dd6afa9 in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:319
    #10 0x7f107dd6afa9 in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:299
    #11 0x7f10843a43da in nsBaseAppShell::Run() /builds/worker/workspace/build/src/widget/nsBaseAppShell.cpp:157:27
    #12 0x7f1088adb7cb in XRE_RunAppShell() /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:875:22
    #13 0x7f107dd6afa9 in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:326:10
    #14 0x7f107dd6afa9 in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:319
    #15 0x7f107dd6afa9 in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:299
    #16 0x7f1088adb1bd in XRE_InitChildProcess(int, char**, XREChildData const*) /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:701:34
    #17 0x4f2dfc in content_process_main /builds/worker/workspace/build/src/browser/app/../../ipc/contentproc/plugin-container.cpp:63:30
    #18 0x4f2dfc in main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:280
    #19 0x7f109c08c82f in __libc_start_main /build/glibc-bfm8X4/glibc-2.23/csu/../csu/libc-start.c:291
    #20 0x42243c in _start (/home/forb1dden/builds/mc-asan/firefox+0x42243c)

0x6150002ede48 is located 72 bytes inside of 456-byte region [0x6150002ede00,0x6150002edfc8)
freed by thread T0 (file:// Content) here:
    #0 0x4c2e92 in __interceptor_free /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:68:3
    #1 0x7f107ed44fa0 in apply<mozilla::PeerConnectionMedia *, void (mozilla::PeerConnectionMedia::*)()> /builds/worker/workspace/build/src/obj-firefox/dist/include/mtransport/runnable_utils.h:104:5
    #2 0x7f107ed44fa0 in mozilla::runnable_args_memfn<mozilla::PeerConnectionMedia*, void (mozilla::PeerConnectionMedia::*)()>::Run() /builds/worker/workspace/build/src/obj-firefox/dist/include/mtransport/runnable_utils.h:174
    #3 0x7f107cf713ee in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1033:14
    #4 0x7f107cf8cd50 in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:508:10
    #5 0x7f1083b23b2c in SpinEventLoopUntil<mozilla::ProcessFailureBehavior::ReportToCaller, (lambda at /builds/worker/workspace/build/src/dom/ipc/ContentChild.cpp:1091:24)> /builds/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:323:25
    #6 0x7f1083b23b2c in mozilla::dom::ContentChild::ProvideWindowCommon(mozilla::dom::TabChild*, mozIDOMWindowProxy*, bool, unsigned int, bool, bool, bool, nsIURI*, nsTSubstring<char16_t> const&, nsTSubstring<char> const&, bool, nsIDocShellLoadInfo*, bool*, mozIDOMWindowProxy**) /builds/worker/workspace/build/src/dom/ipc/ContentChild.cpp:1091
    #7 0x7f1083baed1c in mozilla::dom::TabChild::ProvideWindow(mozIDOMWindowProxy*, unsigned int, bool, bool, bool, nsIURI*, nsTSubstring<char16_t> const&, nsTSubstring<char> const&, bool, nsIDocShellLoadInfo*, bool*, mozIDOMWindowProxy**) /builds/worker/workspace/build/src/dom/ipc/TabChild.cpp:1061:16
    #8 0x7f1088a2aedb in nsWindowWatcher::OpenWindowInternal(mozIDOMWindowProxy*, char const*, char const*, char const*, bool, bool, bool, nsIArray*, bool, bool, nsIDocShellLoadInfo*, mozIDOMWindowProxy**) /builds/worker/workspace/build/src/toolkit/components/windowwatcher/nsWindowWatcher.cpp:852:24
    #9 0x7f1088a308cc in OpenWindow2 /builds/worker/workspace/build/src/toolkit/components/windowwatcher/nsWindowWatcher.cpp:444:10
    #10 0x7f1088a308cc in non-virtual thunk to nsWindowWatcher::OpenWindow2(mozIDOMWindowProxy*, char const*, char const*, char const*, bool, bool, bool, nsISupports*, bool, bool, nsIDocShellLoadInfo*, mozIDOMWindowProxy**) /builds/worker/workspace/build/src/toolkit/components/windowwatcher/nsWindowWatcher.cpp
    #11 0x7f107fecce64 in nsGlobalWindowOuter::OpenInternal(nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const&, bool, bool, bool, bool, bool, nsIArray*, nsISupports*, nsIDocShellLoadInfo*, bool, nsPIDOMWindowOuter**) /builds/worker/workspace/build/src/dom/base/nsGlobalWindowOuter.cpp:7193:21
    #12 0x7f107fecbc8d in OpenJS /builds/worker/workspace/build/src/dom/base/nsGlobalWindowOuter.cpp:5599:10
    #13 0x7f107fecbc8d in nsGlobalWindowOuter::OpenOuter(nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const&, mozilla::ErrorResult&) /builds/worker/workspace/build/src/dom/base/nsGlobalWindowOuter.cpp:5574
    #14 0x7f107fe6c022 in nsGlobalWindowInner::Open(nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const&, mozilla::ErrorResult&) /builds/worker/workspace/build/src/dom/base/nsGlobalWindowInner.cpp:3793:3
    #15 0x7f1081823204 in mozilla::dom::WindowBinding::open(JSContext*, JS::Handle<JSObject*>, nsGlobalWindowInner*, JSJitMethodCallArgs const&) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/WindowBinding.cpp:2189:56
    #16 0x7f1081821600 in mozilla::dom::WindowBinding::genericMethod(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/WindowBinding.cpp:15271:13
    #17 0x7f1088dc4464 in CallJSNative /builds/worker/workspace/build/src/js/src/jscntxtinlines.h:291:15
    #18 0x7f1088dc4464 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:473
    #19 0x7f1088daa548 in CallFromStack /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:528:12
    #20 0x7f1088daa548 in Interpret(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3096
    #21 0x7f1088d96cb0 in js::RunScript(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:423:12
    #22 0x7f1088dc7401 in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:706:15
    #23 0x7f1088dc7b9f in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:738:12
    #24 0x7f10898d6646 in ExecuteScript(JSContext*, JS::AutoObjectVector&, JS::Handle<JSScript*>, JS::Value*) /builds/worker/workspace/build/src/js/src/jsapi.cpp:4678:12
    #25 0x7f10802efc66 in nsJSUtils::ExecutionContext::CompileAndExec(JS::CompileOptions&, JS::SourceBufferHolder&, JS::MutableHandle<JSScript*>) /builds/worker/workspace/build/src/dom/base/nsJSUtils.cpp:266:8
    #26 0x7f1084217f21 in mozilla::dom::ScriptLoader::EvaluateScript(mozilla::dom::ScriptLoadRequest*) /builds/worker/workspace/build/src/dom/script/ScriptLoader.cpp:2287:25
    #27 0x7f10842133c9 in mozilla::dom::ScriptLoader::ProcessRequest(mozilla::dom::ScriptLoadRequest*) /builds/worker/workspace/build/src/dom/script/ScriptLoader.cpp:1929:10
    #28 0x7f10841f6315 in mozilla::dom::ScriptLoader::ProcessScriptElement(nsIScriptElement*) /builds/worker/workspace/build/src/dom/script/ScriptLoader.cpp:1627:10
    #29 0x7f10841f22e7 in mozilla::dom::ScriptElement::MaybeProcessScript() /builds/worker/workspace/build/src/dom/script/ScriptElement.cpp:147:18
    #30 0x7f107f0c5e26 in AttemptToExecute /builds/worker/workspace/build/src/obj-firefox/dist/include/nsIScriptElement.h:226:18
    #31 0x7f107f0c5e26 in nsHtml5TreeOpExecutor::RunScript(nsIContent*) /builds/worker/workspace/build/src/parser/html/nsHtml5TreeOpExecutor.cpp:735
    #32 0x7f107f0bf0cd in nsHtml5TreeOpExecutor::RunFlushLoop() /builds/worker/workspace/build/src/parser/html/nsHtml5TreeOpExecutor.cpp:539:7
    #33 0x7f107f0cb23b in nsHtml5ExecutorFlusher::Run() /builds/worker/workspace/build/src/parser/html/nsHtml5StreamParser.cpp:130:20
    #34 0x7f107cf4ab04 in mozilla::SchedulerGroup::Runnable::Run() /builds/worker/workspace/build/src/xpcom/threads/SchedulerGroup.cpp:396:25
    #35 0x7f107cf713ee in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1033:14
    #36 0x7f107cf8cd50 in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:508:10

previously allocated by thread T0 (file:// Content) here:
    #0 0x4c31d3 in malloc /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:88:3
    #1 0x4f3c9d in moz_xmalloc /builds/worker/workspace/build/src/memory/mozalloc/mozalloc.cpp:70:17
    #2 0x7f107eca9eea in operator new /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/mozalloc.h:159:12
    #3 0x7f107eca9eea in mozilla::PeerConnectionImpl::Initialize(mozilla::dom::PeerConnectionObserver&, nsGlobalWindowInner*, mozilla::PeerConnectionConfiguration const&, nsISupports*) /builds/worker/workspace/build/src/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:655
    #4 0x7f107ecaf3da in mozilla::PeerConnectionImpl::Initialize(mozilla::dom::PeerConnectionObserver&, nsGlobalWindowInner&, mozilla::dom::RTCConfiguration const&, nsISupports*, mozilla::ErrorResult&) /builds/worker/workspace/build/src/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:735:9
    #5 0x7f1080b3fae7 in mozilla::dom::PeerConnectionImplBinding::initialize(JSContext*, JS::Handle<JSObject*>, mozilla::PeerConnectionImpl*, JSJitMethodCallArgs const&) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/PeerConnectionImplBinding.cpp:83:9
    #6 0x7f108220c5e7 in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:3042:13
    #7 0x7f1088dc4464 in CallJSNative /builds/worker/workspace/build/src/js/src/jscntxtinlines.h:291:15
    #8 0x7f1088dc4464 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:473
    #9 0x7f1088daa548 in CallFromStack /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:528:12
    #10 0x7f1088daa548 in Interpret(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3096
    #11 0x7f1088d96cb0 in js::RunScript(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:423:12
    #12 0x7f1088dc499c in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:495:15
    #13 0x7f1088dc54c2 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:541:10
    #14 0x7f10898c119c in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/jsapi.cpp:2995:12
    #15 0x7f1080d0cab7 in mozilla::dom::RTCPeerConnectionJSImpl::__Init(mozilla::dom::RTCConfiguration const&, mozilla::dom::Optional<JS::Handle<JSObject*> > const&, mozilla::ErrorResult&, JSCompartment*) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/RTCPeerConnectionBinding.cpp:8588:8
    #16 0x7f1080d27976 in mozilla::dom::RTCPeerConnection::Constructor(mozilla::dom::GlobalObject const&, JSContext*, mozilla::dom::RTCConfiguration const&, mozilla::dom::Optional<JS::Handle<JSObject*> > const&, mozilla::ErrorResult&, JS::Handle<JSObject*>) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/RTCPeerConnectionBinding.cpp:9989:16
    #17 0x7f1080e4f772 in mozilla::dom::RTCPeerConnectionBinding::_constructor(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/RTCPeerConnectionBinding.cpp:5755:63
    #18 0x7f1088dc5b0a in CallJSNative /builds/worker/workspace/build/src/js/src/jscntxtinlines.h:291:15
    #19 0x7f1088dc5b0a in CallJSNativeConstructor /builds/worker/workspace/build/src/js/src/jscntxtinlines.h:324
    #20 0x7f1088dc5b0a in InternalConstruct(JSContext*, js::AnyConstructArgs const&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:580
    #21 0x7f1088daa3c2 in Interpret(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3088:18
    #22 0x7f1088d96cb0 in js::RunScript(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:423:12
    #23 0x7f1088dc7401 in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:706:15
    #24 0x7f1088dc7b9f in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:738:12
    #25 0x7f10898d6646 in ExecuteScript(JSContext*, JS::AutoObjectVector&, JS::Handle<JSScript*>, JS::Value*) /builds/worker/workspace/build/src/js/src/jsapi.cpp:4678:12
    #26 0x7f10802efc66 in nsJSUtils::ExecutionContext::CompileAndExec(JS::CompileOptions&, JS::SourceBufferHolder&, JS::MutableHandle<JSScript*>) /builds/worker/workspace/build/src/dom/base/nsJSUtils.cpp:266:8
    #27 0x7f1084217f21 in mozilla::dom::ScriptLoader::EvaluateScript(mozilla::dom::ScriptLoadRequest*) /builds/worker/workspace/build/src/dom/script/ScriptLoader.cpp:2287:25
    #28 0x7f10842133c9 in mozilla::dom::ScriptLoader::ProcessRequest(mozilla::dom::ScriptLoadRequest*) /builds/worker/workspace/build/src/dom/script/ScriptLoader.cpp:1929:10
    #29 0x7f10841f6315 in mozilla::dom::ScriptLoader::ProcessScriptElement(nsIScriptElement*) /builds/worker/workspace/build/src/dom/script/ScriptLoader.cpp:1627:10
    #30 0x7f10841f22e7 in mozilla::dom::ScriptElement::MaybeProcessScript() /builds/worker/workspace/build/src/dom/script/ScriptElement.cpp:147:18
    #31 0x7f107f0c5e26 in AttemptToExecute /builds/worker/workspace/build/src/obj-firefox/dist/include/nsIScriptElement.h:226:18
    #32 0x7f107f0c5e26 in nsHtml5TreeOpExecutor::RunScript(nsIContent*) /builds/worker/workspace/build/src/parser/html/nsHtml5TreeOpExecutor.cpp:735
    #33 0x7f107f0bf0cd in nsHtml5TreeOpExecutor::RunFlushLoop() /builds/worker/workspace/build/src/parser/html/nsHtml5TreeOpExecutor.cpp:539:7
    #34 0x7f107f0cb23b in nsHtml5ExecutorFlusher::Run() /builds/worker/workspace/build/src/parser/html/nsHtml5StreamParser.cpp:130:20
    #35 0x7f107cf4ab04 in mozilla::SchedulerGroup::Runnable::Run() /builds/worker/workspace/build/src/xpcom/threads/SchedulerGroup.cpp:396:25

SUMMARY: AddressSanitizer: heap-use-after-free /builds/worker/workspace/build/src/obj-firefox/dist/include/mtransport/sigslot.h:318:13 in ~lock_block
Shadow bytes around the buggy address:
  0x0c2a80055b70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c2a80055b80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c2a80055b90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c2a80055ba0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c2a80055bb0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x0c2a80055bc0: fd fd fd fd fd fd fd fd fd[fd]fd fd fd fd fd fd
  0x0c2a80055bd0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c2a80055be0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c2a80055bf0: fd fd fd fd fd fd fd fd fd fa fa fa fa fa fa fa
  0x0c2a80055c00: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c2a80055c10: 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
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  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
==4360==ABORTING
Flags: in-testsuite?
(Reporter)

Comment 1

a year ago
Posted file prefs.js
Group: core-security → media-core-security
Keywords: csectype-uaf, sec-high
I'm not sure I understand the stack trace which causes the freeing.

Maybe the assumption here https://dxr.mozilla.org/mozilla-central/rev/947b1418ebb23e1faf716cec11237c6861c5d061/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp#1255
no longer holds true?
Michael, can you have a look at this?
Rank: 9
Flags: needinfo?(mfroman)
Priority: -- → P1
(Assignee)

Comment 4

a year ago
I'll take a look.
(Assignee)

Updated

a year ago
Assignee: nobody → mfroman
Flags: needinfo?(mfroman)
(Assignee)

Comment 5

a year ago
The first error I see when running trigger.html with an asan build is:
Assertion failure: sIPCConnection.IsValid(), at /builds/worker/workspace/build/src/dom/media/CubebUtils.cpp:458

Let me see if this happens on a local debug build.
(Assignee)

Comment 6

a year ago
I have a debug build from c9bec96cc789 on m-c that also asserts when running the trigger.html attachment. For reference:
https://dxr.mozilla.org/mozilla-central/source/dom/media/CubebUtils.cpp#458
(Assignee)

Comment 7

a year ago
Paul, can you take a look at this assertion?
Flags: needinfo?(padenot)
(Assignee)

Comment 8

a year ago
For clarification, I was running this on my linux machine.  I do not get the assert when running with --disable-e10s on my local debug build.
You need to copy the file `pref.js` to the root of the profile directory. Also it appears you have an issue with audio remoting. Running without e10s is a way to disable remoting to sidestep this (probably unrelated) issue.
Flags: needinfo?(padenot)
(Assignee)

Comment 10

a year ago
We're spinning the event loop here[1] which causes main thread execution to resume before we've exited PeerConnectionImpl::IceGatheringStateChange.  In this case, execution resuming means resuming teardown on PeerConnectionMedia and we free the PeerConnectionMedia that calls PeerConnectionImpl::IceGatheringStateChange here via a signal[2].  The cause of the UAF is release the sigslot lockblock which no longer exists.

Replacing the RUN_ON_THREAD (which immediately executes the PeerConnectionObserver::OnStateChange because we're already on mThread) with a mThread->Dispatch fixes this crash.

Randell, does this fall under the queue jumping bug?  Should we fix this single issue here, possibly under a bug for a broken PeerConnectionMedia teardown ordering?

[1] https://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp#3257
[2] https://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp?q=%2Bfunction%3Amozilla%3A%3APeerConnectionMedia%3A%3ASelfDestruct%28%29&redirect_type=single#1359
Flags: needinfo?(rjesup)
Yes, this is the queue-jumping bug I think.  I'm open with how to land a fix - Nils?
Flags: needinfo?(rjesup) → needinfo?(drno)
Lets land the fix for this issue here, since I need to get back to the queue jumping thing (but I don't see that happening soon).
Flags: needinfo?(drno)
(Assignee)

Comment 13

a year ago
The queue jumping issue is causing a tear-down ordering problem that results in signals getting dispatched to an already deleted instance.
Attachment #8941483 - Flags: review?(docfaraday)
Comment on attachment 8941483 [details] [diff] [review]
Don't allow queue-jumping for gathering state change events.

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

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +3254,4 @@
>      return;
>    }
>    WrappableJSErrorResult rv;
> +  // use Dispatch rather than RUN_ON_THREAD (which immediately executes

Let's avoid this comment, to make it harder for someone to figure out the security hole we're plugging.
Attachment #8941483 - Flags: review?(docfaraday) → review+
(Assignee)

Updated

a year ago
Attachment #8941483 - Attachment description: fixes a queue jumping issue that causes a tear-down ordering bug → Don't allow queue-jumping for gathering state change events.
(Assignee)

Comment 15

a year ago
Attachment #8941483 - Attachment is obsolete: true
(Assignee)

Comment 16

a year ago
Remove comment per bwc.
Attachment #8941488 - Attachment is obsolete: true
(Assignee)

Comment 17

a year ago
Comment on attachment 8941489 [details] [diff] [review]
Don't allow queue-jumping for gathering state change events.

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

carry forward r+ from bwc after removing comment.
Attachment #8941489 - Flags: review+
(Assignee)

Comment 18

a year ago
Comment on attachment 8941489 [details] [diff] [review]
Don't allow queue-jumping for gathering state change events.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Not very easily since there is an open queue-jumping bug, and knowning that doesn't reveal what problem the queue jumping caused.

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.

If not all supported branches, which bug introduced the flaw?
The queue-jumping behavior was introduced back in 2012.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Not yet, but they should be easy to create because it is a very small change - one place in one file.

How likely is this patch to cause regressions; how much testing does it need?
Very unlikely.
Attachment #8941489 - Flags: sec-approval?
I'd like to give this sec-approval+ and then beta+ to make the last beta but I need Gerry's approval to do so.

If the patch applies, this should be nominated for Beta. 

Gerry, this is a very simple patch and it affects things forever. We should consider it for ESR52 as well.
status-firefox57: --- → wontfix
status-firefox58: --- → affected
status-firefox-esr52: --- → affected
tracking-firefox58: --- → +
tracking-firefox59: --- → +
tracking-firefox-esr52: --- → 58+
Flags: needinfo?(gchang)
Al,
Yes, I also want to include this in beta 16.
Flags: needinfo?(gchang)
Comment on attachment 8941489 [details] [diff] [review]
Don't allow queue-jumping for gathering state change events.

sec-approval+.
Let's get a beta nomination too.
Attachment #8941489 - Flags: sec-approval? → sec-approval+
(Assignee)

Comment 22

a year ago
Comment on attachment 8941489 [details] [diff] [review]
Don't allow queue-jumping for gathering state change events.

Approval Request Comment
[Feature/Bug causing the regression]:
The queue-jumping behavior was introduced back in 2012 by bug 824851.

[User impact if declined]:
In this case, a UAF during PeerConnection teardown is fairly easy to provoke over a small number of attempts.

[Is this code covered by automated tests?]:
Many mochitests exercise this code, but none are specifically looking for timing-related teardown issue.

[Has the fix been verified in Nightly?]:
Not yet.

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

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

[Is the change risky?]:
No.

[Why is the change risky/not risky?]:
It is one small change in one file that ensures dispatch to a thread vs queue-jumping if already on the requested thread.

[String changes made/needed]:
None.
Attachment #8941489 - Flags: approval-mozilla-beta?
Hi Al, Dan, should we consider uplifting this to ESR52.6?
Flags: needinfo?(dveditz)
Flags: needinfo?(abillings)
I'd like to see this in ERS52.
Flags: needinfo?(abillings)
Comment on attachment 8941489 [details] [diff] [review]
Don't allow queue-jumping for gathering state change events.

This was approved for 58 (in comments) but didn't get checked into the last Beta. This needs to be in RC1.
Flags: needinfo?(rkothari)
Flags: needinfo?(gchang)
Attachment #8941489 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: needinfo?(gchang)
Attachment #8941489 - Flags: approval-mozilla-esr52?
https://hg.mozilla.org/mozilla-central/rev/2fa8c2b1c991
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox59: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Comment on attachment 8941489 [details] [diff] [review]
Don't allow queue-jumping for gathering state change events.

Sec-high, ESR52+
Flags: needinfo?(rkothari)
Attachment #8941489 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Whiteboard: [adv-main58+][adv-esr52.6+]
Flags: needinfo?(dveditz)
Flags: qe-verify-
Whiteboard: [adv-main58+][adv-esr52.6+] → [adv-main58+][adv-esr52.6+][post-critsmash-triage]
Group: media-core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.