sandbox: Extend BrokerPolicy to support file creation

sandbox/linux/syscall_broker/broker_file_permission_unittest.cc

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "sandbox/linux/syscall_broker/broker_file_permission.h"
+
+#include <fcntl.h>
+#include <string.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+
+#include "base/logging.h"
+#include "sandbox/linux/tests/test_utils.h"
+#include "sandbox/linux/tests/unit_tests.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace sandbox {
+
+namespace syscall_broker {
+

[jln (very slow on Chromium), 2014/11/26 01:02:31]

add also an anonymous namespace (so that the test functions don't get exported).

[leecam, 2014/11/26 18:35:33]

Done.

+// Creation tests are DEATH tests as a bad permission causes termination.

[jln (very slow on Chromium), 2014/11/26 01:02:32]

Please, use SANDBOX_TEST() instad.

[leecam, 2014/11/26 18:35:33]

Done.

+SANDBOX_DEATH_TEST(BrokerFilePermission, CreateGood, DEATH_SUCCESS()) {
+  const char k_Path[] = "/tmp/good";

[jln (very slow on Chromium), 2014/11/26 01:02:31]

No "_", just kPath (valid a bunch of times in this file).

[leecam, 2014/11/26 18:35:33]

Done.

+  BrokerFilePermission perm = BrokerFilePermission::ReadOnly(k_Path);
+}
+
+SANDBOX_DEATH_TEST(BrokerFilePermission, CreateGoodRecursive, DEATH_SUCCESS()) {
+  const char k_Path[] = "/tmp/good/";
+  BrokerFilePermission perm = BrokerFilePermission::ReadOnlyRecursive(k_Path);
+}
+
+SANDBOX_DEATH_TEST(BrokerFilePermission, CreateBad, DEATH_MESSAGE("")) {

[jln (very slow on Chromium), 2014/11/26 01:02:31]

Is there a message to specify ?

Death test are notably unreliable (exit codes are a huge mess) without them (and
this is where I fear that you'll have to revert your changes to bring back CHECK
<<, right? :) - sorry.

One generic message for everything should work. To avoid duplication (of the 
error string), you could expose a GetBlahErrorMessageForTests() that returns a
const char* pointing to the string.

[leecam, 2014/11/26 18:35:33]

Done.

+  const char k_Path[] = "/tmp/bad/";
+  BrokerFilePermission perm = BrokerFilePermission::ReadOnly(k_Path);
+}
+
+SANDBOX_DEATH_TEST(BrokerFilePermission,
+                   CreateBadRecursive,
+                   DEATH_MESSAGE("")) {
+  const char k_Path[] = "/tmp/bad";
+  BrokerFilePermission perm = BrokerFilePermission::ReadOnlyRecursive(k_Path);
+}
+
+SANDBOX_DEATH_TEST(BrokerFilePermission, CreateBadNotAbs, DEATH_MESSAGE("")) {
+  const char k_Path[] = "tmp/bad";
+  BrokerFilePermission perm = BrokerFilePermission::ReadOnly(k_Path);
+}
+
+SANDBOX_DEATH_TEST(BrokerFilePermission, CreateBadEmpty, DEATH_MESSAGE("")) {
+  const char k_Path[] = "";
+  BrokerFilePermission perm = BrokerFilePermission::ReadOnly(k_Path);
+}
+
+void CheckPerm(BrokerFilePermission& perm,

[jln (very slow on Chromium), 2014/11/26 01:02:31]

Please, add a comment to explain what this function does.

[jln (very slow on Chromium), 2014/11/26 01:02:31]

Non const reference are evil (and forbidden)! Looks like this can just be const.
Otherwise, use pointer.

[leecam, 2014/11/26 18:35:33]

Done.

[leecam, 2014/11/26 18:35:33]

Done.

+               const char* path,
+               int access_flags,
+               bool create) {
+  const char* file_to_open;

[jln (very slow on Chromium), 2014/11/26 01:02:31]

= NULL;

[leecam, 2014/11/26 18:35:33]

Done.

+
+  // check bad perms
+  switch (access_flags) {
+    case O_RDONLY:
+      ASSERT_TRUE(perm.CheckOpen(path, O_RDONLY, &file_to_open, NULL));
+      ASSERT_FALSE(perm.CheckOpen(path, O_WRONLY, &file_to_open, NULL));
+      ASSERT_FALSE(perm.CheckOpen(path, O_RDWR, &file_to_open, NULL));
+      break;
+    case O_WRONLY:
+      ASSERT_FALSE(perm.CheckOpen(path, O_RDONLY, &file_to_open, NULL));
+      ASSERT_TRUE(perm.CheckOpen(path, O_WRONLY, &file_to_open, NULL));
+      ASSERT_FALSE(perm.CheckOpen(path, O_RDWR, &file_to_open, NULL));
+      break;
+    case O_RDWR:
+      ASSERT_TRUE(perm.CheckOpen(path, O_RDONLY, &file_to_open, NULL));
+      ASSERT_TRUE(perm.CheckOpen(path, O_WRONLY, &file_to_open, NULL));
+      ASSERT_TRUE(perm.CheckOpen(path, O_RDWR, &file_to_open, NULL));
+      break;
+    default:
+      // Bad test case
+      NOTREACHED();
+  }
+
+// O_SYNC can be defined as (__O_SYNC|O_DSYNC)
+#ifdef O_DSYNC
+  const int sync_flag = O_SYNC & ~O_DSYNC;

[jln (very slow on Chromium), 2014/11/26 01:02:32]

style: kSyncFlag

[leecam, 2014/11/26 18:35:33]

Done.

+#else
+  const int sync_flag = O_SYNC;
+#endif
+
+  // check every possible flag and act accordingly.
+  for (int i = 2; i < 32; i++) {

[jln (very slow on Chromium), 2014/11/26 01:02:31]

Why not start at 0?

[leecam, 2014/11/26 18:35:33]

This checks the additional open(2) flags. The O_RDONLY, O_WRONLY and O_RDWR
flags are checked above and are needed in every test, so they are handled
slightly differently.
 

[jln (very slow on Chromium), 2014/11/26 19:49:06]

This needs to be documented.

At the very least, make it a documented constant:

const int kNumberOfBitsInOAccMode = 2;
COMPILE_ASSERT(O_ACCMODE == (1 << kNumberOfBitsInOAccMode) - 1);

[leecam, 2014/11/26 20:55:47]

Done.

+    int flag = 1 << i;
+    switch (flag) {
+      case O_APPEND:
+      case O_ASYNC:
+      case O_DIRECT:
+      case O_DIRECTORY:
+#ifdef O_DSYNC
+      case O_DSYNC:
+#endif
+      case O_EXCL:
+      case O_LARGEFILE:
+      case O_NOATIME:
+      case O_NOCTTY:
+      case O_NOFOLLOW:
+      case O_NONBLOCK:
+#if (O_NONBLOCK != O_NDELAY)
+      case O_NDELAY:
+#endif
+      case sync_flag:
+      case O_TRUNC:
+        ASSERT_TRUE(
+            perm.CheckOpen(path, access_flags | flag, &file_to_open, NULL));
+        break;
+      case O_CLOEXEC:
+      case O_CREAT:
+      default:
+        ASSERT_FALSE(
+            perm.CheckOpen(path, access_flags | flag, &file_to_open, NULL));
+    }
+  }
+  if (create) {
+    bool unlink;
+    ASSERT_TRUE(perm.CheckOpen(path, O_CREAT | O_EXCL | access_flags,
+                               &file_to_open, &unlink));
+    ASSERT_FALSE(unlink);
+  } else {
+    ASSERT_FALSE(perm.CheckOpen(path, O_CREAT | O_EXCL | access_flags,
+                                &file_to_open, NULL));
+  }
+}
+
+TEST(BrokerFilePermission, ReadOnly) {
+  const char k_Path[] = "/tmp/good";
+  BrokerFilePermission perm = BrokerFilePermission::ReadOnly(k_Path);
+  CheckPerm(perm, k_Path, O_RDONLY, false);

[jln (very slow on Chromium), 2014/11/26 01:02:32]

Add comments that CheckPerm() has to be the very last thing for ASSERT there to
work as expected.

[leecam, 2014/11/26 18:35:33]

Done.

+}
+
+TEST(BrokerFilePermission, ReadOnlyRecursive) {
+  const char k_Path[] = "/tmp/good/";
+  const char k_PathFile[] = "/tmp/good/file";
+  BrokerFilePermission perm = BrokerFilePermission::ReadOnlyRecursive(k_Path);
+  CheckPerm(perm, k_PathFile, O_RDONLY, false);
+}
+
+TEST(BrokerFilePermission, WriteOnly) {
+  const char k_Path[] = "/tmp/good";
+  BrokerFilePermission perm = BrokerFilePermission::WriteOnly(k_Path);
+  CheckPerm(perm, k_Path, O_WRONLY, false);
+}
+
+TEST(BrokerFilePermission, ReadWrite) {
+  const char k_Path[] = "/tmp/good";
+  BrokerFilePermission perm = BrokerFilePermission::ReadWrite(k_Path);
+  CheckPerm(perm, k_Path, O_RDWR, false);
+}
+
+TEST(BrokerFilePermission, ReadWriteCreate) {
+  const char k_Path[] = "/tmp/good";
+  BrokerFilePermission perm = BrokerFilePermission::ReadWriteCreate(k_Path);
+  CheckPerm(perm, k_Path, O_RDWR, true);
+}
+
+void CheckUnlink(BrokerFilePermission& perm,
+                 const char* path,
+                 int access_flags) {
+  bool unlink;
+  ASSERT_FALSE(perm.CheckOpen(path, access_flags, NULL, &unlink));
+  ASSERT_FALSE(perm.CheckOpen(path, access_flags | O_CREAT, NULL, &unlink));
+  ASSERT_TRUE(
+      perm.CheckOpen(path, access_flags | O_CREAT | O_EXCL, NULL, &unlink));
+  ASSERT_TRUE(unlink);
+}
+
+TEST(BrokerFilePermission, ReadWriteCreateUnlink) {
+  const char k_Path[] = "/tmp/good";
+  BrokerFilePermission perm =
+      BrokerFilePermission::ReadWriteCreateUnlink(k_Path);
+  CheckUnlink(perm, k_Path, O_RDWR);
+}
+
+TEST(BrokerFilePermission, ReadWriteCreateUnlinkRecursive) {
+  const char k_Path[] = "/tmp/good/";
+  const char k_PathFile[] = "/tmp/good/file";
+  BrokerFilePermission perm =
+      BrokerFilePermission::ReadWriteCreateUnlinkRecursive(k_Path);
+  CheckUnlink(perm, k_PathFile, O_RDWR);
+}
+
+class BrokerFilePermissionTester {
+ public:
+  static bool ValidatePath(const char* path) {
+    return BrokerFilePermission::ValidatePath(path);
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(BrokerFilePermissionTester);
+};
+
+TEST(BrokerFilePermission, ValidatePath) {
+  ASSERT_TRUE(BrokerFilePermissionTester::ValidatePath("/path"));

[jln (very slow on Chromium), 2014/11/26 01:02:31]

s/ASSERT/EXPECT in this function.

[leecam, 2014/11/26 18:35:33]

Done.

+  ASSERT_TRUE(BrokerFilePermissionTester::ValidatePath("/"));
+  ASSERT_TRUE(BrokerFilePermissionTester::ValidatePath("/..path"));
+
+  ASSERT_FALSE(BrokerFilePermissionTester::ValidatePath(""));
+  ASSERT_FALSE(BrokerFilePermissionTester::ValidatePath("bad"));
+  ASSERT_FALSE(BrokerFilePermissionTester::ValidatePath("/bad/"));
+  ASSERT_FALSE(BrokerFilePermissionTester::ValidatePath("bad/"));
+  ASSERT_FALSE(BrokerFilePermissionTester::ValidatePath("/bad/.."));
+  ASSERT_FALSE(BrokerFilePermissionTester::ValidatePath("/bad/../bad"));
+  ASSERT_FALSE(BrokerFilePermissionTester::ValidatePath("/../bad"));
+}
+
+}  // namespace syscall_broker
+
+}  // namespace sandbox