Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

only trigger DUO116 when shell=True #31

Merged
merged 6 commits into from Oct 14, 2020
Merged

only trigger DUO116 when shell=True #31

merged 6 commits into from Oct 14, 2020

Conversation

konstruktoid
Copy link
Contributor

Using shell=False, to make sure default arguments are set, would trigger DUO116:subprocess module with shell=True

This PR allows shell=False.

$ cat -n /tmp/shell.py 
     1  #!/usr/bin/python3
     2  
     3  import subprocess
     4  
     5  command = "/bin/echo"
     6  
     7  subprocess.call(command, shell=False)
     8  subprocess.call(command, shell=True)
     9  subprocess.call(command, shell=False)
$ python3 -m flake8 /tmp/shell.py 
/tmp/shell.py:3:1: S404 Consider possible security implications associated with subprocess module.
/tmp/shell.py:7:1: S603 subprocess call - check for execution of untrusted input.
/tmp/shell.py:8:1: DUO116 use of "shell=True" is insecure in "subprocess" module
/tmp/shell.py:8:1: S602 subprocess call with shell=True identified, security issue.
/tmp/shell.py:9:1: S603 subprocess call - check for execution of untrusted input.
$ python3 -m flake8 -h
[...]
Installed plugins: dlint: 0.10.3, flake8-bandit: 2.1.2, flake8-bugbear:
20.1.4, flake8-darglint: 1.5.5, flake8-executable: 2.0.4, flake8_deprecated:
1.2, mccabe: 0.6.1, naming: 0.11.1, pycodestyle: 2.6.0, pyflakes: 2.2.0,
radon: 4.3.2

Signed-off-by: Thomas Sjögren konstruktoid@users.noreply.github.com

Signed-off-by: Thomas Sjögren <konstruktoid@users.noreply.github.com>
@konstruktoid konstruktoid changed the title only trigger S602 when shell=True only trigger DUO116 when shell=True Oct 12, 2020
@mschwager
Copy link
Contributor

Hi @konstruktoid, thanks for the improvements.

I originally went with checking if the kwarg is present because it defaults to False, so if it's present that means it's likely possible that it's conditionally enabled, e.g. shell=conditional_variable. However, you're correct that it doesn't account for shell=False as a false positive.

What do you think of checking if kwarg_present and not kwarg_false? We could combine these checks similar to https://github.com/dlint-py/dlint/blob/master/dlint/linters/bad_onelogin_kwarg_use.py#L34.

Signed-off-by: Thomas Sjögren <konstruktoid@users.noreply.github.com>
@konstruktoid
Copy link
Contributor Author

Hi @mschwager, I created present_and_true since double negations can get kind of confusing.

Signed-off-by: Thomas Sjögren <konstruktoid@users.noreply.github.com>
@konstruktoid
Copy link
Contributor Author

$ pytest -vv tests/test_bad_subprocess_use.py 
=========================================================================================== test session starts ============================================================================================
platform darwin -- Python 3.6.2, pytest-4.6.9, py-1.9.0, pluggy-0.13.1 -- /Library/Frameworks/Python.framework/Versions/3.6/bin/python3.6
cachedir: .pytest_cache
metadata: {'Python': '3.6.2', 'Platform': 'Darwin-19.6.0-x86_64-i386-64bit', 'Packages': {'pytest': '4.6.9', 'py': '1.9.0', 'pluggy': '0.13.1', 'ansible': '2.9.14', 'molecule': '3.0.8'}, 'Plugins': {'metadata': '1.10.0', 'testinfra': '5.3.1', 'molecule': '1.3.0', 'html': '2.1.1', 'plus': '0.2', 'benchmark': '3.2.3', 'cov': '2.8.1'}, 'env': 'ANSIBLE_SCP_IF_SSH=y '}
benchmark: 3.2.3 (defaults: timer=time.perf_counter disable_gc=False min_rounds=5 min_time=0.000005 max_time=1.0 calibration_precision=10 warmup=False warmup_iterations=100000)
rootdir: /../Github/dlint
plugins: metadata-1.10.0, testinfra-5.3.1, molecule-1.3.0, html-2.1.1, plus-0.2, benchmark-3.2.3, cov-2.8.1
collected 2 items                                                                                                                                                                                          

tests/test_bad_subprocess_use.py::TestBadSubprocessUse::test_bad_subprocess_usage PASSED                                                                                                             [ 50%]
tests/test_bad_subprocess_use.py::TestBadSubprocessUse::test_subprocess_usage PASSED                                                                                                                 [100%]

========================================================================================= 2 passed in 0.05 seconds =========================================================================================

@mschwager
Copy link
Contributor

Thanks for the updates, but not kwarg_false is not quite the same as kwarg_true here. For example, not kwarg_false also catches things like shell=variable_name. My intent in that case would be: if you're using a variable that likely means that shell could be conditionally True and we should detect it.

In other words, the logic would be: anytime shell is present (not using the default of False) and not explicitly set to False (abundance of caution, I guess) we should detect it. Does that make sense?

Signed-off-by: Thomas Sjögren <konstruktoid@users.noreply.github.com>
Signed-off-by: Thomas Sjögren <konstruktoid@users.noreply.github.com>
@konstruktoid
Copy link
Contributor Author

Good point, I get your logic there.
Updated the PR, but can shell be anything than a truthy value?

tests/test_bad_subprocess_use.py Outdated Show resolved Hide resolved
@konstruktoid
Copy link
Contributor Author

konstruktoid commented Oct 14, 2020

Good point, I get your logic there.
Updated the PR, but can shell be anything than a truthy value?

Replying to myself:

$ cat -n /tmp/shell.py 
     1  #!/usr/bin/python3
     2  
     3  import subprocess
     4  
     5  shell_true = True
     6  shell_false = False
     7  command = ["echo", "X"]
     8  
     9  subprocess.call(command)
    10  subprocess.call(command, shell=False)
    11  subprocess.call(command, shell=True)
    12  subprocess.call(command, shell=False)
    13  subprocess.call(command, shell=shell_true)
    14  subprocess.call(command, shell=shell_false)
$ flake8 --select=DUO /tmp/shell.py 
/tmp/shell.py:11:1: DUO116 use of "shell=True" is insecure in "subprocess" module
/tmp/shell.py:13:1: DUO116 use of "shell=True" is insecure in "subprocess" module
/tmp/shell.py:14:1: DUO116 use of "shell=True" is insecure in "subprocess" module
$ python3 /tmp/shell.py 
X
X

X

X
$

Updated: We won't catch line 14, but I'm fine with that.

Signed-off-by: Thomas Sjögren <konstruktoid@users.noreply.github.com>
@mschwager
Copy link
Contributor

Nice! Thanks for these improvements!

@mschwager mschwager merged commit 3645954 into dlint-py:master Oct 14, 2020
2 checks passed
@konstruktoid konstruktoid deleted the subprocesshell branch October 15, 2020 06:58
konstruktoid added a commit to konstruktoid/disruella that referenced this pull request Nov 2, 2020
Signed-off-by: Thomas Sjögren <konstruktoid@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants