Linux syscall broker: fix bionic incompat

Rmdir() and stat() don't work when using bionic on some architectures.
Rmdir() uses __NR_unlinkat with AT_REMOVEDIR and stat uses
__NR_fstatat64 and __NR_newstatat with AT_SYMLINK_NOFOLLOW.
We should allow these syscalls when COMMAND_RMDIR and
COMMAND_STAT are allowed, respectively.

Change-Id: I54953e10f899e07ebadf2ee784179e897fcc1dd2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2391618
Commit-Queue: Matthew Denton <mpdenton@google.com>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#805515}

sandbox/linux/syscall_broker/broker_host.cc

@@ -207,12 +207,6 @@ void StatFileForIPC(const BrokerCommandSet& allowed_command_set,
         reply->AddDataToMessage(reinterpret_cast<char*>(&sb), sizeof(sb)));
   } else {
     DCHECK(command_type == COMMAND_STAT64);
-#if defined(__ANDROID_API__) && __ANDROID_API__ < 21
-    // stat64 is not defined for older Android API versions in newer NDK
-    // versions.
-    RAW_CHECK(reply->AddIntToMessage(-ENOSYS));
-    return;
-#else
     struct stat64 sb;
     int sts = follow_links ? stat64(file_to_access, &sb)
                            : lstat64(file_to_access, &sb);
@@ -223,7 +217,6 @@ void StatFileForIPC(const BrokerCommandSet& allowed_command_set,
     RAW_CHECK(reply->AddIntToMessage(0));
     RAW_CHECK(
         reply->AddDataToMessage(reinterpret_cast<char*>(&sb), sizeof(sb)));
-#endif  // defined(__ANDROID_API__) && __ANDROID_API__ < 21
   }
 }
 

sandbox/linux/syscall_broker/broker_process.cc

@@ -157,6 +157,9 @@ bool BrokerProcess::IsSyscallAllowed(int sysno) const {
 #if defined(__NR_fstatat)
     case __NR_fstatat:
 #endif
+#if defined(__NR_fstatat64)
+    case __NR_fstatat64:
+#endif
 #if defined(__x86_64__) || defined(__aarch64__)
     case __NR_newfstatat:
 #endif
@@ -174,9 +177,13 @@ bool BrokerProcess::IsSyscallAllowed(int sysno) const {
 
 #if !defined(__aarch64__)
     case __NR_unlink:
+      return !fast_check_in_client_ ||
+             allowed_command_set_.test(COMMAND_UNLINK);
 #endif
     case __NR_unlinkat:
+      // If rmdir() doesn't exist, unlinkat is used with AT_REMOVEDIR.
       return !fast_check_in_client_ ||
+             allowed_command_set_.test(COMMAND_RMDIR) ||
              allowed_command_set_.test(COMMAND_UNLINK);
 
     default:
@@ -243,6 +250,33 @@ int BrokerProcess::Unlink(const char* pathname) const {
 #define BROKER_UNPOISON_STRING(x)
 #endif
 
+namespace {
+// Validates the args passed to a *statat*() syscall and performs the syscall
+// using |broker_process|.
+int PerformStatat(const sandbox::arch_seccomp_data& args,
+                  BrokerProcess* broker_process,
+                  bool arch64) {
+  if (static_cast<int>(args.args[0]) != AT_FDCWD)
+    return -EPERM;
+  // Only allow the AT_SYMLINK_NOFOLLOW flag which is used by some libc
+  // implementations for lstat().
+  if ((static_cast<int>(args.args[3]) & ~AT_SYMLINK_NOFOLLOW) != 0)
+    return -EINVAL;
+
+  const bool follow_links =
+      !(static_cast<int>(args.args[3]) & AT_SYMLINK_NOFOLLOW);
+  if (arch64) {
+    return broker_process->Stat64(
+        reinterpret_cast<const char*>(args.args[1]), follow_links,
+        reinterpret_cast<struct stat64*>(args.args[2]));
+  }
+
+  return broker_process->Stat(reinterpret_cast<const char*>(args.args[1]),
+                              follow_links,
+                              reinterpret_cast<struct stat*>(args.args[2]));
+}
+}  // namespace
+
 // static
 intptr_t BrokerProcess::SIGSYS_Handler(const sandbox::arch_seccomp_data& args,
                                        void* aux_broker_process) {
@@ -367,23 +401,15 @@ intptr_t BrokerProcess::SIGSYS_Handler(const sandbox::arch_seccomp_data& args,
 #endif
 #if defined(__NR_fstatat)
     case __NR_fstatat:
-      if (static_cast<int>(args.args[0]) != AT_FDCWD)
-        return -EPERM;
-      if (static_cast<int>(args.args[3]) != 0)
-        return -EINVAL;
-      return broker_process->Stat(reinterpret_cast<const char*>(args.args[1]),
-                                  true,
-                                  reinterpret_cast<struct stat*>(args.args[2]));
+      return PerformStatat(args, broker_process, /*arch64=*/false);
+#endif
+#if defined(__NR_fstatat64)
+    case __NR_fstatat64:
+      return PerformStatat(args, broker_process, /*arch64=*/true);
 #endif
 #if defined(__NR_newfstatat)
     case __NR_newfstatat:
-      if (static_cast<int>(args.args[0]) != AT_FDCWD)
-        return -EPERM;
-      if (static_cast<int>(args.args[3]) != 0)
-        return -EINVAL;
-      return broker_process->Stat(reinterpret_cast<const char*>(args.args[1]),
-                                  true,
-                                  reinterpret_cast<struct stat*>(args.args[2]));
+      return PerformStatat(args, broker_process, /*arch64=*/false);
 #endif
 #if defined(__NR_unlink)
     case __NR_unlink:
@@ -391,13 +417,24 @@ intptr_t BrokerProcess::SIGSYS_Handler(const sandbox::arch_seccomp_data& args,
           reinterpret_cast<const char*>(args.args[0]));
 #endif
 #if defined(__NR_unlinkat)
-    case __NR_unlinkat:
-      // TODO(tsepez): does not support AT_REMOVEDIR flag.
+    case __NR_unlinkat: {
       if (static_cast<int>(args.args[0]) != AT_FDCWD)
         return -EPERM;
+
+      int flags = static_cast<int>(args.args[2]);
+
+      if (flags == AT_REMOVEDIR) {
+        return broker_process->Rmdir(
+            reinterpret_cast<const char*>(args.args[1]));
+      }
+
+      if (flags != 0)
+        return -EPERM;
+
       return broker_process->Unlink(
           reinterpret_cast<const char*>(args.args[1]));
-#endif
+    }
+#endif  // defined(__NR_unlinkat)
     default:
       RAW_CHECK(false);
       return -ENOSYS;

sandbox/linux/syscall_broker/broker_process_unittest.cc

@@ -21,6 +21,8 @@
 
 #include "base/bind.h"
 #include "base/callback.h"
+#include "base/containers/flat_map.h"
+#include "base/containers/flat_set.h"
 #include "base/files/file_util.h"
 #include "base/files/scoped_file.h"
 #include "base/logging.h"
@@ -30,6 +32,7 @@
 #include "base/strings/stringprintf.h"
 #include "build/build_config.h"
 #include "sandbox/linux/syscall_broker/broker_client.h"
+#include "sandbox/linux/syscall_broker/broker_command.h"
 #include "sandbox/linux/tests/scoped_temporary_file.h"
 #include "sandbox/linux/tests/test_utils.h"
 #include "sandbox/linux/tests/unit_tests.h"
@@ -1453,80 +1456,108 @@ TEST(BrokerProcess, UnlinkHost) {
 }
 
 TEST(BrokerProcess, IsSyscallAllowed) {
-  const struct {
-    int sysno;
-    BrokerCommand command;
-  } kSyscallToCommandMap[] = {
+  const base::flat_map<BrokerCommand, base::flat_set<int>> kSysnosForCommand = {
+      {COMMAND_ACCESS,
+       {__NR_faccessat,
 #if defined(__NR_access)
-    {__NR_access, COMMAND_ACCESS},
+        __NR_access
 #endif
-    {__NR_faccessat, COMMAND_ACCESS},
+       }},
+      {COMMAND_MKDIR,
+       {__NR_mkdirat,
 #if defined(__NR_mkdir)
-    {__NR_mkdir, COMMAND_MKDIR},
+        __NR_mkdir
 #endif
-    {__NR_mkdirat, COMMAND_MKDIR},
+       }},
+      {COMMAND_OPEN,
+       {__NR_openat,
 #if defined(__NR_open)
-    {__NR_open, COMMAND_OPEN},
+        __NR_open
 #endif
-    {__NR_openat, COMMAND_OPEN},
+       }},
+      {COMMAND_READLINK,
+       {__NR_readlinkat,
 #if defined(__NR_readlink)
-    {__NR_readlink, COMMAND_READLINK},
+        __NR_readlink
 #endif
-    {__NR_readlinkat, COMMAND_READLINK},
+       }},
+      {COMMAND_RENAME,
+       {__NR_renameat,
 #if defined(__NR_rename)
-    {__NR_rename, COMMAND_RENAME},
+        __NR_rename
 #endif
-    {__NR_renameat, COMMAND_RENAME},
+       }},
+      {COMMAND_UNLINK,
+       {__NR_unlinkat,
+#if defined(__NR_unlink)
+        __NR_unlink
+#endif
+       }},
+      {COMMAND_RMDIR,
+       {__NR_unlinkat,
 #if defined(__NR_rmdir)
-    {__NR_rmdir, COMMAND_RMDIR},
+        __NR_rmdir
 #endif
+       }},
+      {COMMAND_STAT,
+       {
 #if defined(__NR_stat)
-    {__NR_stat, COMMAND_STAT},
+           __NR_stat,
 #endif
 #if defined(__NR_lstat)
-    {__NR_lstat, COMMAND_STAT},
+           __NR_lstat,
 #endif
 #if defined(__NR_fstatat)
-    {__NR_fstatat, COMMAND_STAT},
+           __NR_fstatat,
+#endif
+#if defined(__NR_fstatat64)
+           __NR_fstatat64,
 #endif
 #if defined(__NR_newfstatat)
-    {__NR_newfstatat, COMMAND_STAT},
+           __NR_newfstatat,
 #endif
 #if defined(__NR_stat64)
-    {__NR_stat64, COMMAND_STAT},
+           __NR_stat64,
 #endif
 #if defined(__NR_lstat64)
-    {__NR_lstat64, COMMAND_STAT},
-#endif
-#if defined(__NR_unlink)
-    {__NR_unlink, COMMAND_UNLINK},
+           __NR_lstat64,
 #endif
-    {__NR_unlinkat, COMMAND_UNLINK},
-  };
+       }}};
+
+  // First gather up all the syscalls numbers we want to test.
+  base::flat_set<int> all_sysnos;
+  for (const auto& command_sysno_set_pair : kSysnosForCommand) {
+    all_sysnos.insert(command_sysno_set_pair.second.begin(),
+                      command_sysno_set_pair.second.end());
+  }
 
-  for (const auto& test : kSyscallToCommandMap) {
+  for (const auto& test : kSysnosForCommand) {
     // Test with fast_check_in_client.
     {
-      SCOPED_TRACE(base::StringPrintf("fast check, sysno=%d", test.sysno));
-      BrokerProcess process(ENOSYS, MakeBrokerCommandSet({test.command}), {},
-                            true, true);
-      EXPECT_TRUE(process.IsSyscallAllowed(test.sysno));
-      for (const auto& other : kSyscallToCommandMap) {
-        SCOPED_TRACE(base::StringPrintf("others test, sysno=%d", other.sysno));
-        EXPECT_EQ(other.command == test.command,
-                  process.IsSyscallAllowed(other.sysno));
+      BrokerCommand command = test.first;
+      const base::flat_set<int>& sysnos = test.second;
+      SCOPED_TRACE(base::StringPrintf("fast check, command=%d", command));
+      BrokerProcess process(ENOSYS, MakeBrokerCommandSet({command}), {},
+                            /*fast_check_in_client=*/true,
+                            /*quiet_failures_for_tests=*/true);
+      // Check that only the correct system calls are allowed.
+      for (int sysno : all_sysnos) {
+        SCOPED_TRACE(base::StringPrintf("test syscalls, sysno=%d", sysno));
+        EXPECT_EQ(sysnos.count(sysno) > 0, process.IsSyscallAllowed(sysno));
       }
     }
 
     // Test without fast_check_in_client.
     {
-      SCOPED_TRACE(base::StringPrintf("no fast check, sysno=%d", test.sysno));
-      BrokerProcess process(ENOSYS, MakeBrokerCommandSet({test.command}), {},
-                            false, true);
-      EXPECT_TRUE(process.IsSyscallAllowed(test.sysno));
-      for (const auto& other : kSyscallToCommandMap) {
-        SCOPED_TRACE(base::StringPrintf("others test, sysno=%d", other.sysno));
-        EXPECT_TRUE(process.IsSyscallAllowed(other.sysno));
+      BrokerCommand command = test.first;
+      SCOPED_TRACE(base::StringPrintf("no fast check, command=%d", command));
+      BrokerProcess process(ENOSYS, MakeBrokerCommandSet({command}), {},
+                            /*fast_check_in_client=*/false,
+                            /*quiet_failures_for_tests=*/true);
+      // Check that all system calls are allowed.
+      for (int sysno : all_sysnos) {
+        SCOPED_TRACE(base::StringPrintf("test syscalls, sysno=%d", sysno));
+        EXPECT_TRUE(process.IsSyscallAllowed(sysno));
       }
     }
   }