1577719 - (CVE-2019-11760) stack-buffer-overflow in nr_reg_register_callback

Status: RESOLVED FIXED

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

Description

[Nils]

This crash was found while dom fuzzing, however the testcase does not reproduce. It looks like the nr_reg_assoc_destroy method is used incorrectly here:
https://searchfox.org/mozilla-central/source/media/mtransport/third_party/nrappkit/src/registry/registrycb.c#185

=================================================================
==1654==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7fed29ec5e10 at pc 0x7fed4011894e bp 0x7fed29ec5d50 sp 0x7fed29ec5d48
READ of size 8 at 0x7fed29ec5e10 thread T5 (IPDL Background)
#0 0x7fed4011894d in r_assoc_destroy /builds/worker/workspace/build/src/media/mtransport/third_party/nrappkit/src/util/libekr/r_assoc.c
#1 0x7fed40117d36 in nr_reg_assoc_destroy /builds/worker/workspace/build/src/media/mtransport/third_party/nrappkit/src/registry/registrycb.c:271:10
#2 0x7fed40117d36 in nr_reg_register_callback /builds/worker/workspace/build/src/media/mtransport/third_party/nrappkit/src/registry/registrycb.c:185
#3 0x7fed4011a477 in NR_reg_register_callback /builds/worker/workspace/build/src/media/mtransport/third_party/nrappkit/src/registry/registrycb.c:404:20
#4 0x7fed4011a477 in r_log_get_destinations /builds/worker/workspace/build/src/media/mtransport/third_party/nrappkit/src/log/r_log.c:571
#5 0x7fed40113826 in _r_log_init /builds/worker/workspace/build/src/media/mtransport/third_party/nrappkit/src/log/r_log.c:646:9
#6 0x7fed40113826 in r_log_init /builds/worker/workspace/build/src/media/mtransport/third_party/nrappkit/src/log/r_log.c:622
#7 0x7fed4011501d in NR_reg_init /builds/worker/workspace/build/src/media/mtransport/third_party/nrappkit/src/registry/registry.c:105:6
#8 0x7fed3566c25a in mozilla::RLogConnector::CreateInstance() /builds/worker/workspace/build/src/media/mtransport/rlogconnector.cpp:106:5
#9 0x7fed35340465 in mozilla::MediaTransportHandlerSTS::EnterPrivateMode() /builds/worker/workspace/build/src/media/webrtc/signaling/src/peerconnection/MediaTransportHandler.cpp:1090:3
#10 0x7fed3535c55a in mozilla::MediaTransportParent::RecvEnterPrivateMode() /builds/worker/workspace/build/src/media/webrtc/signaling/src/peerconnection/MediaTransportParent.cpp:118:20
#11 0x7fed346a2c45 in mozilla::dom::PMediaTransportParent::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/PMediaTransportParent.cpp:435:64
#12 0x7fed34d29ca0 in mozilla::ipc::PBackgroundParent::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/PBackgroundParent.cpp:3588:32
#13 0x7fed33ec0a86 in mozilla::ipc::MessageChannel::DispatchAsyncMessage(mozilla::ipc::ActorLifecycleProxy*, IPC::Message const&) /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:2184:25
#14 0x7fed33ebb7eb in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:2108:9
#15 0x7fed33ebdda7 in mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1955:3
#16 0x7fed33ebec37 in mozilla::ipc::MessageChannel::MessageTask::Run() /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1986:13
#17 0x7fed32cd7800 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1225:14
#18 0x7fed32cdd628 in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:486:10
#19 0x7fed33ecb842 in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) /builds/worker/workspace/build/src/ipc/glue/MessagePump.cpp:303:20
#20 0x7fed33dc5632 in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:315:10
#21 0x7fed33dc5632 in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:308
#22 0x7fed33dc5632 in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:290
#23 0x7fed32cd114a in nsThread::ThreadFunc(void*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:458:11
#24 0x7fed55d870bd in _pt_root /builds/worker/workspace/build/src/nsprpub/pr/src/pthreads/ptthread.c:198:5
#25 0x7fed559cb6da in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x76da)
#26 0x7fed549a988e in clone (/lib/x86_64-linux-gnu/libc.so.6+0x12188e)

