Rust: Add command injection query (CWE-078) - #22323
Conversation
Detects user-controlled data flowing into std::process::Command and tokio::process::Command (both command name and arguments). - Extension library with sources, sinks, and barriers - Models-as-data sinks for Command::new, .arg(), .args() (std + tokio) - Query help (.qhelp) with examples - Test cases with inline expectations Query ID: rust/command-line-injection
geoffw0
left a comment
geoffw0
left a comment
There was a problem hiding this comment.
Hi @lcmangalagiri,
I've just pushed a few commits onto this PR (I hope that's OK - I would usually do a PR onto your branch, but I don't seem to be able to do that here). Please take a look and confirm you're happy with my changes. I've addressed a few things:
- the
.expectedfile for the test was missing - it can be created with thecodeql test run . --learncommand, or using VSCode if you have things set up, but I've done that for you here. - I wanted to have a test that includes string concatenation, a common anti-pattern for shell command use. I've added that here as well and the query is successful on it.
- I've also added the good and bad examples as tests, to verify that they compile and the query behaves as expected on them. We're currently getting a spurious result on the good test.
- we can fix this with barrier guards, but I don't have time for that today.
- the
security-severitytag should ideally be generated with a (sadly internal) script, though copying from another query with the same CWE tags as you have is very likely to produce the same result. I've run the script, and in this case it did change the number slightly. I'm not sure it's critical either way.
Concern: Flagging every .arg() call may produce false positives. A hardcoded command with user-controlled arguments (e.g., Command::new("grep").arg(user_input)) is less severe than user-controlled shell commands. The query doesn't distinguish these.
I would say a case like that should at least put user_input through a "sanitizer" function of some sort. Usually we don't get taint flow through a sanitizer implementation, so there won't be a false positive result when that is done. If we do get one, and it shows up noisily in any of our various levels of real-world testing (MRVA, DCA and QA runs), we can address it then (e.g. by making the popular sanitizer function(s) into flow barriers).
Other than that, I'm very happy with this query. MRVA found some (valid) real-world results, which is a great starting point - and I think we could find more results in future with some additional models. But the next thing will be adding barrier guards and running this through CI and docs review.
I'm away on Friday, my hope is that we can finish and merge this when I'm back on Monday. Then I'll take a look at your other new query.
| // GOOD: use a fixed command with the user input as a separate argument, | ||
| // avoiding shell interpretation |
There was a problem hiding this comment.
I found this comment slightly confusing because it's talking about things that didn't change between the Bad and Good versions.
| // GOOD: use a fixed command with the user input as a separate argument, | |
| // avoiding shell interpretation | |
| // GOOD: user input is checked against an allowlist before passing into a shell command |
|
... looks like I didn't manage to push my commits to this branch. I'll have to try another way ... |
|
If you're able to add my commits, they're here: main...geoffw0:ql:rust-cwe-078-command-injection |
2 participants
Detects user-controlled data flowing into std::process::Command and tokio::process::Command (both command name and arguments).
Query include:
Query ID: rust/command-line-injection