Bug 1759196 - Fix the Linux sandbox's handling of 32-bit arguments on 64-bit platforms. r=gcp,bobowen

Background: When 32-bit types are passed in registers on x86-64 (and
probably other platforms?), the function call ABI does not specify the
contents of the upper half, and the Linux kernel syscall ABI appears to
have the same behavior.

In practice, the upper half is usually zero (or maybe sign-extended from
the lower half), because 64-bit operations aren't cheaper than 32-bit,
and 32-bit operations zero-extend their outputs; therefore, this case
usually doesn't happen in the first place, and any kind of spill or
register move will zero the upper half.  However, arbitrary values are
possible, and a case like this has occurred with the Firefox profiler
using `clock_gettime`.  (This paragraph is applicable to x86-64 and
ARM64; other 64-bit architecutures may behave differently.)

But the Chromium seccomp-bpf compiler, when testing the value of a 32-bit
argument on a 64-bit platform, requires that the value be zero-extended
or sign-extended, and (incorrectly, as far as I can tell) considers
anything else an ABI violation.

With this patch, when that case is detected, we use the `SIGSYS` handler
to zero-extend the problematic argument and re-issue the syscall.

(It would also be possible to just ignore the upper half, and that would
be faster, but that could lead to subtle security holes if the type
used in `bpf_dsl` is incorrect and the kernel really does treat it as
64-bit.)

Differential Revision: https://phabricator.services.mozilla.com/D143964

security/sandbox/chromium-shim/patches/after_update/linux_32bit_arg_fixup.patch

