-
Couldn't load subscription status.
- Fork 249
Taint propagates from methods of tainted objects #167
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep that works. Thanks 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++
So Fresh, So Clean
Also, some news, I've been busy making slides these past few weeks for PyBay, and so will return to my open expr_star_handler PR soon.
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`
976b128 to
8811343
Compare
Thanks for the reminder. Fixed.
Uh oh!
There was an error while loading. Please reload this page.
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