Node: Built-in method to escape shell arguments

[ThisIsMissEm]

Is your feature request related to a problem? Please describe.

Yes, I was using execa which is a light wrapper for childProcess.spawn to execute a script (call it ./scripts/configure) which took user input as an argument. One of my users supplied "Users & Permissions Management" as that input, which caused the script to hang as the resulting spawned process was:

./scripts/configure Users & Permissions Management

I realised as soon as the bug was reported that I should've escaped the string passed into my function that called execa, so then I looked for modules to correctly escape shell arguments, and they seem pretty complex. Which leads to the question: do I really want to depend on a third-party module to correctly escape shell arguments? Am I just trading one security risk for another?

Describe the solution you'd like

Have a method like childProcess.escapeArgument(arg: string): string which correctly escapes the given value such that it is just a string for all terminals (cross-platform).

Clarification: I am not arguing for childProcess.spawn to escape arguments into strings by default, as that'd be a breaking change, even though it would likely be for the best (if you wanna pass multiple arguments, use the array syntax, not a string). Instead, I'm just asking for a method built-in that's well tested to escape an argument into a string argument for a shell command.

Describe alternatives you've considered

Various NPM modules, writing it myself, etc. All just shift the security responsibility to arguably worse places. This seems like due to the security benefits it can give, it'd be a good candidate for being a built-in function, ideally backported to LTS's

[ThisIsMissEm]

There are also some really bad suggestions on StackOverflow about how to do this: https://stackoverflow.com/questions/1779858/how-do-i-escape-a-string-for-a-shell-command-in-node

(my favourite there is "covert it to base64, then pipe base64 -d then pipe to your command")

[ThisIsMissEm]

More background context: https://twitter.com/thisismissem/status/1296071082504531970?s=21

[devsnek]

I think, overall, you just shouldn't be trying to pass user input into shell commands. if you use child_process.spawn directly you can pass arguments as an array without worrying about escaping but you're still passing arbitrary input into exec.

[ThisIsMissEm]

the problem is even with child_process.spawn you'll still end up with incorrect command execution.

const value = "Users & Permissions Management";
child_process.spawn('./scripts/configure', [value])

Will still result in ./scripts/configure Users & Permissions Management, and you do sometimes need to be able to accept user input and safely pass it to another command.

[devsnek]

@ThisIsMissEm that would only be a problem if ./scripts/configure is a shell script. every shell has different rules for how arguments are parsed, so i'm not sure there's a productive path forward to us solving that in node.

[ThisIsMissEm]

Existing Modules:

• shell-escape
• shell-quote
• sqh

Each of these produces different output. In ruby, the shellwords.escape method, which is commonly recommended also produces different output to all of those, iirc.

[ThisIsMissEm]

In my specific case, it's ('yarn', ['configure', '--set-something', value]) and the script under yarn configure actually also executes a child process, so it's at least two child process executions away from the arguments parser

But if I could easily escape value such that is was a string and always a single string, then the value would pass through fine.

[ThisIsMissEm]

@devsnek this is purely about providing a way to escape a string for shell usage — I would imagine majority are similar, maybe some differences between windows and unix/posix compliant shells, but that'd be it, I'd imagine.

[devsnek]

userland is the appropriate place for this imo.

[bnoordhuis]

Just so we're on the same page, the algorithm for escaping one shell argument is this:

