Yield, YieldFrom, AugAssign propagate taint #155
Conversation
bcaller
force-pushed
the
yield
branch
2 times, most recently
from
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. |
| @@ -9,7 +9,7 @@ This directory contains miscellaneous code that is imported from different parts | |||
|
|
|||
| - `get_call_names`_ used in `vars_visitor.py`_ when visiting a Subscript, and `framework_helper.py`_ on function decorators in `is_flask_route_function`_ | |||
|
|
|||
| - `get_call_names_as_string`_ used in `expr_visitor.py`_ to create ret_function_name as RHS and yield_function_name as LHS, and in stmt_visitor.py when connecting a function to a loop. | |||
| - `get_call_names_as_string`_ used in `expr_visitor.py`_ to create ret_function_name as RHS and yld_function_name as LHS, and in stmt_visitor.py when connecting a function to a loop. | |||
There was a problem hiding this comment.
Thanks for updating this 👍 I definitely would have forgot to.
pyt/cfg/expr_visitor.py
Outdated
| path=self.filenames[-1]) | ||
| rhs_visitor.result + [LHS], | ||
| path=self.filenames[-1], | ||
| line_number=node.lineno) |
There was a problem hiding this comment.
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
| call_foo = 7 | ||
| _exit = 8 | ||
|
|
||
| self.assertInCfg([(entry_foo, entry), |
There was a problem hiding this comment.
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.
|
|
||
| def _remove_sink_from_secondary_nodes(self): | ||
| try: | ||
| self.reassignment_nodes.remove(self.sink) | ||
| except ValueError: # pragma: no cover | ||
| pass | ||
|
|
||
| def _remove_non_propagating_yields(self): |
| @@ -4,6 +4,8 @@ | |||
| from enum import Enum | |||
| from collections import namedtuple | |||
|
|
|||
| from pyt.core.node_types import YieldNode | |||
There was a problem hiding this comment.
Nit: Maybe ..core.node_types just since everything else does that style.
| rhs_visitor.visit(node.value) | ||
| except AttributeError: | ||
| rhs_visitor.result = 'EmptyYield' | ||
| if node.value is None: |
There was a problem hiding this comment.
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
| path=self.filenames[-1]) | ||
| rhs_visitor_result + [LHS], | ||
| path=self.filenames[-1], | ||
| line_number=node.lineno) |
There was a problem hiding this comment.
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.