Closed Bug 1524335 Opened 6 years ago Closed 6 years ago

Stack buffer overflow in IncomingRtpPacket when running mochitest

Categories

(Core :: WebRTC: Networking, defect, P2)

Product:
Core
Component:
WebRTC: Networking
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla67
Tracking Flags:
Tracking Status
firefox-esr60 --- unaffected
firefox65 --- wontfix
firefox66 --- fixed
firefox67 --- fixed

People

(Reporter: ytausky, Assigned: dminor)

References

Details

(Keywords: csectype-bounds, sec-moderate, Whiteboard: [post-critsmash-triage][adv-main66+])

Attachments

(1 file)

Bug 1524335: Make InOrderPacket take last_sequence_number by reference; r=dmajor
47 bytes, text/x-phabricator-request
lizzard
: approval-mozilla-beta+
Details | Review

I got the following error when running dom/media/tests/mochitest/test_peerConnection_threeUnbundledConnections.html with an optimized + ASan build:

 0:15.98 GECKO(29415) ==29416==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x70000162a1f2 at pc 0x000118872c64 bp 0x700001629810 sp 0x700001629808
 0:15.98 GECKO(29415) READ of size 2 at 0x70000162a1f2 thread T13
 0:15.98 GECKO(29415) ==29416==WARNING: invalid path to external symbolizer!
 0:15.98 GECKO(29415) ==29416==WARNING: Failed to use and restart external symbolizer!
 0:16.00 GECKO(29415)     #0 0x118872c63 in webrtc::RtpReceiverImpl::IncomingRtpPacket(webrtc::RTPHeader const&, unsigned char const*, unsigned long, webrtc::PayloadUnion) (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0xcacec63)
 0:16.01 GECKO(29415)     #1 0x118a43d34 in webrtc::voe::Channel::ReceivePacket(unsigned char const*, unsigned long, webrtc::RTPHeader const&) (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0xcc9fd34)
 0:16.03 GECKO(29415)     #2 0x118a4ac1a in webrtc::voe::Channel::OnRtpPacket(webrtc::RtpPacketReceived const&) (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0xcca6c1a)
 0:16.04 GECKO(29415)     #3 0x1183ae56e in webrtc::RtpStreamReceiverController::OnRtpPacket(webrtc::RtpPacketReceived const&) (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0xc60a56e)
 0:16.05 GECKO(29415)     #4 0x11839f1ab in webrtc::internal::Call::DeliverRtp(webrtc::MediaType, unsigned char const*, unsigned long, webrtc::PacketTime const&) (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0xc5fb1ab)
 0:16.06 GECKO(29415)     #5 0x10df9f4f7 in mozilla::WebrtcAudioConduit::DeliverPacket(void const*, int) (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0x21fb4f7)
 0:16.08 GECKO(29415)     #6 0x10df9d250 in mozilla::WebrtcAudioConduit::ReceivedRTPPacket(void const*, int, unsigned int) (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0x21f9250)
 0:16.08 GECKO(29415)     #7 0x10dff065a in mozilla::MediaPipeline::RtpPacketReceived(mozilla::MediaPacket&) (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0x224c65a)
 0:16.09 GECKO(29415)     #8 0x10e041db4 in mozilla::MediaTransportHandlerSTS::PacketReceived(mozilla::TransportLayer*, mozilla::MediaPacket&) (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0x229ddb4)
 0:16.10 GECKO(29415)     #9 0x10e2b8a3c in mozilla::TransportLayerSrtp::PacketReceived(mozilla::TransportLayer*, mozilla::MediaPacket&) (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0x2514a3c)
 0:16.11 GECKO(29415)     #10 0x10e2b4262 in mozilla::TransportLayerIce::IcePacketReceived(mozilla::NrIceMediaStream*, int, unsigned char const*, int) (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0x2510262)
 0:16.11 GECKO(29415)     #11 0x10e233edd in mozilla::NrIceCtx::msg_recvd(void*, nr_ice_peer_ctx_*, nr_ice_media_stream_*, int, unsigned char*, int) (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0x248fedd)
 0:16.12 GECKO(29415)     #12 0x1199f0633 in nr_ice_peer_ctx_deliver_packet_maybe (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0xdc4c633)
 0:16.13 GECKO(29415)     #13 0x1199fcb48 in nr_ice_socket_readable_cb (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0xdc58b48)
 0:16.15 GECKO(29415)     #14 0x10e21b9e9 in mozilla::runnable_args_memfn<RefPtr<mozilla::NrUdpSocketIpc>, void (mozilla::NrUdpSocketIpc::*)(RefPtr<mozilla::nr_udp_message>), RefPtr<mozilla::nr_udp_message> >::Run() (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0x24779e9)
 0:16.15 GECKO(29415)     #15 0x10c0a2f49 in nsThread::ProcessNextEvent(bool, bool*) (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0x2fef49)
 0:16.16 GECKO(29415)     #16 0x10c0aa80c in NS_ProcessNextEvent(nsIThread*, bool) (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0x30680c)
 0:16.17 GECKO(29415)     #17 0x10c387a20 in mozilla::net::nsSocketTransportService::Run() (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0x5e3a20)
 0:16.18 GECKO(29415)     #18 0x10c38a15c in non-virtual thunk to mozilla::net::nsSocketTransportService::Run() (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0x5e615c)
 0:16.18 GECKO(29415)     #19 0x10c0a2f49 in nsThread::ProcessNextEvent(bool, bool*) (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0x2fef49)
 0:16.20 GECKO(29415)     #20 0x10c0aa80c in NS_ProcessNextEvent(nsIThread*, bool) (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0x30680c)
 0:16.20 GECKO(29415)     #21 0x10d1d9141 in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0x1435141)
 0:16.21 GECKO(29415)     #22 0x10d10f181 in MessageLoop::Run() (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0x136b181)
 0:16.22 GECKO(29415)     #23 0x10c09c837 in nsThread::ThreadFunc(void*) (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0x2f8837)
 0:16.22 GECKO(29415)     #24 0x103db3164 in _pt_root (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/libnss3.dylib:x86_64+0x51d164)
 0:16.22 GECKO(29415)     #25 0x7fff775b0304 in _pthread_body (/usr/lib/system/libsystem_pthread.dylib:x86_64+0x3304)
 0:16.22 GECKO(29415)     #26 0x7fff775b326e in _pthread_start (/usr/lib/system/libsystem_pthread.dylib:x86_64+0x626e)
 0:16.22 GECKO(29415)     #27 0x7fff775af414 in thread_start (/usr/lib/system/libsystem_pthread.dylib:x86_64+0x2414)
 0:16.22 GECKO(29415) Address 0x70000162a1f2 is located in stack of thread T13 at offset 2514 in frame
 0:16.23 GECKO(29415)     #0 0x11887213f in webrtc::RtpReceiverImpl::IncomingRtpPacket(webrtc::RTPHeader const&, unsigned char const*, unsigned long, webrtc::PayloadUnion) (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0xcace13f)
 0:16.23 GECKO(29415)   This frame has 7 object(s):
 0:16.23 GECKO(29415)     [32, 33) 'is_red' (line 183)
 0:16.23 GECKO(29415)     [48, 368) 'ref.tmp9' (line 191)
 0:16.23 GECKO(29415)     [432, 2336) 'webrtc_rtp_header' (line 195)
 0:16.23 GECKO(29415)     [2464, 2466) 'audio_level' (line 200)
 0:16.23 GECKO(29415)     [2480, 2488) 'lock' (line 215)
 0:16.23 GECKO(29415)     [2512, 2516) 'agg.tmp36' <== Memory access at offset 2514 is inside this variable
 0:16.23 GECKO(29415)     [2528, 2848) 'ref.tmp59' (line 231)
 0:16.23 GECKO(29415) HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork
 0:16.23 GECKO(29415)       (longjmp and C++ exceptions *are* supported)
 0:16.23 GECKO(29415) Thread T13 created by T0 here:
 0:16.23 GECKO(29415)     #0 0x10438737d in wrap_pthread_create (/Users/yarontausky/clang/build/7.0.0-host/lib/clang/7.0.0/lib/darwin/libclang_rt.asan_osx_dynamic.dylib:x86_64h+0x5437d)
 0:16.23 GECKO(29415)     #1 0x103dafe3e in _PR_CreateThread (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/libnss3.dylib:x86_64+0x519e3e)
 0:16.23 GECKO(29415)     #2 0x103dafa1e in PR_CreateThread (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/libnss3.dylib:x86_64+0x519a1e)
 0:16.24 GECKO(29415)     #3 0x10c09eae5 in nsThread::Init(nsTSubstring<char> const&) (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0x2faae5)
 0:16.25 GECKO(29415)     #4 0x10c0a8ba6 in nsThreadManager::NewNamedThread(nsTSubstring<char> const&, unsigned int, nsIThread**) (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0x304ba6)
 0:16.25 GECKO(29415)     #5 0x10c0ad82c in NS_NewNamedThread(nsTSubstring<char> const&, nsIThread**, nsIRunnable*, unsigned int) (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0x30982c)
 0:16.27 GECKO(29415)     #6 0x10c38529f in mozilla::net::nsSocketTransportService::Init() (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0x5e129f)
 0:16.29 GECKO(29415)     #7 0x10c02a086 in mozilla::xpcom::CreateInstanceImpl(mozilla::xpcom::ModuleID, nsISupports*, nsID const&, void**) (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0x286086)
 0:16.31 GECKO(29415)     #8 0x10c050656 in nsComponentManagerImpl::GetServiceLocked((anonymous namespace)::MutexLock&, (anonymous namespace)::EntryWrapper&, nsID const&, void**) (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0x2ac656)
 0:16.32 GECKO(29415)     #9 0x10c04341a in nsComponentManagerImpl::GetServiceByContractID(char const*, nsID const&, void**) (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0x29f41a)
 0:16.33 GECKO(29415)     #10 0x10c05a11f in nsGetServiceByContractIDWithError::operator()(nsID const&, void**) const (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0x2b611f)
 0:16.34 GECKO(29415)     #11 0x10bea7542 in nsCOMPtr_base::assign_from_gs_contractid_with_error(nsGetServiceByContractIDWithError const&, nsID const&) (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0x103542)
 0:16.34 GECKO(29415)     #12 0x10c2ba8de in mozilla::net::nsIOService::SetOffline(bool) (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0x5168de)
 0:16.35 GECKO(29415)     #13 0x10c2b94c2 in mozilla::net::nsIOService::Init() (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0x5154c2)
 0:16.36 GECKO(29415)     #14 0x10c2bd04f in mozilla::net::nsIOService::GetInstance() (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0x51904f)
 0:16.36 GECKO(29415)     #15 0x10c02a9ea in mozilla::xpcom::CreateInstanceImpl(mozilla::xpcom::ModuleID, nsISupports*, nsID const&, void**) (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0x2869ea)
 0:16.37 GECKO(29415)     #16 0x10c050656 in nsComponentManagerImpl::GetServiceLocked((anonymous namespace)::MutexLock&, (anonymous namespace)::EntryWrapper&, nsID const&, void**) (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0x2ac656)
 0:16.38 GECKO(29415)     #17 0x10c04341a in nsComponentManagerImpl::GetServiceByContractID(char const*, nsID const&, void**) (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0x29f41a)
 0:16.39 GECKO(29415)     #18 0x10e39f791 in nsScriptSecurityManager::Init() (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0x25fb791)
 0:16.40 GECKO(29415)     #19 0x10e3a06d4 in nsScriptSecurityManager::InitStatics() (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0x25fc6d4)
 0:16.41 GECKO(29415)     #20 0x10ddddf90 in nsXPConnect::InitStatics() (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0x2039f90)
 0:16.41 GECKO(29415)     #21 0x10dd6a768 in xpcModuleCtor() (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0x1fc6768)
 0:16.42 GECKO(29415)     #22 0x116098a68 in nsLayoutModuleInitialize() (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0xa2f4a68)
 0:16.43 GECKO(29415)     #23 0x10c045470 in nsComponentManagerImpl::Init() (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0x2a1470)
 0:16.43 GECKO(29415)     #24 0x10c104067 in NS_InitXPCOM2 (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0x360067)
 0:16.44 GECKO(29415)     #25 0x1198cfb92 in XRE_InitEmbedding2(nsIFile*, nsIFile*, nsIDirectoryServiceProvider*) (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0xdb2bb92)
 0:16.45 GECKO(29415)     #26 0x10d1e639f in mozilla::ipc::ScopedXREEmbed::Start() (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0x144239f)
 0:16.46 GECKO(29415)     #27 0x11430cf53 in mozilla::dom::ContentProcess::Init(int, char**) (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0x8568f53)
 0:16.47 GECKO(29415)     #28 0x1198d136d in XRE_InitChildProcess(int, char**, XREChildData const*) (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0xdb2d36d)
 0:16.47 GECKO(29415)     #29 0x103889185 in main (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/plugin-container.app/Contents/MacOS/plugin-container:x86_64+0x100001185)
 0:16.47 GECKO(29415)     #30 0x7fff773bced8 in start (/usr/lib/system/libdyld.dylib:x86_64+0x16ed8)
 0:16.47 GECKO(29415) SUMMARY: AddressSanitizer: stack-buffer-overflow (/Users/yarontausky/src/mozilla-central/obj/dist/Nightly.app/Contents/MacOS/XUL:x86_64+0xcacec63) in webrtc::RtpReceiverImpl::IncomingRtpPacket(webrtc::RTPHeader const&, unsigned char const*, unsigned long, webrtc::PayloadUnion)
 0:16.47 GECKO(29415) Shadow bytes around the buggy address:
 0:16.47 GECKO(29415)   0x1e00002c53e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
 0:16.47 GECKO(29415)   0x1e00002c53f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
 0:16.47 GECKO(29415)   0x1e00002c5400: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
 0:16.47 GECKO(29415)   0x1e00002c5410: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
 0:16.47 GECKO(29415)   0x1e00002c5420: 00 00 00 00 00 00 00 00 f2 f2 f2 f2 f2 f2 f2 f2
 0:16.47 GECKO(29415) =>0x1e00002c5430: f2 f2 f2 f2 f2 f2 f2 f2 02 f2 00 f2 f2 f2[02]f2
 0:16.47 GECKO(29415)   0x1e00002c5440: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8
 0:16.47 GECKO(29415)   0x1e00002c5450: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8
 0:16.48 GECKO(29415)   0x1e00002c5460: f8 f8 f8 f8 f8 f8 f8 f8 f3 f3 f3 f3 f3 f3 f3 f3
 0:16.48 GECKO(29415)   0x1e00002c5470: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
 0:16.48 GECKO(29415)   0x1e00002c5480: 00 00 00 00 00 00 00 00 00 00 00 00 f1 f1 f1 f1
 0:16.48 GECKO(29415) Shadow byte legend (one shadow byte represents 8 application bytes):
 0:16.48 GECKO(29415)   Addressable:           00
 0:16.48 GECKO(29415)   Partially addressable: 01 02 03 04 05 06 07
 0:16.48 GECKO(29415)   Heap left redzone:       fa
 0:16.48 GECKO(29415)   Freed heap region:       fd
 0:16.48 GECKO(29415)   Stack left redzone:      f1
 0:16.48 GECKO(29415)   Stack mid redzone:       f2
 0:16.48 GECKO(29415)   Stack right redzone:     f3
 0:16.48 GECKO(29415)   Stack after return:      f5
 0:16.48 GECKO(29415)   Stack use after scope:   f8
 0:16.48 GECKO(29415)   Global redzone:          f9
 0:16.48 GECKO(29415)   Global init order:       f6
 0:16.48 GECKO(29415)   Poisoned by user:        f7
 0:16.48 GECKO(29415)   Container overflow:      fc
 0:16.48 GECKO(29415)   Array cookie:            ac
 0:16.48 GECKO(29415)   Intra object redzone:    bb
 0:16.48 GECKO(29415)   ASan internal:           fe
 0:16.48 GECKO(29415)   Left alloca redzone:     ca
 0:16.48 GECKO(29415)   Right alloca redzone:    cb
 0:16.48 GECKO(29415)   Shadow gap:              cc
 0:16.48 GECKO(29415) ==29416==ABORTING
Group: core-security → media-core-security
Keywords: csectype-bounds, sec-moderate
Priority: -- → P2

Dan could you please have a look at this one as well?
I'm guessing this might also be a new issue/regression caused by the import/update in bug 1376873.

Assignee: nobody → dminor

I found one crash in the field in the same function https://crash-stats.mozilla.com/report/index/0f913628-647b-4abd-ba8b-6a3010190125
But that one is with 64 and the stack looks not useful. So probably unrelated.

Hi Yaron, thank you for reporting this. Out of curiosity, is this reproducible for you? I've left the test running repeatedly on Linux and OS X ASAN builds without it reproducing and I was wondering if this was a one off failure for you or something reproducible. Thanks!

Flags: needinfo?(ytausky)

It is; I just ran the test two times and both gave the same error. I don't get this error when running with --disable-e10s, but then the test itself fails -- not sure if it's related. I'm running on macOS with the following mozconfig:

mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/obj

# -j4 allows 4 tasks to run in parallel. Set the number to be the amount of
# cores in your machine. In Paris or Toronto use -j100
mk_add_options MOZ_MAKE_FLAGS="-j8"

# Enable debug builds
#ac_add_options --enable-debug
ac_add_options --enable-debug-symbols

# Turn off compiler optimization. This will make applications run slower,
# will allow you to debug the applications under a debugger, like GDB.
ac_add_options --enable-optimize

ac_add_options --enable-address-sanitizer

#enable FFmpeg code
#ac_add_options --enable-ffmpeg

#Malloc debug
ac_add_options --disable-jemalloc

#ac_add_options --with-ccache=/usr/local/bin/ccache
#mk_add_options 'export RUSTC_WRAPPER=sccache'
#mk_add_options 'export CARGO_INCREMENTAL=1'

mk_add_options 'export CC=/Users/yarontausky/clang/cc'
mk_add_options 'export CXX=/Users/yarontausky/clang/c++'

export CFLAGS="-fsanitize=address"
export CXXFLAGS="-fsanitize=address"
export LDFLAGS="-fsanitize=address"

ac_add_options --with-macos-sdk=/Users/yarontausky/src/MacOSX10.13.sdk
Flags: needinfo?(ytausky)

Thank you very much, I'm able to reproduce this now.

The problem seems to be occurring in inlined code: InOrderPacket [1] calls IsNewerSequenceNumber [2] which in turn calls IsNewer [3]. If I hardcode IsNewer to return true, the problem does not occur.

I've also tried moving the largish stack allocation here [4] to the heap, but the problem still occurs in this case. If we were close to running out stack space, given that a small change to IsNewer prevents the problem, I would have expected moving this to the heap would also have prevented the problem. I'm starting to suspect there is something about IsNewer itself that is causing problems.

