Can't un-taint nodes with in place string modification

[CaledoniaProject]

I'm testing codeql with this source code: bug.cpp.txt
And I created this query:

/**
 * @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(Parameter p, Function f |
        source.asParameter() = p
        and p.hasName("argv")
        and f.hasName("main")
        and p.getFunction() = f
    )
  }

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

  override predicate isSanitizer(DataFlow::Node node) {
    exists(FunctionCall fc |
      node.asExpr() = fc.getAnArgument()
      and
      (
        fc.getTarget().hasGlobalOrStdName("clean_data")
        or
        fc.getTarget().hasGlobalOrStdName("clean_data_2")
      )
    )
  }
}

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

isSanitizer does not work for clean_data, and no_cmdi_1 is a false positive.
How can I fix it?

[MathiasVP]

Hi @CaledoniaProject,

This is a common issue for C/C++ dataflow analysis. It's a bit unfortunate that the no_cmdi_1 case doesn't work out of the box. Let me start by posting a sanizier that does work correct for your cases:

override predicate isSanitizer(DataFlow::Node node) {
  exists(FunctionCall fc |
    // The function call is always executed before `node`
    dominates(fc, node.asExpr()) and
    (
      // clean_data sanitizes the argument.
      // So we mark any use future use of the argument as a sanitizer.
      fc.getTarget().hasGlobalOrStdName("clean_data") and
      globalValueNumber(node.asExpr()).getAnExpr() = fc.getAnArgument()
      or
      // clean_data sanitizes the return value.
      // So we mark any use future use of the return value as a sanitizer.
      fc.getTarget().hasGlobalOrStdName("clean_data_2") and
      globalValueNumber(node.asExpr()).getAnExpr() = fc
    )
  )
}

Consider the flow in no_cmdi_1. The reason we have to use dominates is that, when we track taint, we find a direct path from the input parameter to the use of input in snprintf without checking if there are any sanitizers in between the parameter and its use. To overcome this, we mark any expression as a sanitizer when:

1. It has a value that is identical to the argument that goes into clean_data, and
2. it is dominated by the call to clean_data (that is, clean_data is guaranteed to be executed before the expression).

That takes care of the clean_data case. The clean_data_2 case is almost the same, except that we mark any dominated use of an expression that is identical to the return value of clean_data_2.

This should hopefully remove the two false positives you're seeing. I hope that helps!

[MathiasVP]

Note that dominates might not perform well on larger databases. If you want better performance you need to use bbDominates (which operator on basic blocks instead of control flow nodes). The following should almost be correct: bbDominates(fc.getBasicBlock(), node.asExpr().getBasicBlock()), except that you need to ensure that, if fc.getBasicBlock() is equal to node.asExpr().getBasicBlock() (i.e., the clean_data(x) and x are in the same block), then you need to ensure that clean_data(x) happens at an index that's smaller than x. This case can be handled via something like:

exists(BasicBlock bb, int i |
  bb.getNode(i) = fc and // `clean_data(x)` is the i'th entry in the basic block `bb`
  bb.getNode(j) = node.asExpr() and // and `x` is the j'th entry in the same basic block
  i < j // and `clean_data(x)` happens before `x`
)