Address 0x7fed29ec5e10 is located in stack of thread T5 (IPDL Background) at offset 80 in frame
#0 0x7fed40117acf in nr_reg_register_callback /builds/worker/workspace/build/src/media/mtransport/third_party/nrappkit/src/registry/registrycb.c:119

This frame has 3 object(s):
[32, 40) 'ptr.addr.i' (line 271)
[64, 72) 'assoc' (line 121) <== Memory access at offset 80 overflows this variable
[96, 105) 'cb_id' (line 125) <== Memory access at offset 80 underflows this variable
HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork
(longjmp and C++ exceptions are supported)
Thread T5 (IPDL Background) created by T0 (Socket Process) here:
#0 0x55a35f8d829d in __interceptor_pthread_create /builds/worker/fetches/llvm-project/llvm/projects/compiler-rt/lib/asan/asan_interceptors.cc:210:3
#1 0x7fed55d791b8 in _PR_CreateThread /builds/worker/workspace/build/src/nsprpub/pr/src/pthreads/ptthread.c:430:14
#2 0x7fed55d62d9e in PR_CreateThread /builds/worker/workspace/build/src/nsprpub/pr/src/pthreads/ptthread.c:503:12
#3 0x7fed32cd3639 in nsThread::Init(nsTSubstring<char> const&) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:672:8
#4 0x7fed32cdc770 in nsThreadManager::NewNamedThread(nsTSubstring<char> const&, unsigned int, nsIThread**) /builds/worker/workspace/build/src/xpcom/threads/nsThreadManager.cpp:415:12
#5 0x7fed32ce028a in NS_NewNamedThread(nsTSubstring<char> const&, nsIThread**, nsIRunnable*, unsigned int) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:139:57
#6 0x7fed33e70282 in NS_NewNamedThread<16> /builds/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:71:10
#7 0x7fed33e70282 in (anonymous namespace)::ParentImpl::CreateBackgroundThread() /builds/worker/workspace/build/src/ipc/glue/BackgroundImpl.cpp:944
#8 0x7fed33e27fe4 in Alloc /builds/worker/workspace/build/src/ipc/glue/BackgroundImpl.cpp:818:30
#9 0x7fed33e27fe4 in mozilla::ipc::BackgroundParent::Alloc(mozilla::dom::ContentParent*, mozilla::ipc::Endpoint<mozilla::ipc::PBackgroundParent>&&) /builds/worker/workspace/build/src/ipc/glue/BackgroundImpl.cpp:640
#10 0x7fed33bf7d87 in mozilla::net::SocketProcessBridgeParent::RecvInitBackground(mozilla::ipc::Endpoint<mozilla::ipc::PBackgroundParent>&&) /builds/worker/workspace/build/src/netwerk/ipc/SocketProcessBridgeParent.cpp:41:8
#11 0x7fed349eb43d in mozilla::net::PSocketProcessBridgeParent::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/PSocketProcessBridgeParent.cpp:130:69
#12 0x7fed33ec0a86 in mozilla::ipc::MessageChannel::DispatchAsyncMessage(mozilla::ipc::ActorLifecycleProxy*, IPC::Message const&) /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:2184:25
#13 0x7fed33ebb7eb in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:2108:9
#14 0x7fed33ebdda7 in mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1955:3
#15 0x7fed33ebec37 in mozilla::ipc::MessageChannel::MessageTask::Run() /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1986:13
#16 0x7fed32cd7800 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1225:14
#17 0x7fed32cdd628 in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:486:10
#18 0x7fed33ec9e64 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/workspace/build/src/ipc/glue/MessagePump.cpp:110:5
#19 0x7fed33dc5632 in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:315:10
#20 0x7fed33dc5632 in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:308
#21 0x7fed33dc5632 in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:290
#22 0x7fed3c0a9149 in nsBaseAppShell::Run() /builds/worker/workspace/build/src/widget/nsBaseAppShell.cpp:137:27
#23 0x7fed3ff96acf in XRE_RunAppShell() /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:934:20
#24 0x7fed33dc5632 in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:315:10
#25 0x7fed33dc5632 in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:308
#26 0x7fed33dc5632 in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:290
#27 0x7fed3ff96376 in XRE_InitChildProcess(int, char**, XREChildData const*) /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:769:34
#28 0x55a35f922d73 in content_process_main /builds/worker/workspace/build/src/browser/app/../../ipc/contentproc/plugin-container.cpp:56:28
#29 0x55a35f922d73 in main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:267
#30 0x7fed548a9b96 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b96)

