child_process: add an API to escape shell argument #35975

PoojaDurgad · 2020-11-05T04:35:22Z

child_process.escapeArgument() escapes shell
arguments so that those can be passed to spawned
scripts, which expect arguments as escaped strings

Fixes: #34840

Co-Authored-By: Ben Noordhuis info@bnoordhuis.n

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

mscdex · 2020-11-05T06:29:35Z

nit: Ben's email address seems to be truncated at the end

doc/api/child_process.md

lib/child_process.js

mscdex · 2020-11-05T06:43:39Z

This needs some tests.

child_process.escapeArgument() escapes shell arguments so that those can be passed to spawned scripts, which expect arguments as escaped strings Fixes: nodejs#34840 Co-Authored-By: Ben Noordhuis <info@bnoordhuis.nl>

lib/child_process.js

doc/api/child_process.md

lib/child_process.js

Trott · 2020-11-05T14:07:07Z

/ping @nodejs/child_process @nodejs/tsc @devsnek

See conversation in #34840 for arguments for and against including this in core.

devsnek

seems better suited to userland

Artoria2e5 · 2020-11-07T11:50:07Z

Implementation is incorrect in that POSIX sh and Windows have very different methods. I don't get how you expect to get away without an OS check.

FWIW, the current escape function is overcompilcated but correct for POSIX sh. The tests address the behavior of the escaper only, not how the escaped string is promised to behave.

mcollina

I'm LGTM for addition of this to Node.js.
I'm unsure about the implementation due to Windows constraints.

I'd recommend to a few our friends at @nodejs/security-wg to check for correctness (esp on Windows). Also @ChALkeR might help in validating the regexp.

gireeshpunathil · 2020-11-08T05:45:43Z

I too support this.

posix vs windows parity: fixable
userland vs core: for a user, this is a capability that comes coherent with spawning.

vdeturckheim

I am -1 on such changes. There are multiple shell implementations over multiple operating systems and it is very hard for me to be confident such method would be relevant in all of theses.
Should there be any situation where this methods allows a bypass, it would probably end up in a CVE being issued against node for "Improper Neutralization of Escape, Meta, or Control Sequences" in the best case or "Remote code execution" if we are not lucky.
But, tbh, I am not happy with this argument that is more based on my ignorance of different setups than actual examples and predictible situations.

ChALkeR · 2020-11-09T17:07:01Z

I tend to agree with @vdeturckheim here.

For a specific example, see e.g. feross/cross-zip#18 (and feross/cross-zip#16 for history).

Afaik, there wasn't a good way to escape the argument inside the powershell command there, except for specifying it as a separate argument (as that PR demonstrates). Note how that is happening inside the argument. The correct solution was to not concatenate the argument inside the command.

I'm afraid that a presence of such an API could be abused for escaping arguments like in case above (i.e. inside the powershell command, which would have been incorrect), and do more harm in that case.

I also don't see the benefit of this change yet — users should use the structured API that accepts an array of arguments instead of building a shell string by concatenation, and introducing this will have a negative effect of moving them to a less secure API where it would be easy to miss escaping (even assuming that it works correctly).

Could someone elaborate on the benefit of this change, please? Which usecases need this over the current API (like execFile)?

Upd: left a comment at #34840 (comment) — I don't think that the usecase described there needs this change.

ChALkeR

Making the concern explicit, see message above.

ChALkeR · 2020-11-09T17:43:30Z

In short: I think that addition of a method for manual escaping will make the issue worse, not better.
A better solution would be to discourage using .exec at all in favor of .execFile.

ChALkeR · 2020-11-09T17:55:06Z

I'll investigate how the the current API/documentation could be improved on this soonish. Likely not today, but this week.

Artoria2e5 · 2020-11-11T22:08:08Z

@ChALkeR

I'll investigate how the the current API/documentation could be improved on this soonish. Likely not today, but this week.

IMO the issue really is that node allows shell: true with an array of args to start with. If there's no shell-calling API then everything is completely unambiguous: what you pass into the array is what the program's main() function will see, save for the case where the programmer must hand-craft a verbatim cmdline for special Windows programs.

But when there is an API that calls programs by launching shell the issue immediately gets complicated. What does an array mean now? Is ['a b'] different from ['a', 'b'] anymore? Node chose to make the two things the same, making platform-dependent escaping a user's problem. To quote your response,

users should use the structured API that accepts an array of arguments instead of building a shell string by concatenation

Node itself is doing it by naive concatenation. There's no structure to start with! If we agree that this is messed up, then Node should at least provide tools for the user to un-mess it. Either in the form of this PR that gives a separate method, or in the form of #29576 that does it in the shared spawn code-path (shell invocation is platform dependent anyways, so why not offer to escape too?).

PS: I hereby authorize PoojaDurgad to use stuff (code, doc, tests) from my PR as they see fit. Windows is a messy beast to tame, and it's better to work together.

gireeshpunathil · 2020-11-12T05:55:42Z

I think there are 3 topics here:

strength / fidelity of method that craft the escape sequence (security)
having this in core versus in userland
this PR versus other PR(s)

On the first one, chances of exploit should not be quoted as a reason to not to have this, if the chances of exploit is coming from unknown possibilities.

If shells of certain platform(s) do not work properly, those can be clearly documented as caveats. We have done that in hundreds on APIs in the past. Not every API abstracts every platform seamlessly.

On the second one: this is coherent with spawn calls. It is not so great to go to third party to prepare your input and then use core API to make the call. I am not sure we want the core to reduce to mere platform abstractions.

On the third one: I recommend (though no compulsion) @Artoria2e5 and @PoojaDurgad to collaborate, bring-in the best from both, rather than running with two PRs.

Mifrill · 2023-11-21T22:38:34Z

test/parallel/test-child-process-escape-argument.js

+assert.strictEqual(escapeArgument('foo bar'), "'foo bar'");
+assert.strictEqual(escapeArgument('foo & bar'), "'foo & bar'");
+assert.strictEqual(escapeArgument('foo & bar$'), "'foo & bar$'");
+assert.strictEqual(escapeArgument('foo%bar'), 'foo%bar');


I suggest adding a couple of tests based on regexs:

Suggested change

assert.strictEqual(escapeArgument('foo%bar'), 'foo%bar');

assert.strictEqual(escapeArgument('foo%bar'), 'foo%bar');

assert.strictEqual(escapeArgument('FOO'), 'FOO', 'Uppercase alphanumeric string');

assert.strictEqual(escapeArgument('foo42'), 'foo42', 'Alphanumeric string with numbers');

assert.strictEqual(escapeArgument('foo_42'), 'foo_42', 'Alphanumeric string with underscore');

assert.strictEqual(escapeArgument('123'), '123', 'Numeric string');

assert.strictEqual(escapeArgument('@#$'), '\'\@\#\$\'' , 'Special characters string');

assert.strictEqual(escapeArgument('`\''), '\'\`\'', 'String with backtick and single quote');

nodejs-github-bot added the child_process Issues and PRs related to the child_process subsystem. label Nov 5, 2020