B603 false positive?

[pzelnip]

Describe the bug I don't understand how I should "check for untrusted input.

To Reproduce Steps to reproduce the behavior:

With the code:

import shlex
import subprocess

def foo():
    args = shlex.split("git rev-parse HEAD")
    return str(subprocess.check_output(args, shell=False), "utf-8").strip()

Gives Issue: [B603:subprocess_without_shell_equals_true] subprocess call - check for execution of untrusted input. I don't understand how this is shell equals true given that "shell=False" is passed, nor how this is untrusted input.

Expected behavior

This line shouldn't be flagged as a warning

Bandit version

bandit --version
bandit 1.4.0

Additional context

[bcaller]

B603 is subprocess_without_shell_equals_true... without. So it knows that shell=False.

However there can still be a problem: bandit is looking at subprocess.check_output(args, shell=False) and sees check_output with argument args.

Bandit doesn't know that args is safe. It just sees a variable which could have come from anywhere.

Unfortunately bandit isn't a code-flow analysis tool so it can't reason about what args is. It just flags a warning. You manually check the warning and decide to either add # nosec at the end of the line or to turn off this B603 test if you find it unhelpful.

It's a false positive but the test is functioning as designed, as a simple linter to warn about possible issues.

[pzelnip]

I see your point, but I suppose I'm having trouble how you could possibly use subprocess.check_output without triggering B603. Ie I get that all it sees is args and it has no idea if args is trusted or not, but even if I put the string literal into the call:

return str(
        subprocess.check_output("git rev-parse HEAD".split(), shell=False), "utf-8"
    ).strip()

I still trigger B603. As such this feels more like "don't use subprocess.check_output" rather than "make sure the input to subprocess.check_output is trusted", so it feels like the description of B603 is misleading/not helpful. Or am I missing a way to use subprocess.check_output without triggering B603?

[bcaller]

Actually you're right. My apologies. I checked the code and the test is a bit dumb. It will flag any use of check_output regardless. The example in the test's docstring is subprocess.check_output(['/bin/ls', '-l']) which I wouldn't consider vulnerable to injection.

I agree with you that the test should be improved to decrease obvious false positives.

[Tatsinnit]

+1 , also linking this to an another one: https://github.com/PyCQA/bandit/issues/373 . This happens with version 1.5.1 as well.

[andrew222651]

It would be easy to change the test to be like django_sql_injection.django_rawsql_used which doesn't generate an issue if the argument is a string literal.

Even then the confidence should not be high, which it is currently.

[Flauschbaellchen]

Is there a timeline when this bug will be fixed? IMHO, the check would be very useful but the bug renders it completely useless and frustrating.

[SanketDG]

t would be easy to change the test to be like django_sql_injection.django_rawsql_used which doesn't generate an issue if the argument is a string literal.

How does this help here?

The best way seems to lower its confidence down to LOW, as there is no definite way of knowing that a input is truly secure.

[jackton1]

+1 Waiting for the timeline on this issue also, Since it's not helpful to have to introduce # nosec each time for both B602 and B404

[BorjaEst]

Related to this issue, the reference https://bandit.readthedocs.io/en/1.7.5/plugins/b603_subprocess_without_shell_equals_true.html says we should use shell=True and gives some references. However, when reading the references, those say the oposite, that we should avoid shell=True. See for example: https://security.openstack.org/guidelines/dg_use-subprocess-securely.html

If you use shell=True, your code is extremely likely to be vulnerable.

Isn't it a false false positive?

[nfantone]

This has been opened since 2018 — I suppose there's still no way to use check_output without triggering B603? And just FYI, the same is also true for B603 in Ruff.