C++: Rewrite cpp/path-injection to not use DefaultTaintTracking

cpp/ql/src/Security/CWE/CWE-022/TaintedPath.ql

@@ -17,8 +17,9 @@
 import cpp
 import semmle.code.cpp.security.FunctionWithWrappers
 import semmle.code.cpp.security.Security
-import semmle.code.cpp.security.TaintTracking
-import TaintedWithPath
+import semmle.code.cpp.ir.IR
+import semmle.code.cpp.ir.dataflow.TaintTracking
+import DataFlow::PathGraph
 
 /**
  * A function for opening a file.
@@ -46,18 +47,91 @@ class FileFunction extends FunctionWithWrappers {
   override predicate interestingArg(int arg) { arg = 0 }
 }
 
-class TaintedPathConfiguration extends TaintTrackingConfiguration {
-  override predicate isSink(Element tainted) {
-    exists(FileFunction fileFunction | fileFunction.outermostWrapperFunctionCall(tainted, _))
+Expr asSourceExpr(DataFlow::Node node) {
+  result = node.asConvertedExpr()
+  or
+  result = node.asDefiningArgument()
+}
+
+Expr asSinkExpr(DataFlow::Node node) {
+  result =
+    node.asOperand()
+        .(SideEffectOperand)
+        .getUse()
+        .(ReadSideEffectInstruction)
+        .getArgumentDef()
+        .getUnconvertedResultExpression()
+}
+
+/**
+ * Holds for a variable that has any kind of upper-bound check anywhere in the program.
+ * This is biased towards being inclusive and being a coarse overapproximation because
+ * there are a lot of valid ways of doing an upper bounds checks if we don't consider
+ * where it occurs, for example:
+ * ```cpp
+ *   if (x < 10) { sink(x); }
+ *
+ *   if (10 > y) { sink(y); }
+ *
+ *   if (z > 10) { z = 10; }
+ *   sink(z);
+ * ```
+ */
+predicate hasUpperBoundsCheck(Variable var) {
+  exists(RelationalOperation oper, VariableAccess access |
+    oper.getAnOperand() = access and
+    access.getTarget() = var and
+    // Comparing to 0 is not an upper bound check
+    not oper.getAnOperand().getValue() = "0"
+  )
+}
+
+class TaintedPathConfiguration extends TaintTracking::Configuration {
+  TaintedPathConfiguration() { this = "TaintedPathConfiguration" }
+
+  override predicate isSource(DataFlow::Node node) { isUserInput(asSourceExpr(node), _) }
+
+  override predicate isSink(DataFlow::Node node) {
+    exists(FileFunction fileFunction |
+      fileFunction.outermostWrapperFunctionCall(asSinkExpr(node), _)
+    )
+  }
+
+  override predicate isSanitizerIn(DataFlow::Node node) { this.isSource(node) }
+
+  override predicate isSanitizer(DataFlow::Node node) {
+    node.asExpr().(Call).getTarget().getUnspecifiedType() instanceof ArithmeticType
+    or
+    exists(LoadInstruction load, Variable checkedVar |
+      load = node.asInstruction() and
+      checkedVar = load.getSourceAddress().(VariableAddressInstruction).getAstVariable() and
+      hasUpperBoundsCheck(checkedVar)
+    )
+  }
+
+  predicate hasFilteredFlowPath(DataFlow::PathNode source, DataFlow::PathNode sink) {
+    this.hasFlowPath(source, sink) and
+    // The use of `isUserInput` in `isSink` in combination with `asSourceExpr` causes
+    // duplicate results. Filter these duplicates. The proper solution is to switch to
+    // using `LocalFlowSource` and `RemoteFlowSource`, but this currently only supports
+    // a subset of the cases supported by `isUserInput`.
+    not exists(DataFlow::PathNode source2 |
+      this.hasFlowPath(source2, sink) and
+      asSourceExpr(source.getNode()) = asSourceExpr(source2.getNode())
+    |
+      not exists(source.getNode().asConvertedExpr()) and exists(source2.getNode().asConvertedExpr())
+    )
   }
 }
 
 from
-  FileFunction fileFunction, Expr taintedArg, Expr taintSource, PathNode sourceNode,
-  PathNode sinkNode, string taintCause, string callChain
+  FileFunction fileFunction, Expr taintedArg, Expr taintSource, TaintedPathConfiguration cfg,
+  DataFlow::PathNode sourceNode, DataFlow::PathNode sinkNode, string taintCause, string callChain
 where
+  taintedArg = asSinkExpr(sinkNode.getNode()) and
   fileFunction.outermostWrapperFunctionCall(taintedArg, callChain) and
-  taintedWithPath(taintSource, taintedArg, sourceNode, sinkNode) and
+  cfg.hasFilteredFlowPath(sourceNode, sinkNode) and
+  taintSource = asSourceExpr(sourceNode.getNode()) and
   isUserInput(taintSource, taintCause)
 select taintedArg, sourceNode, sinkNode,
   "This argument to a file access function is derived from $@ and then passed to " + callChain + ".",

cpp/ql/test/query-tests/Security/CWE/CWE-022/SAMATE/TaintedPath/TaintedPath.expected

@@ -1,19 +1,8 @@
 edges
-| CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:55:27:55:38 | ... + ... | CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:77:23:77:26 | (const char *)... |
-| CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:55:27:55:38 | ... + ... | CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:77:23:77:26 | data |
-| CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:55:27:55:38 | ... + ... | CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:77:23:77:26 | data |
-| CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:55:27:55:38 | ... + ... | CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:77:23:77:26 | data indirection |
-| CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:55:27:55:38 | fgets output argument | CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:77:23:77:26 | (const char *)... |
-| CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:55:27:55:38 | fgets output argument | CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:77:23:77:26 | data |
-| CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:55:27:55:38 | fgets output argument | CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:77:23:77:26 | data |
 | CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:55:27:55:38 | fgets output argument | CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:77:23:77:26 | data indirection |
-subpaths
 nodes
-| CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:55:27:55:38 | ... + ... | semmle.label | ... + ... |
 | CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:55:27:55:38 | fgets output argument | semmle.label | fgets output argument |
-| CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:77:23:77:26 | (const char *)... | semmle.label | (const char *)... |
-| CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:77:23:77:26 | data | semmle.label | data |
-| CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:77:23:77:26 | data | semmle.label | data |
 | CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:77:23:77:26 | data indirection | semmle.label | data indirection |
+subpaths
 #select
-| CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:77:23:77:26 | data | CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:55:27:55:38 | ... + ... | CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:77:23:77:26 | data | This argument to a file access function is derived from $@ and then passed to fopen(filename). | CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:55:27:55:38 | ... + ... | user input (fgets) |
+| CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:77:23:77:26 | data | CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:55:27:55:38 | fgets output argument | CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:77:23:77:26 | data indirection | This argument to a file access function is derived from $@ and then passed to fopen(filename). | CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:55:27:55:38 | ... + ... | user input (fgets) |

cpp/ql/test/query-tests/Security/CWE/CWE-022/semmle/tests/TaintedPath.expected

@@ -1,19 +1,24 @@
 edges
-| test.c:9:23:9:26 | argv | test.c:17:11:17:18 | (const char *)... |
-| test.c:9:23:9:26 | argv | test.c:17:11:17:18 | (const char *)... |
-| test.c:9:23:9:26 | argv | test.c:17:11:17:18 | fileName |
-| test.c:9:23:9:26 | argv | test.c:17:11:17:18 | fileName |
-| test.c:9:23:9:26 | argv | test.c:17:11:17:18 | fileName |
-| test.c:9:23:9:26 | argv | test.c:17:11:17:18 | fileName |
 | test.c:9:23:9:26 | argv | test.c:17:11:17:18 | fileName indirection |
-| test.c:9:23:9:26 | argv | test.c:17:11:17:18 | fileName indirection |
-subpaths
+| test.c:31:22:31:25 | argv | test.c:32:11:32:18 | fileName indirection |
+| test.c:37:17:37:24 | fileName | test.c:38:11:38:18 | fileName indirection |
+| test.c:37:17:37:24 | scanf output argument | test.c:38:11:38:18 | fileName indirection |
+| test.c:43:17:43:24 | fileName | test.c:44:11:44:18 | fileName indirection |
+| test.c:43:17:43:24 | scanf output argument | test.c:44:11:44:18 | fileName indirection |
 nodes
 | test.c:9:23:9:26 | argv | semmle.label | argv |
