python-security/pyt

Specify which arguments of functions count as sinks

bcaller opened this issue · 5 comments

bcaller commented

SQLAlchemy allows you to construct SQL queries while preventing injection:

from sqlalchemy import text
query = text("SELECT id FROM members WHERE name = :name_of_member")
db.execute(query, name_of_member=tainted_user_input)

Here, execute is a sink with respect to its first argument. Any tainted input in the query should raise a warning. However, input to the remaining keyword arguments is fine and shouldn't be flagged. I want to reduce false positives.

I would like to be able to specify this somehow. Probably involving changes to pyt/vulnerabilities/vulnerabilities.py. Any thoughts?

KevinHock commented

You're amazing @bcaller, when I woke up to your 3 PRs this morning it was like Christmas Day 🎄 🎅 🎁 ☃️

I know exactly what is involved with this, I have a branch that I started with this a while ago, but my focus has been on implementing expr_star_handler (bool op's ifexp's etc.) in my current PR.

Here is the branch we have to (1) change the def-use chain code, which tells us how a source reaches a sink, to specify the 1st or 2nd argument etc. (2) change the current vulnerabilitility_definition files from our proprietary .pytformat, to JSON and (3) change the vulnerabilities.py file to take this into account. Feel free to take this one :)

KevinHock commented

Actually, that branch does more than what this issue is. This issue can solely be solved by changing the vulnerability definitions to json and vulnerabilities.py b/c we only care about the sinks, not the blackbox functions along the way.

bcaller commented

Hi @KevinHock. Thanks 🎅 just some very small changes.

I'll have a look at the branch. In vulnerabilities.py I might try to alter get_sink_args and get_tainted_node_in_sink_args to get the arguments by position / keyword. Would mean running RHSVisitor separately on each argument. I'll have a play.

  • 👍1
  • ❤️1
KevinHock commented

Awesome 👍
I always wanted this feature :)
Let me know what you would be interested in working on next, perhaps you would be interested in Customization is needed for open-redirects, to eliminate all false-positives, because if something tainted is used in string formatting, it typically needs to be at the very beginning of the string to be a vulnerability. from our last evaluation.

KevinHock commented

You might want the green square for closing this issue so I'll let you 👍