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

Status: RESOLVED FIXED

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

Description

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

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 1

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: Bug 1412480 - Fix syscall argument types in seccomp-bpf sandbox traps.

The values in arch_seccomp_data::args are uint64_t even on 32-bit
platforms, and syscall takes varargs, so the arguments need to be
explicitly cast to the word size in order to be passed correctly.

Review commit: https://reviewboard.mozilla.org/r/194772/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194772/

Comment 2

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: Bug 1412480 - Statically check for overly large syscall arguments.

See the previous patch for an explanation of the mistake that this is
meant to catch.

Note that, even for arguments that really are 64-bit on 32-bit platforms
(typically off_t), it's generally not safe to pass them directly to
syscall(): some architectures, like ARM, use ABIs that require such
arguments to be passed in aligned register pairs, and they'll be aligned
differently for syscall() vs. the actual system call due to the leading
system call number argument.  The syscall(2) man page discusses this
and documents that such arguments should be split into high/low halves,
passed separately, and manually padded.

Therefore, this patch rejects any argument types larger than a word.

Review commit: https://reviewboard.mozilla.org/r/194774/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194774/

Comment 3

[Gian-Carlo Pascutto [:gcp]]

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?

Comment 4

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

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.)

Comment 5

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(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.

Comment 6

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8923644 [details]
Bug 1412480 - Statically check for overly large syscall arguments.

https://reviewboard.mozilla.org/r/194774/#review201038

Comment 7

[Pulsebot]

Pushed by [e-mail]:
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

Comment 8

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/48b83b14ff3d
https://hg.mozilla.org/mozilla-central/rev/eeb1aae7683b