@@ -0,0 +1,84 @@
+commit e0a00891b67ec162a17aa241a83b171b313de9fe
+Author: Jed Davis <jld@mozilla.com>
+Date:   Mon Apr 18 18:00:10 2022 -0600
+
+    Make the sandbox fix up non-extended 32-bit types.
+
+diff --git a/security/sandbox/chromium/sandbox/linux/bpf_dsl/policy_compiler.cc b/security/sandbox/chromium/sandbox/linux/bpf_dsl/policy_compiler.cc
+index 347304889eae4..b909fc37f6174 100644
+--- a/security/sandbox/chromium/sandbox/linux/bpf_dsl/policy_compiler.cc
++++ b/security/sandbox/chromium/sandbox/linux/bpf_dsl/policy_compiler.cc
+@@ -19,6 +19,7 @@
+ #include "sandbox/linux/bpf_dsl/policy.h"
+ #include "sandbox/linux/bpf_dsl/seccomp_macros.h"
+ #include "sandbox/linux/bpf_dsl/syscall_set.h"
++#include "sandbox/linux/seccomp-bpf/syscall.h"
+ #include "sandbox/linux/system_headers/linux_filter.h"
+ #include "sandbox/linux/system_headers/linux_seccomp.h"
+ #include "sandbox/linux/system_headers/linux_syscalls.h"
+@@ -318,8 +319,7 @@ CodeGen::Node PolicyCompiler::MaskedEqualHalf(int argno,
+     // Special logic for sanity checking the upper 32-bits of 32-bit system
+     // call arguments.
+ 
+-    // TODO(mdempsky): Compile Unexpected64bitArgument() just per program.
+-    CodeGen::Node invalid_64bit = Unexpected64bitArgument();
++    CodeGen::Node invalid_64bit = Unexpected64bitArgument(argno);
+ 
+     const uint32_t upper = SECCOMP_ARG_MSB_IDX(argno);
+     const uint32_t lower = SECCOMP_ARG_LSB_IDX(argno);
+@@ -335,8 +335,13 @@ CodeGen::Node PolicyCompiler::MaskedEqualHalf(int argno,
+               BPF_JMP + BPF_JEQ + BPF_K, 0, passed, invalid_64bit));
+     }
+ 
+-    // On 64-bit platforms, the upper 32-bits may be 0 or ~0; but we only allow
+-    // ~0 if the sign bit of the lower 32-bits is set too:
++    // On 64-bit platforms, the ABI (at least on x86_64) allows any value
++    // for the upper half, but to avoid potential vulnerabilties if an
++    // argument is incorrectly tested as a 32-bit type, we require it to be
++    // either zero-extended or sign-extended.  That is, the upper 32-bits
++    // may be 0 or ~0; but we only allow ~0 if the sign bit of the lower
++    // 32-bits is set too:
++    //
+     //   LDW  [upper]
+     //   JEQ  0, passed, (next)
+     //   JEQ  ~0, (next), invalid
+@@ -424,8 +429,18 @@ CodeGen::Node PolicyCompiler::MaskedEqualHalf(int argno,
+               BPF_JMP + BPF_JEQ + BPF_K, value, passed, failed)));
+ }
+ 
+-CodeGen::Node PolicyCompiler::Unexpected64bitArgument() {
+-  return CompileResult(panic_func_("Unexpected 64bit argument detected"));
++CodeGen::Node PolicyCompiler::Unexpected64bitArgument(int argno) {
++  // This situation is unlikely, but possible.  Return to userspace,
++  // zero-extend the problematic argument, and re-issue the syscall.
++  return CompileResult(bpf_dsl::Trap(
++      [](const arch_seccomp_data& args_ref, void* aux) -> intptr_t {
++        arch_seccomp_data args = args_ref;
++        int argno = (int)(intptr_t)aux;
++        args.args[argno] &= 0xFFFFFFFF;
++        return Syscall::Call(args.nr, args.args[0], args.args[1], args.args[2],
++                             args.args[3], args.args[4], args.args[5]);
++      },
++      (void*)(intptr_t)argno));
+ }
+ 
+ CodeGen::Node PolicyCompiler::Return(uint32_t ret) {
+diff --git a/security/sandbox/chromium/sandbox/linux/bpf_dsl/policy_compiler.h b/security/sandbox/chromium/sandbox/linux/bpf_dsl/policy_compiler.h
+index 48b1d780d956f..2acf878474a7d 100644
+--- a/security/sandbox/chromium/sandbox/linux/bpf_dsl/policy_compiler.h
++++ b/security/sandbox/chromium/sandbox/linux/bpf_dsl/policy_compiler.h
+@@ -132,9 +132,11 @@ class SANDBOX_EXPORT PolicyCompiler {
+                                 CodeGen::Node passed,
+                                 CodeGen::Node failed);
+ 
+-  // Returns the fatal CodeGen::Node that is used to indicate that somebody
+-  // attempted to pass a 64bit value in a 32bit system call argument.
+-  CodeGen::Node Unexpected64bitArgument();
++  // Returns the CodeGen::Node that is used to handle the case where a
++  // system call argument was expected to be a 32-bit type, but the
++  // value in the 64-bit register doesn't correspond to a
++  // zero-extended or sign-extended 32-bit value.
++  CodeGen::Node Unexpected64bitArgument(int argno);
+ 
+   const Policy* policy_;
+   TrapRegistry* registry_;

security/sandbox/chromium-shim/patches/after_update/patch_order.txt

@@ -7,3 +7,4 @@ arm64_set_LoaderThreads.patch
 change_to_DCHECK_in_CloseHandleWrapper.patch
 move_shared_memory_duplication_after_initialization.patch
 allow_ntpath_in_SignedPolicy_GenerateRules.patch
+linux_32bit_arg_fixup.patch

security/sandbox/chromium/sandbox/linux/bpf_dsl/policy_compiler.cc

@@ -19,6 +19,7 @@
 #include "sandbox/linux/bpf_dsl/policy.h"
 #include "sandbox/linux/bpf_dsl/seccomp_macros.h"
 #include "sandbox/linux/bpf_dsl/syscall_set.h"
+#include "sandbox/linux/seccomp-bpf/syscall.h"
 #include "sandbox/linux/system_headers/linux_filter.h"
 #include "sandbox/linux/system_headers/linux_seccomp.h"
 #include "sandbox/linux/system_headers/linux_syscalls.h"
@@ -318,8 +319,7 @@ CodeGen::Node PolicyCompiler::MaskedEqualHalf(int argno,
     // Special logic for sanity checking the upper 32-bits of 32-bit system
     // call arguments.
 
-    // TODO(mdempsky): Compile Unexpected64bitArgument() just per program.
-    CodeGen::Node invalid_64bit = Unexpected64bitArgument();
+    CodeGen::Node invalid_64bit = Unexpected64bitArgument(argno);
 
     const uint32_t upper = SECCOMP_ARG_MSB_IDX(argno);
     const uint32_t lower = SECCOMP_ARG_LSB_IDX(argno);
@@ -335,8 +335,13 @@ CodeGen::Node PolicyCompiler::MaskedEqualHalf(int argno,
               BPF_JMP + BPF_JEQ + BPF_K, 0, passed, invalid_64bit));
     }
 
-    // On 64-bit platforms, the upper 32-bits may be 0 or ~0; but we only allow
-    // ~0 if the sign bit of the lower 32-bits is set too:
+    // On 64-bit platforms, the ABI (at least on x86_64) allows any value
+    // for the upper half, but to avoid potential vulnerabilties if an
+    // argument is incorrectly tested as a 32-bit type, we require it to be
+    // either zero-extended or sign-extended.  That is, the upper 32-bits
+    // may be 0 or ~0; but we only allow ~0 if the sign bit of the lower
+    // 32-bits is set too:
+    //
     //   LDW  [upper]
     //   JEQ  0, passed, (next)
     //   JEQ  ~0, (next), invalid
@@ -424,8 +429,18 @@ CodeGen::Node PolicyCompiler::MaskedEqualHalf(int argno,
               BPF_JMP + BPF_JEQ + BPF_K, value, passed, failed)));
 }
 
