1279146 - (CVE-2016-5258) WebRTC - Use After Free in socket thread

Reporter

Description

•

8 years ago

Attached file UAF_socket_thread.html — Details

Steps to reproduce:

1.Open repro UAF_socket_thread.html in Firefox browser.
2.Firefox crashes at corrupted address in nr_ice_peer_ctx_find_component() in socket thread.


Firefox version: 50.0a1 (2016-06-09)

Exception thrown: read access violation.
pstr was 0x2FE07494.


>	xul.dll!nr_ice_peer_ctx_find_component(nr_ice_peer_ctx_ * pctx, nr_ice_media_stream_ * str, int component_id, nr_ice_component_ * * compp) Line 717	C
 	xul.dll!nr_ice_media_stream_send(nr_ice_peer_ctx_ * pctx, nr_ice_media_stream_ * str, int component, unsigned char * data, int len) Line 734	C
 	xul.dll!mozilla::NrIceMediaStream::SendPacket(int component_id, const unsigned char * data, unsigned int len) Line 568	C++
 	xul.dll!mozilla::TransportLayerIce::SendPacket(const unsigned char * data, unsigned int len) Line 172	C++
 	xul.dll!mozilla::TransportLayerNSPRAdapter::Write(const void * buf, int length) Line 114	C++
 	xul.dll!mozilla::TransportLayerWrite(PRFileDesc * f, const void * buf, int length) Line 142	C++
 	xul.dll!mozilla::TransportLayerSend(PRFileDesc * f, const void * buf, int amount, int flags, unsigned int to) Line 234	C++
 	nss3.dll!ssl_DefSend(sslSocketStr * ss, const unsigned char * buf, int len, int flags) Line 103	C
 	nss3.dll!ssl3_SendRecord(sslSocketStr * ss, ssl3CipherSpec * cwSpec, SSL3ContentType type, const unsigned char * pIn, int nIn, int flags) Line 3234	C
 	nss3.dll!SSL3_SendAlert(sslSocketStr * ss, SSL3AlertLevel level, SSL3AlertDescription desc) Line 3528	C
 	nss3.dll!ssl_SecureClose(sslSocketStr * ss) Line 777	C
 	nss3.dll!ssl_Close(PRFileDesc * fd) Line 2507	C
 	nss3.dll!PR_Close(PRFileDesc * fd) Line 104	C
 	xul.dll!mozilla::TransportLayerDtls::~TransportLayerDtls() Line 374	C++
 	[External Code]	
 	xul.dll!mozilla::TransportFlow::ClearLayers(std::deque<mozilla::TransportLayer *,std::allocator<mozilla::TransportLayer *> > * layers) Line 58	C++
 	xul.dll!mozilla::TransportFlow::DestroyFinal(nsAutoPtr<std::deque<mozilla::TransportLayer *,std::allocator<mozilla::TransportLayer *> > > layers) Line 45	C++
 	xul.dll!mozilla::runnable_args_func<void (__cdecl*)(nsAutoPtr<std::deque<mozilla::TransportLayer *,std::allocator<mozilla::TransportLayer *> > >),nsAutoPtr<std::deque<mozilla::TransportLayer *,std::allocator<mozilla::TransportLayer *> > > >::Run() Line 118	C++
 	xul.dll!mozilla::TransportFlow::~TransportFlow() Line 39	C++
 	xul.dll!mozilla::TransportFlow::Release() Line 19	C++
 	xul.dll!nsProxyReleaseEvent<mozilla::TransportFlow>::Run() Line 33	C++
 	xul.dll!nsThread::ProcessNextEvent(bool aMayWait, bool * aResult) Line 1029	C++
 	xul.dll!NS_ProcessNextEvent(nsIThread * aThread, bool aMayWait) Line 290	C++
 	xul.dll!mozilla::net::nsSocketTransportService::Run() Line 914	C++
 	xul.dll!nsThread::ProcessNextEvent(bool aMayWait, bool * aResult) Line 1029	C++
 	xul.dll!NS_ProcessNextEvent(nsIThread * aThread, bool aMayWait) Line 290	C++
 	xul.dll!mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate * aDelegate) Line 354	C++
 	xul.dll!MessageLoop::RunHandler() Line 229	C++
 	xul.dll!MessageLoop::Run() Line 209	C++
 	xul.dll!nsThread::ThreadFunc(void * aArg) Line 468	C++
 	nss3.dll!_PR_NativeRunThread(void * arg) Line 419	C
 	nss3.dll!pr_root(void * arg) Line 95	C
 	[External Code]	
 	[Frames below may be incorrect and/or missing, no symbols loaded for ucrtbase.dll]

Looben Yang

Reporter

Comment 1

•

8 years ago

In official Linux Asan build, I got:


Firefox version: 50.0a1 (2016-06-08)

