Hi @mrlzh,

This is because CodeQL doesn't view the call to operator= as a definition that is guaranteed to redefine the content of newline, and so it will sometimes jump over such calls. To solve this, you can mark any uses of newline that is dominated by a call to operator= as a sanitizer, and your solution should work as expected. Here's an example:

/**
 * @name test
 * @kind path-problem
 * @problem.severity error
 * @precision high
 * @id cpp/test
 * @tags security
 *       directory-traversal-vulnerability
 */

import cpp
import semmle.code.cpp.dataflow.TaintTracking
import semmle.code.cpp.security.Security
import DataFlow::PathGraph

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

  override predicate isSource(DataFlow::Node source) {
    exists(FunctionCall func |
      func.getTarget().getName() = "set_inarg" and
      func.getArgument(0) = source.asExpr()
    )
  }

  override predicate isSink(DataFlow::Node node) {
    exists(FunctionCall func |
      func.getTarget().getName() = "fopen" and
      node.asExpr() = func.getArgument(0)
    )
  }

  override predicate isSanitizer(DataFlow::Node node) {
    exists(DataFlow::Node n |
      // This is _almost_ the sanitizer you wrote originally. See its definition below
      isDefinedByOperatorAssign(n) and
      // And I added this.
      dominates(n.asDefiningArgument(), node.asExpr())
    )
  }
}

predicate isDefinedByOperatorAssign(DataFlow::Node node) {
  exists(FunctionCall fc |
    fc.getTarget().getName() = "operator=" and // Note: This is `operator=` and not `operator+`.
    fc.getQualifier() = node.asDefiningArgument()
  )
}

from DataFlow::PathNode source, DataFlow::PathNode sink, DirectoryTraversalTaint config
where config.hasFlowPath(source, sink)
select sink.getNode(), source, sink, "DirectoryTraversal vulnerability"

When I run this locally I get 0 results as expected.

Thanks for @MathiasVP
I tried your example but it didn't work.
Here is a new demo.

Path1 jumps from line 40 to line 44.I can not feel the existence of 'newline' and 'newline2'.
Path3 miss 'newline' and 'operator+'.

Path result miss so many key PathNode. Is it possible to fix?
Is there a better way to block flow through code 'newline + ".jpg"' and 'newline += ".jpg"'?

We payed for codeql enterprise and used for discover vulnerabilities during CI. These problems lead to a large number of false positives which bring trouble to push vulnerability fixing.

Hi @mrlzh,

My solution worked in dofileop_safe2 because we made a sanitizer involving operator=, but the code in dofileop_safe3 involves a call to call to a constructor which we haven't marked as any kind of sanitizer. You can probably prevent both of these false positives (i.e., the one in dofileop_safe2 and in dofileop_safe3) with this sanitizer:

class DirectoryTraversalTaint extends TaintTracking::Configuration {
  DirectoryTraversalTaint() { this = "DirectoryTraversalTaint" }
  
  // ... as before

  override predicate isSanitizer(DataFlow::Node node) {
    exists(DataFlow::Node n |
      isResultOfOperatorPlus(n) and
      dominates(n.asExpr(), node.asExpr())
    )
  }
}

predicate isResultOfOperatorPlus(DataFlow::Node node) {
  exists(FunctionCall fc |
    fc.getTarget().hasName("operator+") and
    node.asExpr() = fc
  )
}

That is, making any result of operator+ as a sanitizer. Depending on your exact codebase you could combine this with the operator= solution I wrote in my first response if you still see some false positives.

Hi @MathiasVP
Code ' newline + ".jpg" ' is safe and ' "/path/" + newline ' is not safe.
However this sanitizer blocks both flows.

override predicate isSanitizer(DataFlow::Node node) {
    exists(DataFlow::Node n |
      isResultOfOperatorPlus(n) and
      dominates(n.asExpr(), node.asExpr())
    )
  }

My complete demo:

#include <stdio.h>
#include <string>

class DemoReq{
    public:
        std::string value;

    void set_inarg(const std::string& value){
        this->value = value;
    }

    std::string& inarg(){
        return this->value;
    }
};

void dofileop_safe1(DemoReq& req)
{
    std::string newline = req.inarg();
    newline += ".jpg";

    FILE *fp;
    fp = fopen(newline.c_str(), "r");
    fclose(fp);
}

void dofileop_safe2(DemoReq& req)
{
    std::string newline = req.inarg();
    newline = "/xxx/" + newline + ".jpg";

    FILE *fp;
    fp = fopen(newline.c_str(), "r");
    fclose(fp);
}

