src: add NODE_SECURITY_REVERT environment variable

Some vendors do not allow passing custom command-line flags to the node
executable. There are concerns around allowing --security-revert in
NODE_OPTIONS because it might be inherited by child processes
unintentionally.

This patch introduces a new environment variable that, if set, is unset
immediately unless it ends with "+sticky". Aside from that optional
suffix, its value is a comma-separated list of CVE identifiers for which
the respective security patches should be reverted.

Closes: nodejs#52017

src/node.cc

@@ -931,7 +931,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)
@@ -960,6 +960,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());

src/node_dotenv.cc

@@ -263,11 +263,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);
   }
 }
 

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<v8::Object> ToObject(Environment* env);
 
+  void DeleteEntry(const char* name);
+
   static std::vector<std::string> GetPathFromArgs(
       const std::vector<std::string>& args);
 

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 *main* 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")
+  XX(CVE_2009_TEST, "CVE-2009-TEST", "Non-existent vulnerability for testing")
 
 enum reversion {
 #define V(code, ...) SECURITY_REVERT_##code,

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$`));
+}