function quote(s) {
  if (s === '') return `''`;
  if (!/[^%+,-.\/:=@_0-9A-Za-z]/.test(s)) return s;
  return `'` + s.replace(/'/g, `'"'`) + `'`;
}

The two if guards are not strictly necessary but they make for nicer output. This one-liner is also valid:

const quote = (s) => `'` + s.replace(/'/g, `'"'`) + `'`;

I'm a bit on the fence as to whether something that simple belongs in core but I wouldn't strenuously object.

[silverwind]

Another alternative is to replace with escaped single quote. Not sure if it'd make any practical difference.

s.replace(/'/gm, `'\\''`)

[bnoordhuis]

https://www.gnu.org/savannah-checkouts/gnu/bash/manual/bash.html#Single-Quotes

Enclosing characters in single quotes (‘'’) preserves the literal value of each character within the quotes. A single quote may not occur between single quotes, even when preceded by a backslash.

[silverwind]

Of course the output would be wrapped in single quotes so it's not actually putting single quotes inside single quotes. I consider it more secure because it keep variables inside single quotes inside such while the "'" method enables variable expansion (which may expose secret environment variables to the user):

$ bash -c 'echo "'"$EDITOR"'"'
nvim
$ bash -c 'echo '\''$EDITOR'\'''
$EDITOR

I guess this difference alone means there can not be one single escaping method because variable expansion may or may not be desired depending on use case.

[bnoordhuis]

while the "'" method enables variable expansion

Right, but note that the sample I posted uses '"' - quote, double qote, quote.

[silverwind]

Indeed, my mistake. The end result is still the same with variable expansion being possible.

```console
$ bash -c 'echo '"'$EDITOR'"''
nvim

[bnoordhuis]

Still not apples to apples. :-)

The argument to bash -c in your example is evaluated by the outer shell. That gotcha doesn't apply to child_process.spawn() or child_process.exec().

Just so we're on the same page, the thing quote() protects against is unsanitized input like this:

let input = `1337'; cat '/etc/passwd`
let banner = child_process.execSync(`/usr/bin/banner '${input}'`)
socket.write(banner)

(Excerpted from my banner-as-a-service SaaS. I'm taking VC funding.)

[silverwind]

Interesting. I thought exec('cmd') was literally equivalent to sh -c 'cmd' in the terminal but I see we're safe from those expansions because there's no outer shell involved. Anyways, I think my method is still slightly superior because it does can not fail with unbalanced quotes:

> String(child_process.execSync(`echo '${`1337'; cat '/etc/passwd`.replace(/'/gm, `'\\''`)}'`))
"1337'; cat '/etc/passwd\n"
> String(child_process.execSync(`echo '${`1337"; cat "/etc/passwd`.replace(/'/gm, `'\\''`)}'`))
'1337"; cat "/etc/passwd\n'
> String(child_process.execSync(`echo '${`1337"; cat '/etc/passwd`.replace(/'/gm, `'\\''`)}'`))
`1337"; cat '/etc/passwd\n`

> String(child_process.execSync(`echo '${`1337'; cat '/etc/passwd`.replace(/'/gm, `'"'`)}'`))
"1337'; cat '/etc/passwd\n"
> String(child_process.execSync(`echo '${`1337"; cat "/etc/passwd`.replace(/'/gm, `'"'`)}'`))
'1337"; cat "/etc/passwd\n'
> String(child_process.execSync(`echo '${`1337"; cat '/etc/passwd`.replace(/'/gm, `'"'`)}'`))
/bin/sh: -c: line 0: unexpected EOF while looking for matching `"'
/bin/sh: -c: line 1: syntax error: unexpected end of file

[devsnek]

I'd still say the ultimate method is to avoid passing random strings to shell scripts via arguments. In the example the OP brought up with yarn, they could use the yarn node api instead of invoking yarn with child_process.

[ThisIsMissEm]

At some point user input will need to travel from node.js to a separate binary, using a javascript API for a thing isn't always an answer (look at anything that does transcoding, as that tends to be a mess of ffmpeg arguments, and using a binary API is often less desirable here).

Whilst there are security issues around passing arbitrary data, as we can see from this issue, there are already multiple suggested ways of escaping data, which makes it much harder for the average developer to know how to do this correctly, ergo, node.js core should provide a recommended way for such a common vector for security attacks.

[devsnek]

ffmpeg isn't a shell, it won't reparse the arguments into something that can spawn new programs. this is only a problem if you're passing arguments to a shell.

[ChALkeR]

The recommended solution is to use execFile, or any other child_process method that accepts structured input (executable name/path and and array of arguments), and not build the shell command with string concatenation at all.