[1] https://searchfox.org/mozilla-central/rev/9eb30227b21e0aa40d51d9f9b08bb0b113c5fadb/media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc#32
[2] https://searchfox.org/mozilla-central/rev/9eb30227b21e0aa40d51d9f9b08bb0b113c5fadb/media/webrtc/trunk/webrtc/modules/include/module_common_types_public.h#89
[3] https://searchfox.org/mozilla-central/rev/9eb30227b21e0aa40d51d9f9b08bb0b113c5fadb/media/webrtc/trunk/webrtc/modules/include/module_common_types_public.h#22
[4] https://searchfox.org/mozilla-central/rev/e00ea598e52bbb35f8c45abf9c2eade17962bb5e/media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc#195

On OS X ASAN builds, the Optional<> copy constructor runs into problems
with this function when it attempts to poison value_, triggering a
false positive stack overflow detection. Passing the argument by reference
works around the problem by avoiding making a copy.

I'm attempting to catch up on the bug and am having trouble connecting the dots from the earlier comments to comment 7. Can you elaborate a bit on how you arrived at the copy constructor's poisoning code?

This smells to me like a "legitimate" ASan complaint, at least to the extent that there may be a logic error in Optional somewhere. ASan says we're attempting to read a value_ that has been poisoned, which would be a violation of Optional's invariants.

