Closed
Bug 1430949
Opened 7 years ago
Closed 7 years ago
Unshare network namespace in sandboxed Linux content processes
Categories
(Core :: Security: Process Sandboxing, enhancement, P1)
Tracking
()
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: jld, Assigned: jld)
References
(Blocks 3 open bugs)
Details
(Whiteboard: sb+)
Attachments
(1 file)
This is logically connected to as bug 1126437 (restrict network access), and code-wise it's in the same place as bug 1213998 (add sandbox bits in process launching), but it probably makes sense to land it separately from both so we can bisect anything that breaks.
This will prevent access to most sockets (Internet-domain, and the “abstract namespace” in the local domain) for the child process's entire lifespan, not just after ContentChild::RecvSetProcessSandbox. Running locally with an LD_PRELOAD shim to log calls to connect(), I'm not seeing anything besides X11, but there may be relevant nonstandard configurations out there.
This mitigation will need to be disabled if the X11 server is remote; that limitation can be removed when we either remove or proxy the X11 connection (see bug 1129492).
Updated•7 years ago
|
Whiteboard: sb+
Comment hidden (mozreview-request) |
Comment 2•7 years ago
|
||
mozreview-review |
Comment on attachment 8944636 [details]
Bug 1430949 - Isolate network namespace in Linux content sandbox level 4.
https://reviewboard.mozilla.org/r/214792/#review220574
::: security/sandbox/linux/launch/SandboxLaunch.cpp:165
(Diff revision 1)
> + if (level >= 4) {
> + // Unshare network namespace if X11 server is local. (The
> + // display name is copied to the environment in XRE_mainStartup,
> + // even if it was specified as a command-line option.)
> + const auto display = PR_GetEnv("DISPLAY");
> + if (display && display[0] == ':') {
For reference, what does "local" mean here? Using Unix Domain Sockets?
This check is technically a bit conservative, right? i.e. you could have localhost here or just "unix", IIRC.
Attachment #8944636 -
Flags: review?(gpascutto) → review+
Assignee | ||
Comment 3•7 years ago
|
||
(In reply to Gian-Carlo Pascutto [:gcp] from comment #2)
> Comment on attachment 8944636 [details]
> > + if (display && display[0] == ':') {
>
> For reference, what does "local" mean here? Using Unix Domain Sockets?
>
> This check is technically a bit conservative, right? i.e. you could have
> localhost here or just "unix", IIRC.
Named Unix-domain sockets only. localhost will not work, because the new network namespace has its own private loopback interface[*]. I'll reword the comment (and thanks for pointing out "unix:").
But there's a potential problem I just found while experimenting: if the hostname isn't specified, and treating it as "unix:" fails, it will fall back to doing TCP to localhost. (The documentation talks about “the most efficient way of communicating to a server on the same machine”.) So it's theoretically possible that $DISPLAY could start with a ':' but the server is TCP-only. This seems unusual — e.g., ssh X forwarding has a “server” that's TCP-only, but it sets $DISPLAY to explicitly use "localhost".
However, there's an alternative to parsing the display name: it should be possible to get the parent process's X11 connection fd and read off its domain with getsockopt(fd, SOL_SOCKET, SO_DOMAIN). I'll try that and see how it works.
[*] Demo: `ssh -Y localhost xeyes` will work; `unshare -Un xeyes` will work; `ssh -Y localhost unshare -Un xeyes` will not.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•7 years ago
|
||
Comment on attachment 8944636 [details]
Bug 1430949 - Isolate network namespace in Linux content sandbox level 4.
(Re-requesting review because it's a completely different patch now. In hindsight maybe I could have regenerated the MozReview ID to make that explicit.)
Attachment #8944636 -
Flags: review+ → review?(gpascutto)
Assignee | ||
Updated•7 years ago
|
Priority: P2 → P1
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8944636 [details]
Bug 1430949 - Isolate network namespace in Linux content sandbox level 4.
https://reviewboard.mozilla.org/r/214792/#review222756
::: security/sandbox/linux/launch/SandboxLaunch.cpp:84
(Diff revision 2)
> + socklen_t optlen = static_cast<socklen_t>(sizeof(domain));
> + int rv = getsockopt(xSocketFd, SOL_SOCKET, SO_DOMAIN, &domain, &optlen);
> + if (NS_WARN_IF(rv != 0)) {
> + return false;
> + }
> + MOZ_RELEASE_ASSERT(static_cast<size_t>(optlen) == sizeof(domain));
I was wondering if this isn't strictly <= ?
Checking, the only systems out there with ILP64 or SILP64 aren't going to be running Firefox, so I guess it's fine.
Also interpreting the value of a mismatch would be problematic, I guess.
Attachment #8944636 -
Flags: review?(gpascutto) → review+
Assignee | ||
Comment 7•7 years ago
|
||
The kernel shouldn't ever copy out too few bytes — the correct type for this option is `int`, according to both the man pages and the source — but I wanted to make absolutely sure we wouldn't somehow use a half-initialized value.
Pushed by jedavis@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a415b43fc1d2
Isolate network namespace in Linux content sandbox level 4. r=gcp
Comment 9•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in
before you can comment on or make changes to this bug.
Description
•