=================================================================
==8502==ERROR: AddressSanitizer: heap-use-after-free on address 0x60e0005a5cf4 at pc 0x7f298b7dd80c bp 0x7f2976b3f200 sp 0x7f2976b3f1f8
READ of size 8 at 0x60e0005a5cf4 thread T6 (Socket Thread)
    #0 0x7f298b7dd80b in nr_ice_peer_ctx_find_component /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/media/mtransport/third_party/nICEr/src/ice/ice_peer_ctx.c:715
    #1 0x7f298b7d63cb in nr_ice_media_stream_send /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/media/mtransport/third_party/nICEr/src/ice/ice_media_stream.c:734
    #2 0x7f29859b3a5a in mozilla::NrIceMediaStream::SendPacket(int, unsigned char const*, unsigned long) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/media/mtransport/nricemediastream.cpp:568
    #3 0x7f29859f99cc in mozilla::TransportLayerIce::SendPacket(unsigned char const*, unsigned long) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/media/mtransport/transportlayerice.cpp:172
    #4 0x7f29859d7277 in mozilla::TransportLayerNSPRAdapter::Write(void const*, int) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/media/mtransport/transportlayerdtls.cpp:112
    #5 0x7f2999298a84 in ssl_DefSend /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/security/nss/lib/ssl/ssldef.c:103
    #6 0x7f29992640fc in ssl3_SendRecord /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/security/nss/lib/ssl/ssl3con.c:3234
    #7 0x7f2999265eb6 in SSL3_SendAlert /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/security/nss/lib/ssl/ssl3con.c:3528
    #8 0x7f29992a9a20 in ssl_SecureClose /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/security/nss/lib/ssl/sslsecur.c:777
    #9 0x7f29859d7b80 in TypeSpecificDelete<PRFileDesc> /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/dist/include/ScopedNSSTypes.h:80
    #10 0x7f29859d7b80 in release /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/dist/include/mozilla/Scoped.h:246
    #11 0x7f29859d7b80 in ~nsCOMPtr_base /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/dist/include/mozilla/Scoped.h:95
    #12 0x7f29859d7b80 in mozilla::TransportLayerDtls::~TransportLayerDtls() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/media/mtransport/transportlayerdtls.cpp:374
    #13 0x7f29859d822d in mozilla::TransportLayerDtls::~TransportLayerDtls() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/media/mtransport/transportlayerdtls.cpp:370
    #14 0x7f29859d03f9 in get /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/media/mtransport/transportflow.cpp:57
    #15 0x7f29859d03f9 in mozilla::TransportFlow::DestroyFinal(nsAutoPtr<std::deque<mozilla::TransportLayer*, std::allocator<mozilla::TransportLayer*> > >) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/media/mtransport/transportflow.cpp:45
    #16 0x7f29859d4a36 in apply<void (*)(nsAutoPtr<std::deque<mozilla::TransportLayer *, std::allocator<mozilla::TransportLayer *> > >), nsAutoPtr<std::deque<mozilla::TransportLayer *, std::allocator<mozilla::TransportLayer *> > > , 0> /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/media/mtransport/runnable_utils.h:79
    #17 0x7f29859d4a36 in mozilla::runnable_args_func<void (*)(nsAutoPtr<std::deque<mozilla::TransportLayer*, std::allocator<mozilla::TransportLayer*> > >), nsAutoPtr<std::deque<mozilla::TransportLayer*, std::allocator<mozilla::TransportLayer*> > > >::Run() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/media/mtransport/runnable_utils.h:118
    #18 0x7f29859cfc9a in RunOnThreadInternal /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/media/mtransport/runnable_utils.h:50
    #19 0x7f29859cfc9a in RUN_ON_THREAD /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/media/mtransport/runnable_utils.h:214
    #20 0x7f29859cfc9a in mozilla::TransportFlow::~TransportFlow() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/media/mtransport/transportflow.cpp:39
    #21 0x7f29859d05ad in mozilla::TransportFlow::~TransportFlow() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/media/mtransport/transportflow.cpp:23
    #22 0x7f29859cf912 in mozilla::TransportFlow::Release() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/media/mtransport/transportflow.cpp:19
    #23 0x7f29849024aa in nsProxyReleaseEvent<mozilla::TransportFlow>::Run() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/dist/include/nsProxyRelease.h:33
    #24 0x7f2983c2a5c4 in nsThread::ProcessNextEvent(bool, bool*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/xpcom/threads/nsThread.cpp:1029
    #25 0x7f2983ca54aa in NS_ProcessNextEvent(nsIThread*, bool) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/xpcom/glue/nsThreadUtils.cpp:290
    #26 0x7f2983e7600c in mozilla::net::nsSocketTransportService::Run() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/netwerk/base/nsSocketTransportService2.cpp:911
    #27 0x7f2983e7896c in non-virtual thunk to mozilla::net::nsSocketTransportService::Run() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/netwerk/base/Unified_cpp_netwerk_base3.cpp:980
    #28 0x7f2983c2a5c4 in nsThread::ProcessNextEvent(bool, bool*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/xpcom/threads/nsThread.cpp:1029
    #29 0x7f2983ca54aa in NS_ProcessNextEvent(nsIThread*, bool) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/xpcom/glue/nsThreadUtils.cpp:290
    #30 0x7f29849c98ae in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/glue/MessagePump.cpp:354
    #31 0x7f298493710c in RunInternal /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/chromium/src/base/message_loop.cc:235
    #32 0x7f298493710c in RunHandler /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/chromium/src/base/message_loop.cc:228
    #33 0x7f298493710c in MessageLoop::Run() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/chromium/src/base/message_loop.cc:208
    #34 0x7f2983c25afc in nsThread::ThreadFunc(void*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/xpcom/threads/nsThread.cpp:466
    #35 0x7f299a4943ef in _pt_root /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/nsprpub/pr/src/pthreads/ptthread.c:216
    #36 0x7f299d9b6181 in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x8181)
    #37 0x7f299cab747c (/lib/x86_64-linux-gnu/libc.so.6+0xfa47c)

0x60e0005a5cf4 is located 84 bytes inside of 152-byte region [0x60e0005a5ca0,0x60e0005a5d38)
freed by thread T6 (Socket Thread) here:
    #0 0x4720a1 in __interceptor_free /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:64
    #1 0x7f298b7da5dc in nr_ice_peer_ctx_destroy_cb /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/media/mtransport/third_party/nICEr/src/ice/ice_peer_ctx.c:487
    #2 0x7f298599ea19 in nsAutoPtr /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/media/mtransport/runnable_utils.h:102
    #3 0x7f298599ea19 in mozilla::runnable_args_memfn<nsAutoPtr<mozilla::nrappkitScheduledCallback>, void (mozilla::nrappkitScheduledCallback::*)()>::Run() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/media/mtransport/runnable_utils.h:169
    #4 0x7f2983c2a5c4 in nsThread::ProcessNextEvent(bool, bool*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/xpcom/threads/nsThread.cpp:1029
    #5 0x7f2983ca54aa in NS_ProcessNextEvent(nsIThread*, bool) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/xpcom/glue/nsThreadUtils.cpp:290
    #6 0x7f2983e7600c in mozilla::net::nsSocketTransportService::Run() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/netwerk/base/nsSocketTransportService2.cpp:911
    #7 0x7f2983e7896c in non-virtual thunk to mozilla::net::nsSocketTransportService::Run() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/netwerk/base/Unified_cpp_netwerk_base3.cpp:980
    #8 0x7f2983c2a5c4 in nsThread::ProcessNextEvent(bool, bool*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/xpcom/threads/nsThread.cpp:1029
    #9 0x7f2983ca54aa in NS_ProcessNextEvent(nsIThread*, bool) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/xpcom/glue/nsThreadUtils.cpp:290
    #10 0x7f29849c98ae in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/glue/MessagePump.cpp:354
    #11 0x7f298493710c in RunInternal /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/chromium/src/base/message_loop.cc:235
    #12 0x7f298493710c in RunHandler /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/chromium/src/base/message_loop.cc:228
    #13 0x7f298493710c in MessageLoop::Run() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/chromium/src/base/message_loop.cc:208
    #14 0x7f2983c25afc in nsThread::ThreadFunc(void*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/xpcom/threads/nsThread.cpp:466
    #15 0x7f299a4943ef in _pt_root /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/nsprpub/pr/src/pthreads/ptthread.c:216
    #16 0x7f299d9b6181 in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x8181)

