Understanding C/C++ taint tracking in the presence of aliasing

[thinkmoore]

Hi, I'm trying to get a better understanding of how taint tracking in CodeQL handles aliasing. I adapted the following query and example from the CWE-022 query in the repository.

/**
 * @name Uncontrolled data used in path expression
 * @description Accessing paths influenced by users can allow an
 *              attacker to access unexpected resources.
 * @kind path-problem
 * @problem.severity warning
 * @security-severity 7.5
 * @precision medium
 * @id cpp/path-injection
 * @tags security
 *       external/cwe/cwe-022
 *       external/cwe/cwe-023
 *       external/cwe/cwe-036
 *       external/cwe/cwe-073
 */

import cpp
import semmle.code.cpp.security.FunctionWithWrappers
import semmle.code.cpp.security.Security
import semmle.code.cpp.security.TaintTracking
import TaintedWithPath

/**
 * A function for opening a file.
 */
class FileFunction extends FunctionWithWrappers {
  FileFunction() {
    exists(string nme | this.hasGlobalName(nme) |
      nme = ["fopen", "_fopen", "_wfopen", "open", "_open", "_wopen", "opendir", "mkdir", "chdir", "rename"]
      or
      // create file function on windows
      nme.matches("CreateFile%")
    )
    or
    this.hasQualifiedName("std", "fopen")
    or
    // on any of the fstream classes, or filebuf
    exists(string nme | this.getDeclaringType().hasQualifiedName("std", nme) |
      nme = ["basic_fstream", "basic_ifstream", "basic_ofstream", "basic_filebuf"]
    ) and
    // we look for either the open method or the constructor
    (this.getName() = "open" or this instanceof Constructor)
  }

  // conveniently, all of these functions take the path as the first parameter!
  override predicate interestingArg(int arg) { arg = 0 }
}

class TaintedPathConfiguration extends TaintTrackingConfiguration {
  override predicate isSink(Element tainted) {
    exists(FileFunction fileFunction | fileFunction.outermostWrapperFunctionCall(tainted, _))
  }

  override predicate taintThroughGlobals() {
    any()
  }
}

from
  FileFunction fileFunction, Expr taintedArg, Expr taintSource, PathNode sourceNode,
  PathNode sinkNode, string taintCause, string callChain
where
  fileFunction.outermostWrapperFunctionCall(taintedArg, callChain) and
  taintedWithPath(taintSource, taintedArg, sourceNode, sinkNode) and
  isUserInput(taintSource, taintCause)
select taintedArg, sourceNode, sinkNode,
  "This argument to a file access function is derived from $@ and then passed to " + callChain,
  taintSource, "user input (" + taintCause + ")"

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

void getinput(char *buf, char* userAndFile) {
    size_t len = strlen(buf);
    strncat(buf+len, userAndFile, FILENAME_MAX-len-1);
}

int main(int argc, char** argv) {
  char *userAndFile = argv[2];
  
  {
    char fileBuffer[FILENAME_MAX] = "/home/";
    char otherBuffer[FILENAME_MAX] = "/home/default";
    char *input = fileBuffer;
    if (argc > 2) {
      input = otherBuffer;
    }
    getinput(input, userAndFile);
    fopen(otherBuffer, "wb+");
  }

  {
    char fileBuffer2[FILENAME_MAX] = "/home/";
    char *input2 = fileBuffer2;
    getinput(input2, userAndFile);
    fopen(fileBuffer2, "wb+");
  }

  {
    char fileBuffer3[FILENAME_MAX] = "/home/";
    char otherBuffer3[FILENAME_MAX] = "/home/default";
    char *input3 = fileBuffer3;
    if (argc > 2) {
      input3 = otherBuffer3;
    }
    size_t len3 = strlen(input3);
    strncat(input3+len3, userAndFile, FILENAME_MAX-len3-1);
    fopen(otherBuffer3, "wb+");
  }

  {
    char fileBuffer4[FILENAME_MAX] = "/home/";
    char *input4 = fileBuffer4;
    size_t len4 = strlen(input4);
    strncat(input4+len4, userAndFile, FILENAME_MAX-len4-1);
    fopen(fileBuffer4, "wb+");
  }
}

