rpms/chromium
2
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity
e740a21485
chromium/chromium-92.0.4515.107-linux-sandbox-fix-fstatat-crash.patch

349 lines
13 KiB
Diff
Raw Blame History

From 60d5e803ef2a4874d29799b638754152285e0ed9 Mon Sep 17 00:00:00 2001
From: Matthew Denton <mpdenton@chromium.org>
Date: Wed, 21 Jul 2021 12:55:11 +0000
Subject: [PATCH] Linux sandbox: fix fstatat() crash
This is a reland of https://crrev.com/c/2801873.
Glibc has started rewriting fstat(fd, stat_buf) to
fstatat(fd, "", stat_buf, AT_EMPTY_PATH). This works because when
AT_EMPTY_PATH is specified, and the second argument is an empty string,
then fstatat just performs an fstat on fd like normal.
Unfortunately, fstatat() also allows stat-ing arbitrary pathnames like
with fstatat(AT_FDCWD, "/i/am/a/file", stat_buf, 0);
The baseline policy needs to prevent this usage of fstatat() since it
doesn't allow access to arbitrary pathnames.
Sadly, if the second argument is not an empty string, AT_EMPTY_PATH is
simply ignored by current kernels.
This means fstatat() is completely unsandboxable with seccomp, since
we *need* to verify that the second argument is the empty string, but
we can't dereference pointers in seccomp (due to limitations of BPF,
and the difficulty of addressing these limitations due to TOCTOU
issues).
So, this CL Traps (raises a SIGSYS via seccomp) on any fstatat syscall.
The signal handler, which runs in the sandboxed process, checks for
AT_EMPTY_PATH and the empty string, and then rewrites any applicable
fstatat() back into the old-style fstat().
Bug: 1164975
Change-Id: I3df6c04c0d781eb1f181d707ccaaead779337291
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3042179
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Commit-Queue: Matthew Denton <mpdenton@chromium.org>
Cr-Commit-Position: refs/heads/master@{#903873}
---
.../seccomp-bpf-helpers/baseline_policy.cc | 8 ++++++
.../baseline_policy_unittest.cc | 17 ++++++++++++-
.../seccomp-bpf-helpers/sigsys_handlers.cc | 25 +++++++++++++++++++
.../seccomp-bpf-helpers/sigsys_handlers.h | 14 +++++++++++
.../linux/syscall_broker/broker_process.cc | 21 ++++++++++------
.../syscall_broker/broker_process_unittest.cc | 18 ++++++-------
sandbox/linux/system_headers/linux_stat.h | 4 +++
7 files changed, 89 insertions(+), 18 deletions(-)
diff --git a/sandbox/linux/seccomp-bpf-helpers/baseline_policy.cc b/sandbox/linux/seccomp-bpf-helpers/baseline_policy.cc
index f2a60bb4d7..9df0d2dbd3 100644
--- a/sandbox/linux/seccomp-bpf-helpers/baseline_policy.cc
+++ b/sandbox/linux/seccomp-bpf-helpers/baseline_policy.cc
@@ -20,6 +20,7 @@
#include "sandbox/linux/seccomp-bpf-helpers/syscall_sets.h"
#include "sandbox/linux/seccomp-bpf/sandbox_bpf.h"
#include "sandbox/linux/services/syscall_wrappers.h"
+#include "sandbox/linux/system_headers/linux_stat.h"
#include "sandbox/linux/system_headers/linux_syscalls.h"
#if !defined(SO_PEEK_OFF)
@@ -304,6 +305,13 @@ ResultExpr EvaluateSyscallImpl(int fs_denied_errno,
return Allow();
}
+ // The fstatat syscalls are file system syscalls, which will be denied below
+ // with fs_denied_errno. However some allowed fstat syscalls are rewritten by
+ // libc implementations to fstatat syscalls, and we need to rewrite them back.
+ if (sysno == __NR_fstatat_default) {
+ return RewriteFstatatSIGSYS(fs_denied_errno);
+ }
+
if (SyscallSets::IsFileSystem(sysno) ||
SyscallSets::IsCurrentDirectory(sysno)) {
return Error(fs_denied_errno);
diff --git a/sandbox/linux/seccomp-bpf-helpers/baseline_policy_unittest.cc b/sandbox/linux/seccomp-bpf-helpers/baseline_policy_unittest.cc
index 68c29b564b..57d307e09d 100644
--- a/sandbox/linux/seccomp-bpf-helpers/baseline_policy_unittest.cc
+++ b/sandbox/linux/seccomp-bpf-helpers/baseline_policy_unittest.cc
@@ -51,7 +51,8 @@ namespace sandbox {
namespace {
-// This also tests that read(), write() and fstat() are allowed.
+// This also tests that read(), write(), fstat(), and fstatat(.., "", ..,
+// AT_EMPTY_PATH) are allowed.
void TestPipeOrSocketPair(base::ScopedFD read_end, base::ScopedFD write_end) {
BPF_ASSERT_LE(0, read_end.get());
BPF_ASSERT_LE(0, write_end.get());
@@ -60,6 +61,20 @@ void TestPipeOrSocketPair(base::ScopedFD read_end, base::ScopedFD write_end) {
BPF_ASSERT_EQ(0, sys_ret);
BPF_ASSERT(S_ISFIFO(stat_buf.st_mode) || S_ISSOCK(stat_buf.st_mode));
+ sys_ret = fstatat(read_end.get(), "", &stat_buf, AT_EMPTY_PATH);
+ BPF_ASSERT_EQ(0, sys_ret);
+ BPF_ASSERT(S_ISFIFO(stat_buf.st_mode) || S_ISSOCK(stat_buf.st_mode));
+
+ // Make sure fstatat with anything other than an empty string is denied.
+ sys_ret = fstatat(read_end.get(), "/", &stat_buf, AT_EMPTY_PATH);
+ BPF_ASSERT_EQ(sys_ret, -1);
+ BPF_ASSERT_EQ(EPERM, errno);
+
+ // Make sure fstatat without AT_EMPTY_PATH is denied.
+ sys_ret = fstatat(read_end.get(), "", &stat_buf, 0);
+ BPF_ASSERT_EQ(sys_ret, -1);
+ BPF_ASSERT_EQ(EPERM, errno);
+
const ssize_t kTestTransferSize = 4;
static const char kTestString[kTestTransferSize] = {'T', 'E', 'S', 'T'};
ssize_t transfered = 0;
diff --git a/sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc b/sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc
index 64edbd68bd..71068a0452 100644
--- a/sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc
+++ b/sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc
@@ -6,6 +6,7 @@
#include "sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.h"
+#include <fcntl.h>
#include <stddef.h>
#include <stdint.h>
#include <string.h>
@@ -22,6 +23,7 @@
#include "sandbox/linux/seccomp-bpf/syscall.h"
#include "sandbox/linux/services/syscall_wrappers.h"
#include "sandbox/linux/system_headers/linux_seccomp.h"
+#include "sandbox/linux/system_headers/linux_stat.h"
#include "sandbox/linux/system_headers/linux_syscalls.h"
#if defined(__mips__)
@@ -355,6 +357,24 @@ intptr_t SIGSYSSchedHandler(const struct arch_seccomp_data& args,
return -ENOSYS;
}
+intptr_t SIGSYSFstatatHandler(const struct arch_seccomp_data& args,
+ void* fs_denied_errno) {
+ if (args.nr == __NR_fstatat_default) {
+ if (*reinterpret_cast<const char*>(args.args[1]) == '\0' &&
+ args.args[3] == static_cast<uint64_t>(AT_EMPTY_PATH)) {
+ return syscall(__NR_fstat_default, static_cast<int>(args.args[0]),
+ reinterpret_cast<default_stat_struct*>(args.args[2]));
+ }
+ return -reinterpret_cast<intptr_t>(fs_denied_errno);
+ }
+
+ CrashSIGSYS_Handler(args, fs_denied_errno);
+
+ // Should never be reached.
+ RAW_CHECK(false);
+ return -ENOSYS;
+}
+
bpf_dsl::ResultExpr CrashSIGSYS() {
return bpf_dsl::Trap(CrashSIGSYS_Handler, NULL);
}
@@ -387,6 +407,11 @@ bpf_dsl::ResultExpr RewriteSchedSIGSYS() {
return bpf_dsl::Trap(SIGSYSSchedHandler, NULL);
}
+bpf_dsl::ResultExpr RewriteFstatatSIGSYS(int fs_denied_errno) {
+ return bpf_dsl::Trap(SIGSYSFstatatHandler,
+ reinterpret_cast<void*>(fs_denied_errno));
+}
+
void AllocateCrashKeys() {
#if !defined(OS_NACL_NONSFI)
if (seccomp_crash_key)
diff --git a/sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.h b/sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.h
index 7a958b93b2..8cd735ce15 100644
--- a/sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.h
+++ b/sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.h
@@ -62,6 +62,19 @@ SANDBOX_EXPORT intptr_t SIGSYSPtraceFailure(const arch_seccomp_data& args,
// sched_setparam(), sched_setscheduler()
SANDBOX_EXPORT intptr_t SIGSYSSchedHandler(const arch_seccomp_data& args,
void* aux);
+// If the fstatat() syscall is functionally equivalent to an fstat() syscall,
+// then rewrite the syscall to the equivalent fstat() syscall which can be
+// adequately sandboxed.
+// If the fstatat() is not functionally equivalent to an fstat() syscall, we
+// fail with -fs_denied_errno.
+// If the syscall is not an fstatat() at all, crash in the same way as
+// CrashSIGSYS_Handler.
+// This is necessary because glibc and musl have started rewriting fstat(fd,
+// stat_buf) as fstatat(fd, "", stat_buf, AT_EMPTY_PATH). We rewrite the latter
+// back to the former, which is actually sandboxable.
+SANDBOX_EXPORT intptr_t
+SIGSYSFstatatHandler(const struct arch_seccomp_data& args,
+ void* fs_denied_errno);
// Variants of the above functions for use with bpf_dsl.
SANDBOX_EXPORT bpf_dsl::ResultExpr CrashSIGSYS();
@@ -72,6 +85,7 @@ SANDBOX_EXPORT bpf_dsl::ResultExpr CrashSIGSYSKill();
SANDBOX_EXPORT bpf_dsl::ResultExpr CrashSIGSYSFutex();
SANDBOX_EXPORT bpf_dsl::ResultExpr CrashSIGSYSPtrace();
SANDBOX_EXPORT bpf_dsl::ResultExpr RewriteSchedSIGSYS();
+SANDBOX_EXPORT bpf_dsl::ResultExpr RewriteFstatatSIGSYS(int fs_denied_errno);
// Allocates a crash key so that Seccomp information can be recorded.
void AllocateCrashKeys();
diff --git a/sandbox/linux/syscall_broker/broker_process.cc b/sandbox/linux/syscall_broker/broker_process.cc
index c2176eb785..e9dad37485 100644
--- a/sandbox/linux/syscall_broker/broker_process.cc
+++ b/sandbox/linux/syscall_broker/broker_process.cc
@@ -113,44 +113,49 @@ bool BrokerProcess::IsSyscallAllowed(int sysno) const {
}
bool BrokerProcess::IsSyscallBrokerable(int sysno, bool fast_check) const {
+ // The syscalls unavailable on aarch64 are all blocked by Android's default
+ // seccomp policy, even on non-aarch64 architectures. I.e., the syscalls XX()
+ // with a corresponding XXat() versions are typically unavailable in aarch64
+ // and are default disabled in Android. So, we should refuse to broker them
+ // to be consistent with the platform's restrictions.
switch (sysno) {
-#if !defined(__aarch64__)
+#if !defined(__aarch64__) && !defined(OS_ANDROID)
case __NR_access:
#endif
case __NR_faccessat:
return !fast_check || allowed_command_set_.test(COMMAND_ACCESS);
-#if !defined(__aarch64__)
+#if !defined(__aarch64__) && !defined(OS_ANDROID)
case __NR_mkdir:
#endif
case __NR_mkdirat:
return !fast_check || allowed_command_set_.test(COMMAND_MKDIR);
-#if !defined(__aarch64__)
+#if !defined(__aarch64__) && !defined(OS_ANDROID)
case __NR_open:
#endif
case __NR_openat:
return !fast_check || allowed_command_set_.test(COMMAND_OPEN);
-#if !defined(__aarch64__)
+#if !defined(__aarch64__) && !defined(OS_ANDROID)
case __NR_readlink:
#endif
case __NR_readlinkat:
return !fast_check || allowed_command_set_.test(COMMAND_READLINK);
-#if !defined(__aarch64__)
+#if !defined(__aarch64__) && !defined(OS_ANDROID)
case __NR_rename:
#endif
case __NR_renameat:
case __NR_renameat2:
return !fast_check || allowed_command_set_.test(COMMAND_RENAME);
-#if !defined(__aarch64__)
+#if !defined(__aarch64__) && !defined(OS_ANDROID)
case __NR_rmdir:
return !fast_check || allowed_command_set_.test(COMMAND_RMDIR);
#endif
-#if !defined(__aarch64__)
+#if !defined(__aarch64__) && !defined(OS_ANDROID)
case __NR_stat:
case __NR_lstat:
#endif
@@ -175,7 +180,7 @@ bool BrokerProcess::IsSyscallBrokerable(int sysno, bool fast_check) const {
return !fast_check || allowed_command_set_.test(COMMAND_STAT);
#endif
-#if !defined(__aarch64__)
+#if !defined(__aarch64__) && !defined(OS_ANDROID)
case __NR_unlink:
return !fast_check || allowed_command_set_.test(COMMAND_UNLINK);
#endif
diff --git a/sandbox/linux/syscall_broker/broker_process_unittest.cc b/sandbox/linux/syscall_broker/broker_process_unittest.cc
index c65f25a78a..f0db08d84e 100644
--- a/sandbox/linux/syscall_broker/broker_process_unittest.cc
+++ b/sandbox/linux/syscall_broker/broker_process_unittest.cc
@@ -1596,52 +1596,52 @@ TEST(BrokerProcess, IsSyscallAllowed) {
const base::flat_map<BrokerCommand, base::flat_set<int>> kSysnosForCommand = {
{COMMAND_ACCESS,
{__NR_faccessat,
-#if defined(__NR_access)
+#if defined(__NR_access) && !defined(OS_ANDROID)
__NR_access
#endif
}},
{COMMAND_MKDIR,
{__NR_mkdirat,
-#if defined(__NR_mkdir)
+#if defined(__NR_mkdir) && !defined(OS_ANDROID)
__NR_mkdir
#endif
}},
{COMMAND_OPEN,
{__NR_openat,
-#if defined(__NR_open)
+#if defined(__NR_open) && !defined(OS_ANDROID)
__NR_open
#endif
}},
{COMMAND_READLINK,
{__NR_readlinkat,
-#if defined(__NR_readlink)
+#if defined(__NR_readlink) && !defined(OS_ANDROID)
__NR_readlink
#endif
}},
{COMMAND_RENAME,
{__NR_renameat,
-#if defined(__NR_rename)
+#if defined(__NR_rename) && !defined(OS_ANDROID)
__NR_rename
#endif
}},
{COMMAND_UNLINK,
{__NR_unlinkat,
-#if defined(__NR_unlink)
+#if defined(__NR_unlink) && !defined(OS_ANDROID)
__NR_unlink
#endif
}},
{COMMAND_RMDIR,
{__NR_unlinkat,
-#if defined(__NR_rmdir)
+#if defined(__NR_rmdir) && !defined(OS_ANDROID)
__NR_rmdir
#endif
}},
{COMMAND_STAT,
{
-#if defined(__NR_stat)
+#if defined(__NR_stat) && !defined(OS_ANDROID)
__NR_stat,
#endif
-#if defined(__NR_lstat)
+#if defined(__NR_lstat) && !defined(OS_ANDROID)
__NR_lstat,
#endif
#if defined(__NR_fstatat)
diff --git a/sandbox/linux/system_headers/linux_stat.h b/sandbox/linux/system_headers/linux_stat.h
index 35788eb22a..83b89efc75 100644
--- a/sandbox/linux/system_headers/linux_stat.h
+++ b/sandbox/linux/system_headers/linux_stat.h
@@ -157,6 +157,10 @@ struct kernel_stat {
};
#endif
+#if !defined(AT_EMPTY_PATH)
+#define AT_EMPTY_PATH 0x1000
+#endif
+
// On 32-bit systems, we default to the 64-bit stat struct like libc
// implementations do. Otherwise we default to the normal stat struct which is
// already 64-bit.
Reference in New Issue View Git Blame Copy Permalink