Skip to content

Conversation

MathiasVP
Copy link
Contributor

@MathiasVP MathiasVP commented Jan 27, 2020

Testing for potential taint differences involving virtual dispatch between DefaultTaintTracking and security.TaintTracking. A couple of tests reveal TPs that are reported in the AST, but not in the IR.

ggolawski and others added 21 commits January 3, 2020 21:52
This adds support for arg-to-arg and arg-to-return taint.
Fix the help according to review comments.
When building SSA, we'll be assuming that stack variables do not escape, at least until we improve our alias analysis. I've added a new `IREscapeAnalysisConfiguration` class to allow the query to control this, and a new `UseSoundEscapeAnalysis.qll` module that can be imported to switch to the sound escape analysis. I've cloned the existing IR and SSA tests to have both sound and unsound versions. There were relatively few diffs in the IR dump tests, and the sanity tests still give the same results after one change described below.

Assuming that stack variables do not escape exposed an existing bug where we do not emit an `Uninitialized` instruction for the temporary variables used by `return` statements and `throw` expressions, even if the initializer is a constructor call or array initializer. I've refactored the code for handling elements that initialize a variable to share a common base class. I added a test case for returning an object initialized by constructor call, and ensured that the IR diffs for the existing `throw` test cases are correct.
There was already a `WriteSideEffectInstruction` class that served as a
superclass for all the specific write side effects. This new class
serves the same purpose for read side effects.
Until we have better tracking of indirections, these flow rules conflate
pointers and their contents.
Fix help and correct formatting.
Our definition of `toString` for the internal tuple objects we create during the
points-to analysis may have been a _tad_ too ambitious. In particular, it can
easily lead to non-termination, e.g. using the following piece of code:

```python
x = ()
while True:
    x = (x, x)
```

This commit cuts off the infinite recursion by replacing _nested_ tuples with
the string "...". In particular this means even non-recursive tuples will be cut
off at that point, so that the following tuples

```python
(1, "2")
((3, 4), [5, 6])
(1, 2, 3, 4, 5)
```

Get the following string representations.

```
"(int 1, '2', )"
"(..., List, )"
"(int 1, int 2, int 3, 2 more...)"
```
@MathiasVP MathiasVP added the C++ label Jan 27, 2020
@MathiasVP MathiasVP marked this pull request as ready for review January 27, 2020 16:34
@MathiasVP MathiasVP requested a review from a team as a code owner January 27, 2020 16:34
Copy link
Contributor

@jbj jbj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise LGTM.

)
}

from Location source, Location sink, string note
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why compare the sources and sinks by Location rather than comparing them as Expr and Element respectively? If the two libraries find elements that are different, then I think we want to know about it even if they have the same location.

@MathiasVP MathiasVP requested review from felicitymay and a team as code owners January 28, 2020 08:46
@jbj
Copy link
Contributor

jbj commented Jan 28, 2020

Something went wrong with the merge from master...

@MathiasVP
Copy link
Contributor Author

Something went wrong with the merge from master...

Oops... Yep. I'll fix it right away

@MathiasVP MathiasVP closed this Jan 28, 2020
@MathiasVP MathiasVP deleted the ql-tests-taint-tracking branch January 28, 2020 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants