Closed Bug 1078838 Opened 8 years ago Closed 8 years ago

Lock down Linux content process clone(2) flags

Categories

(Core :: Security, defect)

All
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: jld, Assigned: jld)

References

Details

Attachments

(1 file, 1 obsolete file)

We should restrict clone(2) in sandboxed processes to the specific set of flags used by pthread_create(3).  Advantages: reduce attack surface in general, block things like CLONE_NEWUSER in particular, and prevent creating new thread groups (so that one SIGKILL will terminate all tasks that the content process could have created).

This is already in place for GMP, because so far it's supported only on GNU/Linux and glibc's thread creation clone flags haven't changed in a decade.  Desktop content would work the same way.

On B2G, it varies.  We should be able to specialize by Android API level:

 <=17: CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_SIGHAND | CLONE_THREAD | CLONE_SYSVSEM | CLONE_DETACHED
(https://android.googlesource.com/platform/bionic/+/40eabe24e4e3ae8ebe437f1f4e43cf39cbba2e9e)
18-20: CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_SIGHAND | CLONE_THREAD | CLONE_SYSVSEM
(https://android.googlesource.com/platform/bionic/+/70b24b1cc2a1a4436b1fea3f8b76616fdcb27224)
(https://android.googlesource.com/platform/bionic/+/877ec6d90418ff1d6597147d355a2229fdffae7e)
 >=21: CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_SIGHAND | CLONE_THREAD | CLONE_SYSVSEM | CLONE_SETTLS | CLONE_PARENT_SETTID | CLONE_CHILD_CLEARTID
Attached patch bug1078838-cloneflags-hg0.diff (obsolete) — Splinter Review
This seems to work.  I should probably try it on a few more phones first.  And it'd be good for someone who knows Android versioning better than I do to look at that part of it.
Attachment #8506474 - Flags: review?(gdestuynder)
Attachment #8506474 - Flags: feedback?(mwu)
Comment on attachment 8506474 [details] [diff] [review]
bug1078838-cloneflags-hg0.diff

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

::: security/sandbox/linux/SandboxFilter.cpp
@@ +79,5 @@
> +  // value has changed a few times, but it's more or less a function
> +  // of Android releases.
> +  static const int flags_common = CLONE_VM | CLONE_FS | CLONE_FILES |
> +    CLONE_SIGHAND | CLONE_THREAD | CLONE_SYSVSEM;
> +#ifdef ANDROID

If we care about Firefox for Android, we can't decide this at compile time. Up to you.

Another alternative is to just use the same flags across all versions.
It looks like CLONE_DETACHED has no effect on modern Linux kernels, so it shouldn't hurt to include that.
Attachment #8506474 - Flags: feedback?(mwu) → feedback+
(In reply to Michael Wu [:mwu] from comment #2)
> If we care about Firefox for Android, we can't decide this at compile time.
> Up to you.

We're not using this on Android (yet?), and I don't know if any non-B2G Android devices have kernel support yet.  But that's a good point; at minimum there should be an #ifdef/#error for that.

> Another alternative is to just use the same flags across all versions.
> It looks like CLONE_DETACHED has no effect on modern Linux kernels, so it
> shouldn't hurt to include that.

I'd have to check the value against all three possibilities, but that's not the worst thing in the world.  (Once I can import the Chromium bpf_dsl library (or reinvent that wheel, but I'd rather not), there will be support for masking off some bits and doing equality tests on the rest.)
Let's just check for any of the known values on Android.  Simpler, less chance of somehow accidentally breaking something, and no meaningful increase in attack surface.

Tested on keon and flame-kk, and with desktop GMP tests on x86_64.
Attachment #8506474 - Attachment is obsolete: true
Attachment #8506474 - Flags: review?(gdestuynder)
Attachment #8507323 - Flags: review?(gdestuynder)
Attachment #8507323 - Flags: review?(gdestuynder) → review+
https://hg.mozilla.org/mozilla-central/rev/6ab760222a4e
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.