New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Harden shell.exec by writing the child process in a source file #782
Comments
This PR refactors `shell.exec()` by putting its child process in a separate code file. This also slightly cleans up dead code. There's more potential to clean this up (e.g. exit status), but this is a good enough start. Issue #782
This PR refactors `shell.exec()` by putting its child process in a separate code file. This also slightly cleans up dead code. There's more potential to clean this up (e.g. exit status), but this is a good enough start. Issue #782
Initial PR has landed. Additional improvements:
|
I'm not sure replacing
But I cannot do that with
|
I'm referring to the call within The API depends on the call to |
This uses `child_process.execFileSync` instead of `execSync` to launch the child process. This further reduces the attack surface, removing a possible point for command injection in the ShellJS implementation. This does not affect backwards compatibility for the `shell.exec` API (the behavior is determined by the call to `child_process.exec` within `src/exec-child.js`). Issue #782
This parameter isn't needed, we can easily rely on exit code status for this. Eliminating the parameter reduces file IO, code complexity, and removes a busy loop. Issue #782
This uses `child_process.execFileSync` instead of `execSync` to launch the child process. This further reduces the attack surface, removing a possible point for command injection in the ShellJS implementation. This does not affect backwards compatibility for the `shell.exec` API (the behavior is determined by the call to `child_process.exec` within `src/exec-child.js`). Issue #782
This parameter isn't needed, we can easily rely on exit code status for this. Eliminating the parameter reduces file IO, code complexity, and removes a busy loop. This also removes some legacy code related to streams. Issue #782
The `paramsFile` is obsolete now that we use `execFileSync()` for our internal implementation. Instead, we pass parameters to the child process directly as a single commandline parameter to reduce file I/O. Issue #782
The `paramsFile` is obsolete now that we use `execFileSync()` for our internal implementation. Instead, we pass parameters to the child process directly as a single commandline parameter to reduce file I/O. Issue #782
This reverts commit 64d5899. Reason for revert: If stdin is large, then the param object can become an extremely long string, exceeding the maximum OS size limit on commandline parameters. Original change's description: > refactor(exec): remove paramsFile (#807) > > The `paramsFile` is obsolete now that we use `execFileSync()` for our > internal implementation. Instead, we pass parameters to the child > process directly as a single commandline parameter to reduce file I/O. > > Issue #782
This reverts commit 64d5899. Reason for revert: If stdin is large, then the param object can become an extremely long string, exceeding the maximum OS size limit on commandline parameters. Original change's description: > refactor(exec): remove paramsFile (#807) > > The `paramsFile` is obsolete now that we use `execFileSync()` for our > internal implementation. Instead, we pass parameters to the child > process directly as a single commandline parameter to reduce file I/O. > > Issue #782 Fixes #818
This reverts commit 64d5899. Reason for revert: If stdin is large, then the param object can become an extremely long string, exceeding the maximum OS size limit on commandline parameters. Original change's description: > refactor(exec): remove paramsFile (#807) > > The `paramsFile` is obsolete now that we use `execFileSync()` for our > internal implementation. Instead, we pass parameters to the child > process directly as a single commandline parameter to reduce file I/O. > > Issue #782 Fixes #818
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). |
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:
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.The text was updated successfully, but these errors were encountered: