Skip to content

Conversation

MathiasVP
Copy link
Contributor

While working on #6825 I discovered some problems with the tests in the annotate_path_to_sink directory.

The tests in this folder are designed to give us a way to test the path explanations generated for dataflow queries by giving us the ability to annotate path components from ast or ir dataflow queries.

To construct the tainted elements in the path path, the test defines tainted roughly like this:

irTaint(source, tainted) and
(
  isSink(tainted)
  or
  exists(Element sink |
    isSink(sink) and
    DefaultTaintTracking::tainted(tainted, sink)
  )
)

this almost works: a tainted value is either a sink, or it's something that ends up in a sink. However, since the first argument of DefaultTaintTracking::tainted is restricted to being user input sources only, the second case only gives us the case where tainted is the source.

This PR provides an attempt at fixing this (for the IR test only as the security.TaintTracking library doesn't have a concept of a path): We use the edges predicate from DefaultTaintTracking (which again uses the edges predicate from the shared dataflow library) to construct the path.

In addition, I've also changed the annotations to distinguish whether the dataflow library considered a path component a sink or not - this is especially important now that we actually realize the whole path in the test.

@MathiasVP MathiasVP added the C++ label Nov 3, 2021
@MathiasVP MathiasVP requested a review from a team as a code owner November 3, 2021 20:32
Copy link
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

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

Great, LGTM!

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.

2 participants