Redirect socketcall() to direct socket calls if supported by kernel

RESOLVED FIXED in Firefox 60

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: jld, Assigned: jld)

Tracking

59 Branch
mozilla60
Unspecified
Linux
Points:
---

Firefox Tracking Flags

(firefox59 wontfix, firefox60 fixed)

Details

(Whiteboard: sb+)

Attachments

(1 attachment)

On x86 kernels which support the direct socket calls added in kernel commit d270c42cd (v4.3; see bug 1273852), we could block socketcall() and implement it in userspace on top of the modern syscalls, which would allow argument filters to work.
Whiteboard: sb+
On second thought, completely blocking socketcall() isn't the best idea: socket calls on the critical path for IPC should avoid the overhead of a signal handler if possible.  This is mainly useful only for socket calls where we want to filter arguments, which currently is just socketpair().

(It would be mildly useful for calls we completely disallow, so that the unpacked arguments wind up in error messages and crash reports, but usually when things break on 32-bit x86 they also break in larger quantity on 64-bit, and I don't want to write that code if it's not actually going to be useful.)
Priority: P2 → P1
Comment hidden (mozreview-request)
I've tested a 32-bit build both on a modern kernel (Debian unstable) and an older one (Ubuntu 14.04.1), and confirmed that it detects the syscall support correctly, filters socketpair() as expected on the former, and doesn't break on the latter.

Comment 4

a year ago
mozreview-review
Comment on attachment 8946716 [details]
Bug 1425274 - Filter socketpair() in content sandbox on 32-bit x86 with new-enough kernels.

https://reviewboard.mozilla.org/r/216690/#review222784

::: security/sandbox/linux/SandboxFilter.cpp:399
(Diff revision 1)
> +  static bool
> +  HasSeparateSocketCalls() {
> +#ifdef __NR_socket
> +    // If there's no socketcall, then obviously there are separate syscalls.
> +#ifdef __NR_socketcall
> +    int fd = syscall(__NR_socket, AF_LOCAL, SOCK_STREAM, 0);

Given that this is called on every socketpair syscall without args, can you cache the result of the check? I guess simple things like a function static won't work here because this is the seccomp-bpf ResultExpr generator thingy...

::: security/sandbox/linux/SandboxFilter.cpp:649
(Diff revision 1)
> -      if (!kSocketCallHasArgs) {
> -        // We can't filter the args if the platform passes them by pointer.
> +      if (!aHasArgs) {
> +        // If this is a socketcall(2) platform, but the kernel also
> +        // supports separate syscalls (>= 4.2.0), we can unpack the
> +        // arguments and filter them.
> +        if (HasSeparateSocketCalls()) {
> +          return Some(Trap(SocketpairUnpackTrap, nullptr));

Does this actually do the filtering? SocketpairUnpackTrap does a Dosyscall(), but it's not clear to me that will recurse into the filters here.
Attachment #8946716 - Flags: review?(gpascutto) → review+
Assignee

Comment 5

a year ago
mozreview-review-reply
Comment on attachment 8946716 [details]
Bug 1425274 - Filter socketpair() in content sandbox on 32-bit x86 with new-enough kernels.

https://reviewboard.mozilla.org/r/216690/#review222784

> Given that this is called on every socketpair syscall without args, can you cache the result of the check? I guess simple things like a function static won't work here because this is the seccomp-bpf ResultExpr generator thingy...

This is called when constructing the filter, not running it — the Chromium framework calls `EvaluateSyscall` once for each syscall number and builds a decision tree of contiguous ranges that all have the same policy, which is compiled to seccomp-bpf instructions.

And this is why I didn't bother caching it: because it should be called only once (when evaluating the policy for `__NR_socketcall`) per sandboxed process.

> Does this actually do the filtering? SocketpairUnpackTrap does a Dosyscall(), but it's not clear to me that will recurse into the filters here.

It has to: it's a system call made by the sandboxed process, therefore it's filtered by seccomp-bpf.

Also, I tested this: `socketpair(AF_UNIX, SOCK_DGRAM, 0, fds)` raises SIGSYS to convert the socketcall to a modern syscall, which raises SIGSYS recursively to do `SOCK_SEQPACKET` instead, which raises a third nested SIGSYS to unpack *that* socketcall, and finally the fourth nested syscall is accepted.  (This could be optimized a little, but the only known user of that dgram-to-seqpacket hack is PulseAudio, and it's not on a critical path.)  Or, if the type is something we completely forbid like `SOCK_RAW`, the second nested SIGSYS handler activation crashes the process instead.

Comment 6

a year ago
Pushed by jedavis@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4e2bf17f806d
Filter socketpair() in content sandbox on 32-bit x86 with new-enough kernels. r=gcp

Comment 7

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4e2bf17f806d
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.