(By the way, the terminology gets confusing when there's code that does manual poisoning. Stack-buffer-overflow generally means you overflowed a buffer that lives on the stack, not that you overflowed the stack itself -- which is why your heap experiment didn't help. Except with manual poisoning, it may not even be an overflow, it just means you're touching a stack variable that was poisoned.)

My first thought was to go look upstream and see if anyone's fixed any bugs in Optional lately... well it turns out they replaced Optional altogether with abseil's version, which doesn't even have poisoning: https://webrtc.googlesource.com/src/+/7ba9e92fa0dfb16579f4f6ecd746397bdfdd174d%5E%21/

In light of this I'm happy to r+ anything that makes the build work...

(In reply to David Major [:dmajor] from comment #9)

This smells to me like a "legitimate" ASan complaint, at least to the extent that there may be a logic error in Optional somewhere. ASan says we're attempting to read a value_ that has been poisoned, which would be a violation of Optional's invariants.

The original should be poisoned as well, so I'm not certain why we'd only see the problem with the copy, unless the problem is with making the copy itself. This did not reproduce on Linux for what it is worth.

(By the way, the terminology gets confusing when there's code that does manual poisoning. Stack-buffer-overflow generally means you overflowed a buffer that lives on the stack, not that you overflowed the stack itself -- which is why your heap experiment didn't help. Except with manual poisoning, it may not even be an overflow, it just means you're touching a stack variable that was poisoned.)

Embarrassing, I had read that as stack overflow :/

My first thought was to go look upstream and see if anyone's fixed any bugs in Optional lately... well it turns out they replaced Optional altogether with abseil's version, which doesn't even have poisoning: https://webrtc.googlesource.com/src/+/7ba9e92fa0dfb16579f4f6ecd746397bdfdd174d%5E%21/

In light of this I'm happy to r+ anything that makes the build work...

Thank you!

Alex, could you please have a look at whether or not this needs to stay a sec bug? Thanks!

Flags: needinfo?(agaynor)

Either a standard stack-buffer-overflow or reading a poisoned value are generally sec bugs, unless there's a specific reason they're not.

This is a sec-moderate so you don't need sec-approval to land though.

Flags: needinfo?(agaynor)

Ok, thanks!

Status: NEW → ASSIGNED

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

Group: media-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox67: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

Please request Beta approval on this when you get a chance.

Blocks: 1376873
status-firefox65: --- → wontfix
status-firefox66: --- → affected
status-firefox-esr60: --- → unaffected
Flags: needinfo?(dminor)

Comment on attachment 9043686 [details]
Bug 1524335: Make InOrderPacket take last_sequence_number by reference; r=dmajor

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

None

User impact if declined

The stack buffer overflow is only present in OS X ASAN builds so is not something a user will hit directly. The use of Optional<uint16_t> in the affected code appears correct to me, so I think this is a bug with Optional<> itself, but it's possible there is a real issue here. If so, the user impact would be problems with receiving audio during a call.

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)

Low risk, this just changes passing an argument to a function from a copy to passing a const reference.

String changes made/needed

Flags: needinfo?(dminor)
Attachment #9043686 - Flags: approval-mozilla-beta?

We don't normally run tests on OS X ASAN builds, so I didn't try to get a regression range. It is possible that the update in bug 1376873 caused this.

Comment on attachment 9043686 [details]
Bug 1524335: Make InOrderPacket take last_sequence_number by reference; r=dmajor

Avoids possible sec issue/audio quality issue, let's uplift for beta 8.

Attachment #9043686 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main66+]
Group: core-security-release
No longer depends on: 1646904
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: