Clear environment variables for nacl_helper

components/nacl/zygote/nacl_fork_delegate_linux.cc

 // Copyright (c) 2012 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/zygote/nacl_fork_delegate_linux.h"
 
 #include <signal.h>
 #include <stdlib.h>
 #include <sys/resource.h>
 #include <sys/socket.h>
 
 #include <set>
 
 #include "base/basictypes.h"
 #include "base/command_line.h"
 #include "base/cpu.h"
 #include "base/files/file_path.h"
 #include "base/files/scoped_file.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/scoped_vector.h"
 #include "base/path_service.h"
 #include "base/pickle.h"
 #include "base/posix/eintr_wrapper.h"
 #include "base/posix/global_descriptors.h"
 #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"
 #include "components/nacl/common/nacl_paths.h"
 #include "components/nacl/common/nacl_switches.h"
 #include "components/nacl/loader/nacl_helper_linux.h"
 #include "content/public/common/content_descriptors.h"
 #include "content/public/common/content_switches.h"
 #include "sandbox/linux/suid/client/setuid_sandbox_client.h"
 
 namespace {
 
 // Note these need to match up with their counterparts in nacl_helper_linux.c
 // and nacl_helper_bootstrap_linux.c.
 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;
   // Using a non-zero segment base is known to be very slow on Intel
   // Atom CPUs.  See "Segmentation-based Memory Protection Mechanism
   // on Intel Atom Microarchitecture: Coding Optimizations" (Leonardo
   // Potenza, Intel).
   //
   // The following list of CPU model numbers is taken from:
   // "Intel 64 and IA-32 Architectures Software Developer's Manual"
@@ skipping 179 matching lines @@
     // The NaCl processes spawned may need to exceed the ambient soft limit
     // on RLIMIT_AS to allocate the untrusted address space and its guard
     // regions.  The nacl_helper itself cannot just raise its own limit,
     // because the existing limit may prevent the initial exec of
     // nacl_helper_bootstrap from succeeding, with its large address space
     // reservation.
     std::vector<int> max_these_limits;
     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
 
     if (enable_layer1_sandbox) {
       // Sanity check that dummy_fd was kept alive for LaunchProcess.
       DCHECK(dummy_fd.is_valid());
     }
   }
   if (IGNORE_EINTR(close(fds[1])) != 0)
@@ skipping 135 matching lines @@
   if (!iter.ReadInt(&remote_exit_code)) {
     LOG(ERROR) << "GetTerminationStatus: pickle failed";
     return false;
   }
 
   *status = static_cast<base::TerminationStatus>(termination_status);
   *exit_code = remote_exit_code;
   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