void dofileop_safe3(DemoReq& req)
{
    std::string newline = req.inarg();
    std::string newline2 = "/xxx/" + newline + ".jpg";

    FILE *fp;
    fp = fopen(newline2.c_str(), "r");
    fclose(fp);
}

void dofileop_unsafe(DemoReq& req)
{
    std::string newline = req.inarg();
    std::string newline2 = "/xxx/" + newline;

    FILE *fp;
    fp = fopen(newline2.c_str(), "r");
    fclose(fp);
}

int main(){
    DemoReq req;
    req.set_inarg("hello");
    dofileop_safe1(req);
    dofileop_safe2(req);
    dofileop_safe3(req);
    dofileop_unsafe(req);
    return 0;
}

On why there are multiple paths and some path miss key PathNode.
Normal result:

After delete class StdStringCStr from cpp/ql/lib/semmle/code/cpp/models/implementations/StdString.qll.
It remains one path.So I thought, some Models definition lead to codeql analyze multiple paths.Function 'hasTaintFlow' lead to miss key PathNode.
Any way to fix?

On why there are multiple paths and some path miss key PathNode.

Two things are worth pointing out.

First: Why are there multiple paths?

In this case, we're finding two paths because we're not realizing that the line:

newline += ".jpg";

will always unconditionally modify the value of newline (remember that newline += ".jpg" is really syntactic sugar for something like newline.operator+=(".jpg") which modifies the value of newline because it's passed by reference). So we're finding a path that involves flow through the call to operator+=, and another path that skips the call because it assumes the value isn't unconditionally modified.

This shouldn't be a problem for your analysis results, though. If we block flow using something like the isSanitizer predicate I used (that uses dominates) we'll specify that flow is blocked if a call to operator+= is part of the path.

Second: The missing PathNodes

I'm not sure what you mean by some paths missing a PathNode. Do you mean that:

• We're not detecting the flow?
• We're detecting the flow, but the path skips over some parts of the code that you'd expect to be shown in the path?

If it's the second case, that's expected. We "prune" the graph as part of the implementation for performance reasons (and to prevent excessively long paths).

Code ' newline + ".jpg" ' is safe and ' "/path/" + newline ' is not safe.
However this sanitizer blocks both flows.

Ah, I see. Sorry, I misunderstood the situation. So the code is only safe if a ".jpg" is appended? In that case, we can specify this using the following sanitizer:

class DirectoryTraversalTaint extends TaintTracking::Configuration {
  // ...

  override predicate isSanitizer(DataFlow::Node node) {
    exists(DataFlow::Node n, Expr e |
      hasAppendedJpg(n, e) and
      dominates(e, node.asExpr())
    )
  }
}

/**
 * Holds if `e` is an expression that's represented by `node`
 * and `node` is a value such that `.jpg` has been appended to it.
 */
predicate hasAppendedJpg(DataFlow::Node node, Expr e) {
  exists(FunctionCall fc |
    fc.getTarget().hasName("operator+") and
    node.asExpr() = fc and
    fc = e
    or
    fc.getTarget().hasName("operator+=") and
    node.asDefiningArgument() = fc.getQualifier() and
    e = fc.getQualifier()
  |
    fc.getAnArgument().getValue() = ".jpg"
  )
}

This sanitizer only reports a path that ends in the dofileop_unsafe function. Is that what you wanted?

reply:Why are there multiple paths?
It is not a problem for safety engineer to analyse results,but it is for developer.
Remove isSanitizer for this demo.You can see dofileop_unsafe alerted four paths.In fact,there are only one vulnerability.
I have find a case one vulnerability report over ten paths.This problem affects fix efficiency and vulnerability statistics

reply:Second: The missing PathNodes
I mean We're detecting the flow, but the path skips over some parts of the code.
How can I defind my 'graph pruned rule'? Codeql often skips important PathNodes and I want to optimize it.

reply: hasAppendedJpg demo
The code is only safe if a node append any string.A easier to understand ql like this.(It doesn't work)

//node is the first argument
// node + "xxx"
// ("xxx"+node) + "xxx"
// node + ("xxx"+"xxx")
exists(FunctionCall fc |
    fc.getTarget().hasName("operator+") and
    fc.getArgument(0)=node.asExpr()
)

Your isSanitizer ql demo realy work for some case.
For most demo:

safe case:
node+"any_string"
node+any_function()
"any_string"+node+"any_string"
("any_string"+node)+"any_string"
("any_string"+node)+any_function()
node+("any_string"+"any_string")

unsafe case:
node+node
"any_string"+node
any_function()+node
("any_string"+"any_string")+node

It is not a good idea by using dominates.