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 000000000000..5cc66ad09b5d --- /dev/null +++ b/security/sandbox/chromium-shim/patches/after_update/linux_32bit_arg_fixup.patch @@ -0,0 +1,84 @@ +commit e0a00891b67ec162a17aa241a83b171b313de9fe +Author: Jed Davis +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 c75c86161613..022b3a9d171a 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 347304889eae..b909fc37f617 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 48b1d780d956..2acf878474a7 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 f9114db26447..20a329a81c18 100644 --- a/security/sandbox/common/test/SandboxTestingChildTests.h +++ b/security/sandbox/common/test/SandboxTestingChildTests.h @@ -83,7 +83,50 @@ static void RunTestsSched(SandboxTestingChild* child) { return sched_getparam((pid_t)(syscall(__NR_gettid) - 1), ¶m_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 diff --git a/security/sandbox/linux/SandboxFilter.cpp b/security/sandbox/linux/SandboxFilter.cpp index f0b0f267edf9..694d01cc86b2 100644 --- a/security/sandbox/linux/SandboxFilter.cpp +++ b/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 clk_id(0); clockid_t this_process = MAKE_PROCESS_CPUCLOCK(getpid(), CPUCLOCK_SCHED);