-| test.c:9:23:9:26 | argv | semmle.label | argv |
-| test.c:17:11:17:18 | (const char *)... | semmle.label | (const char *)... |
-| test.c:17:11:17:18 | fileName | semmle.label | fileName |
-| test.c:17:11:17:18 | fileName | semmle.label | fileName |
 | test.c:17:11:17:18 | fileName indirection | semmle.label | fileName indirection |
+| test.c:31:22:31:25 | argv | semmle.label | argv |
+| test.c:32:11:32:18 | fileName indirection | semmle.label | fileName indirection |
+| test.c:37:17:37:24 | fileName | semmle.label | fileName |
+| test.c:37:17:37:24 | scanf output argument | semmle.label | scanf output argument |
+| test.c:38:11:38:18 | fileName indirection | semmle.label | fileName indirection |
+| test.c:43:17:43:24 | fileName | semmle.label | fileName |
+| test.c:43:17:43:24 | scanf output argument | semmle.label | scanf output argument |
+| test.c:44:11:44:18 | fileName indirection | semmle.label | fileName indirection |
+subpaths
 #select
-| test.c:17:11:17:18 | fileName | test.c:9:23:9:26 | argv | test.c:17:11:17:18 | fileName | This argument to a file access function is derived from $@ and then passed to fopen(filename). | test.c:9:23:9:26 | argv | user input (argv) |
+| test.c:17:11:17:18 | fileName | test.c:9:23:9:26 | argv | test.c:17:11:17:18 | fileName indirection | This argument to a file access function is derived from $@ and then passed to fopen(filename). | test.c:9:23:9:26 | argv | user input (argv) |
+| test.c:32:11:32:18 | fileName | test.c:31:22:31:25 | argv | test.c:32:11:32:18 | fileName indirection | This argument to a file access function is derived from $@ and then passed to fopen(filename). | test.c:31:22:31:25 | argv | user input (argv) |
+| test.c:38:11:38:18 | fileName | test.c:37:17:37:24 | fileName | test.c:38:11:38:18 | fileName indirection | This argument to a file access function is derived from $@ and then passed to fopen(filename). | test.c:37:17:37:24 | fileName | user input (scanf) |
+| test.c:44:11:44:18 | fileName | test.c:43:17:43:24 | fileName | test.c:44:11:44:18 | fileName indirection | This argument to a file access function is derived from $@ and then passed to fopen(filename). | test.c:43:17:43:24 | fileName | user input (scanf) |

cpp/ql/test/query-tests/Security/CWE/CWE-022/semmle/tests/stdlib.h

@@ -11,3 +11,7 @@ FILE *fopen(const char *filename, const char *mode);
 int sprintf(char *s, const char *format, ...);
 size_t strlen(const char *s);
 char *strncat(char *s1, const char *s2, size_t n);
+int scanf(const char *format, ...);
+void *malloc(size_t size);
+double strtod(const char *ptr, char **endptr);
+char *getenv(const char *name);

cpp/ql/test/query-tests/Security/CWE/CWE-022/semmle/tests/test.c

@@ -26,5 +26,38 @@ int main(int argc, char** argv) {
     strncat(fileName+len, fixed, FILENAME_MAX-len-1);
     fopen(fileName, "wb+");
   }
+
+  {
+    char *fileName = argv[1];
+    fopen(fileName, "wb+"); // BAD
+  }
+
+  {
+    char fileName[20];
+    scanf("%s", fileName);
+    fopen(fileName, "wb+"); // BAD
+  }
+
+  {
+    char *fileName = (char*)malloc(20 * sizeof(char));
+    scanf("%s", fileName);
+    fopen(fileName, "wb+"); // BAD
+  }
+
+  {
+    char *aNumber = getenv("A_NUMBER");
+    double number = strtod(aNumber, 0);
+    char fileName[20];
+    sprintf(fileName, "/foo/%f", number);
+    fopen(fileName, "wb+"); // GOOD
+  }
+
+  {
+    void read(const char *fileName);
+    read(argv[1]); // BAD [NOT DETECTED]
+  }
 }
 
+void read(char *fileName) {
+  fopen(fileName, "wb+");
+}