[tor-commits] [tor-browser] 07/43: Bug 1759196 - Fix the Linux sandbox's handling of 32-bit arguments on 64-bit platforms. r=gcp, bobowen a=RyanVM
gitolite role
git at cupani.torproject.org
Tue May 31 07:06:50 UTC 2022
This is an automated email from the git hooks/post-receive script.
pierov pushed a commit to branch tor-browser-91.10.0esr-11.0-1
in repository tor-browser.
commit 15510a7e88352cc416e2d8aae8064653be38ad0c
Author: Jed Davis <jld at mozilla.com>
AuthorDate: Tue Apr 26 00:09:17 2022 +0000
Bug 1759196 - Fix the Linux sandbox's handling of 32-bit arguments on 64-bit platforms. r=gcp,bobowen a=RyanVM
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/D144643
---
.../after_update/linux_32bit_arg_fixup.patch | 84 ++++++++++++++++++++++
.../patches/after_update/patch_order.txt | 1 +
.../sandbox/linux/bpf_dsl/policy_compiler.cc | 27 +++++--
.../sandbox/linux/bpf_dsl/policy_compiler.h | 8 ++-
.../sandbox/common/test/SandboxTestingChildTests.h | 52 ++++++++++++++
security/sandbox/linux/SandboxFilter.cpp | 6 +-
6 files changed, 167 insertions(+), 11 deletions(-)
diff --git a/security/sandbox/chromium-shim/patches/after_update/linux_32bit_arg_fixup.patch b/security/sandbox/chromium-shim/patches/after_update/linux_32bit_arg_fixup.patch
new file mode 100644
index 0000000000000..5cc66ad09b5d6
--- /dev/null
+++ b/security/sandbox/chromium-shim/patches/after_update/linux_32bit_arg_fixup.patch
@@ -0,0 +1,84 @@
+commit e0a00891b67ec162a17aa241a83b171b313de9fe
+Author: Jed Davis <jld at 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_;
diff --git a/security/sandbox/chromium-shim/patches/after_update/patch_order.txt b/security/sandbox/chromium-shim/patches/after_update/patch_order.txt
index c75c861616134..022b3a9d171a6 100644
--- a/security/sandbox/chromium-shim/patches/after_update/patch_order.txt
+++ b/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
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_;
diff --git a/security/sandbox/common/test/SandboxTestingChildTests.h b/security/sandbox/common/test/SandboxTestingChildTests.h
index 61ab0991f5e51..1b52627a05fd4 100644
--- a/security/sandbox/common/test/SandboxTestingChildTests.h
+++ b/security/sandbox/common/test/SandboxTestingChildTests.h
@@ -31,9 +31,54 @@
namespace mozilla {
+// 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
+}
+
void RunTestsContent(SandboxTestingChild* child) {
MOZ_ASSERT(child, "No SandboxTestingChild*?");
+ RunGenericTests(child);
+
#ifdef XP_UNIX
struct stat st;
static const char kAllowedPath[] = "/usr/lib";
@@ -117,6 +162,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;
@@ -147,6 +194,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, false, ENOSYS, [&] {
@@ -167,6 +216,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 = {};
diff --git a/security/sandbox/linux/SandboxFilter.cpp b/security/sandbox/linux/SandboxFilter.cpp
index 6b07186e8c2e2..f201b316b1eaf 100644
--- a/security/sandbox/linux/SandboxFilter.cpp
+++ b/security/sandbox/linux/SandboxFilter.cpp
@@ -701,8 +701,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);
return If(clk_id == CLOCK_MONOTONIC, Allow())
#ifdef CLOCK_MONOTONIC_COARSE
--
To stop receiving notification emails like this one, please contact
the administrator of this repository.
More information about the tor-commits
mailing list