add a LinuxSandbox::HasOpenDirectories() sanity check

content/common/sandbox_linux.cc

 // Copyright (c) 2012 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 <dirent.h>
 #include <fcntl.h>
 #include <sys/resource.h>
 #include <sys/stat.h>
 #include <sys/time.h>
 #include <sys/types.h>
 
 #include <limits>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/memory/singleton.h"
 #include "base/posix/eintr_wrapper.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/time/time.h"
 #include "content/common/sandbox_linux.h"
 #include "content/common/sandbox_seccomp_bpf_linux.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/common/sandbox_linux.h"
 #include "sandbox/linux/suid/client/setuid_sandbox_client.h"
 
 namespace {
 
 void LogSandboxStarted(const std::string& sandbox_name) {
@@ skipping 107 matching lines @@
     if (IsRunningTSAN())
       return false;
     // The GPU process is allowed to call InitializeSandbox() with threads for
     // now, because it loads third party libraries.
     if (process_type != switches::kGpuProcess)
       CHECK(false) << error_message;
     LOG(ERROR) << error_message;
     return false;
   }
 
+  if (linux_sandbox->HasOpenDirectories()) {
+    LOG(ERROR) << "InitializeSandbox() called after unexpected directries "

[jln (very slow on Chromium), 2013/10/22 01:10:50]

Let's LOG(FATAL).

[Mostyn Bramley-Moore, 2013/10/23 23:15:19]

Done in patchset 2.

+                  "have been opened- the setuid sandbox may be at risk, if "
+                  "the BPF sandbox is not running.";
+  }
+
   // Attempt to limit the future size of the address space of the process.
   linux_sandbox->LimitAddressSpace(process_type);
 
   // First, try to enable seccomp-bpf.
   seccomp_bpf_started = linux_sandbox->StartSeccompBpf(process_type);
 
   return seccomp_bpf_started;
 }
 
 int LinuxSandbox::GetStatus() const {
@@ skipping 45 matching lines @@
   }
 
   // At least "..", "." and the current thread should be present.
   CHECK_LE(3UL, task_stat.st_nlink);
   // Counting threads via /proc/self/task could be racy. For the purpose of
   // determining if the current proces is monothreaded it works: if at any
   // time it becomes monothreaded, it'll stay so.
   return task_stat.st_nlink == 3;
 }
 
+bool LinuxSandbox::HasOpenDirectories() const {
+  short num_dirs = 0;
+
+  DIR* fd_dir = NULL;

[jln (very slow on Chromium), 2013/10/22 01:10:50]

It's too dangerous and error-prone to make sure that we call closedir().

Either create a new OpenDirDeleter class and use a scoped_ptr, or use a
ScopedClosureRunner.

I would prefer the former. Don't forget to CHECK closedir().

[Mostyn Bramley-Moore, 2013/10/23 23:15:19]

Done in patchset 2.

+  int fd_fd;
+
+  if (proc_fd_ >= 0) {
+    fd_fd = openat(proc_fd_, "self/fd", O_RDONLY|O_DIRECTORY);
+  }

[jln (very slow on Chromium), 2013/10/22 01:10:50]

style: else on the same line.

[Mostyn Bramley-Moore, 2013/10/23 23:15:19]

Done.

+  else {
+    fd_fd = open("/proc/self/fd", O_RDONLY|O_DIRECTORY);
+  }
+  fd_dir = fdopendir(fd_fd);

[jln (very slow on Chromium), 2013/10/22 01:10:50]

Declare fd_dir only here as needed. Add a comment that ownership of "fd_fd" is
transfered to fd_dir as the API is not obvious.

[Mostyn Bramley-Moore, 2013/10/23 23:15:19]

The OpenDirDeleter code should take care of this.

+  if (!fd_dir) {

[jln (very slow on Chromium), 2013/10/22 01:10:50]

Add a #if !DEFINED(NDEBUG) ... CHECK(fd_dir) above (see line 199 for why).

[Mostyn Bramley-Moore, 2013/10/23 23:15:19]

I ended up using this in a few places, so I created a REAL_DEBUG_CHECK macro to
keep things concise.

+    // We're unable to find the real answer, guess false.
+    return false;
+  }
+
+  struct dirent* e = NULL;
+  struct stat s;
+  while ((e = readdir(fd_dir))) {

[jln (very slow on Chromium), 2013/10/22 01:10:50]

Please, use readdir_r, I wouldn't want to rely on the fact that we're
mono-threaded.

You can just allocate the dirent on the stack.

[Mostyn Bramley-Moore, 2013/10/23 23:15:19]

Done.

+    if (strcmp(e->d_name, ".") == 0 || strcmp(e->d_name, "..") == 0)
+      continue;
+
+    // Skip over the /proc file descriptor.
+    int fd_num;
+    if (proc_fd_ != -1 && base::StringToInt(e->d_name, &fd_num)) {
+      if (fd_num == proc_fd_)
+        continue;
+    }
+
+    if (fstatat(fd_fd, e->d_name, &s, 0) == 0) {

[jln (very slow on Chromium), 2013/10/22 01:10:50]

This is quote subtle and error prone, so please CHECK this use of fstatat.

[Mostyn Bramley-Moore, 2013/10/23 23:15:19]

Done.

+      if (S_ISDIR(s.st_mode)) {
+        num_dirs++;
+        // We had to open /proc/self/fd/ so we really want to check if

[jln (very slow on Chromium), 2013/10/22 01:10:50]

proc_fd_ and fd_fd should be treated the same, no ?

It's a lot safer to check the exact same file descriptor than just counting.

Unfortunately in theory fdopendir() could be at liberty to dup() etc, but your
current test would already fail under such a scenario.

You could still maintain a counter a DCHECK that it's less than 2.

[Mostyn Bramley-Moore, 2013/10/22 19:10:04]

Yes, except that we know fd_fd must be valid whereas proc_fd_ might not be.
If we don't want to count duplicate file descriptors, then I suppose we should
instead check for the device/inode pair of /proc and /proc/self/fd.
Are these the situations we want to catch:

1) proc_fd_ is valid, /proc/self/fd is open, and a third distinct directory (ie
neither /proc nor /proc/self/fd) is also open.

2) proc_fd_ is invalid and any directory besides /proc/self/fd is open.

[Mostyn Bramley-Moore, 2013/10/23 23:15:19]

In patchset 2 I replaced this counting logic with comparisons to the
device/inode pairs for the managed directories.

+        // there are more than one directory open (ignoring /proc/).
+        if (num_dirs > 1) {
+          closedir(fd_dir);
+          return true;
+        }
+      }
+    }
+  }
+
+  closedir(fd_dir);
+  return false;
+}
+
 bool LinuxSandbox::seccomp_bpf_started() const {
   return seccomp_bpf_started_;
 }
 
 sandbox::SetuidSandboxClient*
     LinuxSandbox::setuid_sandbox_client() const {
   return setuid_sandbox_client_.get();
 }
 
 // For seccomp-bpf, we use the SandboxSeccompBpf class.
@@ skipping 55 matching lines @@
 void LinuxSandbox::SealSandbox() {
   if (proc_fd_ >= 0) {
     int ret = HANDLE_EINTR(close(proc_fd_));
     CHECK_EQ(0, ret);
     proc_fd_ = -1;
   }
 }
 
 }  // namespace content