Py: add unsafe-shell-command-construction

[erik-krogh]

The query is almost direct copy-paste from Ruby.
The query doesn't (currently) flag any CVEs, but that will hopefully happen in the future.

Evaluation looks OK.
Shows a slight slowdown from adding a new query, but I can't find a reason for the slowdown when I try locally.

Full MRVA run.

Many of the results are just 🤦 without being security relevant, but they are bad enough style that I think the results are OK regardless of whether it's a security issue.

The results in localstack are FPs, due to it spuriously finding that shell can be set to True. But I don't think we can fix that.
Lots of the results are file paths inserted into shell strings.

[github-actions]

QHelp previews:

python/ql/src/Security/CWE-078/UnsafeShellCommandConstruction.qhelp

Unsafe shell command constructed from library input

Dynamically constructing a shell command with inputs from library functions may inadvertently change the meaning of the shell command. Clients using the exported function may use inputs containing characters that the shell interprets in a special way, for instance quotes and spaces. This can result in the shell command misbehaving, or even allowing a malicious user to execute arbitrary commands on the system.

Recommendation

If possible, provide the dynamic arguments to the shell as an array to APIs such as subprocess.run to avoid interpretation by the shell.

Alternatively, if the shell command must be constructed dynamically, then add code to ensure that special characters do not alter the shell command unexpectedly.

Example

The following example shows a dynamically constructed shell command that downloads a file from a remote URL.

import os

def download(path): 
    os.system("wget " + path) # NOT OK

The shell command will, however, fail to work as intended if the input contains spaces or other special characters interpreted in a special way by the shell.

Even worse, a client might pass in user-controlled data, not knowing that the input is interpreted as a shell command. This could allow a malicious user to provide the input http://example.org; cat /etc/passwd in order to execute the command cat /etc/passwd.

To avoid such potentially catastrophic behaviors, provide the input from library functions as an argument that does not get interpreted by a shell:

import subprocess

def download(path): 
    subprocess.run(["wget", path]) # OK

References

• OWASP: Command Injection.
• Common Weakness Enumeration: CWE-78.
• Common Weakness Enumeration: CWE-88.
• Common Weakness Enumeration: CWE-73.

[erik-krogh]

The equivalent predicate in JavaScript is way bigger, with more cases in the disjunction.

I imagine this will grow over time, but for now I'll start with what I've seen a need for.

[RasmusWL]

Overall looks good, thanks 👍 I would like to see a few minor changes, and then we need to discuss the precision of the query a bit 👍

[RasmusWL]

Let's talk a bit more about what the precision should be.

Many of the results are just 🤦 without being security relevant, but they are bad enough style that I think the results are OK regardless of whether it's a security issue.

This ☝️ part of your PR comment makes it sound like most of the results are following a bad pattern that ideally should be fixed. But also that it's like it is a code smell and not a security issue.

Based on that, I'm thinking it might be more appropriate to use @precision medium, but happy to hear your thoughts on this.

(I looked over some of the MRVA results, but most of the ones I looked at seemed "meh" to me)

[erik-krogh]

I think it's a code-smell with high precision, but a security issue with medium precision.

So I suggest that we lower the severity to warning instead of lowering the precision.
Thoughts?