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/callback_helpers.h"
@@ skipping 38 matching lines @@
 }
 
 bool IsRunningTSAN() {
 #if defined(THREAD_SANITIZER)
   return true;
 #else
   return false;
 #endif
 }
 
+#if !defined(NDEBUG)

[jln (very slow on Chromium), 2013/10/25 19:49:52]

Maybe do DEBUG_NOTREACHED() instead which does CHECK(false) in debug mode (see
below)

But I don't think you need this at all.

[Mostyn Bramley-Moore, 2013/10/26 07:53:44]

Removed this in favour of DIRDeleter.

+// For when using DCHECK would be incorrect, since it can be enabled in
+// non official release mode.

[jln (very slow on Chromium), 2013/10/25 19:49:52]

Just explain what it does, the "for when using DCHECK would be incorrect" is
unclear out of context.

[Mostyn Bramley-Moore, 2013/10/26 07:53:44]

Removed this in favour of DIRDeleter.

+#define REAL_DEBUG_CHECK(x) CHECK(x)
+#else
+#define REAL_DEBUG_CHECK(x)
+#endif  // !defined(NDEBUG)
+
+class OpenDirDeleter {

[jln (very slow on Chromium), 2013/10/25 19:49:52]

Sorry, what I meant by creating an OpenDirDeleter() was to

create a scoped_ptr<DIR, DIRDeleter>, i.e. override the "deleter" part of a
scoped pointer. The goal was to keep things very simple without having to create
a giant class.

What you have created is essentially a "ScopedOpenDir", but with a slightly odd
interface.

[Mostyn Bramley-Moore, 2013/10/26 07:53:44]

I hadn't noticed this version of scoped_ptr- looks very useful.

+ public:
+
+  OpenDirDeleter() : dir_(NULL) {}
+
+  ~OpenDirDeleter() {
+    if (dir_) {
+      int result = closedir(dir_);
+      (void)result;
+      REAL_DEBUG_CHECK(result == 0);
+    }
+  }
+
+  // Just like the real fdopendir except it returns bool instead
+  // of a directory stream.  Returns false if this method on this object
+  // was successfully called previously, or if the requested dir could not
+  // be opened.  Assumes ownership of fd.
+  bool fdopendir(int fd) {
+    if (dir_) {
+      return false;
+    }
+
+    // The "real" fdopendir takes ownership of fd here.
+    dir_ = ::fdopendir(fd);
+    if (!dir_) {
+      return false;
+    }
+    return true;
+  }
+
+  // Call readdir_r on the directory stream descriptor and return the result.
+  int readdir_r(struct dirent *entry, struct dirent **result) {
+    return ::readdir_r(dir_, entry, result);
+  }
+
+ private:
+

[jln (very slow on Chromium), 2013/10/25 19:49:52]

FYI (since this class may disappear): always add DISALLOW_COPY_AND_ASSIGN()

[Mostyn Bramley-Moore, 2013/10/25 20:41:15]

Will do, thanks for the tip.

+  DIR *dir_;
+};
+
 }  // namespace
 
 namespace content {
 
 LinuxSandbox::LinuxSandbox()
     : proc_fd_(-1),
       seccomp_bpf_started_(false),
       pre_initialized_(false),
       seccomp_bpf_supported_(false),
       setuid_sandbox_client_(sandbox::SetuidSandboxClient::Create()) {
@@ skipping 63 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(FATAL) << "InitializeSandbox() called after unexpected directries "

[jln (very slow on Chromium), 2013/10/25 19:49:52]

nit: directories

[Mostyn Bramley-Moore, 2013/10/26 07:53:44]

Done.

+                  "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 {
+  // Note: this function won't catch the following cases:
+  // 1) proc_fd_ is valid and /proc is opened elsewhere a second time (but not
+  //    closed).
+  // 2) /proc/self/fd is opened elsewhere a second time (but not closed).
+
+  // If /proc is already open, store the device/inode pair that uniquely
+  // identifies it.
+  dev_t proc_device = 0;
+  ino_t proc_inode = 0;
+
+  // To store the device/inode pair that uniquely identifies /proc/self/fd.
+  dev_t proc_self_fd_device = 0;
+  ino_t proc_self_fd_inode = 0;
+
+  int proc_self_fd = 0;
+
+  struct stat s;

[jln (very slow on Chromium), 2013/10/25 19:49:52]

Please, scope your variables as much as possible. For instance you can declare
these inside the "if", which will also allow you to avoid re-using the same name
twice for different purposes.

[Mostyn Bramley-Moore, 2013/10/26 07:53:44]

Done.

+  int stat_res;
+
+  if (proc_fd_ >= 0) {

[jln (very slow on Chromium), 2013/10/25 19:49:52]

You're handling the case where we don't have proc_fd_ available here, so
anything below can be a normal check.

But the structure is wrong.

Do something like:

int proc_fd = proc_fd_;

#if !defined(NDEBUG)
CHECK_GE(0, proc_fd_)
#endif

if (proc_fd_ == -1) {
 proc_fd = open()
 if (proc_fd < 0) return false
}

// Now the code can continue just knowing that you have a proc fd in proc_fd.

[Mostyn Bramley-Moore, 2013/10/25 20:41:15]

I could leave a "the code after this point can assume we're in debug mode"
comment, but isn't it better to use something more explicit, like a DCHECK?

[jln (very slow on Chromium), 2013/10/26 01:15:15]

We should check that in debug mode we have proc_fd_ available. That's done with
something like.

#if !defined(NDEBUG)
CHECK_GE(0, proc_fd_)
#endif

There is nothing else to do, after that, y ou can just handle the case where
proc_fd == -1 as explained above.

This will keep the code very simple, without convoluted logic.

[Mostyn Bramley-Moore, 2013/10/26 07:53:44]

Rather than create a local proc_fd and having to be careful to close it, I
reused proc_fd_ (splitting out the code the opens /proc into a separate
function).  This should then be handled safely by SealSandbox().

+    proc_self_fd = openat(proc_fd_, "self/fd", O_RDONLY|O_DIRECTORY);
+    REAL_DEBUG_CHECK(proc_self_fd != -1);

[jln (very slow on Chromium), 2013/10/25 19:49:52]

This should be a normal CHECK.

[Mostyn Bramley-Moore, 2013/10/26 07:53:44]

Done.

+    if (proc_self_fd == -1) {
+      // We can't read the list of open file descriptors, so guess false.
+      return false;
+    }
+
+    stat_res = fstat(proc_fd_, &s);
+    REAL_DEBUG_CHECK(stat_res == 0);

[jln (very slow on Chromium), 2013/10/25 19:49:52]

This could be a normal check

[Mostyn Bramley-Moore, 2013/10/26 07:53:44]

Removed this, since we don't need to stat /proc.

+    if (stat_res != 0) {
+      // If we continue, we'll get a false positive, so guess false.
+      return false;
+    }
+    proc_device = s.st_dev;
+    proc_inode = s.st_ino;
+
+  } else {
+    proc_self_fd = open("/proc/self/fd", O_RDONLY|O_DIRECTORY);
+    REAL_DEBUG_CHECK(proc_self_fd != -1);

[jln (very slow on Chromium), 2013/10/25 19:49:52]

This should be a normal CHECK

[Mostyn Bramley-Moore, 2013/10/26 07:53:44]

Removed this case, since we return early if proc_fd_ is not available.

+    if (proc_self_fd == -1) {
+      // We can't read the list of open file descriptors, so guess false.
+      return false;
+    }
+  }
+
+  stat_res = fstat(proc_self_fd, &s);
+  REAL_DEBUG_CHECK(stat_res == 0);

[jln (very slow on Chromium), 2013/10/25 19:49:52]

This should be a normal CHECK

[Mostyn Bramley-Moore, 2013/10/26 07:53:44]

Removed this, since we don't need to stat /proc/self/fd.

+  if (stat_res != 0) {
+    // If we continue we'll get a false positive, so guess false.
+    return false;
+  }
+  proc_self_fd_device = s.st_dev;
+  proc_self_fd_inode = s.st_ino;
+
+  // We now have unique identifiers for /proc/self/fd and possibly /proc,
+  // let's see if there are any other directories open.

[jln (very slow on Chromium), 2013/10/25 19:49:52]

Are you hitting issue with implementation details of these libraries re-opening
these things?

I'm particularly worried about "something" keeping /proc open.

You should go through /proc/self/fd, and discard whatever number proc_fd is and
whatever number proc_self_fd is and that's all, no?

[Mostyn Bramley-Moore, 2013/10/25 20:41:15]

No, I'm just trying to be more thorough and cover the possibility that you
mentioned earlier, where one of these fd's has been dup'ed, without returning a
false positive.  

I figure that it is safe to not return true in this case, since closing any one
of a set of dup'ed fd's should close them all, and we are careful to close the
original one.  
Would you prefer me to change to comparing fd's directly?

[jln (very slow on Chromium), 2013/10/26 01:15:15]

No, we absolutely need to fail in this case, that's a dangerous situation.
No, if a bunch of fd-s are duped, closing one won't close the others.
Yes please.

[Mostyn Bramley-Moore, 2013/10/26 07:53:44]

Ah right, I misunderstood this part of the dup manpage :/
Done.

+
+  scoped_ptr<OpenDirDeleter> odd(new OpenDirDeleter());

[jln (very slow on Chromium), 2013/10/25 19:49:52]

As explained above, I was thinking of a custom deleter for the scoped_ptr. If
that's not possible for some reason (please explain why), maybe a
ScopedClosureRunner() ?

Let's use a ScopedOpendir class as a last resort.

[Mostyn Bramley-Moore, 2013/10/26 07:53:44]

Done.

+
+  // Ownership of proc_self_fd is transferred here, it must not be closed
+  // or modified afterwards except by odd.
+  if (!odd->fdopendir(proc_self_fd)) {
+    REAL_DEBUG_CHECK(false);
+    // We can't read the list of open file descriptors, so guess false.
+    return false;
+  }
+
+  struct dirent e, *de;
+  while (!odd->readdir_r(&e, &de) && de) {
+    if (strcmp(e.d_name, ".") == 0 || strcmp(e.d_name, "..") == 0)
+      continue;
+
+    // Stat the file pointed to by the current file descriptor.
+    // It's OK to use proc_self_fd here, fstatat won't modify it.
+    stat_res = fstatat(proc_self_fd, e.d_name, &s, 0);
+    REAL_DEBUG_CHECK(stat_res == 0);
+    if (stat_res != 0) {
+      // We failed to check this file descriptor but maybe a later one will
+      // succeed(?), so let's continue rather than guessing false immediately.
+      continue;
+    }
+
+    if (S_ISDIR(s.st_mode)) {
+      // Check if this directory is one of the managed directories that are
+      // OK to have open.
+
+      if (proc_fd_ != -1 && proc_device
+          && proc_device == s.st_dev && proc_inode == s.st_ino) {
+        continue; // Skip over /proc.
+      }
+
+      if (proc_self_fd_device == s.st_dev && proc_self_fd_inode == s.st_ino) {
+        continue; // Skip over /proc/self/fd.
+      }
+
+      // We found an open dir that wasn't /proc or /proc/self/fd.
+      return true;
+    }
+  }
+
+  // No open unmanaged directories found. \o/
+  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 58 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