diff --git a/src/node.cc b/src/node.cc index 07a91f485502b8..d73b8fee6eb104 100644 --- a/src/node.cc +++ b/src/node.cc @@ -927,7 +927,7 @@ static ExitCode InitializeNodeWithArgsInternal( } } - per_process::dotenv_file.AssignNodeOptionsIfAvailable(&node_options); + per_process::dotenv_file.GetIfAvailable("NODE_OPTIONS", &node_options); } #if !defined(NODE_WITHOUT_NODE_OPTIONS) @@ -956,6 +956,47 @@ static ExitCode InitializeNodeWithArgsInternal( if (exit_code != ExitCode::kNoFailure) return exit_code; } + // Apply security reverts based on an environment variable. + const char* const security_revert_var = "NODE_SECURITY_REVERT"; + std::string security_revert; + if (credentials::SafeGetenv(security_revert_var, &security_revert) || + per_process::dotenv_file.GetIfAvailable(security_revert_var, + &security_revert)) { + // We unset the environment variable by default to prevent it from being + // inherited by child processes. This can be prevented by the user by + // appending "+sticky" to the value of the environment variable. + const char* const sticky_str = "+sticky"; + size_t maybe_sticky_pos = security_revert.length() - strlen(sticky_str); + const bool sticky = security_revert.length() >= strlen(sticky_str) && + security_revert.rfind(sticky_str) == maybe_sticky_pos; + if (sticky) { + security_revert.erase(maybe_sticky_pos); + } + + { + // Ignore the environment variable if the CLI argument was set. + Mutex::ScopedLock lock(per_process::cli_options_mutex); + if (per_process::reverted_cve == 0) { + std::string revert_error; + for (const std::string_view& cve : SplitString(security_revert, ",")) { + Revert(std::string(cve).c_str(), &revert_error); + if (!revert_error.empty()) { + errors->emplace_back(std::move(revert_error)); + // TODO(joyeecheung): merge into kInvalidCommandLineArgument. + return ExitCode::kInvalidCommandLineArgument2; + } + } + } + } + + // Unset the environment variable unless it has been marked as sticky. + if (!sticky) { + Mutex::ScopedLock lock(per_process::env_var_mutex); + per_process::dotenv_file.DeleteEntry(security_revert_var); + uv_os_unsetenv(security_revert_var); + } + } + // Set the process.title immediately after processing argv if --title is set. if (!per_process::cli_options->title.empty()) uv_set_process_title(per_process::cli_options->title.c_str()); diff --git a/src/node_dotenv.cc b/src/node_dotenv.cc index 8d92e917082488..079b00d53d1633 100644 --- a/src/node_dotenv.cc +++ b/src/node_dotenv.cc @@ -171,11 +171,19 @@ Dotenv::ParseResult Dotenv::ParsePath(const std::string_view path) { return ParseResult::Valid; } -void Dotenv::AssignNodeOptionsIfAvailable(std::string* node_options) { - auto match = store_.find("NODE_OPTIONS"); +bool Dotenv::GetIfAvailable(const char* name, std::string* out) { + auto match = store_.find(name); + if (match == store_.end()) { + return false; + } + *out = match->second; + return true; +} +void Dotenv::DeleteEntry(const char* name) { + auto match = store_.find(name); if (match != store_.end()) { - *node_options = match->second; + store_.erase(match); } } diff --git a/src/node_dotenv.h b/src/node_dotenv.h index 5872627f0c25fe..29b95eb68a2d36 100644 --- a/src/node_dotenv.h +++ b/src/node_dotenv.h @@ -23,10 +23,12 @@ class Dotenv { void ParseContent(const std::string_view content); ParseResult ParsePath(const std::string_view path); - void AssignNodeOptionsIfAvailable(std::string* node_options); + bool GetIfAvailable(const char* name, std::string* out); void SetEnvironment(Environment* env); v8::Local ToObject(Environment* env); + void DeleteEntry(const char* name); + static std::vector GetPathFromArgs( const std::vector& args); diff --git a/src/node_revert.h b/src/node_revert.h index edf6ad772eecb0..5322b321602d7b 100644 --- a/src/node_revert.h +++ b/src/node_revert.h @@ -11,12 +11,12 @@ * a given LTS or Stable may be added to this list, and only with TSC * consensus. * - * For *master* this list should always be empty! + * For *main* this list should always be empty, aside from a single test entry! **/ namespace node { -#define SECURITY_REVERSIONS(XX) \ -// XX(CVE_2016_PEND, "CVE-2016-PEND", "Vulnerability Title") +#define SECURITY_REVERSIONS(XX) \ + XX(CVE_2009_TEST, "CVE-2009-TEST", "Non-existent vulnerability for testing") enum reversion { #define V(code, ...) SECURITY_REVERT_##code, diff --git a/test/parallel/test-env-security-revert.js b/test/parallel/test-env-security-revert.js new file mode 100644 index 00000000000000..8fc8c20e3537b5 --- /dev/null +++ b/test/parallel/test-env-security-revert.js @@ -0,0 +1,125 @@ +'use strict'; + +require('../common'); + +const assert = require('node:assert'); +const { spawnSync } = require('node:child_process'); +const { writeFileSync } = require('node:fs'); + +const tmpdir = require('../common/tmpdir'); +tmpdir.refresh(); + +const testCVE = 'CVE-2009-TEST'; +const testVulnerabilityTitle = 'Non-existent vulnerability for testing'; +const testCVEWarningMessage = `SECURITY WARNING: Reverting ${testCVE}: ${testVulnerabilityTitle}`; +const testProcKey = `REVERT_${testCVE.replace(/-/g, '_')}`; +const invalidCVE = 'CVE-2009-NOPE'; +const invalidCVERegExp = new RegExp(`^.+: Error: Attempt to revert an unknown CVE \\[${invalidCVE}\\]$`); + +const node = process.execPath; + +function run({ env, dotenv, args, otherEnv }) { + const dotenvPath = tmpdir.resolve('.env'); + if (dotenv != null) { + writeFileSync(dotenvPath, `NODE_SECURITY_REVERT=${dotenv}\n`); + } + + return spawnSync(node, [ + ...(dotenv != null ? ['--env-file', dotenvPath] : []), + ...(args ?? []), + '--print', `JSON.stringify([ + process.env.NODE_SECURITY_REVERT, + Object.entries(process).filter(([name]) => name.startsWith("REVERT_")), + child_process.execFileSync(${JSON.stringify(node)}, [ + '--print', 'process.env.NODE_SECURITY_REVERT' + ]).toString().trim().split('\\n'), + ])`, + ], { + env: { + ...process.env, + ...(otherEnv ?? {}), + NODE_SECURITY_REVERT: env, + }, + encoding: 'utf8' + }); +} + +function expectSuccess(params) { + const { status, stdout, stderr } = run(params); + assert.strictEqual(status, 0, `${status} ${stderr}`); + assert.strictEqual(stderr.length, 0, stderr); + const lines = stdout.trim().split(/\r?\n/g); + assert.notStrictEqual(lines.length, 0); + const output = lines.pop(); + return [lines, JSON.parse(output)]; +} + +function expectError(params) { + const { status, stdout, stderr } = run(params); + assert.notStrictEqual(status, 0); + return [stdout.trim(), stderr.trim()]; +} + +for (const method of ['env', 'dotenv']) { + // Default: no security revert. + assert.deepStrictEqual(expectSuccess({}), [[], [null, [], ['undefined']]]); + + // Revert a single CVE patch without child processes inheriting this behavior. + assert.deepStrictEqual(expectSuccess({ [method]: testCVE }), [ + [testCVEWarningMessage], + [null, [[testProcKey, true]], ['undefined']], + ]); + + // Revert a single CVE patch with child processes inheriting this behavior. + assert.deepStrictEqual(expectSuccess({ [method]: `${testCVE}+sticky` }), [ + [testCVEWarningMessage], + [`${testCVE}+sticky`, [[testProcKey, true]], [testCVEWarningMessage, `${testCVE}+sticky`]], + ]); + + // Try to revert a CVE patch that does not exist. + for (const suffix of ['', '+sticky']) { + const [stdout, stderr] = expectError({ [method]: `${invalidCVE}${suffix}` }); + assert.strictEqual(stdout, ''); + assert.match(stderr, invalidCVERegExp); + } + + // Try to revert two CVE patches, one of which does not exist. + for (const suffix of ['', '+sticky']) { + const [stdout, stderr] = expectError({ [method]: `${testCVE},${invalidCVE}${suffix}` }); + assert.strictEqual(stdout, testCVEWarningMessage); + assert.match(stderr, invalidCVERegExp); + } + + // The command-line argument should take precedence over the environment variable + // and is never inherited by child processes. + assert.deepStrictEqual(expectSuccess({ + [method]: invalidCVE, + args: ['--security-revert', testCVE], + }), [ + [testCVEWarningMessage], + [null, [[testProcKey, true]], ['undefined']], + ]); +} + +// The environment variable should take precedence over a dotenv file. +assert.deepStrictEqual(expectSuccess({ + env: testCVE, + dotenv: invalidCVE +}), [ + [testCVEWarningMessage], + [null, [[testProcKey, true]], ['undefined']], +]); + +// We don't want security reverts to be inherited implicitly, thus, neither +// --security-revert nor --env-file should be allowed in NODE_OPTIONS. +for (const NODE_OPTIONS of [ + `--env-file=${tmpdir.resolve('.env')}`, + `--security-revert=${testCVE}`, +]) { + const [stdout, stderr] = expectError({ + otherEnv: { NODE_OPTIONS } + }); + assert.strictEqual(stdout, ''); + const optPrefix = NODE_OPTIONS.substring(0, NODE_OPTIONS.indexOf('=') + 1); + assert.match(stderr, new RegExp(`^.+: ${optPrefix} is not allowed in NODE_OPTIONS$`)); +}