Skip to content

Commit f2e97f8

Browse files
bnoordhuisMylesBorins
authored andcommitted
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: #11051 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
1 parent c408a3b commit f2e97f8

File tree

4 files changed

+34
-20
lines changed

4 files changed

+34
-20
lines changed

src/node.cc

+27-16
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,7 @@ static node_module* modlist_addon;
167167

168168
#if defined(NODE_HAVE_I18N_SUPPORT)
169169
// Path to ICU data (for i18n / Intl)
170-
static const char* icu_data_dir = nullptr;
170+
static std::string icu_data_dir; // NOLINT(runtime/string)
171171
#endif
172172

173173
// used by C++ modules as well
@@ -945,12 +945,21 @@ Local<Value> UVException(Isolate* isolate,
945945

946946

947947
// Look up environment variable unless running as setuid root.
948-
inline const char* secure_getenv(const char* key) {
948+
inline bool SafeGetenv(const char* key, std::string* text) {
949949
#ifndef _WIN32
950-
if (getuid() != geteuid() || getgid() != getegid())
951-
return nullptr;
950+
// TODO(bnoordhuis) Should perhaps also check whether getauxval(AT_SECURE)
951+
// is non-zero on Linux.
952+
if (getuid() != geteuid() || getgid() != getegid()) {
953+
text->clear();
954+
return false;
955+
}
952956
#endif
953-
return getenv(key);
957+
if (const char* value = getenv(key)) {
958+
*text = value;
959+
return true;
960+
}
961+
text->clear();
962+
return false;
954963
}
955964

956965

@@ -3136,11 +3145,11 @@ void SetupProcessObject(Environment* env,
31363145
"icu",
31373146
OneByteString(env->isolate(), U_ICU_VERSION));
31383147

3139-
if (icu_data_dir != nullptr) {
3148+
if (!icu_data_dir.empty()) {
31403149
// Did the user attempt (via env var or parameter) to set an ICU path?
31413150
READONLY_PROPERTY(process,
31423151
"icu_data_dir",
3143-
OneByteString(env->isolate(), icu_data_dir));
3152+
OneByteString(env->isolate(), icu_data_dir.c_str()));
31443153
}
31453154
#endif
31463155

@@ -3855,7 +3864,7 @@ static void ParseArgs(int* argc,
38553864
#endif /* HAVE_OPENSSL */
38563865
#if defined(NODE_HAVE_I18N_SUPPORT)
38573866
} else if (strncmp(arg, "--icu-data-dir=", 15) == 0) {
3858-
icu_data_dir = arg + 15;
3867+
icu_data_dir.assign(arg + 15);
38593868
#endif
38603869
} else if (strcmp(arg, "--expose-internals") == 0 ||
38613870
strcmp(arg, "--expose_internals") == 0) {
@@ -4372,12 +4381,11 @@ void Init(int* argc,
43724381
#endif
43734382

43744383
#if defined(NODE_HAVE_I18N_SUPPORT)
4375-
if (icu_data_dir == nullptr) {
4376-
// if the parameter isn't given, use the env variable.
4377-
icu_data_dir = secure_getenv("NODE_ICU_DATA");
4378-
}
4384+
// If the parameter isn't given, use the env variable.
4385+
if (icu_data_dir.empty())
4386+
SafeGetenv("NODE_ICU_DATA", &icu_data_dir);
43794387
// Initialize ICU.
4380-
// If icu_data_dir is nullptr here, it will load the 'minimal' data.
4388+
// If icu_data_dir is empty here, it will load the 'minimal' data.
43814389
if (!i18n::InitializeICUDirectory(icu_data_dir)) {
43824390
FatalError(nullptr, "Could not initialize ICU "
43834391
"(check NODE_ICU_DATA or --icu-data-dir parameters)");
@@ -4718,8 +4726,11 @@ int Start(int argc, char** argv) {
47184726
Init(&argc, const_cast<const char**>(argv), &exec_argc, &exec_argv);
47194727

47204728
#if HAVE_OPENSSL
4721-
if (const char* extra = secure_getenv("NODE_EXTRA_CA_CERTS"))
4722-
crypto::UseExtraCaCerts(extra);
4729+
{
4730+
std::string extra_ca_certs;
4731+
if (SafeGetenv("NODE_EXTRA_CA_CERTS", &extra_ca_certs))
4732+
crypto::UseExtraCaCerts(extra_ca_certs);
4733+
}
47234734
#ifdef NODE_FIPS_MODE
47244735
// In the case of FIPS builds we should make sure
47254736
// the random source is properly initialized first.
@@ -4728,7 +4739,7 @@ int Start(int argc, char** argv) {
47284739
// V8 on Windows doesn't have a good source of entropy. Seed it from
47294740
// OpenSSL's pool.
47304741
V8::SetEntropySource(crypto::EntropySource);
4731-
#endif
4742+
#endif // HAVE_OPENSSL
47324743

47334744
v8_platform.Initialize(v8_thread_pool_size);
47344745
V8::Initialize();

src/node_i18n.cc

+3-3
Original file line numberDiff line numberDiff line change
@@ -62,10 +62,10 @@ bool flag_icu_data_dir = false;
6262

6363
namespace i18n {
6464

65-
bool InitializeICUDirectory(const char* icu_data_path) {
66-
if (icu_data_path != nullptr) {
65+
bool InitializeICUDirectory(const std::string& path) {
66+
if (!path.empty()) {
6767
flag_icu_data_dir = true;
68-
u_setDataDirectory(icu_data_path);
68+
u_setDataDirectory(path.c_str());
6969
return true; // no error
7070
} else {
7171
UErrorCode status = U_ZERO_ERROR;

src/node_i18n.h

+2-1
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
55

66
#include "node.h"
7+
#include <string>
78

89
#if defined(NODE_HAVE_I18N_SUPPORT)
910

@@ -13,7 +14,7 @@ extern bool flag_icu_data_dir;
1314

1415
namespace i18n {
1516

16-
bool InitializeICUDirectory(const char* icu_data_path);
17+
bool InitializeICUDirectory(const std::string& path);
1718

1819
} // namespace i18n
1920
} // namespace node

src/node_internals.h

+2
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,8 @@
1212
#include <stdint.h>
1313
#include <stdlib.h>
1414

15+
#include <string>
16+
1517
struct sockaddr;
1618

1719
// Variation on NODE_DEFINE_CONSTANT that sets a String value.

0 commit comments

Comments
 (0)