From a8734af4422e6f8a4e3640971883577795796d6e Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Sat, 28 Jan 2017 13:27:02 +0100 Subject: [PATCH] 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: https://github.com/nodejs/node/pull/11051 Reviewed-By: Anna Henningsen Reviewed-By: James M Snell Reviewed-By: Sam Roberts --- src/node.cc | 56 +++++++++++++++++++++++++++----------------- src/node_config.cc | 7 +++--- src/node_i18n.cc | 12 +++++----- src/node_i18n.h | 3 ++- src/node_internals.h | 4 +++- 5 files changed, 49 insertions(+), 33 deletions(-) diff --git a/src/node.cc b/src/node.cc index a678c177f3..8c5e811d6e 100644 --- a/src/node.cc +++ b/src/node.cc @@ -156,7 +156,7 @@ static const char* trace_enabled_categories = nullptr; #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 @@ -189,7 +189,7 @@ bool trace_warnings = false; bool config_preserve_symlinks = false; // Set in node.cc by ParseArgs when --redirect-warnings= is used. -const char* config_warning_file; +std::string config_warning_file; // NOLINT(runtime/string) bool v8_initialized = false; @@ -924,12 +924,21 @@ Local 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; } @@ -3089,11 +3098,11 @@ void SetupProcessObject(Environment* env, #if defined(NODE_HAVE_I18N_SUPPORT) && defined(U_ICU_VERSION) // ICU-related versions are now handled on the js side, see bootstrap_node.js - 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 @@ -3741,7 +3750,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) { @@ -4228,13 +4237,14 @@ void Init(int* argc, #endif // Allow for environment set preserving symlinks. - if (auto preserve_symlinks = secure_getenv("NODE_PRESERVE_SYMLINKS")) { - config_preserve_symlinks = (*preserve_symlinks == '1'); + { + std::string text; + config_preserve_symlinks = + SafeGetenv("NODE_PRESERVE_SYMLINKS", &text) && text[0] == '1'; } - if (auto redirect_warnings = secure_getenv("NODE_REDIRECT_WARNINGS")) { - config_warning_file = redirect_warnings; - } + if (config_warning_file.empty()) + SafeGetenv("NODE_REDIRECT_WARNINGS", &config_warning_file); // Parse a few arguments which are specific to Node. int v8_argc; @@ -4262,12 +4272,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)"); @@ -4532,8 +4541,11 @@ int Start(int argc, char** argv) { Init(&argc, const_cast(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. @@ -4542,7 +4554,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); // Enable tracing when argv has --trace-events-enabled. diff --git a/src/node_config.cc b/src/node_config.cc index 60001207f1..a096372812 100644 --- a/src/node_config.cc +++ b/src/node_config.cc @@ -46,11 +46,12 @@ void InitConfig(Local target, if (config_preserve_symlinks) READONLY_BOOLEAN_PROPERTY("preserveSymlinks"); - if (config_warning_file != nullptr) { + if (!config_warning_file.empty()) { Local name = OneByteString(env->isolate(), "warningFile"); Local value = String::NewFromUtf8(env->isolate(), - config_warning_file, - v8::NewStringType::kNormal) + config_warning_file.data(), + v8::NewStringType::kNormal, + config_warning_file.size()) .ToLocalChecked(); target->DefineOwnProperty(env->context(), name, value).FromJust(); } diff --git a/src/node_i18n.cc b/src/node_i18n.cc index a98fdca4d1..40d048fa36 100644 --- a/src/node_i18n.cc +++ b/src/node_i18n.cc @@ -404,12 +404,8 @@ static void GetVersion(const FunctionCallbackInfo& args) { } } -bool InitializeICUDirectory(const char* icu_data_path) { - if (icu_data_path != nullptr) { - flag_icu_data_dir = true; - u_setDataDirectory(icu_data_path); - return true; // no error - } else { +bool InitializeICUDirectory(const std::string& path) { + if (path.empty()) { UErrorCode status = U_ZERO_ERROR; #ifdef NODE_HAVE_SMALL_ICU // install the 'small' data. @@ -418,6 +414,10 @@ bool InitializeICUDirectory(const char* icu_data_path) { // no small data, so nothing to do. #endif // !NODE_HAVE_SMALL_ICU return (status == U_ZERO_ERROR); + } else { + flag_icu_data_dir = true; + u_setDataDirectory(path.c_str()); + return true; // No error. } } diff --git a/src/node_i18n.h b/src/node_i18n.h index 21a579526d..0bfd3c5c85 100644 --- 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 #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); int32_t ToASCII(MaybeStackBuffer* buf, const char* input, diff --git a/src/node_internals.h b/src/node_internals.h index 7c1b79d62f..9d57becc26 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -13,6 +13,8 @@ #include #include +#include + struct sockaddr; // Variation on NODE_DEFINE_CONSTANT that sets a String value. @@ -45,7 +47,7 @@ extern bool config_preserve_symlinks; // Set in node.cc by ParseArgs when --redirect-warnings= is used. // Used to redirect warning output to a file rather than sending // it to stderr. -extern const char* config_warning_file; +extern std::string config_warning_file; // NOLINT(runtime/string) // Tells whether it is safe to call v8::Isolate::GetCurrent(). extern bool v8_initialized;