-CodeGen::Node PolicyCompiler::Unexpected64bitArgument() {
-  return CompileResult(panic_func_("Unexpected 64bit argument detected"));
+CodeGen::Node PolicyCompiler::Unexpected64bitArgument(int argno) {
+  // This situation is unlikely, but possible.  Return to userspace,
+  // zero-extend the problematic argument, and re-issue the syscall.
+  return CompileResult(bpf_dsl::Trap(
+      [](const arch_seccomp_data& args_ref, void* aux) -> intptr_t {
+        arch_seccomp_data args = args_ref;
+        int argno = (int)(intptr_t)aux;
+        args.args[argno] &= 0xFFFFFFFF;
+        return Syscall::Call(args.nr, args.args[0], args.args[1], args.args[2],
+                             args.args[3], args.args[4], args.args[5]);
+      },
+      (void*)(intptr_t)argno));
 }
 
 CodeGen::Node PolicyCompiler::Return(uint32_t ret) {

security/sandbox/chromium/sandbox/linux/bpf_dsl/policy_compiler.h

@@ -132,9 +132,11 @@ class SANDBOX_EXPORT PolicyCompiler {
                                 CodeGen::Node passed,
                                 CodeGen::Node failed);
 
-  // Returns the fatal CodeGen::Node that is used to indicate that somebody
-  // attempted to pass a 64bit value in a 32bit system call argument.
-  CodeGen::Node Unexpected64bitArgument();
+  // Returns the CodeGen::Node that is used to handle the case where a
+  // system call argument was expected to be a 32-bit type, but the
+  // value in the 64-bit register doesn't correspond to a
+  // zero-extended or sign-extended 32-bit value.
+  CodeGen::Node Unexpected64bitArgument(int argno);
 
   const Policy* policy_;
   TrapRegistry* registry_;

security/sandbox/common/test/SandboxTestingChildTests.h

@@ -83,7 +83,50 @@ static void RunTestsSched(SandboxTestingChild* child) {
     return sched_getparam((pid_t)(syscall(__NR_gettid) - 1), &param_pid_Ntid);
   });
 }
+#endif
+
+// Tests that apply to every process type (more or less)
+static void RunGenericTests(SandboxTestingChild* child, bool aIsGMP = false) {
+#ifdef XP_LINUX
+  // Check ABI issues with 32-bit arguments on 64-bit platforms.
+  if (sizeof(void*) == 8) {
+    static constexpr uint64_t kHighBits = 0xDEADBEEF00000000;
+
+    struct timespec ts0, ts1;
+    child->ErrnoTest("high_bits_gettime"_ns, true, [&] {
+      return syscall(__NR_clock_gettime, kHighBits | CLOCK_MONOTONIC, &ts0);
+    });
+    // Try to make sure we got the correct clock by reading it again and
+    // comparing to see if the times are vaguely similar.
+    int rv = clock_gettime(CLOCK_MONOTONIC, &ts1);
+    MOZ_RELEASE_ASSERT(rv == 0);
+    MOZ_RELEASE_ASSERT(ts0.tv_sec <= ts1.tv_sec + 1);
+    MOZ_RELEASE_ASSERT(ts1.tv_sec <= ts0.tv_sec + 60);
+
+    // Check some non-zeroth arguments.  (fcntl is convenient for
+    // this, but GMP has a stricter policy, so skip it there.)
+    if (!aIsGMP) {
+      int flags;
+      child->ErrnoTest("high_bits_fcntl_getfl"_ns, true, [&] {
+        flags = syscall(__NR_fcntl, 0, kHighBits | F_GETFL);
+        return flags;
+      });
+      MOZ_RELEASE_ASSERT(flags == fcntl(0, F_GETFL));
+
+      int fds[2];
+      rv = pipe(fds);
+      MOZ_RELEASE_ASSERT(rv >= 0);
+      child->ErrnoTest("high_bits_fcntl_setfl"_ns, true, [&] {
+        return syscall(__NR_fcntl, fds[0], kHighBits | F_SETFL,
+                       kHighBits | O_NONBLOCK);
+      });
+      flags = fcntl(fds[0], F_GETFL);
+      MOZ_RELEASE_ASSERT(flags >= 0);
+      MOZ_RELEASE_ASSERT(flags & O_NONBLOCK);
+    }
+  }
 #endif  // XP_LINUX
+}
 
 #ifdef XP_WIN
 /**
@@ -280,6 +323,8 @@ void RunWinTestWin32k(SandboxTestingChild* child,
 void RunTestsContent(SandboxTestingChild* child) {
   MOZ_ASSERT(child, "No SandboxTestingChild*?");
 
+  RunGenericTests(child);
+
 #ifdef XP_UNIX
   struct stat st;
   static const char kAllowedPath[] = "/usr/lib";
@@ -457,6 +502,8 @@ void RunTestsContent(SandboxTestingChild* child) {
 void RunTestsSocket(SandboxTestingChild* child) {
   MOZ_ASSERT(child, "No SandboxTestingChild*?");
 
+  RunGenericTests(child);
+
 #ifdef XP_UNIX
   child->ErrnoTest("getaddrinfo"_ns, true, [&] {
     struct addrinfo* res;
@@ -519,6 +566,8 @@ void RunTestsSocket(SandboxTestingChild* child) {
 void RunTestsRDD(SandboxTestingChild* child) {
   MOZ_ASSERT(child, "No SandboxTestingChild*?");
 
+  RunGenericTests(child);
+
 #ifdef XP_UNIX
 #  ifdef XP_LINUX
   child->ErrnoValueTest("ioctl_tiocsti"_ns, ENOSYS, [&] {
@@ -580,6 +629,9 @@ void RunTestsRDD(SandboxTestingChild* child) {
 
 void RunTestsGMPlugin(SandboxTestingChild* child) {
   MOZ_ASSERT(child, "No SandboxTestingChild*?");
+
+  RunGenericTests(child, /* aIsGMP = */ true);
+
 #ifdef XP_UNIX
 #  ifdef XP_LINUX
   struct utsname utsname_res = {};
@@ -633,6 +685,8 @@ void RunTestsGMPlugin(SandboxTestingChild* child) {
 void RunTestsGenericUtility(SandboxTestingChild* child) {
   MOZ_ASSERT(child, "No SandboxTestingChild*?");
 
+  RunGenericTests(child);
+
 #ifdef XP_UNIX
 #  ifdef XP_LINUX
   child->ErrnoValueTest("ioctl_tiocsti"_ns, ENOSYS, [&] {
@@ -668,6 +722,8 @@ void RunTestsGenericUtility(SandboxTestingChild* child) {
 void RunTestsUtilityAudioDecoder(SandboxTestingChild* child) {
   MOZ_ASSERT(child, "No SandboxTestingChild*?");
 
+  RunGenericTests(child);
+
 #ifdef XP_UNIX
 #  ifdef XP_LINUX
   // getrusage is allowed in Generic Utility and on AudioDecoder

security/sandbox/linux/SandboxFilter.cpp

@@ -744,8 +744,10 @@ class SandboxPolicyCommon : public SandboxPolicyBase {
         // clockid_t can encode a pid or tid to monitor another
         // process or thread's CPU usage (see CPUCLOCK_PID and related
         // definitions in include/linux/posix-timers.h in the kernel
-        // source).  Those values could be detected by bit masking,
-        // but it's simpler to just have a default-deny policy.
+        // source).  For threads, the kernel allows only tids within
+        // the calling process, so it isn't a problem if we don't
+        // filter those; pids do need to be restricted to the current
+        // process in order to not leak information.
         Arg<clockid_t> clk_id(0);
         clockid_t this_process =
             MAKE_PROCESS_CPUCLOCK(getpid(), CPUCLOCK_SCHED);