1151607 - Filesystem and network isolation for Linux GeckoMediaPlugin child processes (with unprivileged user namespaces)

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

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

Bug 986397 covers a lot of territory, and a lot of it has annoying prerequisites, but:

1. GeckoMediaPlugin child processes are already well-behaved in terms of resource access.
2. Everything besides PID namespace isolation can be done without changes to how child processes are started.
3. Unprivileged user namespaces allow avoiding the difficulties of shipping a setuid root executable, at the cost of universal support.

So this bug is for that combination of constraints: GMP (not content), applying chroot and network namespace and SysV IPC namespace isolation (but not pid namespaces), on kernels that support unprivileged user namespaces.

This will borrow the part of bug 1088387 about having a hook into sandboxing while the child process is single-threaded (but not the questionable cross-thread forwarding of arbitrary syscalls which was the focus of that bug).

Comment 1

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

Attachment: Step 0: sort includes to make the following patches cleaner.

This is trivial cleanup and could be squashed into the next patch, but it's clearer where #includes are/aren't being added, this way.

Comment 2

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

Attachment: Step 1: Add Linux sandboxing hook for when child processes are still single-threaded.

This is, mostly, the part that was broken out of bug 1088387.  Needs IPC review for touching early content process startup.

Comment 3

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

Attachment: Step 2: Apply net/ipc namespace separation and chroot to media plugins.

…and this is where most of the actual code for this bug is.

Comment 4

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

Comment on attachment 8588801 [details] [diff] [review]
Step 2: Apply net/ipc namespace separation and chroot to media plugins.

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

::: security/sandbox/linux/common/SandboxInfo.cpp
@@ +116,5 @@
> +  //
> +  // Also, unshare never works under valgrind, but testing with clone
> +  // might return a false positive:
> +#ifdef MOZ_VALGRIND
> +  if (RUNNING_ON_VALGRIND) {

Insufficient ifdef; will fix.

Comment 5

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

Comment on attachment 8588801 [details] [diff] [review]
Step 2: Apply net/ipc namespace separation and chroot to media plugins.

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

::: security/sandbox/linux/LinuxCapabilities.h
@@ +66,5 @@
> +    return SetCurrentRaw();
> +  }
> +
> +  void Normalize() {
> +    for (size_t i = 0; i < _LINUX_CAPABILITY_U32S_3; ++i) {

…and there are Android kernel header issues, because of course there are.  I thought I'd checked for that locally, but maybe not on the final version of the patch.  Will fix.

Comment 6

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

Attachment: Step 1.5: Avoid unlikely false positives in Linux SandboxInfo feature detection.

Better idea: handle the possibilty of valgrind by directly testing whether unshare() is supported.  (The syscall been in the kernel since 2.6.16, so calling it with no flags – which is explicitly documented as being allowed and a no-op — should fail only in the case of that kind of interposition.)

Also, breaking this out into its own patch.

Comment 7

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

Attachment: bug1151607-p2-isolate-hg0.diff

Now with assorted fixes to the B2G build.  Verified on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=49479ce9e0df

Comment 8

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

Comment on attachment 8588800 [details] [diff] [review]
Step 1: Add Linux sandboxing hook for when child processes are still single-threaded.

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

::: security/sandbox/linux/SandboxUtil.cpp
@@ +23,5 @@
> +    MOZ_DIAGNOSTIC_ASSERT(false, "Couldn't access /proc/self/task!");
> +    return false;
> +  }
> +  MOZ_DIAGNOSTIC_ASSERT(sb.st_nlink >= 3);
> +  return sb.st_nlink == 3;

How does this work? (on a regular linux here i see < 3 hardlinks for a regular process, as in non-gecko without threads)

Comment 9

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

(In reply to Guillaume Destuynder [:kang] from comment #8)
> Comment on attachment 8588800 [details] [diff] [review]
> > +  MOZ_DIAGNOSTIC_ASSERT(sb.st_nlink >= 3);
> > +  return sb.st_nlink == 3;
> 
> How does this work? (on a regular linux here i see < 3 hardlinks for a
> regular process, as in non-gecko without threads)

The three links should be the link from the parent directory, the "." link, and the ".." link from the child.  Can you pastebin the output of "stat /proc/self/task" where you're seeing fewer links for single-threaded processes?

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8588800 [details] [diff] [review]
Step 1: Add Linux sandboxing hook for when child processes are still single-threaded.

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

r=me on the ContentChild changes

::: security/sandbox/linux/Sandbox.cpp
@@ +422,5 @@
>    BroadcastSetThreadSandbox(aType);
>  }
>  
> +void
> +SandboxEarlyInit(GeckoProcessType aType, bool aIsNuwa)

I don't get it, |aIsNuwa| is unused?

::: security/sandbox/linux/Sandbox.h
@@ +24,5 @@
>  
>  namespace mozilla {
>  
> +// This must be called early, while the process is still single-threaded.
> +MOZ_SANDBOX_EXPORT void SandboxEarlyInit(GeckoProcessType aType, bool aIsNuwa);

I *think* we can forward-declare enums now, so you shouldn't need the nsXULAppAPI include here.

Comment 11

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

(In reply to Jed Davis [:jld] {UTC-7} from comment #9)
> (In reply to Guillaume Destuynder [:kang] from comment #8)
> > Comment on attachment 8588800 [details] [diff] [review]
> > > +  MOZ_DIAGNOSTIC_ASSERT(sb.st_nlink >= 3);
> > > +  return sb.st_nlink == 3;
> > 
> > How does this work? (on a regular linux here i see < 3 hardlinks for a
> > regular process, as in non-gecko without threads)
> 
> The three links should be the link from the parent directory, the "." link,
> and the ".." link from the child.  Can you pastebin the output of "stat
> /proc/self/task" where you're seeing fewer links for single-threaded
> processes?

My method of calling stat (via find -links) didnt actually call stat the way i expected, it works fine calling stat directly. All good!

Comment 12

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

Comment on attachment 8589139 [details] [diff] [review]
bug1151607-p2-isolate-hg0.diff

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

::: security/sandbox/linux/SandboxChroot.cpp
@@ +73,5 @@
> +
> +static int
> +OpenDeletedDirectory()
> +{
> +  // We don't need (nor want) this to persist between invocations of

it still *may* persist until reboot/cleanup if deletion fails. Won't really cause a problem tho, mkdtemp should choose the next available name.

@@ +159,5 @@
> +  caps.Effective(CAP_SYS_CHROOT) = true;
> +  // And leave everything else off.
> +  if (!caps.SetCurrent()) {
> +    SANDBOX_LOG_ERROR("capset: %s", strerror(errno));
> +    MOZ_CRASH("Can't limit chroot thread's capabilities");

how do we ensure we generally get this cap right now?

Comment 13

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

(In reply to Ben Turner [:bent] (use the needinfo flag!) from comment #10)
> > +void
> > +SandboxEarlyInit(GeckoProcessType aType, bool aIsNuwa)
> 
> I don't get it, |aIsNuwa| is unused?

Most of the body of that function is added in the last patch.  (I originally had a comment saying that, which the later patch removed, but I took it out at some point for some reason.)

> > +// This must be called early, while the process is still single-threaded.
> > +MOZ_SANDBOX_EXPORT void SandboxEarlyInit(GeckoProcessType aType, bool aIsNuwa);
> 
> I *think* we can forward-declare enums now, so you shouldn't need the
> nsXULAppAPI include here.

It looks like the enum needs to be defined with an explicit base type (e.g., `enum GeckoProcessType : uint8_t { ... }`) to be able to do that.

Comment 14

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

(In reply to Guillaume Destuynder [:kang] (PTO til 2015-04-19) from comment #12)
> @@ +159,5 @@
> > +  caps.Effective(CAP_SYS_CHROOT) = true;
> > +  // And leave everything else off.
> > +  if (!caps.SetCurrent()) {
> > +    SANDBOX_LOG_ERROR("capset: %s", strerror(errno));
> > +    MOZ_CRASH("Can't limit chroot thread's capabilities");
> 
> how do we ensure we generally get this cap right now?

This is in SandboxChroot::ThreadMain, which is called from SandboxChroot::StaticThreadMain, which is called by the pthread_create in SandboxChroot::Prepare, which checks for the cap before creating the thread, which is created as a clone of pthread_create's caller.  I'll add a comment.

Comment 15

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/53a41684adcb
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ed5d64f054b
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a186f904c5d
https://hg.mozilla.org/integration/mozilla-inbound/rev/acc410f0b28c

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/53a41684adcb
https://hg.mozilla.org/mozilla-central/rev/4ed5d64f054b
https://hg.mozilla.org/mozilla-central/rev/9a186f904c5d
https://hg.mozilla.org/mozilla-central/rev/acc410f0b28c