isAdditionalTaintStep does not work here

[CaledoniaProject]

I'm using this source code read.cpp.txt. The code first read the length of the buffer, then read arbitrary data into a fixed buffer.

I wrote the following query to find this pattern. And I need to connect the nodes processed by atoi method, but the following query yields nothing:

/**
 * @name test
 * @kind path-problem
 */

import cpp
import DataFlow::PathGraph
import semmle.code.cpp.dataflow.DataFlow
import semmle.code.cpp.dataflow.TaintTracking
import semmle.code.cpp.valuenumbering.GlobalValueNumbering

private class MyTaintConfig extends TaintTracking::Configuration {
  MyTaintConfig() { this = "MyTaintConfig" }

  override predicate isSource(DataFlow::Node source) {
    exists(FunctionCall fc |
      fc.getArgument(1) = source.asExpr()
      and fc.getTarget().hasGlobalOrStdName("read")
    )
  }

  override predicate isSink(DataFlow::Node sink) {
    exists(FunctionCall fc | 
      fc.getArgument(2) = sink.asExpr()
      and fc.getTarget().hasGlobalOrStdName("read")
    )
  }

  override predicate isAdditionalTaintStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
    exists(FunctionCall fc | 
      fc.getTarget().hasGlobalName("atoi")
      and fc.getArgument(0) = fromNode.asExpr()
      and fc = toNode.asExpr()
    )
  }
}

from DataFlow::PathNode source, DataFlow::PathNode sink, MyTaintConfig conf
where conf.hasFlowPath(source, sink)
select sink.getNode(), source, sink, "length controlled from $@.", source.getNode(), "this user input"

[smowton]

Replace your source definition with

  override predicate isSource(DataFlow::Node source) {
    exists(FunctionCall fc |
      fc.getArgument(1) = source.(DataFlow::PostUpdateNode).getPreUpdateNode().asExpr()
      and fc.getTarget().hasGlobalOrStdName("read")
    )
  }

Reason: the PostUpdateNode is created when an expression or statement can have a side-effect upon one of its arguments or operands. In this case there is one dataflow node representing buff flowing into the call to read and another one representing it flowing back out after any side-effect has taken place. It is this second node, the PostUpdateNode, that flows to a future use of buff, in this case the argument to atoi.

Similarly, if you had a function transfersTaint(char *from, char *to) { ... } then an isAdditionalTaintStep definition for it would need to use a post-update node (PUN) for the to argument but a normal, non-PUN node for from, representing the fact that taint is consumed from an argument passed in, and propagates to an argument flowing back out to subsequent users.

[CaledoniaProject]

Looks like I should always use PostUpdateNode, because it covers more cases than the DataFlow::Node, am I right?

[smowton]

They don't always exist: for Java they exist for:

• the e in e[idx] = v and e.field = v
• Any arg a1 ... an of mutable type (i.e., not a primitive, String, Void or a boxed type) in qualifier.call(a1, a2, ..., an)
• Any this in this.call(...) or this.field = ...
• Similarly any implicit this qualifier in call(...) or field = v

So you should only use them in the context of an expression that may experience a side-effect.