-
Notifications
You must be signed in to change notification settings - Fork 249
Yield, YieldFrom, AugAssign propagate taint #155
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
b9f740c to
0cfc1b0
Compare
Thanks so much for making this, it looks great :)
I'll review in-depth tomorrow as I'm still on-a-roll with my open PR.
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.
Ben Caller is the best.
🚢
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.
Thanks for updating this 👍 I definitely would have forgot to.
pyt/cfg/expr_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.
Nit: To make things more DRY, you won't need to specify node.lineno
https://github.com/python-security/pyt/blob/master/pyt/core/node_types.py#L42-L43
tests/cfg/cfg_test.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.
Super nit: Over time I've been trying to do
self.assertInCfg([ (entry_foo, entry), (a, entry_foo), (_if, a), (yld_if, _if), (yld, _if), (yld, yld_if), # Different from return (exit_foo, yld), (call_foo, exit_foo), (_exit, call_foo) ])
but it's a WIP.
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.
❤️
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.
Nit: Maybe ..core.node_types just since everything else does that style.
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.
You write such high quality code, but the amazing commit messages is what really separates you from the rest.
pyt/cfg/expr_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.
Nit: So for this I don't think you need to pass line_number due to
https://github.com/python-security/pyt/blob/master/pyt/core/node_types.py#L42-L43
not just the last thing yielded as before. It behaves like a cross between AugAssign (everything yielded is added to yld_* return variable) and Return (we make a yld_* return variable though don't immediately exit the function).
It was a str when it should've been a List[str]
It was a str instead of List[str]
Before, the variable would be tainted only if the last += was tainted. Now url = 'http://' url += TAINT url += '?x=y' url marked as tainted.
done
Previously
+=only propagated taint of the last assigment. Now, the original variable is added toright_hand_side_variables.Yield and YieldFrom are treated a bit like augmented assignment, giving the function a final return variable
yld_a bit likeret_. Really it's a generator function, but we can treat it as a normal function. So anything yielded propagates taint to the overall "return" value of the function.