Ensure seccomp-bpf cannot be silently disabled for non-SFI NaCl

components/nacl/loader/nacl_helper_linux.cc

 // Copyright 2013 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.
 
 // A mini-zygote specifically for Native Client.
 
 #include "components/nacl/loader/nacl_helper_linux.h"
 
 #include <errno.h>
 #include <fcntl.h>
@@ skipping 25 matching lines @@
 #include "ipc/ipc_switches.h"
 #include "sandbox/linux/services/libc_urandom_override.h"
 
 namespace {
 
 struct NaClLoaderSystemInfo {
   size_t prereserved_sandbox_size;
   long number_of_cores;
 };
 
+// This is a poor man's check on whether we are sandboxed.
+bool IsSandboxed() {
+  int proc_fd = open("/proc/self/exe", O_RDONLY);
+  if (proc_fd >= 0) {
+    close(proc_fd);
+    return false;
+  }
+  return true;
+}
+
+void InitializeSandbox(bool uses_nonsfi_mode) {
+  if (uses_nonsfi_mode) {
+    if (getenv("NACL_DANGEROUS_DISABLE_NONSFI_SANDBOX")) {

[hamaji, 2014/04/04 14:36:58]

We need to whitelist this (and other NACL-prefixed variables such as
NACLVERBOSITY?) when we fix http://crbug.com/358413.

This is extremely useful when we debug our app.

[Mark Seaborn, 2014/04/04 15:45:37]

Is there anything specific you want it for?

I'll let Julien comment on whether this is OK. :-)

Having a command line option would be preferred over an env var.  (The reason
native_client has tended to use env vars is the difficulty of plumbing command
line options over from the Chromium side.)

I'd prefer leaving this out for now, but we can still discuss the use cases for
it.

[hamaji, 2014/04/04 18:57:48]

Yes. Our app has a hack to attach GDB appropriately. The hack is like:

1. If /tmp/bare-metal-gdb.lock exists, our app waits until this file is removed
at the beginning.
2. Our launch script attaches GDB to nacl_helper and set up Python extension.
3. The python extension tells GDB the load address of the loader by Python
extension. It also sets a breakpoint to the loader so newly loaded objects will
be also handled by the Python extension.
4. Our launch script removes /tmp/bare-metal-gdb.lock. The program starts.

This is an ugly hack, but I don't think we will think about how non-SFI NaCl
supports GDB in M36. So, to keep this hack in our side works, this is mandatory
for us.

I think we also want to use this for:
https://code.google.com/p/chromium/issues/detail?id=341378 Though this is not a
high priority right now, we want to abandon our trusted plugin mode.

Of course, changing this to a flag is OK for us. Let me see.
I hope he is fine with this :) I think we can easily make SFI NaCl unsafe if you
can add env variables, so I'm guessing this is acceptable.

+      LOG(ERROR) << "DANGEROUS: Running non-SFI NaCl without sandbox!";
+      return;
+    }
+    const bool setuid_sandbox_enabled = IsSandboxed();
+    CHECK(setuid_sandbox_enabled)
+        << "SUID sandbox is mandatory for non-SFI NaCl";
+    // TODO(hamaji): Add a strict seccomp sandbox for non-SFI NaCl.

[Mark Seaborn, 2014/04/04 15:45:37]

Nit: I suspect you wouldn't be changing this part of the code when addressing
the TODO, so a TODO doesn't belong here, because it would be easy to forget to
remove it.  There's an issue tracking this, which is enough.

[hamaji, 2014/04/04 18:57:48]

I was planning to change this to nacl::nonsfi::InitializeSandbox() and stop
checking the return value. I guess InitializeSandbox() should just crash when it
fails, instead of returning false.

But yeah, I agree this TODO does not make much sense. I'll remove this.

+    // https://code.google.com/p/chromium/issues/detail?id=359285
+    bool bpf_sandbox_initialized = InitializeBPFSandbox();

[Mark Seaborn, 2014/04/04 15:45:37]

When I talked with Julien yesterday, he pointed out that
"--disable-seccomp-filter-sandbox" would stop this enabling the sandbox, which
we don't want to allow to happen.  That option is currently safe(ish), but
wouldn't be safe for Non-SFI NaCl.

[hamaji, 2014/04/04 18:57:48]

It seems the sandbox is enabled even with --disable-seccomp-filter-sandbox. I
didn't check this well, but nacl_helper does not have full args when we set up
seccomp sandbox in nacl_helper_linux.cc?

+    CHECK(bpf_sandbox_initialized)
+        << "Could not initialize NaCl's second "
+        << "layer sandbox (seccomp-bpf) for non-SFI mode.";
+  } else {
+    bool bpf_sandbox_initialized = InitializeBPFSandbox();
+    if (!bpf_sandbox_initialized) {
+      LOG(ERROR) << "Could not initialize NaCl's second "
+                 << "layer sandbox (seccomp-bpf) for SFI mode.";
+    }
+  }
+}
+
 // The child must mimic the behavior of zygote_main_linux.cc on the child
 // side of the fork. See zygote_main_linux.cc:HandleForkRequest from
 //   if (!child) {
 void BecomeNaClLoader(const std::vector<int>& child_fds,
                       const NaClLoaderSystemInfo& system_info,
                       bool uses_nonsfi_mode) {
   VLOG(1) << "NaCl loader: setting up IPC descriptor";
   // don't need zygote FD any more
   if (IGNORE_EINTR(close(kNaClZygoteDescriptor)) != 0)
     LOG(ERROR) << "close(kNaClZygoteDescriptor) failed.";
-  bool sandbox_initialized = InitializeBPFSandbox();
-  if (!sandbox_initialized) {
-    LOG(ERROR) << "Could not initialize NaCl's second "
-      << "layer sandbox (seccomp-bpf).";
-  }
+  InitializeSandbox(uses_nonsfi_mode);
   base::GlobalDescriptors::GetInstance()->Set(
       kPrimaryIPCChannel,
       child_fds[content::ZygoteForkDelegate::kBrowserFDIndex]);
 
   base::MessageLoopForIO main_message_loop;
   NaClListener listener;
   listener.set_uses_nonsfi_mode(uses_nonsfi_mode);
   listener.set_prereserved_sandbox_size(system_info.prereserved_sandbox_size);
   listener.set_number_of_cores(system_info.number_of_cores);
   listener.Listen();
@@ skipping 108 matching lines @@
   base::TerminationStatus status;
   if (known_dead)
     status = base::GetKnownDeadTerminationStatus(child_to_wait, &exit_code);
   else
     status = base::GetTerminationStatus(child_to_wait, &exit_code);
   output_pickle->WriteInt(static_cast<int>(status));
   output_pickle->WriteInt(exit_code);
   return true;
 }
 
-// This is a poor man's check on whether we are sandboxed.
-bool IsSandboxed() {
-  int proc_fd = open("/proc/self/exe", O_RDONLY);
-  if (proc_fd >= 0) {
-    close(proc_fd);
-    return false;
-  }
-  return true;
-}
-
 // Honor a command |command_type|. Eventual command parameters are
 // available in |input_iter| and eventual file descriptors attached to
 // the command are in |attached_fds|.
 // Reply to the command on |reply_fds|.
 bool HonorRequestAndReply(int reply_fd,
                           int command_type,
                           const std::vector<int>& attached_fds,
                           const NaClLoaderSystemInfo& system_info,
                           PickleIterator* input_iter) {
   Pickle write_pickle;
@@ skipping 186 matching lines @@
   // Now handle requests from the Zygote.
   while (true) {
     bool request_handled = HandleZygoteRequest(kNaClZygoteDescriptor,
                                                system_info);
     // Do not turn this into a CHECK() without thinking about robustness
     // against malicious IPC requests.
     DCHECK(request_handled);
   }
   NOTREACHED();
 }