Closed Bug 1430949 Opened 2 years ago Closed 2 years ago

Unshare network namespace in sandboxed Linux content processes

Categories

(Core :: Security: Process Sandboxing, enhancement, P1)

Unspecified
Linux
enhancement

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).
Whiteboard: sb+
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+
(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 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)
Priority: P2 → P1
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+
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
https://hg.mozilla.org/mozilla-central/rev/a415b43fc1d2
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Depends on: 1434927
You need to log in before you can comment on or make changes to this bug.