Add support for sinks introduced with "from .... import ..."

[fkromer]

Right now sinks seem to be considered during vulnerability analysis only in case of "module scope imports". E.g. vulnerabilities w.r.t. sink subprocess.call( are only detected in case the production code imports module scope wise:

import subprocess

subprocess.call(

In case the production code introduces the sink via module import the vulnerability won't be detected.

from subprocess import call

call(

[bcaller]

Yep. It's the same with import x as y. I would like pyt to resolve fully qualified names where possible. I think whoever works on this should look at how bandit does this.

[KevinHock]

This would definitely be a great feature

[fkromer]

Yep. It's the same with import x as y. I would like pyt to resolve fully qualified names where possible. I think whoever works on this should look at how bandit does this.

If one looks at Bandit examples of B602, B603 and B604 it seems like Bandit doesn't consider this either. In case Popen is imported as pop via from subprocess import Popen as pop the example of B604 doesn't document a raise pop('/bin/gcc --version', shell=True).

[adrianbn]

I am interested in this, and in general in being able to identify sinks in a better way than currently. I tried bandit 1.5.1 on python 3.7.0 and the following code triggers B604:

from subprocess import Popen as pop

pop('/bin/ls', shell=True)

>> Issue: [B404:blacklist] Consider possible security implications associated with Popen module.
   Severity: Low   Confidence: High
   Location: test-bandit.py:1
   More Info: https://bandit.readthedocs.io/en/latest/blacklists/blacklist_imports.html#b404-import-subprocess
1       from subprocess import Popen as pop
2
3       pop('/bin/ls', shell=True)

--------------------------------------------------
>> Issue: [B602:subprocess_popen_with_shell_equals_true] subprocess call with shell=True seems safe, but may be changed in the future, consider rewriting without shell
   Severity: Low   Confidence: High
   Location: test-bandit.py:3
   More Info: https://bandit.readthedocs.io/en/latest/plugins/b602_subprocess_popen_with_shell_equals_true.html
2
3       pop('/bin/ls', shell=True)

Is there a reason why sinks are defined like this subprocess.call(? Wouldn't it be better to define the package and method separately and include which argument of the function is actually not supposed to receive tainted data?

[KevinHock]

@adrianbn

I'd be really curious how Bandit does that :D, but unlike most things, e.g. #nosec comments, what Bandit does might not help us.

Is there a reason why sinks are defined like this subprocess.call(?

Mostly due to not tracking namespaces in the case of "blackbox" calls (importing things that aren't from another file in the repo). It's a great idea to do what you suggest though.

We sort of do track namespaces with e.g. imports of another file here

pyt/pyt/cfg/expr_visitor.py

Lines 557 to 564 in 4b495ad

	_id = get_call_names_as_string(node.func)
	local_definitions = self.module_definitions_stack[-1]
	alias = handle_aliases_in_calls(_id, local_definitions.import_alias_mapping)
	if alias:
	definition = local_definitions.get_definition(alias)
	else:
	definition = local_definitions.get_definition(_id)

include which argument of the function is actually not supposed to receive tainted data

#182 added that capability, but we haven't merged anything into https://github.com/python-security/pyt/blob/master/pyt/vulnerability_definitions/all_trigger_words.pyt to use it.

[adrianbn]

Got it, seems like some of the pre-requisite features required to tackle this are there.

[amacfie]

Close?

[KevinHock]

Thanks @amacfie!