previously allocated by thread T0 here:
    #0 0x4722a1 in malloc /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:74
    #1 0x7f298b80b3c6 in r_malloc /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/media/mtransport/third_party/nrappkit/src/util/libekr/r_memory.c:76
    #2 0x7f298b80b3c6 in r_calloc /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/media/mtransport/third_party/nrappkit/src/util/libekr/r_memory.c:101
    #3 0x7f298b7d9eeb in nr_ice_peer_ctx_create /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/media/mtransport/third_party/nICEr/src/ice/ice_peer_ctx.c:58
    #4 0x7f29859a6219 in mozilla::NrIceCtx::Initialize(bool, std::string const&, std::string const&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/media/mtransport/nricectx.cpp:631
    #5 0x7f29859a38f8 in mozilla::NrIceCtx::Initialize(bool) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/media/mtransport/nricectx.cpp:518
    #6 0x7f29859ace26 in mozilla::NrIceCtxHandler::Create(std::string const&, bool, bool, bool, bool, bool, mozilla::NrIceCtx::Policy) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/media/mtransport/nricectxhandler.cpp:44
    #7 0x7f29858c597b in mozilla::PeerConnectionMedia::Init(std::vector<mozilla::NrIceStunServer, std::allocator<mozilla::NrIceStunServer> > const&, std::vector<mozilla::NrIceTurnServer, std::allocator<mozilla::NrIceTurnServer> > const&, mozilla::NrIceCtx::Policy) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:368
    #8 0x7f2985881b2e in mozilla::PeerConnectionImpl::Initialize(mozilla::dom::PeerConnectionObserver&, nsGlobalWindow*, mozilla::PeerConnectionConfiguration const&, nsISupports*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:695
    #9 0x7f2985886b88 in mozilla::PeerConnectionImpl::Initialize(mozilla::dom::PeerConnectionObserver&, nsGlobalWindow&, mozilla::dom::RTCConfiguration const&, nsISupports*, mozilla::ErrorResult&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:775
    #10 0x7f29873bb9ca in mozilla::dom::PeerConnectionImplBinding::initialize(JSContext*, JS::Handle<JSObject*>, mozilla::PeerConnectionImpl*, JSJitMethodCallArgs const&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/dom/bindings/PeerConnectionImplBinding.cpp:80
    #11 0x7f29886deaf5 in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/dom/bindings/BindingUtils.cpp:2784
    #12 0x7f298d7bbab5 in EnterBaseline(JSContext*, js::jit::EnterJitData&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/js/src/jit/BaselineJIT.cpp:155
    #13 0x7f298d7bb202 in js::jit::EnterBaselineMethod(JSContext*, js::RunState&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/js/src/jit/BaselineJIT.cpp:194
    #14 0x7f298e3166bd in js::RunScript(JSContext*, js::RunState&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/js/src/vm/Interpreter.cpp:388
    #15 0x7f298e346f7b in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/js/src/vm/Interpreter.cpp:470
    #16 0x7f298e2f9d71 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/js/src/vm/Interpreter.cpp:516
    #17 0x7f298deec2d3 in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/js/src/jsapi.cpp:2895
    #18 0x7f29874c54c6 in mozilla::dom::RTCPeerConnectionJSImpl::__Init(mozilla::dom::RTCConfiguration const&, mozilla::dom::Optional<JS::Handle<JSObject*> > const&, mozilla::ErrorResult&, JSCompartment*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/dom/bindings/RTCPeerConnectionBinding.cpp:7755
    #19 0x7f29874d66ac in mozilla::dom::RTCPeerConnection::Constructor(mozilla::dom::GlobalObject const&, JSContext*, mozilla::dom::RTCConfiguration const&, mozilla::dom::Optional<JS::Handle<JSObject*> > const&, mozilla::ErrorResult&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/dom/bindings/RTCPeerConnectionBinding.cpp:8886
    #20 0x7f298760e73d in mozilla::dom::RTCPeerConnectionBinding::_constructor(JSContext*, unsigned int, JS::Value*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/dom/bindings/RTCPeerConnectionBinding.cpp:5001
    #21 0x7f298e34774c in CallJSNative /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/js/src/jscntxtinlines.h:235
    #22 0x7f298e34774c in CallJSNativeConstructor /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/js/src/jscntxtinlines.h:268
    #23 0x7f298e34774c in InternalConstruct(JSContext*, js::AnyConstructArgs const&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/js/src/vm/Interpreter.cpp:553
    #24 0x7f298e335454 in ConstructFromStack /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/js/src/vm/Interpreter.cpp:580
    #25 0x7f298e335454 in Interpret(JSContext*, js::RunState&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/js/src/vm/Interpreter.cpp:2865
    #26 0x7f298e31689e in js::RunScript(JSContext*, js::RunState&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/js/src/vm/Interpreter.cpp:398
    #27 0x7f298e3491cb in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/js/src/vm/Interpreter.cpp:676
    #28 0x7f298e34984d in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/js/src/vm/Interpreter.cpp:708
    #29 0x7f298defdad7 in Evaluate(JSContext*, JS::Handle<JSObject*>, JS::Handle<js::StaticScope*>, JS::ReadOnlyCompileOptions const&, JS::SourceBufferHolder&, JS::MutableHandle<JS::Value>) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/js/src/jsapi.cpp:4461
    #30 0x7f298defe5f1 in Evaluate /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/js/src/jsapi.cpp:4488
    #31 0x7f298defe5f1 in JS::Evaluate(JSContext*, JS::AutoVectorRooter<JSObject*>&, JS::ReadOnlyCompileOptions const&, JS::SourceBufferHolder&, JS::MutableHandle<JS::Value>) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/js/src/jsapi.cpp:4549

