906996 - Remove unlink() from seccomp-bpf whitelist

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

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

STR: start b2g Messages app; poke the button to compose a new message.  Crash is immediate and 100% reproducible, for me.  This is on keon, but I don't think it matters.

I'm assuming that seccomp is being reasonable in disallowing unlink, so I've filed this bug with where the [[Modules/Core]] wiki page maps the files that look sort of responsible.  Adjust categorization as needed.

(gdb) bt
#0  unlink () at bionic/libc/arch-arm/syscalls/unlink.S:10
#1  0x420087aa in PR_Delete (name=0x43bde8b8 "/data/local/tmp/mozilla-temp-561545566")
    at /home/jld/src/B2G/gecko/nsprpub/pr/src/pthreads/ptio.c:3598
#2  0x4108928e in nsLocalFile::OpenNSPRFileDesc (this=0x442dc430, flags=-2147483644, 
    mode=<value optimized out>, _retval=0xbec8dbd4)
    at /home/jld/src/B2G/gecko/xpcom/io/nsLocalFileUnix.cpp:390
#3  0x4108aa16 in NS_OpenAnonymousTemporaryFile (aOutFileDesc=0xbec8dbd4)
    at /home/jld/src/B2G/gecko/xpcom/io/nsAnonymousTemporaryFile.cpp:99
#4  0x40a66d94 in mozilla::MediaCache::Init (this=0x446c8510)
    at /home/jld/src/B2G/gecko/content/media/MediaCache.cpp:536
#5  0x40a66e56 in InitMediaCache (this=0x44508088)
    at /home/jld/src/B2G/gecko/content/media/MediaCache.cpp:612
#6  mozilla::MediaCacheStream::Init (this=0x44508088)
    at /home/jld/src/B2G/gecko/content/media/MediaCache.cpp:2288
#7  0x40a72016 in mozilla::ChannelMediaResource::Open (this=0x44508000, aStreamListener=0x446a5638)
    at /home/jld/src/B2G/gecko/content/media/MediaResource.cpp:593
#8  0x40a68d90 in mozilla::MediaDecoder::OpenResource (this=0x442077a0, aStreamListener=0x446a5638)
    at /home/jld/src/B2G/gecko/content/media/MediaDecoder.cpp:462
#9  0x40a69dd6 in mozilla::MediaDecoder::Load (this=0x10, aStreamListener=0x1, aCloneDonor=0x1)
    at /home/jld/src/B2G/gecko/content/media/MediaDecoder.cpp:476
#10 0x40a2cb02 in mozilla::dom::HTMLMediaElement::FinishDecoderSetup (this=0x4369fc10, 
    aDecoder=0x442077a0, aStream=<value optimized out>, aListener=0x446a5638, aCloneDonor=0x0)
    at /home/jld/src/B2G/gecko/content/html/content/src/HTMLMediaElement.cpp:2546
#11 0x40a2d69a in mozilla::dom::HTMLMediaElement::InitializeDecoderForChannel (this=0x4369fc10, 
    aChannel=<value optimized out>, aListener=0x446a5638)
    at /home/jld/src/B2G/gecko/content/html/content/src/HTMLMediaElement.cpp:2507
#12 0x40a2e72a in mozilla::dom::HTMLMediaElement::MediaLoadListener::OnStartRequest (
    this=0x446a5620, aRequest=0x43ef4ac0, aContext=0x0)
    at /home/jld/src/B2G/gecko/content/html/content/src/HTMLMediaElement.cpp:338
#13 0x407f589e in nsJARChannel::OnStartRequest (this=<value optimized out>, 
    req=<value optimized out>, ctx=<value optimized out>)
    at /home/jld/src/B2G/gecko/modules/libjar/nsJARChannel.cpp:969
#14 0x4071f0de in nsInputStreamPump::OnStateStart (this=0x43fae520)
    at /home/jld/src/B2G/gecko/netwerk/base/src/nsInputStreamPump.cpp:463
#15 0x4071f3ba in nsInputStreamPump::OnInputStreamReady (this=0x43fae520, 
    stream=<value optimized out>)
    at /home/jld/src/B2G/gecko/netwerk/base/src/nsInputStreamPump.cpp:388
#16 0x41090c1a in nsInputStreamReadyEvent::Run (this=0x446a5660)
    at /home/jld/src/B2G/gecko/xpcom/io/nsStreamUtils.cpp:160
#17 0x41099d10 in nsThread::ProcessNextEvent (this=0x40316940, mayWait=<value optimized out>, 
    result=0xbec8dff7) at /home/jld/src/B2G/gecko/xpcom/threads/nsThread.cpp:622
#18 0x41079778 in NS_ProcessNextEvent (thread=0x43fae520, mayWait=false)
    at /home/jld/src/B2G/objdir-gecko/xpcom/build/nsThreadUtils.cpp:238
#19 0x40e1ae4c in mozilla::ipc::MessagePump::Run (this=0x40301b80, aDelegate=0xbec8e904)
    at /home/jld/src/B2G/gecko/ipc/glue/MessagePump.cpp:81
#20 0x40e1aefe in mozilla::ipc::MessagePumpForChildProcess::Run (this=0x40301b80, 
    aDelegate=0xbec8e904) at /home/jld/src/B2G/gecko/ipc/glue/MessagePump.cpp:234
#21 0x410b6194 in MessageLoop::RunInternal (this=0x1000000)
    at /home/jld/src/B2G/gecko/ipc/chromium/src/base/message_loop.cc:220
#22 0x410b6212 in MessageLoop::RunHandler (this=0xbec8e904)
    at /home/jld/src/B2G/gecko/ipc/chromium/src/base/message_loop.cc:213
#23 MessageLoop::Run (this=0xbec8e904)
    at /home/jld/src/B2G/gecko/ipc/chromium/src/base/message_loop.cc:187
#24 0x40dcf630 in nsBaseAppShell::Run (this=0x4368de80)
    at /home/jld/src/B2G/gecko/widget/xpwidgets/nsBaseAppShell.cpp:161
#25 0x406c461a in XRE_RunAppShell ()
    at /home/jld/src/B2G/gecko/toolkit/xre/nsEmbedFunctions.cpp:679
#26 0x40e1aecc in mozilla::ipc::MessagePumpForChildProcess::Run (this=0x40301b80, 
    aDelegate=0xbec8e904) at /home/jld/src/B2G/gecko/ipc/glue/MessagePump.cpp:201
#27 0x410b6194 in MessageLoop::RunInternal (this=0x4368de80)
    at /home/jld/src/B2G/gecko/ipc/chromium/src/base/message_loop.cc:220
#28 0x410b6212 in MessageLoop::RunHandler (this=0xbec8e904)
    at /home/jld/src/B2G/gecko/ipc/chromium/src/base/message_loop.cc:213
#29 MessageLoop::Run (this=0xbec8e904)
    at /home/jld/src/B2G/gecko/ipc/chromium/src/base/message_loop.cc:187
#30 0x406c4b0e in XRE_InitChildProcess (aArgc=-1094129004, aArgv=0xbec8ea0c, aProcess=1077134336)
    at /home/jld/src/B2G/gecko/toolkit/xre/nsEmbedFunctions.cpp:516
#31 0x000086a0 in main (argc=7, argv=0xbec8ea94)
    at /home/jld/src/B2G/gecko/ipc/app/MozillaRuntimeMain.cpp:85

Comment 1

[Julien Wajsberg [:julienw]]

So, this doesn't happen on Buri.

It happens on Keon 100%.

I tried with the same gecko in both, but the cause is probably deeper.

Is there something we can try in the Messages app to workaround this ? I don't understand exactly what happens here :)

Comment 2

[Julien Wajsberg [:julienw]]

Ok, I've cat /proc/<N>/status, and on buri there is no "seccomp" line.

Comment 3

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

Right.  Currently seccomp is only keon and peak, because :kang had to backport the seccomp-bpf kernel patches, and those are the only targets where the kernel is built from source as part of the B2G/Gonk build.  Some of the TARGET_NO_KERNEL devices might be fixable, if they can use a documented protocol to flash the kernel image, but I don't know if buri/hamachi is one of them.

Comment 4

[Julien Wajsberg [:julienw]]

How could we solve this correctly in your opinion?

Why is this happening only in the Messages app?

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Julien Wajsberg [:julienw] from comment #4)
> How could we solve this correctly in your opinion?

We either need to move the file I/O for the media cache to the parent process, or we need to use argument filtering to ensure that we allow unlinking only of files in the media cache but disallow unlinking of other files.

Comment 6

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

(In reply to Brian Smith (:briansmith, was :[e-mail]) from comment #5)
> We either need to move the file I/O for the media cache to the parent
> process, or we need to use argument filtering to ensure that we allow
> unlinking only of files in the media cache but disallow unlinking of other
> files.

seccomp-bpf can't inspect the path passed to unlink.  There's no facility for reading the process's memory, and it would be a time-of-check/time-of-use issue if there were one.  The closest thing we could do, as far as I'm aware, is intercept the library call in libmozglue and remote it to the parent process.

Comment 7

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Looks like it's trying to open an anonymous, temporary file. Can NS_OpenAnonymousTemporaryFile remote to the parent?

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

The anonymous temporary file is for caching downloaded media data. We need that to play video and audio. The file is unlinked since we don't need the media cache to be persistent.

The media cache usage could be remoted to a single media cache in the parent process. We discussed it in bug 785662 but Roc was opposed to it at that time. The advantage would be that we'd not need a separate media cache per process, so we'd reduce disk usage by the media cache.

I'm not sure if anyone from the media team is free to work on this.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The media cache on B2G is tiny, only 4MB. Using a shared cache would be nice in theory but unnecessary complexity in practice.

I assume our seccomp-bpf filters let us call close()? In that case, we could extend NS_OpenAnonymousTemporaryFile so that in a content process, it makes an IPDL call to the parent process to open and unlink the file, passing the file descriptor back to the content process.

Comment 10

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

I think MediaCache was the only unlink()er; I'm currently testing whether that's correct.

Comment 11

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

(In reply to Jed Davis [:jld] from comment #10)
> I think MediaCache was the only unlink()er; I'm currently testing whether
> that's correct.

Nope: https://tbpl.mozilla.org/?tree=Try&rev=911fe7807b54

Comment 12

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

Attachment: bug906996-finally-hg0.diff

It took a while, but it looks like we finally got there.

This try push is a little confusing because it was on top of inbound: https://treeherder.mozilla.org/#/jobs?repo=try&revision=db922dce686a

Comment 13

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

Comment on attachment 8576193 [details] [diff] [review]
bug906996-finally-hg0.diff

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

woohoo!

Comment 14

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

Try run linked in comment #12; it should be correctly starred now.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/35ad2e5b036b

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/35ad2e5b036b