add a LinuxSandbox::HasOpenDirectories() sanity check

content/common/sandbox_linux.cc

Index: content/common/sandbox_linux.cc
diff --git a/content/common/sandbox_linux.cc b/content/common/sandbox_linux.cc
index 1746390e84176e08add63a17a9adb68c2c5daaed..fb8b2505f685246c81e78d7e7c6a5ab1d036808a 100644
--- a/content/common/sandbox_linux.cc
+++ b/content/common/sandbox_linux.cc
@@ -2,6 +2,7 @@
 // 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>
@@ -16,6 +17,7 @@
 #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"
@@ -143,6 +145,12 @@ bool LinuxSandbox::InitializeSandbox() {
     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);
 
@@ -208,6 +216,54 @@ bool LinuxSandbox::IsSingleThreaded() const {
   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_;
 }