src: add NODE_SECURITY_REVERT environment variable

[tniessen]

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.

This is not a particularly elegant approach, but since this should only be used under exceptional circumstances, I am not too worried about that.

Closes: #52017

---

[nodejs-github-bot]

Review requested:

• @nodejs/startup
• @nodejs/security-wg
• @nodejs/security

[RafaelGSS]

Some context in #52090 (comment).

[GeoffreyBooth]

cc @anonrig

If this can’t be defined in a .env file that’s passed to --env-file, I think the documentation for --env-file needs to mention that (or you allow it to be defined in a file). I think unless there’s a security reason to disallow specifying this in a .env file, we should allow it there.

[mhdawson]

I know we don't have any testing at this point, but I think definining a "dummy" security revert that can be used for tests could make sense.

[anonrig]

Nit:

Suggested change

	for (const std::string_view& cve : SplitString(security_revert, ",")) {
	for (const auto& cve : SplitString(security_revert, ",")) {

[tniessen]

@anonrig Thank you for reviewing the implementation, I appreciate it. I admittedly threw this together during the collab summit rather quickly to discuss the idea, and if folks think this is the right approach, I'll be happy to fix up the implementation and mark it as ready for review :)

@GeoffreyBooth I am leaning towards allowing it in dotenv files, but I wasn't quite sure where to put this code in that case. I was hoping to keep it close to the code that currently implements --security-revert and ideally want to apply it before we initialize any V8 isolate. I admittedly also don't fully understand the flow of how dotenv files are loaded and didn't find much documentation or comments — it seems to me that NODE_OPTIONS is handled specially and the rest of the environment is populated later. Maybe @anonrig has a tip for keeping this close to the handling of --security-revert while still using dotenv properly :)

[tniessen]

@GeoffreyBooth I added support for setting the variable through dotenv files.

@mhdawson I added a test case.

[RafaelGSS]

LGTM

[tniessen]

Added more tests to cover precedence rules.

[mhdawson]

I wonder if we need to make them exclusive. I think it is already possible to revert a CVE twice from the command line so not sure we need to only allow one or the other?

[mhdawson]

LGTM

[tniessen]

The conflict should be resolved by #52543.

[RafaelGSS]

Can you rebase @tniessen?

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/58527/

[mhdawson]

Requesting changes until we resolve the coversation happening in #52090

[lwr]

It is big problem #52554 which is locked so we can only discuss here.

yes we know we can use shell: true instead of execFile, the question is, how can we process (escape) the input args instead? there is no any simple way to do this?

or else SHOULD WE have to translate this c routine into javascript?

node/deps/uv/src/win/process.c

Line 449 in a523c34

WCHAR* quote_cmd_arg(const WCHAR *source, WCHAR *target) {

or else following this article? https://learn.microsoft.com/en-us/archive/blogs/twistylittlepassagesallalike/everyone-quotes-command-line-arguments-the-wrong-way

[tniessen]

the question is, how can we process (escape) the input args instead?

FWIW, unlike any other operating system, Windows does not have a singular escaping mechanism for command-line arguments. Each executable potentially parses them differently. You can find a summary in golang/go#1849:

Microsoft's C startup code defines a standard way for C programs to split the command line into separate arguments and go's syscall.StartProcess does the correct escaping for those cases.

However, some programs - and most notably the cmd.exe shell - do their own command line parameter parsing.

In other words, there is no single correct escaping algorithm for executing arbitrary child processes on Windows. libuv internally follows the default behavior of C applications that have been compiled using MSVC (essentially reverse-engineering CommandLineToArgvW()), but this behavior is not correct for cmd.exe (and thus not for any .bat or .cmd file). There are also plenty of Windows applications that use GetCommandLineW() directly, and who knows what escaping rules they apply.

or else following this article? learn.microsoft.com/en-us/archive/blogs/twistylittlepassagesallalike/everyone-quotes-command-line-arguments-the-wrong-way

That looks mostly correct, except it will also fail when some executable does not follow the conventions of CommandLineToArgvW(). The only correct solution is to know the escaping rules of every executable that you are going to execute, including the shell itself, and to then apply these escaping rules to arguments before invoking the shell with the escaped arguments.

Unfortunately, this is a fundamental design mistake in Windows, and there is not much that Node.js can do about it.

[aleen42]

According to https://nodejs.org/en/blog/vulnerability/april-2024-security-releases-2

Due to the improper handling of batch files in child_process.spawn / child_process.spawnSync, a malicious command line argument can inject arbitrary commands and achieve code execution even if the shell option is not enabled. ...

Why not solve the problem of the "improper handling"? Is it so complicated that a broken behaviour is the only solution?