Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Issue 413855 Disallow interaction with other processes at the seccomp-bpf layer
Starred by 4 users Project Member Reported by jln@chromium.org, Sep 12 2014 Back to list
Status: Verified
Owner:
Closed: Jan 2015
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux, Chrome
Pri: 2
Type: Bug


Sign in to add a comment
As explained in https://code.google.com/p/chromium/wiki/LinuxSandboxing, we have a layered approach to sandboxing in most cases.

We still would like to prevent process interaction at the seccomp-bpf layer. It is especially useful for renderers since they share a single PID namespace.

We have recently restricted kill and are in the process of restricting "clockid" operations such as clock_getcpuclockid().

We should audit the baseline policy as well as the renderer/utility/GPU policies for more cases like this and close them.
 
Comment 1 by rickyz@chromium.org, Sep 13 2014
I looked through the various policies, and these are the syscalls I saw which allow process interaction:

baseline policy:
clock_gettime (I see you've already added a RestrictClockID for this)
get_robust_list (I don't see any calls to this in glibc and it has no library function, can we just block it?) 

cros arm gpu policy:
all AdvancedScheduler syscalls

gpu policy:
sched_getaffinity
sched_setaffinity
setpriority
(unrelated, this also allows mmap and mprotect - is it worth  blocking PROT_EXEC and MAP_FIXED?)

ppapi and renderer policies:
sched_getaffinity
sched_getparam
sched_getscheduler
sched_setscheduler

What is usually a good way to tell whether adding new restrictions breaks anything?  I didn't see anything in glibc or chromium code search that seems to rely on calling these functions on a different process/thread, but I wouldn't be surprised if I missed some things.
Project Member Comment 2 by bugdroid1@chromium.org, Sep 13 2014
Comment 3 by jln@chromium.org, Sep 15 2014
Good investigation! If you want to, feel free to assign this bug to yourself.

In general, we expect to have pretty good test coverage of all features, via the browser_tests and content_browser_tests features. They're a good way to test. Unfortunately, there are still a lot of cases where we only figure out what happens when our users (hopefully dev/beta channels) run into issue.

You may have notice that for a lot of syscalls, we redirect to a SIGSYS handler that will crash the process (and thus generate a crash report). When we add a new restriction, we try to study if we get any related crash reports.

It's also indeed good to do a sanity check and see if you can find these functions used somewhere in the source (gclient recurse git grep "blah"). V8 and libevent have both been prime candidates int he past.
Of course we still have dependencies that we don't replicate in third_party (like libc), which makes things difficult. Maybe it would be a good idea one day to formally list them so that we can more thoroughly search for system call sources.

get_robust_list is particularly interesting because anything that relates to futexes scare us (and it has a history of vulnerabilities). We should look into restricting it indeed. mseaborn@ will be a good person to add on a bug if we do that, he knows futex very well and is usually a good point of contact for what NaCl (which uses a modified baseline policy) needs.
Comment 4 by rickyz@chromium.org, Sep 15 2014
On further digging, the various sched_ and setpriority calls are actually difficult to filter, as they are mostly called from threads, and we do not know the valid thread IDs when setting up the sandbox (theoretically, all of the calls that I saw could have specified 0 for the pid parameter to indicate the current thread, but glibc code usually specifies it explicitly).

On the other hand, after a little more reading, get_robust_list looks pretty safe to remove - in fact it's so rarely used that Kees tried to remove it once :-)  It sounds like the only valid usecases for the function are for debugging and checkpointing process (http://www.openwall.com/lists/kernel-hardening/2012/08/03/20), and that's why the syscall is still there.
Comment 5 by jln@chromium.org, Sep 15 2014
Blockedon: chromium:399473
Ricky: take a look at issue 399473 for setpriority.

I've changed base/ so that we use setpriority with 0. And I allowed it with the process id / first thread's tid as an argument.

Couldn't the same approach be used for sched_?

Another strategy would be to use a SIGSYS handler:
- Allow the tid to be 0 or the id of the thread group in seccomp-bpf, otherwise SIGSYS
- The SIGSYS handler rewrite the tid to 0 if tid == gettid().

So far we didn't do the second part, but we could. There would be one difficulty with NaCl, because NaCl doesn't let us do non-crashing SIGSYS handlers for now (I only added support for crashing SIGSYS handler); but we could add that if needed.
Comment 6 by rickyz@chromium.org, Sep 16 2014
The problematic calls were pretty deep in glibc, so I don't think we can fix the callers.  I'll take a look at the SIGSYS approach - hopefully the extra trip to userspace shouldn't be a big problem for these calls.
Comment 7 by jln@chromium.org, Sep 16 2014
Are you sure though that these would get called? In particular are you sure that this isn't code that runs only monothreaded for instance? (Where tgid == pid == tid).

The SIGSYS approach should be fine, we could do renderers + utility processes first and then change NaCl to allow non crashing SIGSYS.
Project Member Comment 8 by bugdroid1@chromium.org, Sep 16 2014
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a75e8729dc917c0089a725e67fa2e9feaec0baef

commit a75e8729dc917c0089a725e67fa2e9feaec0baef
Author: rickyz <rickyz@chromium.org>
Date: Tue Sep 16 02:28:10 2014

Linux sandbox: Disallow get_robust_list and set_robust_list.

These are only used for futexes that are shared between processes, which should not be happening in Chromium.
BUG= 413855 

Review URL: https://codereview.chromium.org/569713004

Cr-Commit-Position: refs/heads/master@{#294986}

[modify] https://chromium.googlesource.com/chromium/src.git/+/a75e8729dc917c0089a725e67fa2e9feaec0baef/sandbox/linux/seccomp-bpf-helpers/baseline_policy.cc
[modify] https://chromium.googlesource.com/chromium/src.git/+/a75e8729dc917c0089a725e67fa2e9feaec0baef/sandbox/linux/seccomp-bpf-helpers/syscall_sets.cc

Project Member Comment 9 by bugdroid1@chromium.org, Sep 26 2014
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/282ba301cf990ce291c45a05b5226df6804ae271

commit 282ba301cf990ce291c45a05b5226df6804ae271
Author: rickyz <rickyz@chromium.org>
Date: Fri Sep 26 22:33:11 2014

Linux sandbox: Allow restricting sched_* on other processes.

Adds a RestrictSchedTarget parameter restriction which only allows
sched_* syscalls if the pid argument is the sandboxed process's pid or
if the pid is 0, which means the current thread.  glibc's pthread
implementation sometimes calls these syscalls with pid equal to the
current tid.  On these calls, the policy triggers a SIGSYS, and the
SIGSYS handler reruns the syscall with a pid argument of 0.

R=jln@chromium.org
BUG= 413855 

Review URL: https://codereview.chromium.org/590213003

Cr-Commit-Position: refs/heads/master@{#297059}

[modify] https://chromium.googlesource.com/chromium/src.git/+/282ba301cf990ce291c45a05b5226df6804ae271/sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc
[modify] https://chromium.googlesource.com/chromium/src.git/+/282ba301cf990ce291c45a05b5226df6804ae271/sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.h
[modify] https://chromium.googlesource.com/chromium/src.git/+/282ba301cf990ce291c45a05b5226df6804ae271/sandbox/linux/seccomp-bpf-helpers/syscall_parameters_restrictions.cc
[modify] https://chromium.googlesource.com/chromium/src.git/+/282ba301cf990ce291c45a05b5226df6804ae271/sandbox/linux/seccomp-bpf-helpers/syscall_parameters_restrictions.h
[modify] https://chromium.googlesource.com/chromium/src.git/+/282ba301cf990ce291c45a05b5226df6804ae271/sandbox/linux/seccomp-bpf-helpers/syscall_parameters_restrictions_unittests.cc

Cc: -rickyz@chromium.org jln@chromium.org
Owner: rickyz@chromium.org
Project Member Comment 12 by bugdroid1@chromium.org, Oct 10 2014
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9c2d68671be845029dc83ee5667aeb32da693d2b

commit 9c2d68671be845029dc83ee5667aeb32da693d2b
Author: rickyz <rickyz@chromium.org>
Date: Fri Oct 10 21:46:23 2014

Linux sandbox: Restrict sched_* calls in the renderer policy.

BUG= 413855 

Review URL: https://codereview.chromium.org/639183003

Cr-Commit-Position: refs/heads/master@{#299193}

[modify] https://chromium.googlesource.com/chromium/src.git/+/9c2d68671be845029dc83ee5667aeb32da693d2b/content/common/sandbox_linux/bpf_renderer_policy_linux.cc

Project Member Comment 13 by bugdroid1@chromium.org, Oct 17 2014
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a401ea2a1d6bc90d2b3d7eab1fd80e8d45e83dea

commit a401ea2a1d6bc90d2b3d7eab1fd80e8d45e83dea
Author: rickyz <rickyz@chromium.org>
Date: Fri Oct 17 02:33:15 2014

Linux sandbox: Restrict sched_* and ioprio_* calls in the cros arm GPU policy.

BUG= 413855 

Review URL: https://codereview.chromium.org/640123002

Cr-Commit-Position: refs/heads/master@{#300046}

[modify] https://chromium.googlesource.com/chromium/src.git/+/a401ea2a1d6bc90d2b3d7eab1fd80e8d45e83dea/content/common/sandbox_linux/bpf_cros_arm_gpu_policy_linux.cc

Hi,

out of curiosity:

I'm fairly sure you've already given some thought to the idea of having separate PID namespaces per process (render) and rejected it for good reasons. Is it due to some design problems (e.g. need to create renders from other places than the chrome_sandbox?"), or performance problems, or something other (e.g. processes need to interact with each other with signals or so)?
Comment 15 by jln@chromium.org, Oct 21 2014
There are a few problems with this, but I'll only name the main problem at the moment: 

- A PID namespace requires an init process
- An init process cannot receive signals others than SIGKILL

So this means that for each renderer, we would need a separate init process. I'm warry of doing it by forking because it would become costly (once the address spaces diverge). If we want to execve a tiny init helper (for each renderer process), we also need to essentially kill the Zygote, which has other issues.

None of these options are great. We should investigate a kernel patch to make PID namespaces less expensive by allowing simple one-process namespaces, or perhaps a simple prctl that says "I can't interract with other processes".
Comment 16 by rickyz@google.com, Oct 21 2014
I did a little more testing, and it looks like the init process can receive signals that it has registered a handler for.

The interface isn't especially friendly for having one process create a bunch of children each in a different PID namespace. It looks like unshare(CLONE_NEWPID) cannot be called multiple times, and the clone system call doesn't update glibc's PID cache. glibc's clone wrapper does update the PID cache, but it requires the caller to give a new stack for the child to run on, even if CLONE_VM is not specified :-( This is annoying, but could it be worth it to get cleaner separation of processes?
Project Member Comment 17 by bugdroid1@chromium.org, Oct 29 2014
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3638a21d79c71ed0b18c1591ce2caea86cfb1aba

commit 3638a21d79c71ed0b18c1591ce2caea86cfb1aba
Author: rickyz <rickyz@chromium.org>
Date: Wed Oct 29 23:50:24 2014

Linux sandbox: Tighten up the NaCl sandbox policy.

Previously, we allowed socket syscalls which were only needed by the
NaCl gdb stub. Now, we only allow these syscalls when the
--enable-nacl-debug flag is present.

Also restricts cross-process interaction for sched_* syscalls now that
non-crashing SIGSYS handlers are allowed under NaCl.

BUG= 270914 , 413855 

Review URL: https://codereview.chromium.org/670603002

Cr-Commit-Position: refs/heads/master@{#301982}

[modify] https://chromium.googlesource.com/chromium/src.git/+/3638a21d79c71ed0b18c1591ce2caea86cfb1aba/chrome/browser/chrome_content_browser_client.cc
[modify] https://chromium.googlesource.com/chromium/src.git/+/3638a21d79c71ed0b18c1591ce2caea86cfb1aba/components/nacl/loader/DEPS
[modify] https://chromium.googlesource.com/chromium/src.git/+/3638a21d79c71ed0b18c1591ce2caea86cfb1aba/components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc
[modify] https://chromium.googlesource.com/chromium/src.git/+/3638a21d79c71ed0b18c1591ce2caea86cfb1aba/components/nacl/zygote/nacl_fork_delegate_linux.cc

Status: Fixed
Labels: VerifyIn-41
Labels: VerifyIn-42
Labels: VerifyIn-43
Labels: VerifyIn-44
Comment 24 by mu...@chromium.org, Jun 23 2015
Labels: -VerifyIn-43
Last 43 build of record is M43-STABLE-8 (6946.63.0, 43.0.2357.130) . All VerifyIn-43 are now VerifyIn-44
Labels: VerifyIn-45
Labels: VerifyIn-46
Labels: VerifyIn-47
Labels: VerifyIn-48
Labels: -VerifyIn-48
Labels: VerifyIn-49
Labels: -VerifyIn-49
Status: Verified
Bulk verified
Sign in to add a comment