Linux sandbox: fix O_CLOEXEC support.

Buggy userland code can sometimes check for O_CLOEXEC when what it
really wants is to check for FD_CLOEXEC. We work around this.

BUG=237283
NOTRY=true

Review URL: https://chromiumcodereview.appspot.com/14787006

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@197733 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 19 insertions, 5 deletions

diff --git a/sandbox/linux/services/broker_process.cc b/sandbox/linux/services/broker_process.cc
index 1a1580b..d2302ea 100644
--- a/sandbox/linux/services/broker_process.cc
+++ b/sandbox/linux/services/broker_process.cc
@@ -34,15 +34,18 @@ static const size_t kMaxMessageLength = 4096;
 // Some flags are local to the current process and cannot be sent over a Unix
 // socket. They need special treatment from the client.
 // O_CLOEXEC is tricky because in theory another thread could call execve()
-// before special treatment is made on the client. Thankfully it can be solved
-// by contract, as callers to Open() are typically sandboxed.
+// before special treatment is made on the client, so a client needs to call
+// recvmsg(2) with MSG_CMSG_CLOEXEC.
 // To make things worse, there are two CLOEXEC related flags, FD_CLOEXEC (see
 // F_GETFD in fcntl(2)) and O_CLOEXEC (see F_GETFL in fcntl(2)). O_CLOEXEC
 // doesn't affect the semantics on execve(), it's merely a note that the
 // descriptor was originally opened with O_CLOEXEC as a flag. And it is sent
 // over unix sockets just fine, so a receiver that would (incorrectly) look at
 // O_CLOEXEC instead of FD_CLOEXEC may be tricked in thinking that the file
-// descriptor will be closed on execve().
+// descriptor will or won't be closed on execve().
+// Since we have to account for buggy userland (see crbug.com/237283), we will
+// open(2) the file with O_CLOEXEC in the broker process if necessary, in
+// addition to calling recvmsg(2) with MSG_CMSG_CLOEXEC.
 static const int kCurrentProcessOpenFlagsMask = O_CLOEXEC;
 
 // Check whether |requested_filename| is in |allowed_file_names|.
@@ -89,7 +92,11 @@ bool IsAllowedOpenFlags(int flags) {
   // Some flags affect the behavior of the current process. We don't support
   // them and don't allow them for now.
   if (flags & kCurrentProcessOpenFlagsMask) {
-    return false;
+    // We make an exception for O_CLOEXEC. Buggy userland could check for
+    // O_CLOEXEC and the only way to set it is to originally open with this
+    // flag. See the comment around kCurrentProcessOpenFlagsMask.
+    if (!(flags & O_CLOEXEC))
+      return false;
   }
 
   // Now check that all the flags are known to us.
@@ -206,7 +213,6 @@ int BrokerProcess::PathAndFlagsSyscall(enum IPCCommands syscall_type,
     // this code if other flags are added.
     RAW_CHECK(kCurrentProcessOpenFlagsMask == O_CLOEXEC);
     recvmsg_flags |= MSG_CMSG_CLOEXEC;
-    flags &= ~O_CLOEXEC;
   }
 
   // There is no point in forwarding a request that we know will be denied.
diff --git a/sandbox/linux/services/broker_process_unittest.cc b/sandbox/linux/services/broker_process_unittest.cc
index 103217e..b6589ef 100644
--- a/sandbox/linux/services/broker_process_unittest.cc
+++ b/sandbox/linux/services/broker_process_unittest.cc
@@ -386,6 +386,14 @@ void TestOpenComplexFlags(bool fast_check_in_client) {
   // Important: use F_GETFD, not F_GETFL. The O_CLOEXEC flag in F_GETFL
   // is actually not used by the kernel.
   ASSERT_TRUE(FD_CLOEXEC & ret);
+
+  // There is buggy userland code that can check for O_CLOEXEC with fcntl(2)
+  // even though it doesn't mean anything. We need to support this case.
+  // See crbug.com/237283.
+  ret = fcntl(fd, F_GETFL);
+  ASSERT_NE(-1, ret);
+  ASSERT_TRUE(O_CLOEXEC & ret);
+
   ASSERT_EQ(0, close(fd));
 
   fd = open_broker.Open(kCpuInfo, O_RDONLY | O_NONBLOCK);