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

Trim the "Reassigned in:" nodes to the ones that are relevant #46

Closed
KevinHock opened this issue May 22, 2017 · 5 comments · Fixed by #47
Closed

Trim the "Reassigned in:" nodes to the ones that are relevant #46

KevinHock opened this issue May 22, 2017 · 5 comments · Fixed by #47
Assignees

Comments

@KevinHock
Copy link
Collaborator

KevinHock commented May 22, 2017

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.

@KevinHock KevinHock self-assigned this May 22, 2017
@KevinHock
Copy link
Collaborator Author

@KevinHock
Copy link
Collaborator Author

KevinHock commented May 23, 2017

We can't just do

    for node in secondary_in_sink:
        if sink_args and node.left_hand_side in sink_args:
            evil_node = node

because, while that works for the example, that only gets the last part of the chain, there should be a better way.

@Thalmann
Copy link
Contributor

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
Copy link
Collaborator Author

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
Copy link
Contributor

A flag to hide reassignments are fine with me. But the default should be as it is now :)

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 a pull request may close this issue.

2 participants