src: make copies of startup environment variables

Mutations of the environment can invalidate pointers to environment variables, so make `secure_getenv()` copy them out instead of returning pointers. PR-URL: nodejs#11051 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sam Roberts <[email protected]>
sam-github · Apr 17, 2017 · 6765c88 · 6765c88
1 parent c60fd7f
commit 6765c88
Show file tree

Hide file tree

Showing 4 changed files with 34 additions and 20 deletions.
diff --git a/src/node.cc b/src/node.cc
@@ -165,7 +165,7 @@ static node_module* modlist_addon;
 
 #if defined(NODE_HAVE_I18N_SUPPORT)
 // Path to ICU data (for i18n / Intl)
-static const char* icu_data_dir = nullptr;
+static std::string icu_data_dir;  // NOLINT(runtime/string)
 #endif
 
 // used by C++ modules as well
@@ -943,12 +943,21 @@ Local<Value> UVException(Isolate* isolate,
 
 
 // Look up environment variable unless running as setuid root.
-inline const char* secure_getenv(const char* key) {
+inline bool SafeGetenv(const char* key, std::string* text) {
 #ifndef _WIN32
-  if (getuid() != geteuid() || getgid() != getegid())
-    return nullptr;
+  // TODO(bnoordhuis) Should perhaps also check whether getauxval(AT_SECURE)
+  // is non-zero on Linux.
+  if (getuid() != geteuid() || getgid() != getegid()) {
+    text->clear();
+    return false;
+  }
 #endif
-  return getenv(key);
+  if (const char* value = getenv(key)) {
+    *text = value;
+    return true;
+  }
+  text->clear();
+  return false;
 }
 
 
@@ -3131,11 +3140,11 @@ void SetupProcessObject(Environment* env,
                     "icu",
                     OneByteString(env->isolate(), U_ICU_VERSION));
 
-  if (icu_data_dir != nullptr) {
+  if (!icu_data_dir.empty()) {
     // Did the user attempt (via env var or parameter) to set an ICU path?
     READONLY_PROPERTY(process,
                       "icu_data_dir",
-                      OneByteString(env->isolate(), icu_data_dir));
+                      OneByteString(env->isolate(), icu_data_dir.c_str()));
   }
 #endif
 
@@ -3850,7 +3859,7 @@ static void ParseArgs(int* argc,
 #endif /* HAVE_OPENSSL */
 #if defined(NODE_HAVE_I18N_SUPPORT)
     } else if (strncmp(arg, "--icu-data-dir=", 15) == 0) {
-      icu_data_dir = arg + 15;
+      icu_data_dir.assign(arg + 15);
 #endif
     } else if (strcmp(arg, "--expose-internals") == 0 ||
                strcmp(arg, "--expose_internals") == 0) {
@@ -4351,12 +4360,11 @@ void Init(int* argc,
 #endif
 
 #if defined(NODE_HAVE_I18N_SUPPORT)
-  if (icu_data_dir == nullptr) {
-    // if the parameter isn't given, use the env variable.
-    icu_data_dir = secure_getenv("NODE_ICU_DATA");
-  }
+  // If the parameter isn't given, use the env variable.
+  if (icu_data_dir.empty())
+    SafeGetenv("NODE_ICU_DATA", &icu_data_dir);
   // Initialize ICU.
-  // If icu_data_dir is nullptr here, it will load the 'minimal' data.
+  // If icu_data_dir is empty here, it will load the 'minimal' data.
   if (!i18n::InitializeICUDirectory(icu_data_dir)) {
     FatalError(nullptr, "Could not initialize ICU "
                      "(check NODE_ICU_DATA or --icu-data-dir parameters)");
@@ -4707,8 +4715,11 @@ int Start(int argc, char** argv) {
   Init(&argc, const_cast<const char**>(argv), &exec_argc, &exec_argv);
 
 #if HAVE_OPENSSL
-  if (const char* extra = secure_getenv("NODE_EXTRA_CA_CERTS"))
-    crypto::UseExtraCaCerts(extra);
+  {
+    std::string extra_ca_certs;
+    if (SafeGetenv("NODE_EXTRA_CA_CERTS", &extra_ca_certs))
+      crypto::UseExtraCaCerts(extra_ca_certs);
+  }
 #ifdef NODE_FIPS_MODE
   // In the case of FIPS builds we should make sure
   // the random source is properly initialized first.
@@ -4717,7 +4728,7 @@ int Start(int argc, char** argv) {
   // V8 on Windows doesn't have a good source of entropy. Seed it from
   // OpenSSL's pool.
   V8::SetEntropySource(crypto::EntropySource);
-#endif
+#endif  // HAVE_OPENSSL
 
   v8_platform.Initialize(v8_thread_pool_size);
   V8::Initialize();

diff --git a/src/node_i18n.cc b/src/node_i18n.cc
@@ -62,10 +62,10 @@ bool flag_icu_data_dir = false;
 
 namespace i18n {
 
-bool InitializeICUDirectory(const char* icu_data_path) {
-  if (icu_data_path != nullptr) {
+bool InitializeICUDirectory(const std::string& path) {
+  if (!path.empty()) {
     flag_icu_data_dir = true;
-    u_setDataDirectory(icu_data_path);
+    u_setDataDirectory(path.c_str());
     return true;  // no error
   } else {
     UErrorCode status = U_ZERO_ERROR;

diff --git a/src/node_i18n.h b/src/node_i18n.h
@@ -4,6 +4,7 @@
 #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
 
 #include "node.h"
+#include <string>
 
 #if defined(NODE_HAVE_I18N_SUPPORT)
 
@@ -13,7 +14,7 @@ extern bool flag_icu_data_dir;
 
 namespace i18n {
 
-bool InitializeICUDirectory(const char* icu_data_path);
+bool InitializeICUDirectory(const std::string& path);
 
 }  // namespace i18n
 }  // namespace node

diff --git a/src/node_internals.h b/src/node_internals.h
@@ -12,6 +12,8 @@
 #include <stdint.h>
 #include <stdlib.h>
 
+#include <string>
+
 struct sockaddr;
 
 // Variation on NODE_DEFINE_CONSTANT that sets a String value.