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

Blackbox functions return tainted value (false positives) #45

Closed
KevinHock opened this issue May 18, 2017 · 5 comments · Fixed by #63
Closed

Blackbox functions return tainted value (false positives) #45

KevinHock opened this issue May 18, 2017 · 5 comments · Fixed by #63
Assignees

Comments

@KevinHock
Copy link
Collaborator

KevinHock commented May 18, 2017

So, there are 2 commented out tests that I added during my interprocedural PR here and here.

I was going to fix these via doing something in an else of this if in visit_call, marking node.something as blackbox but I'm wondering what the best place to check something at, maybe in reaching_definitions_base, or adding them to the sanitizers dictionary so they can be marked as sanitized here.

Thoughts?

@KevinHock KevinHock self-assigned this May 18, 2017
@Thalmann
Copy link
Contributor

Hmm, i do not know if this even makes much sense.

The first example about absolute import will not even run(ImportError).
The second one will fail when we make the post.

So i do not think we need to support analysis of code the python interpreter does not even understand.

@StefanMich
Copy link
Contributor

In my opinion the best solution would be to mark it as a vulnerability and in some way indicate that it is due to the name not being imported. That way the developer is able to disregard the vulnerability if he knows that he uses some dynamic magic to set the name

@KevinHock
Copy link
Collaborator Author

KevinHock commented May 21, 2017

I probably could have explained this better, say

import subprocess
from flask import Flask, render_template, request

# This is a lib we can't possibly see inside of
import scrypt


app = Flask(__name__)

@app.route('/menu', methods=['GET'])
def menu():
    param = request.args.get('suggestion')

    # This is a function we can't possibly see inside of
    foobar = scrypt.encrypt('echo ' + param + ' >> ' + 'menu.txt', 'password')
    command = scrypt.encrypt('echo ' + param + ' >> ' + 'menu.txt', 'password')
    hey = command
    subprocess.call(hey, shell=True)

    with open('menu.txt','r') as f:
        menu = f.read()

    return render_template('command_injection.html', menu=menu)

if __name__ == '__main__':
    app.run(debug=True)

scrypt.encrypt could be any call from a library that we can't possibly know e.g. If all tainted args means a tainted return value.

So we shouldn't report it as a vulnerability with as much confidence as everything else. We can talk more about the UI part when I make the PR on Monday or Tuesday.

The way I did this was, to put the call in a set called blackbox_calls and then check in the assignment_with_call type function if the call is in blackbox calls. If it is, I add the assignment to blackbox assignments, which can be checked when sanitation is checked in vulnerabilities.py. This will get more complicated when I handle nested function calls but I'll write tests and incrementally handle things.

@KevinHock
Copy link
Collaborator Author

KevinHock commented May 21, 2017

Maybe one day we'll do it like this awesome paper but, we're not there yet.

@KevinHock
Copy link
Collaborator Author

KevinHock commented May 21, 2017

Maybe this update to the example file is better, thoughts? (Why would you exec an scrypt hash is a valid question but I didn't feel like running pyt until I find a better example.)

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.

3 participants