Closed Bug 1412480 Opened 2 years ago Closed 2 years ago

The Linux sandbox isn't forwarding syscall arguments correctly on 32-bit platforms

Categories

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

Unspecified
Linux
enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: jld, Assigned: jld)

References

Details

(Whiteboard: sb+)

Attachments

(2 files)

When we trap system calls with seccomp-bpf, the Chromium code passes our callback a struct arch_seccomp_data, which matches the layout of the data provided by the kernel to the BPF program.  In particular, it stores syscall arguments as uint64_t even on 32-bit platforms.

So, when we pass these arguments directly to the syscall() function, which uses C varargs, they occupy 2 words each instead of 1 (and may also be padded to aligned register pairs, depending on architecture).  In the general case, this is bad.


(I note that Chromium's sandbox::Syscall, which I considered using in bug 1410191 and then didn't, casts all the arguments to intptr_t and avoids this problem.  (It's probably invoking implementation-defined behavior in some cases by casting to a signed type, but then so are we whenever we cast one of those `uint64_t`s back to an int.)  That approach would solve this problem, but I'd prefer to require callers to cast to the actual type in question, where possible, for clarity.)
Comment on attachment 8923643 [details]
Bug 1412480 - Fix syscall argument types in seccomp-bpf sandbox traps.

https://reviewboard.mozilla.org/r/194772/#review200022

This sounds pretty scary, but we already shipped this, didn't we?
Attachment #8923643 - Flags: review?(gpascutto) → review+
It's a problem only when arguments are passed directly from the args[] array to syscall without a cast.  Also, if it's a single argument at the end (and, maybe, if it's in an even-numbered position) on a little-endian platform, the lower half will be in the right place and things will work anyway.

So I think the impact of this on 32-bit x86 is:
* tkill will send signal 0 (no signal, just check process existence); on desktop this is used only by musl libc.
* sched_setscheduler will pass nullptr for the policy argument and probably fail with EFAULT.  

(Also the traps for sched_get_priority_{min,max} are competely wrong, but that's a separate bug.)
(In reply to Jed Davis [:jld] (⏰UTC-6) from comment #4)
> * sched_setscheduler will pass nullptr for the policy argument and probably
> fail with EFAULT.  

…and this applies only to the media plugin policy; in content we're allowing sched_* syscalls without filtering, which is yet another bug.
Whiteboard: sb+
Comment on attachment 8923644 [details]
Bug 1412480 - Statically check for overly large syscall arguments.

https://reviewboard.mozilla.org/r/194774/#review201038
Attachment #8923644 - Flags: review?(gpascutto) → review+
Pushed by jedavis@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/48b83b14ff3d
Fix syscall argument types in seccomp-bpf sandbox traps. r=gcp
https://hg.mozilla.org/integration/autoland/rev/eeb1aae7683b
Statically check for overly large syscall arguments. r=gcp
https://hg.mozilla.org/mozilla-central/rev/48b83b14ff3d
https://hg.mozilla.org/mozilla-central/rev/eeb1aae7683b
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.