Harden shell.exec by writing the child process in a source file

[nfischer]

Inspired by #524.

Exec has some security concerns, as pointed out in other issues. Due to the API contract, it's inherently vulnerable to shell-command-injection (same as child_process.exec()). Additionally, due to its architecture, it's also vulnerable to javascript-code-injection. This is because we write out a string (containing JS code) to a file and exec it.

I don't think we have any known security vulnerabilities here, but this is poor design and easy to mess up in future changes.

We should be able to improve the design while maintaining backwards compatibility by adopting the architecture in #524. We can write the child process code in a source file and pass parameters via IPC (more on that later).

This has 3 main benefits:

1. Reduces attack surface (no JS injection)
2. Child process code is more readable
3. Meaningful line coverage for child process code

Care needs to be taken when passing child parameters. It's not valid to pass values on the shell commandline (shells do weird escape regarding \n, \t, etc.). I think our best option for IPC is the filesystem. I think IPC modules typically use sockets, but I'm not sure how well these perform for Windows. I think it's acceptable to use the filesystem for now, and come back later with a different IPC system as an enhancement.

[nfischer]

Initial PR has landed. Additional improvements:

• Swap child_process.execSync with child_process.execFileSync in src/exec.js (this is still backwards compatible, but continues to reduce attack surface) (Use execFileSync to launch child process #790)
• Replace codeFile with actual exit status (reduce file IO) (Remove codeFile parameter #791)
• Consider passing parameters via CLI arg (reduce file IO, this should be OK after switching to execFileSync)
• Watch for exceptions when calling execFileSync (exceptions might mean the command has non-zero exit status or that there was a bug running the command)

[freitagbr]

I'm not sure replacing child_process.execSync with child_process.execFileSync will work, at least with the same API. For example, I can do this with child_process.execSync:

> child_process.execSync('echo hi; echo yo').toString()
'hi\nyo\n'

But I cannot do that with child_process.execFileSync:

> child_process.execFileSync('echo', ['hi; echo yo']).toString()
'hi; echo yo\n'

[nfischer]

I'm referring to the call within src/exec.js. Its responsibility is "launch a subprocess to run exec-child.js". This is an implementation detail, not exposing the API.

The API depends on the call to child_process.exec() from within src/exec-child.js. That's the part which cannot be changed.

[nfischer]

I think this issue is done. I unchecked the bit about passing via CLI arg, since it turns out that's not OK (see #819).