I expect this query to return four results, one for each sub-example in the file. However, only the final example involving fileBuffer4 is found. That result is also confusing as it reports two paths: one with two nodes argv and fileBuffer4 and one that additionally includes parameters and the return value of getinput, which is not called in that part of the example.

Does the query as I have written it make sense? Is this kind of pointer aliasing outside of what the taint tracking module is intended to reason about?

Thanks!

[edoardopirovano]

Greetings! Thanks for getting in touch with this question. The query as you have written it certainly makes sense, and it's a pity that our taint tracking isn't smart enough to find all the vulnerable cases in your example. A few preliminary notes:

• The semmle.code.cpp.ir.dataflow.TaintTracking is a newer interface for the taint tracking library, and would probably be a better choice then semmle.code.cpp.security.TaintTracking (which still exists for backwards compatibility) when writing a new query. I've done that rewrite below - note this doesn't make a material difference to any of your other issues.
• In the first two cases, we don't appear to be tracking flow through getinput. This needs some investigation but may be a bug in our dataflow tracking.
• In the third case, we don't seem to be figuring out that otherBuffer3 and input3 might be the same. Indeed this would be a good thing to observe for taint tracking, but the IR dataflow library that underlies our security taint tracking may have conflicting priorities here, so this isn't necessarily something we would be able to easily change.

I've passed this on the the C analysis team who can hopefully give you some deeper answers here on why you are observing this and whether there is anything we could improve.

cc. @geoffw0 @rdmarsh2

Rewrite to use semmle.code.cpp.ir.dataflow.TaintTracking

/**
 * @name Uncontrolled data used in path expression
 * @description Accessing paths influenced by users can allow an
 *              attacker to access unexpected resources.
 * @kind path-problem
 * @problem.severity warning
 * @security-severity 7.5
 * @precision medium
 * @id cpp/path-injection
 * @tags security
 *       external/cwe/cwe-022
 *       external/cwe/cwe-023
 *       external/cwe/cwe-036
 *       external/cwe/cwe-073
 */

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

/**
 * A function for opening a file.
 */
class FileFunction extends FunctionWithWrappers {
  FileFunction() {
    exists(string nme | this.hasGlobalName(nme) |
      nme =
        [
          "fopen", "_fopen", "_wfopen", "open", "_open", "_wopen", "opendir", "mkdir", "chdir",
          "rename"
        ]
      or
      // create file function on windows
      nme.matches("CreateFile%")
    )
    or
    this.hasQualifiedName("std", "fopen")
    or
    // on any of the fstream classes, or filebuf
    exists(string nme | this.getDeclaringType().hasQualifiedName("std", nme) |
      nme = ["basic_fstream", "basic_ifstream", "basic_ofstream", "basic_filebuf"]
    ) and
    // we look for either the open method or the constructor
    (this.getName() = "open" or this instanceof Constructor)
  }

  // conveniently, all of these functions take the path as the first parameter!
  override predicate interestingArg(int arg) { arg = 0 }
}

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

  override predicate isSource(DataFlow::Node source) { isUserInput(source.asExpr(), _) }

  override predicate isSink(DataFlow::Node tainted) {
    exists(FileFunction fileFunction |
      fileFunction.outermostWrapperFunctionCall(tainted.asExpr(), _)
    )
  }
}

from DataFlow::PathNode source, DataFlow::PathNode sink, TaintedPathConfiguration config
where config.hasFlowPath(source, sink)
select sink.getNode(), source, sink, "User input used in file path."

[thinkmoore]

Thanks for the pointer to semmle.code.cpp.ir.dataflow.TaintTracking @edoardopirovano. Look forward to hearing from the C analysis team.