C#: Update all security queries to path-problems

[calumgrant]

Converts all of the security queries to type path-problem. Update the expected qltest output.

In most cases, the change has simply involved changing DataFlow::Node to DataFlow::PathNode, but there are a few cases where this has not been done. For example, when there would be semantic changes, or if the sources and sinks have additional member predicates. In this case, the predicate Node::getPathNode is used instead.

Do not merge this yet - we still need to validate the performance.

[hvitved]

Many tests still need to have their expected output updated. I generally prefer the style used by e.g. https://lgtm.com/query/rule:1506493930005/lang:cpp/ (see comment), but let me know what you think.

[hvitved]

I think I would prefer

from TaintTrackingConfiguration c, DataFlow::PathNode source, DataFlow::PathNode sink
where c.hasFlowPath(source, sink)
select sink, source, sink, "$@ flows to here and is used in a path.", source, "User-provided value"

[aschackmull]

Agree.

[aschackmull]

Or actually I think this might be slightly better:

from TaintTrackingConfiguration c, DataFlow::PathNode source, DataFlow::PathNode sink
where c.hasFlowPath(source, sink)
select sink.getNode(), source, sink, "$@ flows to here and is used in a path.", source.getNode(), "User-provided value"

[hvitved]

Why? Isn't toString() and getLocation() on DataFlow::Node and DataFlow::PathNode the same?

[aschackmull]

toString() is different when you have field flow, as the PathNode will include the accesspath.

[calumgrant]

It seems like there's a choice between

from Configuration c, DataFlow::Node source, DataFlow::Node sink
where c.hasFlow(source, sink)
select sink, source.getPathNode(c), sink.getPathNode(c),
  "$@ could flow here.", source, "User input"

vs

from Configuration c, DataFlow::PathNode source, DataFlow::PathNode sink
where c.hasFlowPath(source, sink)
select sink.getNode(), source, sink,
  "$@ could flow here.", source.getNode(), "User input"

The second is actually longer. Either way, this mapping to and fro seems somewhat unsatisfactory. I have a slight preference for the first version actually, because

• The query is actually dealing with dataflow nodes. Paths are an implementation detail.
• You often use a specific type of source and sink, not DataFlow::Node. Often the source and sink have additional predicates relating to information about the source and the sink. PathNodes don't capture any of this.
• You could forget to call getNode().

Anyway, this sounds like something to be discussed offline in the CPH office.

[hvitved]

I think those are some convincing arguments, @calumgrant!

[aschackmull]

You're missing an important argument for using PathNode. There are potentially many PathNodes corresponding to a single Node (e.g. if there are multiple configurations in scope with overlapping sources or sinks, which can easily happen), and in that case using getPathNode to get back to the PathNodes becomes semantically shaky, as you'll risk multiplying your results.

[calumgrant]

Yes but getPathNode(c) ensures that only nodes of configuration c are used in the path. Therefore I think the risk of multiplication -- which only occurs in the edges relation -- is the same in either case.

[hvitved]

Still, you may get a pair of path nodes for which there isn't actually flow, as guaranteed by c.hasFlowPath(). So, unless the engine filters out results in the select predicate, for which the edges predicate does not produce a path from the select predicate's second to third column, I think we need to use the other approach.

[hvitved]

Should be source.getNode().toString().

[hvitved]

use c.hasFlowPath(sourcePath, sinkPath) instead.

[calumgrant]

No, in this case it gives different results due to hackery around sources and sinks. The configuration's hasFlow has been changed, and hasFlow isn't final so I guess it's allowed.

[aschackmull]

It's probably best to avoid such hacks, as it's not very clear what's going on. It would be much clearer to just define the negationbody from the overridden hasFlow as a separate member predicate, which could then be explicitly anti-joined here.

[xiemaisi]

Drive-by comment: I was reminded today that our analysis toolchain assumes that each node selected by a path query appears in the edges relation unless an explicit nodes relation is provided. So in particular if you have an empty edges relation, any query results (which might well exist, since the path from source to sink could be empty) are discarded.

Are you guarding against that case somehow? (I think the easiest workaround is to define a nodes query relation in PathGraph.)

[aschackmull]

Drive-by comment: I was reminded today that our analysis toolchain assumes that each node selected by a path query appears in the edges relation unless an explicit nodes relation is provided. So in particular if you have an empty edges relation, any query results (which might well exist, since the path from source to sink could be empty) are discarded.

Are you guarding against that case somehow? (I think the easiest workaround is to define a nodes query relation in PathGraph.)

I don't know the exact structure of PathNodes in C#, but for java the PathNodes corresponding to sinks are special, so if a source is also a sink such that the Node path is the empty path then the corresponding PathNode path in the edges relation will have length 1.

[xiemaisi]

OK, glad to hear that you're already handling this for Java. Does the workaround of creating paths of length 1 have any additional benefits over explicitly specifying nodes (e.g., are there other parts of the toolchain that assume paths have at least length one)?

[aschackmull]

OK, glad to hear that you're already handling this for Java. Does the workaround of creating paths of length 1 have any additional benefits over explicitly specifying nodes (e.g., are there other parts of the toolchain that assume paths have at least length one)?

The main reason for modelling sinks explicitly was AFAIR to throw away the call context on the final node in a path in order to avoid potential issues with duplicated results (if a sink was reachable from multiple call contexts). I'm not sure about what the tool chain assumptions re. path length are, if any.

[hvitved]

@calumgrant : Happy to merge after you have added the nodes predicate suggested by @xiemaisi and fixed the tests.