NaCl: Add sanity check for number of open FDs at startup

components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc

 // Copyright 2014 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 "components/nacl/loader/sandbox_linux/nacl_sandbox_linux.h"
 
 #include <errno.h>
 #include <fcntl.h>
 #include <sys/stat.h>
 #include <sys/types.h>
@@ skipping 28 matching lines @@
   return true;
 }
 
 }  // namespace
 
 NaClSandbox::NaClSandbox()
     : layer_one_enabled_(false),
       layer_one_sealed_(false),
       layer_two_enabled_(false),
       layer_two_is_nonsfi_(false),
-      proc_fd_(-1) {
+      proc_fd_(-1),
+      setuid_sandbox_client_(sandbox::SetuidSandboxClient::Create()) {
   proc_fd_.reset(
       HANDLE_EINTR(open("/proc", O_DIRECTORY | O_RDONLY | O_CLOEXEC)));
   PCHECK(proc_fd_.is_valid());
 }
 
 NaClSandbox::~NaClSandbox() {
 }
 
 bool NaClSandbox::IsSingleThreaded() {
   CHECK(proc_fd_.is_valid());
   base::ScopedFD proc_self_task(HANDLE_EINTR(openat(
       proc_fd_.get(), "self/task/", O_RDONLY | O_DIRECTORY | O_CLOEXEC)));
   PCHECK(proc_self_task.is_valid());
   return sandbox::ThreadHelpers::IsSingleThreaded(proc_self_task.get());
 }
 
 bool NaClSandbox::HasOpenDirectory() {
   CHECK(proc_fd_.is_valid());
   sandbox::Credentials credentials;
   return credentials.HasOpenDirectory(proc_fd_.get());
 }
 
 void NaClSandbox::InitializeLayerOneSandbox() {
   // Check that IsSandboxed() works. We should not be sandboxed at this point.
   CHECK(!IsSandboxed()) << "Unexpectedly sandboxed!";
-  scoped_ptr<sandbox::SetuidSandboxClient> setuid_sandbox_client(
-      sandbox::SetuidSandboxClient::Create());
-  const bool suid_sandbox_child = setuid_sandbox_client->IsSuidSandboxChild();
 
-  if (suid_sandbox_child) {
-    setuid_sandbox_client->CloseDummyFile();
+  if (setuid_sandbox_client_->IsSuidSandboxChild()) {
+    setuid_sandbox_client_->CloseDummyFile();
 
     // Make sure that no directory file descriptor is open, as it would bypass
     // the setuid sandbox model.
     CHECK(!HasOpenDirectory());
 
     // Get sandboxed.
-    CHECK(setuid_sandbox_client->ChrootMe());
+    CHECK(setuid_sandbox_client_->ChrootMe());
     CHECK(IsSandboxed());
     layer_one_enabled_ = true;
   }
 }
 
 void NaClSandbox::InitializeLayerTwoSandbox(bool uses_nonsfi_mode) {
   // seccomp-bpf only applies to the current thread, so it's critical to only
   // have a single thread running here.
   DCHECK(!layer_one_sealed_);
   CHECK(IsSingleThreaded());
+
+  if (setuid_sandbox_client_->IsSuidSandboxChild()) {

[jln (very slow on Chromium), 2014/05/07 23:08:24]

Do you mind putting this in its own bool-returning function? It looks a little
weird to implement a sanity check in an initialization function rather than just
call it.

CHECK(HasExpectedNumberOfOpenFds());

Also I expect NaClSandbox and the sandbox in content/ to inherit from a common
class (in sandbox/), hopefully not too long for now) and this would help making
the divide natural.

Your choice though.

[Mark Seaborn, 2014/05/09 22:02:31]

I've created a separate function, but not a bool-returning one.

The problem with a bool-returning function is that I can't use CHECK_EQ to print
the actual number of FDs on error, which makes debugging failures more awkward.
FWIW, I tend to avoid implementation inheritance, though it depends on the
specifics.

+    // We expect to have the following FDs open:
+    //  1-3) stdin, stdout, stderr.
+    //  4) /dev/urandom FD.

[jln (very slow on Chromium), 2014/05/07 23:08:24]

Do you want to reference base::GetUrandomFD() ?

[Mark Seaborn, 2014/05/09 22:02:31]

Done.

+    //  5) A dummy pipe FD used to overwrite kSandboxIPCChannel.
+    //  6) The socket created by the SUID sandbox helper, used by ChrootMe().
+    //     After ChrootMe(), this is no longer connected to anything.
+    //     (Only present when running under the SUID sandbox.)

[jln (very slow on Chromium), 2014/05/07 23:08:24]

I will close this.

+    //  7) The socket for the Chrome IPC channel that's connected to the
+    //     browser process, kPrimaryIPCChannel.

[jln (very slow on Chromium), 2014/05/07 23:08:24]

It should only be open in SFI mode though:
https://codereview.chromium.org/255903004

Could you mention that it's a dummy in non-SFI mode?

[jln (very slow on Chromium), 2014/05/07 23:17:02]

Sorry, was confused with kSandboxIPCChannel.

+    //
+    // This sanity check ensures that dynamically loaded libraries don't
+    // leave any FDs open before we enable the sandbox.
+    sandbox::Credentials credentials;
+    CHECK_EQ(credentials.CountOpenFds(proc_fd_.get()), 7);

[jln (very slow on Chromium), 2014/05/07 23:08:24]

I was puzzled as to why this would work in non-sfi mode and I remembered that I
replaced the kPrimaryIPCChannel with a dummy, instead of closing it.

[jln (very slow on Chromium), 2014/05/07 23:17:02]

err, I meant kSandboxIPCChannel.

+  }
+
   if (uses_nonsfi_mode) {
     layer_two_enabled_ = nacl::nonsfi::InitializeBPFSandbox();
     layer_two_is_nonsfi_ = true;
   } else {
     layer_two_enabled_ = nacl::InitializeBPFSandbox();
   }
 }
 
 void NaClSandbox::SealLayerOneSandbox() {
   if (!layer_two_enabled_) {
@@ skipping 29 matching lines @@
     static const char kNoBpfMsg[] =
         "The seccomp-bpf sandbox is not engaged for NaCl:";
     if (can_be_no_sandbox)
       LOG(ERROR) << kNoBpfMsg << kItIsDangerousMsg;
     else
       LOG(FATAL) << kNoBpfMsg << kItIsNotAllowedMsg;
   }
 }
 
 }  // namespace nacl