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

components/nacl/loader/nacl_helper_linux.cc

Index: components/nacl/loader/nacl_helper_linux.cc
diff --git a/components/nacl/loader/nacl_helper_linux.cc b/components/nacl/loader/nacl_helper_linux.cc
index 26578c02de1a9ee59081bff9b858e305c092458b..3aa00d8161eeac18f5ec36f2db6ac32fad7dfc6d 100644
--- a/components/nacl/loader/nacl_helper_linux.cc
+++ b/components/nacl/loader/nacl_helper_linux.cc
@@ -43,6 +43,40 @@ struct NaClLoaderSystemInfo {
   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) {
@@ -53,11 +87,7 @@ void BecomeNaClLoader(const std::vector<int>& child_fds,
   // 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]);
@@ -186,16 +216,6 @@ bool HandleGetTerminationStatusRequest(PickleIterator* input_iter,
   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|.