Repository navigation
(Not an issue right now) Handle multiple returns #53
Description
I'll try to work on this relatively soon, but to think out loud..
In interprocedural_cfg.py, we have
def return_handler(self, node, function_nodes):
"""Handle the return from a function during a function call."""
call_node = None
for n in function_nodes:
if isinstance(n, ConnectToExitNode):
LHS = CALL_IDENTIFIER + 'call_' + str(self.function_index)
previous_node = self.nodes[-1]
if not call_node:
RHS = 'ret_' + get_call_names_as_string(node.func)
r = RestoreNode(LHS + ' = ' + RHS, LHS, [RHS],
line_number=node.lineno,
path=self.filenames[-1])
call_node = self.append_node(r)
previous_node.connect(call_node)
else:
# lave rigtig kobling
passwhich cleaned is
def return_handler(self, call_node, function_nodes):
"""Handle the return from a function during a function call.
Args:
call_node(ast.Call) : The node that calls the definition.
function_nodes(list[Node]): List of nodes of the function being called.
"""
for node in function_nodes:
# Only Return's and Raise's can be of type ConnectToExitNode
if isinstance(node, ConnectToExitNode):
# Create e.g. ¤call_1 = ret_func_foo RestoreNode
LHS = CALL_IDENTIFIER + 'call_' + str(self.function_call_index)
RHS = 'ret_' + get_call_names_as_string(call_node.func)
return_node = RestoreNode(LHS + ' = ' + RHS,
LHS,
[RHS],
line_number=call_node.lineno,
path=self.filenames[-1])
self.nodes[-1].connect(return_node)
self.nodes.append(return_node)
return Firstly, the for loop and the if statement seem to just serve the purpose of "Is there a node of type Return or Raise in the function?" But I think all functions should have at least one return node, right? I'm not sure if I understand the original intention that well e.g. what was going to be in the else?
Secondly, here is an example to illustrate the problem/need to handle multiple returns:
TODO
Activity
KevinHock commentedon Jun 22, 2017
Okay, so, I was going to suggest we do something to sort of OR all the different return values, such that if one was tainted then we would say it returned a tainted value. (Maybe this could be accomplished with e.g.
ret_foo = first_ret + second_ret + third_ret.)But, it turns out there's another issue that we would need to fix for this to even be an issue, because we currently do the ORing above even when we should not, on all variables e.g.
says the return value is tainted.
Where it should only say it's tainted when e.g.
if we decide to fix the 1st example in this comment, then I'm pretty sure we'll introduce a false-negative in the 2nd example, because it will see
ret_maybe_return_the_arg = 'apple'and think the tainted value (assigned inret_maybe_return_the_arg = 'huh' + foowas overwritten.)Overall, finding out that the 2nd example works right now, and thinking the 1st example is contrived and people don't normally write code like that, I care a lot less about any of this. But I'm glad I doc'd it ;)
If we were going to fix the first example, it would probably be in https://github.com/python-security/pyt/blob/master/pyt/reaching_definitions_taint.py but I don't plan on working on this any time soon.
[-]Handle multiple returns[/-][+](Not an issue right now) Handle multiple returns[/+]KevinHock commentedon Jul 19, 2017
To do the ORing ( e.g.
ret_foo = first_ret + second_ret + third_ret) you could easily do it inreturn_connection_handler.adrianbn commentedon Nov 22, 2018
Disclaimer: I may be saying things here that are known by the contributors to pyt. I'm sorry if this is just too obvious at this point.
I think this is an example of flow/path insensitivity, right? The first example says that pyt is flow insensitive, and the second one that pyt is path insensitive. Most commercial analyzers are flow sensitive, but not path sensitive due to the exponential explosion of path constraints involved.
I just started looking into
pytso I'm not quite sure yet how it performs taint analysis, but it looks like it assigns constraints to the nodes / qualifiers to the variables and uses reaching definitions + fixed point to solve them to get a result? I would have to look more into reaching definitions to see if the following applies. If it does, we're on a good place to solve at least the first issue described here.In the example:
we would need to assign each assignment to
whatupa different qualifier,whatup1, andwhatup2. Then the value that reaches thereturniswhatup2which is not tainted. This will result in a correct analysis. This is equivalent to rewriting the program in Static Single Assignment (SSA).Although I agree with @KevinHock that code like the above is rare, this issue can materialize in different ways that are more plausible.
I'd argue those examples, specially the second one warrant trying to address this issue.
On the other hand, in the example:
we would need to add a path condition to each constraint. That will allow pyt to solve for those constraints that have compatible path conditions (e.g.
foo.startswith('h')). In this particular case, I don't think pyt can solvefoo.startswith('h')iffoois a source, so it should probably report an issue with a lower confidence level. In certain circumstances it may be possible to tell whether the path condition is true and compatible with other paths along the code. There's a video from University of Maryland that covers this very concisely: https://www.youtube.com/watch?v=UhY8_Wb40B0Again sorry if I'm being captain obvious or too naive. I'm just trying to understand how pyt works and finding areas I can contribute to and I got to this open issue.
KevinHock commentedon Nov 22, 2018
You're definitely right about everything you just said @adrianbn! Thank you for contributing to and being interested in PyT :)
It turns out that PyT currently is flow-sensitive, and I didn't update this GitHub issue, my bad! 🤐 Here is where the vulnerability finding happens, I initially thought I fixed this when I added def-use chains, in this PR, but I put a print statement after
if tainted_node_in_sink_arg:and it never got executed, so it must have been fixed before that. I might have fixed some bug in the reaching definitions code a long time ago and forgot exactly what.Repro: I re-read what I had written in this issue and thought, "That seems weird, we should kill the previous definition? Maybe it is specific to return values of functions." I went to that part of the code, and figured I'd run it on an example to sanity check.
Before changing, we run:
After changing, let's run:
So that's good, maybe this is specific to return values, let me try my example.
Okay, so that's good, I should have updated this Github issue.
Comment out the overwriting assignment:
and we get
💥
KevinHock commentedon Nov 22, 2018
I'm not certain what constraints/qualifiers mean, but the we mostly do reaching definitions + fixed point. I wrote some documentation in this readme, but let me know if you'd like me to clarify anything :D
We sort of kind of do that with the following:
pyt/pyt/vulnerabilities/vulnerabilities.py
Lines 431 to 432 in 4b495ad
pyt/pyt/vulnerabilities/vulnerabilities.py
Lines 311 to 312 in 4b495ad
pyt/tests/vulnerabilities/vulnerabilities_test.py
Line 283 in 33dedd4
No worries! I'm glad you are interested @adrianbn 😁 One area you could contribute to, although it's rather difficult, is that we currently use inlining instead of summaries, for inter-procedural analysis, which makes PyT slower than it needs to be.
Here are some videos, specifically the last one, explains function summaries well:
#57 Call Graphs
#58 Interprocedural Data Flow Analysis
#59 Procedure Summaries in Data Flow Analysis
Feel free to ping me with any questions. My current open PR tries to address certain expressions, so we can handle
BoolOp's andIfExp's etc., I think you've motivated me to work on it / clean it up 👍 (It sort of became like a big chapter in a book I need a long plane ride to finish or something.)KevinHock commentedon Nov 22, 2018
We could try to turn Python code into SSA, and prune it via liveness, and add predicates so it's PSSA, and try to solve those predicates with e.g.
z3str3etc. or something similar. But handling all node types, being performant, and being battle tested (i.e. no recursion bugs or crashes), is higher priority to me, b/c I want it to be usable and run-able on tons of codebases.KevinHock commentedon Nov 24, 2018
I'm gonna go ahead and close this issue 👍 b/c it was solved before.