Skip to content

Commit

Permalink
src: make copies of startup environment variables
Browse files Browse the repository at this point in the history
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 <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
  • Loading branch information
bnoordhuis authored and sam-github committed Feb 8, 2017
1 parent 62f513a commit a8734af
Show file tree
Hide file tree
Showing 5 changed files with 49 additions and 33 deletions.
56 changes: 34 additions & 22 deletions src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -924,12 +924,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;
}


Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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)");
Expand Down Expand Up @@ -4532,8 +4541,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.
Expand All @@ -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.
Expand Down
7 changes: 4 additions & 3 deletions src/node_config.cc
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,12 @@ void InitConfig(Local<Object> target,
if (config_preserve_symlinks)
READONLY_BOOLEAN_PROPERTY("preserveSymlinks");

if (config_warning_file != nullptr) {
if (!config_warning_file.empty()) {
Local<String> name = OneByteString(env->isolate(), "warningFile");
Local<String> 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();
}
Expand Down
12 changes: 6 additions & 6 deletions src/node_i18n.cc
Original file line number Diff line number Diff line change
Expand Up @@ -404,12 +404,8 @@ static void GetVersion(const FunctionCallbackInfo<Value>& 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.
Expand All @@ -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.
}
}

Expand Down
3 changes: 2 additions & 1 deletion src/node_i18n.h
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS

#include "node.h"
#include <string>

#if defined(NODE_HAVE_I18N_SUPPORT)

Expand All @@ -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<char>* buf,
const char* input,
Expand Down
4 changes: 3 additions & 1 deletion src/node_internals.h
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
#include <stdint.h>
#include <stdlib.h>

#include <string>

struct sockaddr;

// Variation on NODE_DEFINE_CONSTANT that sets a String value.
Expand Down Expand Up @@ -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;
Expand Down

0 comments on commit a8734af

Please sign in to comment.