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

components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc

Index: components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc
diff --git a/components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc b/components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc
index 2c920212847ab8670c78c80237bd180f3bf599f8..a3c1c90e0605b8e37696e9f7a9e7281c5d8733dd 100644
--- a/components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc
+++ b/components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc
@@ -46,7 +46,8 @@ NaClSandbox::NaClSandbox()
       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());
@@ -72,19 +73,16 @@ bool NaClSandbox::HasOpenDirectory() {
 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;
   }
@@ -95,6 +93,24 @@ void NaClSandbox::InitializeLayerTwoSandbox(bool uses_nonsfi_mode) {
   // 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;