Closed
Bug 1137007
Opened 10 years ago
Closed 10 years ago
Detect namespace support and SECCOMP_FILTER_FLAG_TSYNC in SandboxInfo
Categories
(Core :: Security: Process Sandboxing, defect)
Tracking
()
RESOLVED
FIXED
mozilla39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: jld, Assigned: jld)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 2 obsolete files)
14.40 KB,
patch
|
jld
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
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)
Assignee | ||
Comment 2•10 years ago
|
||
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 3•10 years ago
|
||
Comment on attachment 8569553 [details] [diff] [review]
bug1137007-more-sandbox-info-hg0.diff
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 about:support.properties has the same order will make it much easier to check the lists are in sync.
Attachment #8569553 -
Flags: feedback?(bmcbride) → review+
Assignee | ||
Comment 4•10 years ago
|
||
Comment on attachment 8569553 [details] [diff] [review]
bug1137007-more-sandbox-info-hg0.diff
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.
Assignee | ||
Updated•10 years ago
|
Attachment #8569553 -
Flags: review?(gdestuynder)
Assignee | ||
Comment 5•10 years ago
|
||
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]
bug1137007-more-sandbox-info-hg1.diff
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+
Assignee | ||
Comment 7•10 years ago
|
||
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+
Assignee | ||
Comment 8•10 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d843554b91cf (the M-1 is just to make sure *something* gets run on emulator; the M-bc covers the Troubleshoot.jsm test).
Keywords: checkin-needed
Comment 9•10 years ago
|
||
Flags: in-testsuite+
Keywords: checkin-needed
Assignee | ||
Comment 10•10 years ago
|
||
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 https://crbug.com/439795 — 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 https://lkml.org/lkml/2014/7/20/222 (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).
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in
before you can comment on or make changes to this bug.
Description
•