[Coding Guideline] Prevent OS Command Injection#370
[Coding Guideline] Prevent OS Command Injection#370sei-dsvoboda wants to merge 6 commits intomainfrom
Conversation
✅ Deploy Preview for scrc-coding-guidelines ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
@PLeVasseur should the bot not be assigning a reviewer automatically for pull requests as well? Or perhaps it's not automatic because it doesn't come from a template? Anywho, here if I understand it correctly we should do the /r? command? |
|
@sei-dsvoboda I've edited your original message to make Github close the original issue once this PR gets merged :) |
|
👋 Hey @alexandruradovici! You've been assigned to review this coding guideline PR. Your Role as ReviewerAs outlined in our contribution guide, please:
Review Checklist
Bot CommandsIf you need to pass this review:
To assign someone else:
Other commands:
|
|
Hey @alexandruradovici would you like to claim this review, or let someone else grab it? |
| :scope: module | ||
| :tags: injection,sanitization | ||
|
|
||
| Commands that are passed to an external OS command interpreter, like ``std::process::Command``, should not allow untrusted input to be parsed as part of the command syntax. |
There was a problem hiding this comment.
In my view, Command is actually performing this, the problem is when it is used with sh -c, meaning the Command runs the interpreter. I think one should never actually run the interpreter, rather than taking care of the parameters.
There was a problem hiding this comment.
Hello, @alexandruradovici
I would agree that running the "sh" interpreter enables the command injection here. There are two general approaches: first, you would sanitize the arguments to prevent anything malicious from happening. Second, you use a less powerful command, such as invoking 'ls' directly without 'sh', or using fs::read_dir()...both compliant solutions use this second technique.
Are you suggesting a change in the wording, or in the code examples?
There was a problem hiding this comment.
My suggestion is to use a more powerful command and advise agains running the interpreter (sh or equivalent) with any command. I would not relate it to the arguments, but to the what kind of commands should or should not be ran.
There was a problem hiding this comment.
I wonder how we should do this.
Something that seems simple enough to do is a deny-list:
- In the worst case, we would be denying non-injection-allowing programs. This is fine, as a failure on our labeling would not lead to a safety hazard.
- An allow list would have the opposite problem: if we fail to account for injectability in a program we mark as "okay", we would be giving the 👍🏻 to a safety hazard. That's why I think an allow list would probably be bad to have.
- For all other command-line programs, we give the user some criteria by which they might be able to know if running it is okay or not (in a non-ambiguous way)
That criteria could be something along the lines of...
- "The program must not be turing-complete given its input": this would deny all possible injection mechanisms, but some valid programs could be denied as well. For example, a program that does arbitrary computation inside its own "isolated world" would be fine to run, but would not be allowed by this rule.
- "The program must not be able to transitively spawn an arbitrary number of other processes": this would be a lot harder to assess, but feels a lot closer to what we want to avoid.
There was a problem hiding this comment.
I would not relate it to the arguments, but to the what kind of commands should or should not be ran.
Ultimately, This is a Rust rule. I don't want the rule to blacklist some commands, because we can't make that blacklist complete. Too many shell commands + too many OS platforms. Creating a whitelist of 'benign' commands is safer, but no less Herculean a problem.
You also have the problem of when arguments are tokenized on Windows (see the BatBadBut vul).
We could try to limit commands in terms of Turing-completeness, but that also sounds like a huge rabbit hole. Which of these commands would we be forbidden from running: bash, python, sqlite3, jq, gcc ?
The only other option I see would be to forbid Commands entirely (at least for security-critical code). Allow exceptions only on an individual basis. Or allow Commands only in unsafe code.
|
✅ Rectified PR #370: latest review by @alexandruradovici is |
4 similar comments
|
✅ Rectified PR #370: latest review by @alexandruradovici is |
|
✅ Rectified PR #370: latest review by @alexandruradovici is |
|
✅ Rectified PR #370: latest review by @alexandruradovici is |
|
✅ Rectified PR #370: latest review by @alexandruradovici is |
|
Hey @alexandruradovici, it's been more than 14 days since you were assigned to review this. Please take one of the following actions:
If no action is taken within 14 days, you may be transitioned from Producer to Observer status per our contribution guidelines. Life happens! If you're dealing with something, just let us know. |
Closes #360
This is a new Rust rule based on the CERT Java rule IDS07-J.