966547 - sip_platform_task (webrtc, sipcc) violates the seccomp-bpf sandbox by using named local-domain sockets

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect)

Description

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

When used as part of WebRTC, sipcc is trying to use SysV message queues, and call mkdir(), from a content process.  (This is exposed after adding readlink() to the whitelist as a workaround for bug 964455.)  The following stack traces occur concurrently, on different threads:


#0  syscall () at bionic/libc/arch-arm/bionic/syscall.S:50
#1  0x4087963c in msgrcv (msgQueue=0x44d0f180, waitForever=1 '\001', ppUserData=0x44bccebc)
    at /home/jld/src/B2G.goldfish/gecko/media/webrtc/signaling/src/sipcc/cpr/linux/cpr_linux_ipc.c:80
#2  cprGetMessage (msgQueue=0x44d0f180, waitForever=1 '\001', ppUserData=0x44bccebc)
    at /home/jld/src/B2G.goldfish/gecko/media/webrtc/signaling/src/sipcc/cpr/linux/cpr_linux_ipc.c:512
#3  0x408212fe in GSMTask (arg=<value optimized out>) at /home/jld/src/B2G.goldfish/gecko/media/webrtc/signaling/src/sipcc/core/gsm/gsm.c:312


#0  mkdir () at bionic/libc/arch-arm/syscalls/mkdir.S:10
#1  0x40066572 in _gettemp (path=0x41bed910 "/data/local/tmp/SIP-kpVGY335", doopen=0x0, domkdir=<value optimized out>, slen=<value optimized out>)
    at bionic/libc/stdio/mktemp.c:154
#2  0x40066612 in mkdtemp (path=0xfffffdfc <Address 0xfffffdfc out of bounds>) at bionic/libc/stdio/mktemp.c:63
#3  0x40875980 in sip_get_sock_dir (out=0x44baee58 "", suffix=0x4178cfd6 "/Main", outlen=<value optimized out>)
    at /home/jld/src/B2G.goldfish/gecko/media/webrtc/signaling/src/sipcc/core/sipstack/sip_platform_task.c:178
#4  0x40875d50 in sip_platform_task_loop (arg=<value optimized out>)
    at /home/jld/src/B2G.goldfish/gecko/media/webrtc/signaling/src/sipcc/core/sipstack/sip_platform_task.c:515


#0  syscall () at bionic/libc/arch-arm/bionic/syscall.S:50
#1  0x40879438 in msgsnd (msgq=0x44d3cf80, msg=<value optimized out>, ppUserData=<value optimized out>)
    at /home/jld/src/B2G.goldfish/gecko/media/webrtc/signaling/src/sipcc/cpr/linux/cpr_linux_ipc.c:66
#2  cprPostMessage (msgq=0x44d3cf80, msg=<value optimized out>, ppUserData=<value optimized out>)
    at /home/jld/src/B2G.goldfish/gecko/media/webrtc/signaling/src/sipcc/cpr/linux/cpr_linux_ipc.c:796
#3  0x40879512 in cprSendMessage (msgQueue=0x44d3cf80, msg=0x44d15100, ppUserData=0xbefa3704)
    at /home/jld/src/B2G.goldfish/gecko/media/webrtc/signaling/src/sipcc/cpr/linux/cpr_linux_ipc.c:660
#4  0x408054e8 in ccappTaskSendMsg (cmd=13, msg=0x44d15100, len=48, UsrInfo=1)
    at /home/jld/src/B2G.goldfish/gecko/media/webrtc/signaling/src/sipcc/core/ccapp/ccapp_task.c:135
#5  0x40805550 in ccappTaskPostMsg (msgId=13, data=0xbefa3760, len=48, appId=1)
    at /home/jld/src/B2G.goldfish/gecko/media/webrtc/signaling/src/sipcc/core/ccapp/ccapp_task.c:102
#6  0x407ff146 in cc_invokeDeviceFeature (feature=0xbefa3760) at /home/jld/src/B2G.goldfish/gecko/media/webrtc/signaling/src/sipcc/core/ccapp/cc_device_feature.c:17
#7  0x407ff1d0 in CC_DeviceFeature_enableVideo (enable=<value optimized out>)
    at /home/jld/src/B2G.goldfish/gecko/media/webrtc/signaling/src/sipcc/core/ccapp/cc_device_feature.c:49
#8  0x40801cac in CCAPI_Device_enableVideo (handle=<value optimized out>, enable=1 '\001')
    at /home/jld/src/B2G.goldfish/gecko/media/webrtc/signaling/src/sipcc/core/ccapp/ccapi_device.c:121
#9  0x407f9472 in CSF::CC_SIPCCDevice::enableVideo (this=0x44d131c0, aDeviceHandle=<value optimized out>)
    at /home/jld/src/B2G.goldfish/gecko/media/webrtc/signaling/src/softphonewrapper/CC_SIPCCDevice.cpp:82
#10 CC_SIPCCDevice (this=0x44d131c0, aDeviceHandle=<value optimized out>)
    at /home/jld/src/B2G.goldfish/gecko/media/webrtc/signaling/src/softphonewrapper/CC_SIPCCDevice.cpp:40
#11 0x407f975a in Wrapper<CSF::CC_SIPCCDevice>::wrap (handle=0) at /home/jld/src/B2G.goldfish/gecko/media/webrtc/signaling/./src/common/Wrapper.h:106
#12 CSF::CC_SIPCCDevice::wrap (handle=0) at /home/jld/src/B2G.goldfish/gecko/media/webrtc/signaling/src/softphonewrapper/CC_SIPCCDevice.cpp:26
#13 0x407fc492 in CSF::CC_SIPCCService::onDeviceEvent (type=CCAPI_DEVICE_EV_CONFIG_CHANGED, handle=0, info=0x442cc9d0)
    at /home/jld/src/B2G.goldfish/gecko/media/webrtc/signaling/src/softphonewrapper/CC_SIPCCService.cpp:611
#14 0x407fc5c6 in CCAPI_DeviceListener_onDeviceEvent (type=3204069216, hDevice=1, dev_info=0xc)
    at /home/jld/src/B2G.goldfish/gecko/media/webrtc/signaling/src/softphonewrapper/CC_SIPCCService.cpp:284
#15 0x40804baa in ccsnap_gen_deviceEvent (event=CCAPI_DEVICE_EV_CONFIG_CHANGED, handle=0)
    at /home/jld/src/B2G.goldfish/gecko/media/webrtc/signaling/src/sipcc/core/ccapp/ccapi_snapshot.c:409
#16 0x4080937a in config_parser_handle_fcp_file (fcpTemplateFile=<value optimized out>)
    at /home/jld/src/B2G.goldfish/gecko/media/webrtc/signaling/src/sipcc/core/common/config_parser.c:618
#17 0x4080a236 in config_setup_main (sipUser=<value optimized out>, sipPassword=<value optimized out>, sipDomain=<value optimized out>)
    at /home/jld/src/B2G.goldfish/gecko/media/webrtc/signaling/src/sipcc/core/common/config_parser.c:632
#18 0x40801942 in parse_setup_properties (device_handle=<value optimized out>, device_name=<value optimized out>, sipUser=0x44d0c7b0 "JSEP", sipPassword=0x44d0c7c8 "", 
    sipDomain=0x44d0c7e0 "127.0.0.1") at /home/jld/src/B2G.goldfish/gecko/media/webrtc/signaling/src/sipcc/core/ccapp/ccapi_config.c:55
#19 0x408019ca in CCAPI_Start_response (device_handle=0, device_name=0x44d0c710 "sipdevice", sipUser=0x44d0c7b0 "JSEP", sipPassword=0x44d0c7c8 "", 
    sipDomain=0x44d0c7e0 "127.0.0.1") at /home/jld/src/B2G.goldfish/gecko/media/webrtc/signaling/src/sipcc/core/ccapp/ccapi_config.c:41
#20 0x407fcca6 in configCtlFetchReq (device_handle=0) at /home/jld/src/B2G.goldfish/gecko/media/webrtc/signaling/src/softphonewrapper/CC_SIPCCService.cpp:85
#21 0x407fccde in configFetchReq (device_handle=0) at /home/jld/src/B2G.goldfish/gecko/media/webrtc/signaling/src/softphonewrapper/CC_SIPCCService.cpp:109
#22 0x407ff70a in registration_processEvent (event=1) at /home/jld/src/B2G.goldfish/gecko/media/webrtc/signaling/src/sipcc/core/ccapp/cc_device_manager.c:263
#23 0x408034cc in CCAPI_Service_start () at /home/jld/src/B2G.goldfish/gecko/media/webrtc/signaling/src/sipcc/core/ccapp/ccapi_service.c:91
#24 0x407fc948 in CSF::CC_SIPCCService::startService (this=0x44d0c700)
    at /home/jld/src/B2G.goldfish/gecko/media/webrtc/signaling/src/softphonewrapper/CC_SIPCCService.cpp:437
#25 0x407e03d6 in CSF::CallControlManagerImpl::startSDPMode (this=0x44d0c600)
    at /home/jld/src/B2G.goldfish/gecko/media/webrtc/signaling/src/callcontrol/CallControlManagerImpl.cpp:222
#26 0x407ef480 in sipcc::PeerConnectionCtx::Initialize (this=0x44d150d0)
    at /home/jld/src/B2G.goldfish/gecko/media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp:290
#27 0x407ef6c6 in sipcc::PeerConnectionCtx::InitializeGlobal (mainThread=<value optimized out>, stsThread=<value optimized out>)
    at /home/jld/src/B2G.goldfish/gecko/media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp:217
#28 0x407f2504 in sipcc::PeerConnectionImpl::Initialize (this=0x432b6560, aObserver=<value optimized out>, aWindow=<value optimized out>, aConfiguration=0x0, aRTCConfiguration=0xbefa3bf8, aThread=0x40202780) at /home/jld/src/B2G.goldfish/gecko/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:776
#29 0x40a64438 in sipcc::PeerConnectionImpl::Initialize (cx=0x4023a1c0, obj=<value optimized out>, self=0x432b6560, args=...)
    at /home/jld/src/B2G.goldfish/gecko/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h:280

(...and from here into the JS engine; this last stack is on the main thread, running dom/media/tests/mochitest/test_dataChannel_basicAudio.html.)

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

so, what are our options other than message queues?  Certainly we could implement (or better re-use) a Mozilla-specific mechanism for this; and have that be a variant backend for the common sipcc code (like linux/etc).

We're still losing /tmp dirs on crashes last I noticed, so this might help with that as well.'

Ethan: what do you think would work best?  What are the requirements on the message channel so we can try to match it to existing mechanisms?

Comment 2

[Adam Roach [:abr]]

I looked into this a long time ago as part of an evaluation of what it would take to transition from pthreads to NSPR threads. The message queues are pretty much as simple as they get: message goes in from any arbitrary thread, and another thread (that generally spends its time blocked on that queue) reads the message and acts on it. Think about the first inter-thread queue you ever wrote, and this is it.

On a first order evaluation, it wouldn't be that hard to convert the message queues by just putting those same messages into a runnable and dispatching it to the right thread. The only part that isn't straightforward is the thread shutdown model, which would require a minor bit of SIPCC rearchitecting. Aside from that, it would be a fairly mechanical operation -- but you'll need to do the conversion to NSPR threads at the same time as you changed the message passing mechanism.

[Leaving Ethan's needinfo turned on in case he has something to add...]

Comment 3

[Ethan Hugg [:ehugg]]

Adam is correct that this a very simple message queue.  I noticed that the cpr_darwin_ipc.c and cpr_android_ipc.c use an even simpler mechanism for this (see cprGetMessage)  Adam do you have an opinion on whether we could just use that for all OS targets?  We could remove a bunch of redundant code and not have to move to NSPR threads.

Comment 4

[Maire Reavy [:mreavy]]

Adam, Ethan -- Do you have a sense of how much work it will be to fix this?  My understanding is that seccomp can't be pref'd until this and bug 964455 are fixed.  It seems like one of you would be the logical owner for this bug.

Comment 5

[Adam Roach [:abr]]

Okay, for the message queue issues, I don't see any reason we couldn't swap in the android version for the linux version -- either in general or when targeting B2G. If that's the case, it might just be a build system change (I would think the Android setup is probably more appropriate for B2G than the Linux one anyway). Worst case, we copy the cpr_android_ipc.c file over the cpr_linux_ipc.c file, and the issue goes away.

I'll let Ethan comment on the mkdtemp issue, as I really don't understand the purpose of this code. It appears to be setting up for some kind of interprocess communication named pipe, which I suspect we don't need. I would guess that this should be something that a person familiar with the SIP part of the code could remove fairly easily -- but I'm not that person.

Comment 6

[Ethan Hugg [:ehugg]]

I'm going to file a new bug to unify the cpr_xxx_ipc files and propose a fix there.  I'm not sure yet what the solution should be for sip_get_sock_dir using mkdtemp.  I will try some experiments and probably propose something in another separate bug.

Comment 7

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Adam Roach [:abr] from comment #5)
> I'll let Ethan comment on the mkdtemp issue, as I really don't understand the purpose of this code.

See Bug 794240 comment 3.

Comment 8

[Adam Roach [:abr]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #7)
> (In reply to Adam Roach [:abr] from comment #5)
> > I'll let Ethan comment on the mkdtemp issue, as I really don't understand the purpose of this code.
> 
> See Bug 794240 comment 3.

Yeah, I understand the mechanics of what's going on here, but I don't know why we're doing it. Do you know *why* we need named pipes? I suspect it's not useful for how we're using the library and could probably be ripped out without much ceremony -- but I'd like to be more certain before doing so.

Comment 9

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Sockets on Linux (unlike Windows) appear to be named pipes, is all I know. I added the mkdir to affect the permissions of the temp files written by the socket code, perhaps there are other more direct ways to do that. Is the problem here the actual mkdir call itself or the fact that we're using named pipes?

Either way I'm in support of any unifying cleanup here.

Comment 10

[Adam Roach [:abr]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #9)
> Sockets on Linux (unlike Windows) appear to be named pipes, is all I know. I
> added the mkdir to affect the permissions of the temp files written by the
> socket code, perhaps there are other more direct ways to do that. Is the
> problem here the actual mkdir call itself or the fact that we're using named
> pipes?

We're talking past each other. Ping me on IRC when you get a chance.

Comment 11

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #9)
> Sockets on Linux (unlike Windows) appear to be named pipes, is all I know. I
> added the mkdir to affect the permissions of the temp files written by the
> socket code, perhaps there are other more direct ways to do that. Is the
> problem here the actual mkdir call itself or the fact that we're using named
> pipes?

Both.  But it looks as if these sockets are just for inter-thread communication within a single process.  If that's the case, could this use socketpair() instead?  Or some other form of synchronization?

Comment 12

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

I now have a tree where I copied cpr_android_ipc.c over cpr_linux_ipc.c, changed sip_platform_task.c to use anonymous local-domain sockets (which might not exist on Windows?), and whitelisted a few things; this resulted in bug 969715.

Comment 13

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

It looks like win32 has its own version of sip_platform_task.c.  (Also, there's already an ifdef'ed out wrapper for socketpair in cpr_win_socket.c, and only cpr_win_socket.c; I don't understand this.)  I'm trying a patch now:

https://tbpl.mozilla.org/?tree=Try&rev=44c8cc2869a1
https://tbpl.mozilla.org/?tree=Try&rev=bbe1d6b86b58

But I don't know if those tests are sufficient coverage for that change.

Comment 14

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Attachment: bug966547-sipcc-socketpair-hg0.diff

Okay.  Exposes socketpair in CPR (with an always-fails stub on win32, as I'm informed that it doesn't have socketpair — it might be better to not declare the function at all, but I don't know what the stylistically appropriate way to do that would be) and uses it in the non-win32 sip_platform_task instead of named sockets.

I did a desktop build with this patch and tested it with http://mozilla.github.io/webrtc-landing/pc_test.html — the sockets are being created and used, and I can send video to myself.  Also, there are try builds: https://tbpl.mozilla.org/?tree=Try&rev=a03681580ace

Comment 15

[Guillaume Destuynder [:kang] [this account is no longer active]]

Comment on attachment 8376034 [details] [diff] [review]
bug966547-sipcc-socketpair-hg0.diff

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

Reviewed seccomp.h (and its fine)

Comment 16

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8376034 [details] [diff] [review]
bug966547-sipcc-socketpair-hg0.diff

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

Looks good to me (modulo I'm making assumptions about seccomp_filter.h, and haven't checked if this conflicts with the already-landed bug 969493)

Comment 17

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

(In reply to Randell Jesup [:jesup] from comment #16)
> Looks good to me (modulo I'm making assumptions about seccomp_filter.h, and
> haven't checked if this conflicts with the already-landed bug 969493)

Thanks for the review.

:kang reviewed the seccomp_filter.h part, so that's okay.

I've done try tuns with this and bug 969493 (and bug 974227 and bug 974230), so the patches should be stackable: https://tbpl.mozilla.org/?tree=Try&rev=e71ac667f826 https://tbpl.mozilla.org/?tree=Try&rev=0664ba4166d9 (the oranges appear to be unrelated).

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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