Taint propagates from methods of tainted objects #167
Conversation
There was a problem hiding this comment.
Looks great so far, this will help us with our
base64.urlsafe_b64decode(
state.encode('ascii')
).split(';')[1]false-negative in our last evaluation https://github.com/oakie/oauth-flask-template/blob/master/auth.py#L63-L72
:D :D :D 🎈
pyt/cfg/stmt_visitor.py
Outdated
| @@ -657,6 +657,10 @@ def add_blackbox_or_builtin_call(self, node, blackbox): | |||
| # `scrypt.outer(scrypt.inner(image_name), scrypt.other_inner(image_name))` | |||
| last_return_value_of_nested_call.connect(call_node) | |||
|
|
|||
| if '.' in call_function_label: | |||
| # taint is a RHS variable of taint.lower() | |||
| rhs_vars.append(call_function_label.split('.', 1)[0]) | |||
There was a problem hiding this comment.
Can you try deleting the
if isinstance(call, BBorBInode):
# Necessary to know e.g.
# `image_name = image_name.replace('..', '')`
# is a reassignment.
vars_visitor = VarsVisitor()
vars_visitor.visit(ast_node.value)
call.right_hand_side_variables.extend(vars_visitor.result)in https://github.com/python-security/pyt/blob/master/pyt/cfg/stmt_visitor.py#L475-L481
This PR should make that code redundant 👍
If my hunch is correct about that, can you update this comment
https://github.com/python-security/pyt/blob/master/pyt/cfg/stmt_visitor.py#L670
There was a problem hiding this comment.
Yep that works. Thanks 👍
| @@ -613,7 +613,7 @@ def add_blackbox_or_builtin_call(self, node, blackbox): | |||
| right_hand_side_variables=[], | |||
| line_number=node.lineno, | |||
| path=self.filenames[-1], | |||
| func_name=call_label.result[:index] | |||
| func_name=call_function_label | |||
|
Also, some news, I've been busy making slides these past few weeks for PyBay, and so will return to my open |
bcaller
force-pushed
the
method-propagation
branch
2 times, most recently
from
d23ab96
to
976b128
Compare
|
Thanks, I've modified the commit. |
|
I think merging the other PR caused some merge conflicts with this one. |
Previously
`x = TAINT.lower()` would be tainted (due to special handling for
assignment_call_nodes)
but
`x = str(TAINT.lower())` wouldn't be tainted.
To fix this, `TAINT` is added to the RHS variables of
`TAINT.lower()`.
This will mean that e.g. `request` will be a RHS variable of
`request.get()`, but I think that will be OK.
In the test which changed, the additional line is because resp has
become tainted.
However, this still leaves the following false negatives to fix another
day:
`assert_vulnerable('result = str("%s" % str(TAINT.lower()))') # FAILS`
`assert_vulnerable('result = str("%s" % TAINT.lower().upper())') # FAILS`
bcaller
force-pushed
the
method-propagation
branch
from
976b128
to
8811343
Compare
|
Thanks for the reminder. Fixed. |
Previously
x = TAINT.lower()would be tainted (due to special handling for assignment_call_nodes)but
x = str(TAINT.lower())wouldn't be tainted.To fix this,
TAINTis added to the RHS variables ofTAINT.lower().This will mean that e.g.
requestwill be a RHS variable ofrequest.get(), but I think that will be OK.In the test which changed, the additional line is because resp has
become tainted.
However, this still leaves the following false negatives to fix another
day:
assert_vulnerable('result = str("%s" % str(TAINT.lower()))') # FAILSassert_vulnerable('result = str("%s" % TAINT.lower().upper())') # FAILS