Thread T6 (Socket Thread) created by T0 here:
    #0 0x45eb15 in __interceptor_pthread_create /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_interceptors.cc:175
    #1 0x7f299a490b40 in _PR_CreateThread /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/nsprpub/pr/src/pthreads/ptthread.c:457
    #2 0x7f299a4906aa in PR_CreateThread /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/nsprpub/pr/src/pthreads/ptthread.c:548
    #3 0x7f2983c2734d in nsThread::Init() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/xpcom/threads/nsThread.cpp:600
    #4 0x7f2983c2e6de in nsThreadManager::NewThread(unsigned int, unsigned int, nsIThread**) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/xpcom/threads/nsThreadManager.cpp:253
    #5 0x7f2983ca4538 in NS_NewThread(nsIThread**, nsIRunnable*, unsigned int) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/xpcom/glue/nsThreadUtils.cpp:64
    #6 0x7f2983e73395 in nsresult NS_NewNamedThread<14ul>(char const (&) [14ul], nsIThread**, nsIRunnable*, unsigned int) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/dist/include/nsThreadUtils.h:78
    #7 0x7f2983e72aec in mozilla::net::nsSocketTransportService::Init() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/netwerk/base/nsSocketTransportService2.cpp:523:19
    #8 0x7f298490b3dc in nsSocketTransportServiceConstructor(nsISupports*, nsID const&, void**) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/netwerk/build/nsNetModule.cpp:80
    #9 0x7f2983bfca95 in nsComponentManagerImpl::CreateInstanceByContractID(char const*, nsISupports*, nsID const&, void**) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/xpcom/components/nsComponentManager.cpp:1203
    #10 0x7f2983bf3704 in nsComponentManagerImpl::GetServiceByContractID(char const*, nsID const&, void**) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/xpcom/components/nsComponentManager.cpp:1559
    #11 0x7f2983c95056 in CallGetService /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/xpcom/glue/nsComponentManagerUtils.cpp:67
    #12 0x7f2983c95056 in nsGetServiceByContractIDWithError::operator()(nsID const&, void**) const /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/xpcom/glue/nsComponentManagerUtils.cpp:292
    #13 0x7f2983c8a57e in nsCOMPtr_base::assign_from_gs_contractid_with_error(nsGetServiceByContractIDWithError const&, nsID const&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/xpcom/glue/nsCOMPtr.cpp:114
    #14 0x7f2983dd3bfa in operator= /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/dist/include/nsCOMPtr.h:645
    #15 0x7f2983dd3bfa in InitializeSocketTransportService /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/netwerk/base/nsIOService.cpp:297
    #16 0x7f2983dd3bfa in mozilla::net::nsIOService::SetOffline(bool) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/netwerk/base/nsIOService.cpp:1092
    #17 0x7f2983dd295e in mozilla::net::nsIOService::Init() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/netwerk/base/nsIOService.cpp:264
    #18 0x7f2983dd52a3 in mozilla::net::nsIOService::GetInstance() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/netwerk/base/nsIOService.cpp:349
    #19 0x7f298490b125 in nsIOServiceConstructor(nsISupports*, nsID const&, void**) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/netwerk/build/nsNetModule.cpp:62
    #20 0x7f2983bfca95 in nsComponentManagerImpl::CreateInstanceByContractID(char const*, nsISupports*, nsID const&, void**) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/xpcom/components/nsComponentManager.cpp:1203
    #21 0x7f2983bf3704 in nsComponentManagerImpl::GetServiceByContractID(char const*, nsID const&, void**) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/xpcom/components/nsComponentManager.cpp:1559
    #22 0x7f2983c94fc1 in CallGetService /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/xpcom/glue/nsComponentManagerUtils.cpp:67
    #23 0x7f2983c94fc1 in nsGetServiceByContractID::operator()(nsID const&, void**) const /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/xpcom/glue/nsComponentManagerUtils.cpp:280
    #24 0x7f2983c78e82 in assign_from_gs_contractid /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/xpcom/glue/nsCOMPtr.cpp:103
    #25 0x7f2983c78e82 in nsCOMPtr /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/dist/include/nsCOMPtr.h:540
    #26 0x7f2983c78e82 in mozilla::services::GetIOService() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/dist/include/mozilla/ServiceList.h:20
    #27 0x7f2983e05f25 in do_GetIOService(nsresult*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/netwerk/base/nsNetUtilInlines.h:46
    #28 0x7f2983e06533 in net_EnsureIOService /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/netwerk/base/nsNetUtilInlines.h:86
    #29 0x7f2983e06533 in NS_NewURI(nsIURI**, nsACString_internal const&, char const*, nsIURI*, nsIIOService*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/netwerk/base/nsNetUtilInlines.h:113
    #30 0x7f2983c62553 in GetManifestURI /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/chrome/nsChromeRegistryChrome.cpp:673
    #31 0x7f2983c62553 in nsChromeRegistry::ManifestProcessingContext::ResolveURI(char const*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/chrome/nsChromeRegistryChrome.cpp:690
    #32 0x7f2983c638e7 in nsChromeRegistryChrome::ManifestLocale(nsChromeRegistry::ManifestProcessingContext&, int, char* const*, int) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/chrome/nsChromeRegistryChrome.cpp:771
    #33 0x7f2983c07078 in ParseManifest(NSLocationType, mozilla::FileLocation&, char*, bool, bool) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/xpcom/components/ManifestParser.cpp:798
    #34 0x7f2983bf75f6 in DoRegisterManifest /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/xpcom/components/nsComponentManager.cpp:625
    #35 0x7f2983bf75f6 in nsComponentManagerImpl::RegisterManifest(NSLocationType, mozilla::FileLocation&, bool) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/xpcom/components/nsComponentManager.cpp:638
    #36 0x7f2983bf7968 in nsComponentManagerImpl::ManifestManifest(nsComponentManagerImpl::ManifestProcessingContext&, int, char* const*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/xpcom/components/nsComponentManager.cpp:647
    #37 0x7f2983c06d40 in ParseManifest(NSLocationType, mozilla::FileLocation&, char*, bool, bool) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/xpcom/components/ManifestParser.cpp:807
    #38 0x7f2983bf75f6 in DoRegisterManifest /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/xpcom/components/nsComponentManager.cpp:625
    #39 0x7f2983bf75f6 in nsComponentManagerImpl::RegisterManifest(NSLocationType, mozilla::FileLocation&, bool) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/xpcom/components/nsComponentManager.cpp:638
    #40 0x7f2983bf5340 in RereadChromeManifests /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/xpcom/components/nsComponentManager.cpp:833
    #41 0x7f2983bf5340 in nsComponentManagerImpl::Init() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/xpcom/components/nsComponentManager.cpp:428
    #42 0x7f2983c7d83d in NS_InitXPCOM2 /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/xpcom/build/XPCOMInit.cpp:714
    #43 0x7f298c2bfa58 in Initialize /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/toolkit/xre/nsAppRunner.cpp:1549
    #44 0x7f298c2bfa58 in XREMain::XRE_main(int, char**, nsXREAppData const*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/toolkit/xre/nsAppRunner.cpp:4491
    #45 0x7f298c2c09de in XRE_main /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/toolkit/xre/nsAppRunner.cpp:4603
    #46 0x48a9a7 in do_main /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/browser/app/nsBrowserApp.cpp:254
    #47 0x48a9a7 in main /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/browser/app/nsBrowserApp.cpp:427
    #48 0x7f299c9deec4 (/lib/x86_64-linux-gnu/libc.so.6+0x21ec4)

SUMMARY: AddressSanitizer: heap-use-after-free /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/media/mtransport/third_party/nICEr/src/ice/ice_peer_ctx.c:715 nr_ice_peer_ctx_find_component
Shadow bytes around the buggy address:
  0x0c1c800acb40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c1c800acb50: 00 00 00 00 fa fa fa fa fa fa fa fa 00 00 00 00
  0x0c1c800acb60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c1c800acb70: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
  0x0c1c800acb80: 00 00 00 00 00 00 00 00 00 00 00 00 fa fa fa fa
=>0x0c1c800acb90: fa fa fa fa fd fd fd fd fd fd fd fd fd fd[fd]fd
  0x0c1c800acba0: fd fd fd fd fd fd fd fa fa fa fa fa fa fa fa fa
  0x0c1c800acbb0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c1c800acbc0: 00 00 00 00 fa fa fa fa fa fa fa fa 00 00 00 00
  0x0c1c800acbd0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c1c800acbe0: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
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:      [==8502==ABORTING

Andrew McCreight [:mccr8]

Updated

•

8 years ago

Flags: needinfo?(rjesup)

Keywords: csectype-uaf, sec-critical

Randell Jesup [:jesup] (needinfo me)

Comment 2

•

8 years ago

Nils - do you have cycles to glance at this before London?

Group: media-core-security

Rank: 10

Flags: needinfo?(rjesup) → needinfo?(drno)

Priority: -- → P1

Andrew McCreight [:mccr8]

Updated

•

8 years ago

Group: core-security

Nils Ohlmeier [:drno]

Assignee

Comment 3

•

8 years ago

Looks like transport layer DTLS on shutdown still tries to send an DTLS packet. We probably need to add a check for existing flags which tell us to not use this any more.
But I'll only be able to take care of this in London as I'm about to leave for the airport.

Al Billings [:abillings - ex-MoCo]

Updated

•

8 years ago

Flags: sec-bounty?

Maire Reavy [:mreavy]

Comment 4

•

8 years ago

Thanks, Nils.  Looking at this in London works.

Assignee: nobody → drno

Nils Ohlmeier [:drno]

Assignee

Comment 5

•

8 years ago

So ice_peer_ctx is freeded only in the process of deleting an IceCtx, which should only happen on destroying a PeerConnection (which would fit through the reload of the page) or through an ICE restart (which I don't see happening in the test page).
The test page also calls SLD and SRD in a couple of unusual places. To really understand how this is exactly happening I would like to see a log file from before the crash. But this is currently not actually crashing for me on my Mac, so I'm planing to repro this on my Asan Linux machine and get a log file.

My current best guess is that this might actually be a problem we had in our code all along:
- if we call the TransportLayerDtls destructor after DTLS has finished (or before DTLS has even started) everything works fine because the DTLS does not want to send any TLS packets on shutdown
- but if we call the TransportLayerDtls destructor while we are still negotiating the DTLS connection (e.g. we send the first packet, but haven't received any reply yet?) the TLS layer actually tries to send the SSL Alert like in stack traces here (which it can't if the IceCtx with it's sockets has been released already)

Flags: needinfo?(drno)

Nils Ohlmeier [:drno]

Assignee

Comment 6

•

8 years ago

Martin, does my theory from comment #5 sounds plausible, or is that complete non-sense?

Flags: needinfo?(martin.thomson)

Martin Thomson [:mt:]

Comment 7

•

8 years ago

Sounds plausible.  I suggest that you ensure that the lower layer is present before trying to use it.  Or disconnect the lower layer on shutdown.

Flags: needinfo?(martin.thomson)

Nils Ohlmeier [:drno]

Assignee

Comment 8

•

8 years ago

Attached patch Potential fix for bug 1279146 (obsolete) — Details — Splinter Review

Disable the lower layer NSPR adapter when destroying TransportLayerDTLS

Nils Ohlmeier [:drno]

Assignee

Comment 9

•

8 years ago

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=48a09d3a1656

Looben Yang

Reporter

Comment 10

•

8 years ago

(In reply to Nils Ohlmeier [:drno] from comment #9)
> Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=48a09d3a1656

Did not see the issue with this build.

Nils Ohlmeier [:drno]

Assignee

Comment 11

•

8 years ago

(In reply to Looben Yang from comment #10)
> Did not see the issue with this build.

Awesome. Feedback is much appreciated, as I currently don't have easy access to ASAN builds.

Nils Ohlmeier [:drno]

Assignee

Updated

•

8 years ago

status-firefox47: --- → affected

status-firefox48: --- → affected

status-firefox49: --- → affected

Nils Ohlmeier [:drno]

Assignee

Updated

•

8 years ago

Attachment #8763559 - Flags: review?(docfaraday)

Looben Yang

Reporter

Comment 12

•

8 years ago

(In reply to Nils Ohlmeier [:drno] from comment #11)
> (In reply to Looben Yang from comment #10)
> > Did not see the issue with this build.
> 
> Awesome. Feedback is much appreciated, as I currently don't have easy access
> to ASAN builds.

Do you have access to Windows?
The attached minimized test case UAF_socket_thread.html is probably only reliable in Windows per my own test. I reproduced the bug with it easily in Windows 10. The stack trace in comment 0 was collected with Visual Studio.

In Linux, I used a non-minimized version of the test case to reliably reproduce the issue. Cause bug reporting guideline asks for minimized test case, I did not upload non-minimized one.

Thanks for the prompt fix. 
If there is anything else i can help, just let me know.

Nils Ohlmeier [:drno]

Assignee

Comment 13

•

8 years ago

Comment on attachment 8763559 [details] [diff] [review]
Potential fix for bug 1279146

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

I would say middle difficulty. Someone who understands (D)TLS and gets the timing sensitivity can easily create code which triggers the UAF.

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?

The code landed in Firefox 18.

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

The original bug 790517 which landed transport layer DTLS for WebRTC.

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

Patch applies cleanly all the way to release.

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

As this is just preventing further network communication during shutdown of a WebRTC PeerConnection I don't think that there is any risk for regression.

Attachment #8763559 - Flags: sec-approval?

Daniel Veditz [:dveditz] (OOO: back Aug 26)

Updated

•

8 years ago

status-firefox47: affected → wontfix

status-firefox-esr45: --- → affected

tracking-firefox48: --- → +

tracking-firefox49: --- → +

tracking-firefox50: --- → +

tracking-firefox-esr45: --- → 48+

Keywords: crash, testcase

Byron Campen [:bwc]

Comment 14

•

8 years ago

Comment on attachment 8763559 [details] [diff] [review]
Potential fix for bug 1279146

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

If I'm not mistaken, the root of the problem is that we have an open NrIceMediaStream after the NrIceCtx has been destroyed; NrIceMediaStream::stream_ is dangling here. We probably want to Close() all of the streams in ~NrIceCtx.

Attachment #8763559 - Flags: review?(docfaraday) → review-

Byron Campen [:bwc]

Comment 15

•

8 years ago

And I bet that the NrIceMediaStream that has outlived its NrIceCtx is in TransportLayerIce::old_stream_; an ICE restart has been started, and then the whole mess has been torn down. TransportLayerIce::ctx_ has been masking this problem for a while.

Daniel Veditz [:dveditz] (OOO: back Aug 26)

Updated

•

8 years ago

Attachment #8763559 - Flags: sec-approval? → sec-approval-

Nils Ohlmeier [:drno]

Assignee

Comment 16

•

8 years ago

(In reply to Byron Campen [:bwc] from comment #14)
> Comment on attachment 8763559 [details] [diff] [review]
> Potential fix for bug 1279146
> 
> Review of attachment 8763559 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> If I'm not mistaken, the root of the problem is that we have an open
> NrIceMediaStream after the NrIceCtx has been destroyed;
> NrIceMediaStream::stream_ is dangling here.

While I agree that NrIceMediaStream::stream_ never appears to be reset after it got set I'm not convinced this is actually the problem here.

> We probably want to Close() all
> of the streams in ~NrIceCtx.

I think that this comment here https://dxr.mozilla.org/mozilla-central/source/media/mtransport/nricemediastream.h#185 is completely mis-leading as it suggests NrIceMediaStream::SendPacket would actually pay attention to state_, which it clearly does not. NrIceMediaStream::SendPacket only returns if stream_ is null, which gets us back to the problem above.
So calling Close() clearly does not prevent anyone from sending something.

I'm willing to put in all these extra cleanups and safety checks. But probably better in a separate ticket, and lets leave the fix for this report as simple as possible.

BTW check out the original TransportLayerDtls::~TransportLayerDtls() function in here https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=790517&attachment=668501
It actually mentions the TLS abort we see popping up here. Closing of the socket got removed and the comment went into the header file instead, with as far as I can tell no code which actually take care of the problem (so far).

(In reply to Byron Campen [:bwc] from comment #15)
> And I bet that the NrIceMediaStream that has outlived its NrIceCtx
> is in TransportLayerIce::old_stream_; an ICE restart has been started,
> and then the whole mess has been torn down.

The problem I have with that theory is that the provided test case does not contain any ICE restart attempt what so ever. And I really hope that ICE restarts don't get "accidentally" triggered from within our code, without JS requesting it.
Looben: could you please check if you can repro with your test case this problem on FF <= 47?

(BTW just to be clear: it is totally plausible to me that actual ICE restarts could cause more or similar problems.)

Flags: needinfo?(loobenyang)

Looben Yang

Reporter

Comment 17

•

8 years ago

(In reply to Nils Ohlmeier [:drno] from comment #16)
> (In reply to Byron Campen [:bwc] from comment #14)
> > Comment on attachment 8763559 [details] [diff] [review]
> > Potential fix for bug 1279146
> > 
> > Review of attachment 8763559 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > If I'm not mistaken, the root of the problem is that we have an open
> > NrIceMediaStream after the NrIceCtx has been destroyed;
> > NrIceMediaStream::stream_ is dangling here.
> 
> While I agree that NrIceMediaStream::stream_ never appears to be reset after
> it got set I'm not convinced this is actually the problem here.
> 
> > We probably want to Close() all
> > of the streams in ~NrIceCtx.
> 
> I think that this comment here
> https://dxr.mozilla.org/mozilla-central/source/media/mtransport/
> nricemediastream.h#185 is completely mis-leading as it suggests
> NrIceMediaStream::SendPacket would actually pay attention to state_, which
> it clearly does not. NrIceMediaStream::SendPacket only returns if stream_ is
> null, which gets us back to the problem above.
> So calling Close() clearly does not prevent anyone from sending something.
> 
> I'm willing to put in all these extra cleanups and safety checks. But
> probably better in a separate ticket, and lets leave the fix for this report
> as simple as possible.
> 
> BTW check out the original TransportLayerDtls::~TransportLayerDtls()
> function in here
> https://bugzilla.mozilla.org/page.cgi?id=splinter.
> html&bug=790517&attachment=668501
> It actually mentions the TLS abort we see popping up here. Closing of the
> socket got removed and the comment went into the header file instead, with
> as far as I can tell no code which actually take care of the problem (so
> far).
> 
> (In reply to Byron Campen [:bwc] from comment #15)
> > And I bet that the NrIceMediaStream that has outlived its NrIceCtx
> > is in TransportLayerIce::old_stream_; an ICE restart has been started,
> > and then the whole mess has been torn down.
> 
> The problem I have with that theory is that the provided test case does not
> contain any ICE restart attempt what so ever. And I really hope that ICE
> restarts don't get "accidentally" triggered from within our code, without JS
> requesting it.
> Looben: could you please check if you can repro with your test case this
> problem on FF <= 47?
> 
> (BTW just to be clear: it is totally plausible to me that actual ICE
> restarts could cause more or similar problems.)

Just tried, failed to reproduce it with my test case on Firefox 47.0

Flags: needinfo?(loobenyang)

Byron Campen [:bwc]

Comment 18

•

8 years ago

(In reply to Nils Ohlmeier [:drno] from comment #16)
> (In reply to Byron Campen [:bwc] from comment #14)
> > Comment on attachment 8763559 [details] [diff] [review]
> > Potential fix for bug 1279146
> > 
> > Review of attachment 8763559 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > If I'm not mistaken, the root of the problem is that we have an open
> > NrIceMediaStream after the NrIceCtx has been destroyed;
> > NrIceMediaStream::stream_ is dangling here.
> 
> While I agree that NrIceMediaStream::stream_ never appears to be reset after
> it got set I'm not convinced this is actually the problem here.
> 
> > We probably want to Close() all
> > of the streams in ~NrIceCtx.
> 
> I think that this comment here
> https://dxr.mozilla.org/mozilla-central/source/media/mtransport/
> nricemediastream.h#185 is completely mis-leading as it suggests
> NrIceMediaStream::SendPacket would actually pay attention to state_, which
> it clearly does not. NrIceMediaStream::SendPacket only returns if stream_ is
> null, which gets us back to the problem above.
> So calling Close() clearly does not prevent anyone from sending something.
this 
   Close() also causes stream_ to be zeroed (nr_ice_remove_media_stream eventually zeroes the outparam).

> 
> I'm willing to put in all these extra cleanups and safety checks. But
> probably better in a separate ticket, and lets leave the fix for this report
> as simple as possible.

   I would at least want to know whether closing the streams fixes the crash by itself. If it does not, it implies the existence of another dangling pointer somewhere.

> 
> BTW check out the original TransportLayerDtls::~TransportLayerDtls()
> function in here
> https://bugzilla.mozilla.org/page.cgi?id=splinter.
> html&bug=790517&attachment=668501
> It actually mentions the TLS abort we see popping up here. Closing of the
> socket got removed and the comment went into the header file instead, with
> as far as I can tell no code which actually take care of the problem (so
> far).

   This was taken care of by declaring member variables in a specific order: https://dxr.mozilla.org/mozilla-central/source/media/mtransport/transportlayerdtls.h#178 The specific problem they're guarding against is the nspr_io_adapter_ being used after it is freed by the ssl_fd_ teardown code.

> 
> (In reply to Byron Campen [:bwc] from comment #15)
> > And I bet that the NrIceMediaStream that has outlived its NrIceCtx
> > is in TransportLayerIce::old_stream_; an ICE restart has been started,
> > and then the whole mess has been torn down.
> 
> The problem I have with that theory is that the provided test case does not
> contain any ICE restart attempt what so ever. And I really hope that ICE
> restarts don't get "accidentally" triggered from within our code, without JS
> requesting it.
> Looben: could you please check if you can repro with your test case this
> problem on FF <= 47?
> 
> (BTW just to be clear: it is totally plausible to me that actual ICE
> restarts could cause more or similar problems.)

   I should probably see what that restart code is doing when this test code is run...

Michael Froman [:mjf]

Comment 19

•

8 years ago

(In reply to Byron Campen [:bwc] from comment #18)
>    I should probably see what that restart code is doing when this test code
> is run...
NSPR_LOG_MODULES=signaling:6 you should see "Offerer restarting" or "Answerer restarting" if ice is restarting.  I tried it here, but I'm not seeing the crash so maybe I'm not running the test properly on Nightly (or my local build).

Nils Ohlmeier [:drno]

Assignee

Comment 20

•

8 years ago

Back in the office I'm able to repro this on my Linux box. And I'm seeing "Answerer restarting ice" in the logs. So Byron is probably right. I'll add the suggested cleanups next week.

Nils Ohlmeier [:drno]

Assignee

Comment 21

•

8 years ago

I have a new patch now, which I'll test on Tuesday.

Nils Ohlmeier [:drno]

Assignee

Comment 22

•

8 years ago

Attached patch bug1279146.patch (obsolete) — Details — Splinter Review

So this patch contains two fixes:
A) cleanup of remaining streams on ICE shutdown
B) prevent DTLS packets from getting send on shutdown

I verified that both approaches fix the provided test on an ASAN build.

We can land both for this bug, or separate them and land the second one (I guess the transportlayerdtls part) in a new, unrelated bug.

Attachment #8763559 - Attachment is obsolete: true

Attachment #8768162 - Flags: review?(docfaraday)

Byron Campen [:bwc]

Comment 23

•

8 years ago

Comment on attachment 8768162 [details] [diff] [review]
bug1279146.patch

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

::: media/mtransport/nricectx.cpp
@@ +670,5 @@
>  }
>  
>  NrIceCtx::~NrIceCtx() {
>    MOZ_MTLOG(ML_DEBUG, "Destroying ICE ctx '" << name_ <<"'");
> +  for (auto it = streams_.begin(); it != streams_.end(); it++) {

I guess for (auto& stream : streams_) would be easier to read here.

Attachment #8768162 - Flags: review?(docfaraday) → review+

Nils Ohlmeier [:drno]

Assignee

Comment 24

•

8 years ago

Attached patch bug1279146.patch — Details — Splinter Review

Addressed Byron's comments.

Carrying forward r=bwc.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=549a449d5040

Attachment #8768162 - Attachment is obsolete: true

Attachment #8768234 - Flags: review+

Nils Ohlmeier [:drno]

Assignee

Comment 25

•

8 years ago

Comment on attachment 8768234 [details] [diff] [review]
bug1279146.patch

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

Pretty hard as the reader would first have to understand that the problem gets triggered only through an ICE restart, which is not mentioned in the patch at all.

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?

Up to Firefox 48.

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

Bug 906986.

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

I'm assuming the patch should apply easily to previous releases. If there should be merge conflicts they should be easy to resolve. I can double check and create patches if needed/requested.

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

As the patch only affects how we are cleaning up things on shutdown I don't see how it could cause any regressions.

Attachment #8768234 - Flags: sec-approval?

Al Billings [:abillings - ex-MoCo]

Comment 26

•

8 years ago

sec-approval+ for trunk.

Please make and nominate Aurora, Beta, and ESR45 patches as well, to go in after this lands on trunk. We'll want to take the fix everywhere.

Al Billings [:abillings - ex-MoCo]

Updated

•

8 years ago

Attachment #8768234 - Flags: sec-approval? → sec-approval+

Nils Ohlmeier [:drno]

Assignee

Comment 27

•

8 years ago

Attached patch bug1279146_beta.patch — Details — Splinter Review

The patch for Beta

Nils Ohlmeier [:drno]

Assignee

Comment 28

•

8 years ago

Attached patch bug1279146_esr.patch (obsolete) — Details — Splinter Review

The patch for ESR45

Nils Ohlmeier [:drno]

Assignee

Comment 29

•

8 years ago

Comment on attachment 8768234 [details] [diff] [review]
bug1279146.patch

Approval Request Comment
[Feature/regressing bug #]: Bug 906986 exposed the problem, but the issue existed probably a lot longer already.

[User impact if declined]: Security risk implied by the UAF.

[Describe test coverage new/current, TreeHerder]: Only manual testing with the test case provided by the reporter.


[Risks and why]: Very low, as this is only adding cleanup on shutdown.

[String/UUID change made/needed]: N/A

Attachment #8768234 - Flags: approval-mozilla-aurora?

Nils Ohlmeier [:drno]

Assignee

Comment 30

•

8 years ago

Comment on attachment 8768486 [details] [diff] [review]
bug1279146_beta.patch

Approval Request Comment
[Feature/regressing bug #]: Bug 906986 exposed the problem, but the issue existed probably a lot longer already.

[User impact if declined]: Security risk implied by the UAF.

[Describe test coverage new/current, TreeHerder]: Only manual testing with the test case provided by the reporter.


[Risks and why]: Very low, as this is only adding cleanup on shutdown.

[String/UUID change made/needed]: N/A

Attachment #8768486 - Flags: approval-mozilla-beta?

Nils Ohlmeier [:drno]

Assignee

Comment 31

•

8 years ago

Comment on attachment 8768487 [details] [diff] [review]
bug1279146_esr.patch

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

User impact if declined: Security risks implied by UAF.

Fix Landed on Version: should land on 50.

Risk to taking this patch (and alternatives if risky): very low as it only adds more cleanup code on shutdown.

String or UUID changes made by this patch: N/A

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Attachment #8768487 - Flags: approval-mozilla-esr45?

Nils Ohlmeier [:drno]

Assignee

Updated

•

8 years ago

Keywords: checkin-needed

Ryan VanderMeulen [:RyanVM]

Comment 32

•

8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc47bd26a869

Keywords: checkin-needed

Carsten Book [:Tomcat]

Comment 33

•

8 years ago

https://hg.mozilla.org/mozilla-central/rev/cc47bd26a869

Status: NEW → RESOLVED

Closed: 8 years ago

status-firefox50: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla50

Daniel Veditz [:dveditz] (OOO: back Aug 26)

Updated

•

8 years ago

Group: media-core-security → core-security-release

Liz Henry (:lizzard) (relman/hg->git project)

Comment 34

•

8 years ago

Comment on attachment 8768234 [details] [diff] [review]
bug1279146.patch

Sec-critical, let's uplift to all branches.

Attachment #8768234 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Liz Henry (:lizzard) (relman/hg->git project)

Comment 35

•

8 years ago

Comment on attachment 8768486 [details] [diff] [review]
bug1279146_beta.patch

This should make it into the beta 7 build tomorrow.

Attachment #8768486 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Liz Henry (:lizzard) (relman/hg->git project)

Comment 36

•

8 years ago

Comment on attachment 8768487 [details] [diff] [review]
bug1279146_esr.patch

Sec-critical, let's uplift this to ESR45.

Attachment #8768487 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+

Carsten Book [:Tomcat]

Comment 37

•

8 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/496264b64d72

status-firefox49: affected → fixed

Carsten Book [:Tomcat]

Comment 38

•

8 years ago

https://hg.mozilla.org/releases/mozilla-beta/rev/9d734024ed35
https://hg.mozilla.org/releases/mozilla-esr45/rev/fe34f5e690c2

status-firefox48: affected → fixed

status-firefox-esr45: affected → fixed

Carsten Book [:Tomcat]

Comment 39

•

8 years ago

backed out from esr45 because of bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=62409&repo=mozilla-esr45

status-firefox-esr45: fixed → affected

Flags: needinfo?(drno)

Andrei Vaida [:avaida]

Updated

•

8 years ago

Flags: qe-verify+

Daniel Veditz [:dveditz] (OOO: back Aug 26)

Updated

•

8 years ago

Flags: sec-bounty? → sec-bounty+

Daniel Veditz [:dveditz] (OOO: back Aug 26)

Updated

•

8 years ago

Attachment #8761474 - Attachment mime type: text/plain → text/html

Nils Ohlmeier [:drno]

Assignee

Comment 40

•

8 years ago

Attached patch bug1279146_esr_working.patch — Details — Splinter Review

Sorry about that. I should have checked that it compiles at least.
This compiles now :-)

Attachment #8768487 - Attachment is obsolete: true

Flags: needinfo?(drno)

Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)

Comment 41

•

8 years ago

https://hg.mozilla.org/releases/mozilla-esr45/rev/cc258670af8f

status-firefox-esr45: affected → fixed

Paul Silaghi, QA [:pauly]

Comment 42

•

8 years ago

I couldn't reproduce the crash on nightly 2016-06-09 on Win 7, Win 10, Ubuntu 14.04.
Looben, since you were the only one who reproduced the issue, could you please try again on the following builds, so we can safely mark them as verified?
https://archive.mozilla.org/pub/firefox/candidates/48.0b7-candidates/
https://archive.mozilla.org/pub/firefox/nightly/2016/07/2016-07-14-03-02-08-mozilla-central/
https://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-esr45-win32/1468516304/
Thanks!

Flags: needinfo?(loobenyang)

Al Billings [:abillings - ex-MoCo]

Updated

•

8 years ago

Alias: CVE-2016-5258

Whiteboard: [adv-main48+][adv-esr45.3+]

Looben Yang

Reporter

Comment 43

•

8 years ago

(In reply to Paul Silaghi, QA [:pauly] from comment #42)
> I couldn't reproduce the crash on nightly 2016-06-09 on Win 7, Win 10,
> Ubuntu 14.04.
> Looben, since you were the only one who reproduced the issue, could you
> please try again on the following builds, so we can safely mark them as
> verified?
> https://archive.mozilla.org/pub/firefox/candidates/48.0b7-candidates/
> https://archive.mozilla.org/pub/firefox/nightly/2016/07/2016-07-14-03-02-08-
> mozilla-central/
> https://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-esr45-win32/
> 1468516304/
> Thanks!

Obviously I'm not the only one reproduced it. see https://bugzilla.mozilla.org/show_bug.cgi?id=1279146#c20

Reproduced it with the exact same test case on this build:
https://ftp.mozilla.org/pub/firefox/nightly/2016/06/2016-06-08-03-02-19-mozilla-central/firefox-50.0a1.en-US.win32.zip

Tried but failed to reproduced it on the following builds:
https://archive.mozilla.org/pub/firefox/nightly/2016/07/2016-07-14-03-02-08-mozilla-central/firefox-50.0a1.en-US.win32.zip
https://archive.mozilla.org/pub/firefox/candidates/48.0b7-candidates/build1/win32/en-US/firefox-48.0b7.zip
https://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-esr45-win32/1468516304/firefox-45.2.1.en-US.win32.zip

Flags: needinfo?(loobenyang)

Paul Silaghi, QA [:pauly]

Comment 44

•

8 years ago

Thank you!

Status: RESOLVED → VERIFIED

status-firefox48: fixed → verified

status-firefox50: fixed → verified

status-firefox-esr45: fixed → verified

Paul Silaghi, QA [:pauly]

Updated

•

8 years ago

Flags: qe-verify+

Daniel Veditz [:dveditz] (OOO: back Aug 26)

Updated

•

8 years ago

Group: core-security-release

Nils Ohlmeier [:drno]

Assignee

Updated

•

8 years ago

Blocks: 1303867

David Lawrence [:dkl]

Updated

•

3 months ago

Keywords: reporter-external

UAF_socket_thread.html 8 years ago Looben Yang 1.74 KB, text/html		Details
Potential fix for bug 1279146 8 years ago Nils Ohlmeier [:drno] 646 bytes, patch	bwc : review- dveditz : sec-approval-	Details \| Diff \| Splinter Review
bug1279146.patch 8 years ago Nils Ohlmeier [:drno] 2.20 KB, patch	bwc : review+	Details \| Diff \| Splinter Review
bug1279146.patch 8 years ago Nils Ohlmeier [:drno] 2.22 KB, patch	drno : review+ lizzard : approval-mozilla-aurora+ abillings : sec-approval+	Details \| Diff \| Splinter Review
bug1279146_beta.patch 8 years ago Nils Ohlmeier [:drno] 2.24 KB, patch	lizzard : approval-mozilla-beta+	Details \| Diff \| Splinter Review
bug1279146_esr.patch 8 years ago Nils Ohlmeier [:drno] 2.27 KB, patch	lizzard : approval-mozilla-esr45+	Details \| Diff \| Splinter Review
bug1279146_esr_working.patch 8 years ago Nils Ohlmeier [:drno] 2.27 KB, patch		Details \| Diff \| Splinter Review