Clear environment variables for nacl_helper

components/nacl/zygote/nacl_fork_delegate_linux.cc

Index: components/nacl/zygote/nacl_fork_delegate_linux.cc
diff --git a/components/nacl/zygote/nacl_fork_delegate_linux.cc b/components/nacl/zygote/nacl_fork_delegate_linux.cc
index 1a42269a198cbc737035064a2a0d79b29573a7b9..442fc24fe7a9ec7c137a6412b2577458473c12b0 100644
--- a/components/nacl/zygote/nacl_fork_delegate_linux.cc
+++ b/components/nacl/zygote/nacl_fork_delegate_linux.cc
@@ -26,6 +26,7 @@
 #include "base/posix/unix_domain_socket_linux.h"
 #include "base/process/kill.h"
 #include "base/process/launch.h"
+#include "base/strings/string_split.h"
 #include "base/third_party/dynamic_annotations/dynamic_annotations.h"
 #include "build/build_config.h"
 #include "components/nacl/common/nacl_nonsfi_util.h"
@@ -44,6 +45,9 @@ const char kNaClHelperReservedAtZero[] =
     "--reserved_at_zero=0xXXXXXXXXXXXXXXXX";
 const char kNaClHelperRDebug[] = "--r_debug=0xXXXXXXXXXXXXXXXX";
 
+const char kNaClDangerousNaClHelperEnvPassthrough[] =

[Mark Seaborn, 2014/06/02 23:10:29]

Could you add a comment saying what this is for?

[elijahtaylor1, 2014/06/03 20:47:54]

Done.

+    "NACL_DANGEROUS_NACL_HELPER_ENV_PASSTHROUGH";

[Mark Seaborn, 2014/06/02 23:10:29]

Maybe just "NACL_ENV_PASSTHROUGH", otherwise it's a bit long?  It's not very
dangerous so the "DANGEROUS" prefix seems unnecessarily scary. :-)

[elijahtaylor1, 2014/06/03 20:47:54]

I added "DANGEROUS" because I was anticipating a request from *you* :)

Done.

+
 #if defined(ARCH_CPU_X86)
 bool NonZeroSegmentBaseIsSlow() {
   base::CPU cpuid;
@@ -243,6 +247,9 @@ void NaClForkDelegate::Init(const int sandboxdesc,
     max_these_limits.push_back(RLIMIT_AS);
     options.maximize_rlimits = &max_these_limits;
 
+    options.clear_environ = true;

[Mark Seaborn, 2014/06/02 23:10:29]

Maybe add a comment like: To avoid information leaks in Non-SFI mode, ensure
that environment variables never appear in the address space of the child
process.

[elijahtaylor1, 2014/06/03 20:47:54]

Done, but modified this comment slightly.  It's not technically true that they
*never* appear in the address space of NaCl Helper, since fork() inherits the
environment.  We just immediately overwrite it.

+    AddPassthroughEnvToOptions(options);
+
     if (!base::LaunchProcess(argv_to_launch, options, NULL))
       status_ = kNaClHelperLaunchFailed;
     // parent and error cases are handled below
@@ -398,4 +405,21 @@ bool NaClForkDelegate::GetTerminationStatus(pid_t pid, bool known_dead,
   return true;
 }
 
+// static
+void NaClForkDelegate::AddPassthroughEnvToOptions(
+    base::LaunchOptions& options) {
+  scoped_ptr<base::Environment> env(base::Environment::Create());
+  std::string pass_through_string;
+  if (env->GetVar(kNaClDangerousNaClHelperEnvPassthrough,
+                  &pass_through_string)) {
+    std::vector<std::string> pass_through_vars;
+    base::SplitStringAlongWhitespace(pass_through_string, &pass_through_vars);

[Mark Seaborn, 2014/06/02 23:10:29]

Nit: Could you split on commas instead?  Spaces are unusual for env vars and
it's sometimes tricky to get shells (and makefiles) to preserve them.

[elijahtaylor1, 2014/06/03 20:47:54]

Done.

+    std::string temp;

[Mark Seaborn, 2014/06/02 23:10:29]

Nit: could go inside the following loop

[elijahtaylor1, 2014/06/03 20:47:54]

Done.

+    for (size_t i = 0; i < pass_through_vars.size(); ++i) {

[jln (very slow on Chromium), 2014/06/02 21:31:47]

Why not use a vector::const_iterator instead?

[elijahtaylor1, 2014/06/03 20:47:54]

verbosity/clarity.  I'll leave as is unless you feel strongly about this.

+      if (env->GetVar(pass_through_vars[i].c_str(), &temp))
+        options.environ[pass_through_vars[i]] = temp;
+    }
+  }
+}
+
 }  // namespace nacl