SUMMARY: AddressSanitizer: stack-buffer-overflow /builds/worker/workspace/build/src/media/mtransport/third_party/nrappkit/src/util/libekr/r_assoc.c in r_assoc_destroy
Shadow bytes around the buggy address:
0x0ffe253d0b70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0ffe253d0b80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0ffe253d0b90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0ffe253d0ba0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0ffe253d0bb0: 00 00 00 00 00 00 00 00 f1 f1 f1 f1 00 f2 f2 f2
=>0x0ffe253d0bc0: 00 f2[f2]f2 00 01 f3 f3 00 00 00 00 00 00 00 00
0x0ffe253d0bd0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0ffe253d0be0: f1 f1 f1 f1 00 00 00 00 00 00 00 00 00 00 00 00
0x0ffe253d0bf0: 00 00 00 00 f2 f2 f2 f2 04 f2 01 f3 00 00 00 00
0x0ffe253d0c00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0ffe253d0c10: f1 f1 f1 f1 04 f3 f3 f3 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
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
Shadow gap: cc
==1654==ABORTING

Comment 1

[Daniel Veditz [:dveditz]]

IPC is in there, maybe some kind of race condition is what's making it hard to reproduce? Could you provide your testcase anyway so we can loop it and see if we eventually can trap it?

Comment 2

[Nils Ohlmeier [:drno]]

Byron could this be fall-out from when you introduced MediaTransportHandler?

Comment 3

[Byron Campen [:bwc]]

Possibly. I'll look into it.

Comment 4

[Byron Campen [:bwc]]

Yeah, I see what is going on here. Should be a relatively easy fix.

Comment 5

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d70963c454d7818d68f28c9a255f9b690a7db02

Comment 6

[Byron Campen [:bwc]]

Attachment: Bug 1577719: Init r_log in the c'tor, to simplify things. r?mjf

Comment 7

[Byron Campen [:bwc]]

Comment on attachment 9091935 [details]
Bug 1577719: Init r_log in the c'tor, to simplify things. r?mjf

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Probably not very easily.
• 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?: Everything but ESR 60
• If not all supported branches, which bug introduced the flaw?: Bug 1521879
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: Not very hard.
• How likely is this patch to cause regressions; how much testing does it need?: Not likely.

Comment 8

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 9091935 [details]
Bug 1577719: Init r_log in the c'tor, to simplify things. r?mjf

We rated this a sec-moderate so it can go into mozilla-central without sec-approval. I'm clearing it.
If we want to uplift this to Beta and ESR68, we should have a discussion.

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/bac926c1886edd2bb5cdb16c66ae24aa6e23d341
https://hg.mozilla.org/mozilla-central/rev/bac926c1886e

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta and ESR68 approval if you think it'd be worth backporting.

Comment 11

[Byron Campen [:bwc]]

I don't see any sign of this on crash-stats in versions past 65, but it is a pretty safe fix I think.

Comment 12

[Byron Campen [:bwc]]

Comment on attachment 9091935 [details]
Bug 1577719: Init r_log in the c'tor, to simplify things. r?mjf

Beta/Release Uplift Approval Request

• User impact if declined: This is a pretty simple fix to a potentially dangerous memory issue.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• 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): We're just doing some init a little bit earlier than we were before, and doing it in a single place instead of multiple, which simplifies things.
• String changes made/needed: None

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is a pretty simple fix to a potentially dangerous memory issue.
• User impact if declined:
• Fix Landed on Version:
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): We're just doing some init a little bit earlier than we were before, and doing it in a single place instead of multiple, which simplifies things.
• String or UUID changes made by this patch: None

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9091935 [details]
Bug 1577719: Init r_log in the c'tor, to simplify things. r?mjf

Safe fix for a crash bug. Approved for 70.0b9 and 68.2esr.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/57e9358e143d

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/6a1a2f27a8fc

Comment 22

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: advisory.txt

Comment 23

[Michael Froman [:mjf]]

:tjr, the advisory.txt attachment seems to have a typo.

Comment 24

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: advisory.txt