Closed Bug 1137007 Opened 5 years ago Closed 5 years ago

Detect namespace support and SECCOMP_FILTER_FLAG_TSYNC in SandboxInfo


(Core :: Security: Process Sandboxing, defect)

Not set



Tracking Status
firefox39 --- fixed


(Reporter: jld, Assigned: jld)


(Blocks 1 open bug)



(1 file, 2 obsolete files)

A prerequisite for using Linux namespaces for sandboxing is detecting whether they're supported on the system.  (This information could also be reported in telemetry/FHR, even if nothing else uses it, although we're not yet doing that — bug 1098428.)  Additionally, supporting pid namespaces without resorting to the worrying measures described in bug 1088387 requires SECCOMP_FILTER_FLAG_TSYNC.
More sandbox info, and matching glue for Troubleshoot/about:support.

Tested on: Ubuntu 14.04, Debian unstable (with and without kernel.unprivileged_userns_clone), B2G emulator-x86-kk, B2G flame-kk, B2G nexus-5-l.
Attachment #8569553 - Flags: review?(gdestuynder)
Attachment #8569553 - Flags: feedback?(bmcbride)
Incidentally, Debian doesn't have SECCOMP_FILTER_FLAG_TSYNC; it's in 3.17 upstream and Ubuntu backported it to (at least) 3.13, but Debian is on 3.16 and hasn't backported it.  Debian jessie might be out of luck — but this is also largely meaningless without unprivileged user namespaces.

More incidentally, B2G kernels are missing most but not all of the namespaces, and not always the same ones on each device.  Fortunately, we mostly don't need them there.
Comment on attachment 8569553 [details] [diff] [review]

Review of attachment 8569553 [details] [diff] [review]:

r+ on the Toolkit changes.

Though it would be good to do some general maintenance on the about:support code, since that list is getting long and currently needs to be kept in sync in 3 places:

::: toolkit/content/aboutSupport.js
@@ +346,5 @@
> +    const keys = ["hasSeccompBPF", "hasSeccompTSync",
> +		  "hasPrivilegedUserNamespaces",
> +		  "hasUserNamespaces", "hasNetNamespaces",
> +		  "hasPidNamespaces", "hasIpcNamespaces",
> +		  "canSandboxContent", "canSandboxMedia"];

Can just delete this to reduce redundancy, and instead...

@@ +351,3 @@
>      let strings = stringBundle();
>      let tbody = $("sandbox-tbody");
>      for (let key of keys) {

... use for-in here:
  for (let key in data) {

(Though most other code actually uses sortedArrayFromObject)

::: toolkit/modules/Troubleshoot.jsm
@@ +507,5 @@
> +    const keys = ["hasSeccompBPF", "hasSeccompTSync",
> +		  "hasPrivilegedUserNamespaces",
> +		  "hasUserNamespaces", "hasNetNamespaces",
> +		  "hasPidNamespaces", "hasIpcNamespaces",
> +		  "canSandboxContent", "canSandboxMedia"];

Reformatting this to single column or re-ordering so this and has the same order will make it much easier to check the lists are in sync.
Attachment #8569553 - Flags: feedback?(bmcbride) → review+
Comment on attachment 8569553 [details] [diff] [review]

Review of attachment 8569553 [details] [diff] [review]:

::: security/sandbox/linux/common/SandboxInfo.cpp
@@ +65,5 @@
> +  static const char* paths[] = {
> +    "/proc/self/ns/user",
> +    "/proc/self/ns/pid",
> +    "/proc/self/ns/net",
> +    "/proc/self/ns/ipc",

This is… not doing what it says it is.  The /proc/*/ns/* nodes indicate the presence of setns(2) support for the namespace.  The case where this would make a significant difference is for pid namespaces, where setns support didn't land until 3.8 — as opposed to 3.0 for net, ipc, and uts.  (The actual namespace support is from various points in the 2.6.x era.)

I think I need to give some more thought to exactly what information I'm trying to gather, here.
Attachment #8569553 - Flags: review?(gdestuynder)
I've reduced it to just user namespaces, since that's the important one; see also the commit message.
Attachment #8569553 - Attachment is obsolete: true
Attachment #8572960 - Flags: review?(gdestuynder)
Comment on attachment 8572960 [details] [diff] [review]

Review of attachment 8572960 [details] [diff] [review]:

Looks good! also discussed this irl with jld. This version's nice :)
Attachment #8572960 - Flags: review?(gdestuynder) → review+
Let's try actually converting the diff into Hg format this time.  Carrying over r+; same as previous patch otherwise.
Attachment #8572960 - Attachment is obsolete: true
Attachment #8574155 - Flags: review+
Try: (the M-1 is just to make sure *something* gets run on emulator; the M-bc covers the Troubleshoot.jsm test).
Keywords: checkin-needed
I suspect that this bug will introduce a startup crash on a small and possibly empty set of 32-bit systems, along similar lines to — which the Chromium devs WONTFIXed, because it was caused by a kernel bug and patchlevel updates with the fix had been available for several months by the time that bug was filed.  Along similiar lines, I don't think it's worth backing out the patch for this, but it might be worth filing a followup.

The kernel bug is explained at (but, having stared at entry_32.S for a while, I think the first paragraph get an important detail backwards: the SYSENTER path is what the bug *does* affect, while the `int $0x80` path is unaffected, which helps explain the part of the Chromium bug where a statically linked executable didn't exhibit the bug).
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Depends on: 1142263
You need to log in before you can comment on or make changes to this bug.