Repository navigation
Trim the "Reassigned in:" nodes to the ones that are relevant #46
Description
So if we have the following code:
@app.route('/menu', methods=['POST'])
def menu():
param = request.form['suggestion']
command = 'echo ' + param + ' >> ' + 'menu.txt'
hey = 'echo ' + param + ' >> ' + 'menu.txt'
yo = 'echo ' + hey + ' >> ' + 'menu.txt'
subprocess.call(command, shell=True)
with open('menu.txt','r') as f:
menu = f.read()
return render_template('command_injection.html', menu=menu)We show the vulnerability output as:
1 vulnerability found:
Vulnerability 1:
File: example/vulnerable_code/command_injection.py
> User input at line 15, trigger word "form[":
param = request.form['suggestion']
Reassigned in:
File: example/vulnerable_code/command_injection.py
> Line 16: command = 'echo ' + param + ' >> ' + 'menu.txt'
File: example/vulnerable_code/command_injection.py
> Line 17: hey = 'echo ' + param + ' >> ' + 'menu.txt'
File: example/vulnerable_code/command_injection.py
> Line 18: yo = 'echo ' + hey + ' >> ' + 'menu.txt'
File: example/vulnerable_code/command_injection.py
> reaches line 20, trigger word "subprocess.call(":
subprocess.call(command,shell=True)
Where we don't really care about Line 17 and 18 in the output, right?
I ran into this while doing #45, once I fix this then I can make the PR fixing both of them.
Metadata
Metadata
Assignees
Labels
No labels
Activity
KevinHock commentedon May 22, 2017
I realize the fix needs to happen around https://github.com/python-security/pyt/blob/master/pyt/vulnerabilities.py#L229 and https://github.com/python-security/pyt/blob/master/pyt/vulnerabilities.py#L239, it shouldn't be that hard, right?
KevinHock commentedon May 23, 2017
We can't just do
because, while that works for the example, that only gets the last part of the chain, there should be a better way.
Thalmann commentedon May 23, 2017
I care about 17 and 18. The reassignments might not be that trivial like in this example :)
IMO: More information is better. We do not want to hide anything from the user running a scan/analysis.
KevinHock commentedon May 23, 2017
I think a good compromise would be to color the assignments leading to the vulnerability different from the rest, but until we do that, what do you think about a command line flag to trim the list? I got a little confused by the output in #11 due to this to be honest.
Thalmann commentedon May 23, 2017
A flag to hide reassignments are fine with me. But the default should be as it is now :)