[linux] O_EXCL requirement in brokered open call too restrictive.

The restriction is only required when "temporary file" permissions
have been granted, since that is what implies no access to
pre-existing files.

Fixes the issue hit by
  https://chromium-review.googlesource.com/c/chromium/src/+/818486

in that the network service may try to ensure a file is present via
a single call with O_CREAT without knowing if it pre-exists.

Change-Id: I3cf3f2389a78a5a46f43505a64d106614e4dcf90
Reviewed-on: https://chromium-review.googlesource.com/820611
Commit-Queue: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Jorge Lucangeli Obes <jorgelo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#523527}

sandbox/linux/syscall_broker/broker_file_permission.cc

@@ -148,13 +148,9 @@ bool BrokerFilePermission::CheckOpen(const char* requested_filename,
     return false;
   }
 
-  // If O_CREAT is present, ensure O_EXCL
-  if ((flags & O_CREAT) && !(flags & O_EXCL)) {
-    return false;
-  }
-
-  // If this file is to be temporary, ensure it's created.
-  if (temporary_only_ && !(flags & O_CREAT)) {
+  // If this file is to be temporary, ensure it is created, not pre-existing.
+  // See https://crbug.com/415681#c17
+  if (temporary_only_ && (!(flags & O_CREAT) || !(flags & O_EXCL))) {
     return false;
   }
 

sandbox/linux/syscall_broker/broker_file_permission.h

@@ -49,6 +49,13 @@ class SANDBOX_EXPORT BrokerFilePermission {
     return BrokerFilePermission(path, true, false, true, true, true);
   }
 
+  // Temporary files must always be newly created and do not confer rights to
+  // use pre-existing files of the same name.
+  static BrokerFilePermission ReadWriteCreateTemporary(
+      const std::string& path) {
+    return BrokerFilePermission(path, false, true, true, true, true);
+  }
+
   static BrokerFilePermission ReadWriteCreateTemporaryRecursive(
       const std::string& path) {
     return BrokerFilePermission(path, true, true, true, true, true);

sandbox/linux/syscall_broker/broker_file_permission_unittest.cc

@@ -161,8 +161,9 @@ void CheckPerm(const BrokerFilePermission& perm,
         ASSERT_TRUE(
             perm.CheckOpen(path, access_flags | flag, &file_to_open, NULL));
         break;
-      case O_CLOEXEC:
       case O_CREAT:
+        continue;  // Handled below.
+      case O_CLOEXEC:
       default:
         ASSERT_FALSE(
             perm.CheckOpen(path, access_flags | flag, &file_to_open, NULL));
@@ -225,7 +226,6 @@ void CheckUnlink(BrokerFilePermission& perm,
                  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);

sandbox/linux/syscall_broker/broker_process_unittest.cc

@@ -641,53 +641,93 @@ TEST(BrokerProcess, BrokerDiesOnClosedChannel) {
 
 TEST(BrokerProcess, CreateFile) {
   std::string temp_str;
+  std::string perm_str;
   {
-    ScopedTemporaryFile tmp_file;
-    temp_str = tmp_file.full_file_name();
+    ScopedTemporaryFile temp_file;
+    ScopedTemporaryFile perm_file;
+    temp_str = temp_file.full_file_name();
+    perm_str = perm_file.full_file_name();
   }
   const char* tempfile_name = temp_str.c_str();
+  const char* permfile_name = perm_str.c_str();
 
   BrokerCommandSet command_set =
       MakeBrokerCommandSet({COMMAND_ACCESS, COMMAND_OPEN});
 
   std::vector<BrokerFilePermission> permissions = {
-      BrokerFilePermission::ReadWriteCreate(tempfile_name)};
+      BrokerFilePermission::ReadWriteCreateTemporary(tempfile_name),
+      BrokerFilePermission::ReadWriteCreate(permfile_name),
+  };
+
   BrokerProcess open_broker(kFakeErrnoSentinel, command_set, permissions);
   ASSERT_TRUE(open_broker.Init(base::BindRepeating(&NoOpCallback)));
 
   int fd = -1;
 
-  // Try without O_EXCL
+  // Opening a temp file using O_CREAT but not O_EXCL must not be allowed
+  // by the broker so as to prevent spying on any pre-existing files.
   fd = open_broker.Open(tempfile_name, O_RDWR | O_CREAT);
   ASSERT_EQ(fd, -kFakeErrnoSentinel);
 
-  const char kTestText[] = "TESTTESTTEST";
-  // Create a file
+  // Opening a temp file in a normal way must not be allowed by the broker,
+  // either.
+  fd = open_broker.Open(tempfile_name, O_RDWR);
+  ASSERT_EQ(fd, -kFakeErrnoSentinel);
+
+  // Opening a temp file with both O_CREAT and O_EXCL is allowed since the
+  // file is known not to exist outside the scope of ScopedTemporaryFile.
   fd = open_broker.Open(tempfile_name, O_RDWR | O_CREAT | O_EXCL);
   ASSERT_GE(fd, 0);
-  {
-    base::ScopedFD scoped_fd(fd);
+  close(fd);
 
-    // Confirm fail if file exists
-    int bad_fd = open_broker.Open(tempfile_name, O_RDWR | O_CREAT | O_EXCL);
-    ASSERT_EQ(bad_fd, -EEXIST);
+  // Manually create a conflict for the temp filename.
+  fd = open(tempfile_name, O_RDWR | O_CREAT, 0600);
+  ASSERT_GE(fd, 0);
+  close(fd);
+
+  // Opening a temp file with both O_CREAT and O_EXCL is allowed but fails
+  // per the OS when there is a conflict with a pre-existing file.
+  fd = open_broker.Open(tempfile_name, O_RDWR | O_CREAT | O_EXCL);
+  ASSERT_EQ(fd, -EEXIST);
 
-    // Write to the descriptor opened by the broker.
+  // Opening a new permanent file without specifying O_EXCL is allowed.
+  fd = open_broker.Open(permfile_name, O_RDWR | O_CREAT);
+  ASSERT_GE(fd, 0);
+  close(fd);
 
+  // Opening an existing permanent file without specifying O_EXCL is allowed.
+  fd = open_broker.Open(permfile_name, O_RDWR | O_CREAT);
+  ASSERT_GE(fd, 0);
+  close(fd);
+
+  // Opening an existing file with O_EXCL is allowed but fails per the OS.
+  fd = open_broker.Open(permfile_name, O_RDWR | O_CREAT | O_EXCL);
+  ASSERT_EQ(fd, -EEXIST);
+
+  const char kTestText[] = "TESTTESTTEST";
+
+  fd = open_broker.Open(permfile_name, O_RDWR);
+  ASSERT_GE(fd, 0);
+  {
+    // Write to the descriptor opened by the broker and close.
+    base::ScopedFD scoped_fd(fd);
     ssize_t len = HANDLE_EINTR(write(fd, kTestText, sizeof(kTestText)));
     ASSERT_EQ(len, static_cast<ssize_t>(sizeof(kTestText)));
   }
 
-  int fd_check = open(tempfile_name, O_RDONLY);
+  int fd_check = open(permfile_name, O_RDONLY);
   ASSERT_GE(fd_check, 0);
   {
     base::ScopedFD scoped_fd(fd_check);
     char buf[1024];
     ssize_t len = HANDLE_EINTR(read(fd_check, buf, sizeof(buf)));
-
     ASSERT_EQ(len, static_cast<ssize_t>(sizeof(kTestText)));
     ASSERT_EQ(memcmp(kTestText, buf, sizeof(kTestText)), 0);
   }
+
+  // Cleanup.
+  unlink(tempfile_name);
+  unlink(permfile_name);
 }
 
 void TestStatHelper(bool fast_check_in_client) {