1439057 - Remove blanket access to /dev/shm from Linux content processes

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, enhancement, P1)

Description

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

Along similar lines to bug 1386404, we should do something about direct access to /dev/shm — this can allow the content process to interfere with (or spy on) interprocess communication that it shouldn't be touching.

We'll need to allow it if audio remoting is disabled, but hopefully that will continue to ride the trains to 60.

Our own IPC uses it for shared memory; on newer kernels we should be able to use memfd_create instead, but for older kernels we'll need to add an "anonymous temporary file" command to the broker.  Either IPC could be modified to call into libmozsandbox, or we could use seccomp-bpf to polyfill memfd_create if it's not implemented, so that IPC just needs to know about memfd_create.  (In the latter case, to avoid a race condition when the sandbox is started, IPC should try memfd_create a second time if memfd_create fails and then direct FS access fails.)

As a convenient side-effect, using the real memfd_create should increase graphics performance vs. brokered access to /dev/shm, although the effect may not be significant.

There may also be libraries calling shm_open/shm_unlink in libc; we could interpose them, but in general we can't allow non-O_EXCL opens.

Comment 1

[Masatoshi Kimura [:emk]]

Will this change affect bug 1438678?

Comment 2

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

(In reply to Masatoshi Kimura [:emk] from comment #1)
> Will this change affect bug 1438678?

Not directly — that's reading the file before sandboxing restricts filesystem access, so if the named case in IPC isn't changed it would still work.  However, I would like to remove named shared memory if possible, because nothing else is using it.  In any case, bug 1438678 is easier in some ways with anonymous shared memory anyway.

Comment 3

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

For reasons passing understanding, memfd_create has no C wrapper in glibc until 2.27… but also we need to be able to build on glibc versions that predate memfd_create being added to the kernel anyway.

So, new idea: have IPC call into libmozsandbox with a regular function call to use the broker client if available, and fall back to the old code if that fails; the broker, which is in libxul, can just use base::SharedMemory itself as a backend (maybe with some refactoring to allow consuming the object and taking the fd).

As for memfd_create… libmozsandbox already has a modern version of the syscall numbers header, and this would allow inserting memfd_create in that part of the call chain.  But that's is a little weird, since it doesn't really belong in sandboxing, and we'd need to have the unsandboxed parent process call the sandbox library in order to benefit.  Another way to do that would be to just drop the syscall numbers into shared_memory_posix.cc under some ifdefs.  But it wouldn't be necessary for sandboxing, just as a performance optimization, so should be a separate bug.

Comment 4

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

Attachment: Bug 1439057 - Tighten /dev/shm access in Linux content sandbox policy.

This isn't strictly necessary for the rest of this bug, but we're going
to be putting the "move" case of ShareToProcessCommon in the critical
path so we might as well optimize out the unnecessary dup+close.

This patch also tries to improve error handling in that method.

Review commit: https://reviewboard.mozilla.org/r/231386/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/231386/

Comment 5

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

Attachment: Bug 1439057 - Use the sandbox broker directly for shared memory file creation on Linux.

Previously the open() and unlink() calls would raise SIGSYS and each do a
broker RPC, and we had to allow access to all of /dev/shm (allowing the
possibility of interfering with other processes) in the broker policy.

This patch allowing creating an anonymous shared memory file with a
single broker RPC, bypassing the signal handling overhead and avoiding
the need to give access to any other files -- the broker returns the file
already unlinked and the client doesn't specify a filename or path.

The big picture for how this works: base::SharedMemory::Create in a
content process calls into libmozsandbox, which calls a method on its
broker client, which messages the parent process, which calls its own
instance of base::SharedMemory::Create, which doesn't have a broker
client and so does the open/unlink itself.

The broker uses a dedicated thread per child process, so the synchronous
IPC here will interfere only with other brokered file I/O for the same
process, and that was already the case before this patch.  (See also bug
1440203, which will avoid the IPC entirely on newer kernels.)

Bonus fix: the global SandboxReporterClient pointer is made atomic, to
match the new global SandboxBrokerClient pointer.

Review commit: https://reviewboard.mozilla.org/r/231388/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/231388/

Comment 6

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

Attachment: Bug 1439057 - Stop allowing access to /dev/shm in the Linux content sandbox.

Review commit: https://reviewboard.mozilla.org/r/231390/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/231390/

Comment 7

[Nathan Froyd [:froydnj]]

Comment on attachment 8962572 [details]
Bug 1439057 - Tighten /dev/shm access in Linux content sandbox policy.

https://reviewboard.mozilla.org/r/231386/#review237108

Makes sense to me.

::: ipc/chromium/src/base/shared_memory_posix.cc:137
(Diff revision 1)
>  }
>  
>  bool SharedMemory::ShareToProcessCommon(ProcessId processId,
>                                          SharedMemoryHandle *new_handle,
>                                          bool close_self) {
> -  const int new_fd = dup(mapped_file_);
> +  DCHECK(mapped_file_ >= 0);

This check seems like something the code should have been doing all along...or should we just return `false` here?  I guess we'll return `false` below, since `new_fd` will either receive the invalid `mapped_file_` or a `dup(mapped_file_)`, which is presumably also -1.

Comment 8

[Nathan Froyd [:froydnj]]

Comment on attachment 8962573 [details]
Bug 1439057 - Use the sandbox broker directly for shared memory file creation on Linux.

https://reviewboard.mozilla.org/r/231388/#review237118

r=me on the ipc/ parts.

::: ipc/chromium/src/base/shared_memory_posix.cc:109
(Diff revision 1)
> +    // Handle a possible race condition: CreateViaSandbox fails
> +    // because the sandbox isn't started, then the sandbox is started,
> +    // then filesystem access is attemped and fails.  In that case a
> +    // second CreateViaSandbox should succeed.

Just to be clear here, because I have zero sandboxing experience: the sandbox is being started on a different thread here and that's why we're racing?  Assuming this is the case, what guarantees that the sandbox startup has necessarily finished by the time we attempt this `CreateViaSandbox` call?  IOW, isn't this sort of a bandaid fix?

Comment 9

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

(In reply to Nathan Froyd [:froydnj] from comment #7)
> Comment on attachment 8962572 [details]
> > +  DCHECK(mapped_file_ >= 0);
> 
> This check seems like something the code should have been doing all
> along...or should we just return `false` here?  I guess we'll return `false`
> below, since `new_fd` will either receive the invalid `mapped_file_` or a
> `dup(mapped_file_)`, which is presumably also -1.

This was sort of checked before, because dup(-1) will fail with EBADF and set off the DCHECK.  (Note that DCHECK is broken at the moment; see bug 1436156.  I'm assuming it will eventually be fixed.)  The situation before and after this patch:

No mapped_file_, before: fail DCHECK, yield -1, return true.
Other failure, before: fail DCHECK, yield -1, return true.

No mapped_file_, after: fail DCHECK, return false.
Other failure, after: return false.


(In reply to Nathan Froyd [:froydnj] from comment #8)
> Comment on attachment 8962573 [details]
> > +    // Handle a possible race condition: CreateViaSandbox fails
> > +    // because the sandbox isn't started, then the sandbox is started,
> > +    // then filesystem access is attemped and fails.  In that case a
> > +    // second CreateViaSandbox should succeed.
> 
> Just to be clear here, because I have zero sandboxing experience: the
> sandbox is being started on a different thread here and that's why we're
> racing?  Assuming this is the case, what guarantees that the sandbox startup
> has necessarily finished by the time we attempt this `CreateViaSandbox`
> call?  IOW, isn't this sort of a bandaid fix?

This can be called from any thread, and SetContentProcessSandbox runs on the main thread[*], but it doesn't need to have finished.  Setting gSandboxBrokerClient (and thus enabling SandboxSharedMemoryCreate) happens-before any thread applies the seccomp-bpf policy, so there are three phases:

1. Accessing /dev/shm works; SandboxSharedMemoryCreate fails.
2. Accessing /dev/shm works; SandboxSharedMemoryCreate works.
3. Accessing /dev/shm fails; SandboxSharedMemoryCreate works.

But there is — and I didn't realize this until just now — a bug: the seccomp-bpf policy can be applied after the open() and before the unlink(), which leaks a zero-length file.  (This can also happen if the process crashes in that window.)  If I pass the broker fd at process startup then all of this goes away, but then we have even more hard-coded file descriptors (bug 1440207).  But the alternative is to… introduce some kind of reader/writer lock to avoid the race?  Which might be worse.

I suppose there's one other alternative: desupport content sandboxing on kernels < 3.11 and use O_TMPFILE.  This isn't completely absurd now that Ubuntu 12.04 is post-EOL, but I don't like it.


[*] More detail: on newer kernels the main thread makes one system call that atomically applies the seccomp-bpf policy to all threads; on older kernels it has to send async signals and get them each to apply the policy to themselves one by one.

Comment 10

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8962573 [details]
Bug 1439057 - Use the sandbox broker directly for shared memory file creation on Linux.

https://reviewboard.mozilla.org/r/231388/#review237894

Comment 11

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8962574 [details]
Bug 1439057 - Stop allowing access to /dev/shm in the Linux content sandbox.

https://reviewboard.mozilla.org/r/231390/#review237926

Comment 12

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

Comment on attachment 8962574 [details]
Bug 1439057 - Stop allowing access to /dev/shm in the Linux content sandbox.

https://reviewboard.mozilla.org/r/231390/#review237960

::: security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp
(Diff revision 1)
>  {
>    // Policy entries that are the same in every process go here, and
>    // are cached over the lifetime of the factory.
>  #if defined(MOZ_CONTENT_SANDBOX)
>    SandboxBroker::Policy* policy = new SandboxBroker::Policy;
> -  policy->AddDir(rdwrcr, "/dev/shm");

This needs to be conditional on `media.cubeb.sandbox`, because PulseAudio uses `/dev/shm` (and there are, as I type this, people on IRC `#media` flipping that pref to troubleshoot a bug, so we should try not to break it).

Comment 13

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

(In reply to Jed Davis [:jld] (⏰UTC-6) from comment #9)
> If I pass the broker fd at process startup then
> all of this goes away, but then we have even more hard-coded file
> descriptors (bug 1440207).  But the alternative is to… introduce some kind
> of reader/writer lock to avoid the race?  Which might be worse.

So, even if I make some much-needed improvements to LaunchOptions to dynamically allocate the fd, there's another problem: the broker policy depends on the remote type (because of file processes), which is in ContentParent and would have to be propagated down through GeckoChildProcessHost and into SandboxLaunchPrepare.  This also complicates the broker's lifetime — it used to be held by the ContentParent, but now it might need to delete itself via runnable.  

> I suppose there's one other alternative: desupport content sandboxing on
> kernels < 3.11 and use O_TMPFILE.  This isn't completely absurd now that
> Ubuntu 12.04 is post-EOL, but I don't like it.

Alternately: remove /dev/shm access if the kernel supports memfd_create (after bug 1440203), and otherwise limit it to a prefix including the pid (my work-in-progress patches for bug 1447867 already give us such a prefix, as a side effect of reducing the possibility of name collision) which I can't think of any useful ways to exploit at the moment.

Comment 14

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

Comment on attachment 8962572 [details]
Bug 1439057 - Tighten /dev/shm access in Linux content sandbox policy.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231386/diff/1-2/

Comment 15

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

New approach, based on the name prefixes introduced in bug 1447867; no broker changes needed.

Comment 16

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8962572 [details]
Bug 1439057 - Tighten /dev/shm access in Linux content sandbox policy.

https://reviewboard.mozilla.org/r/231386/#review243330

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/baeab3bff807
Tighten /dev/shm access in Linux content sandbox policy. r=froydnj,gcp

Comment 18

[Raul Gurzau (:RaulG)]

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

Comment 19

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7793bcae69f6
Backed out 2 changesets (bug 1439057, bug 1447867)

Comment 20

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

Backed out: https://hg.mozilla.org/mozilla-central/rev/7793bcae69f6

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/02395f0e8074
Tighten /dev/shm access in Linux content sandbox policy. r=gcp

Comment 22

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/02395f0e8074

Comment 23

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

Sorry; forgot to flip the status-61 flag when backing out.