C++: Pull in the latest version of TaintedPath.ql from CodeQL

c/cert/src/rules/FIO32-C/DoNotPerformFileOperationsOnDevices.ql

@@ -15,8 +15,9 @@ import cpp
 import codingstandards.c.cert
 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
 
 // Query TaintedPath.ql from the CodeQL standard library
 /**
@@ -45,20 +46,93 @@ 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
   not isExcluded(taintedArg, IO3Package::doNotPerformFileOperationsOnDevicesQuery()) and
+  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,
+  "This argument to a file access function is derived from $@ and then passed to " + callChain + ".",
   taintSource, "user input (" + taintCause + ")"

c/cert/test/rules/FIO32-C/DoNotPerformFileOperationsOnDevices.expected

@@ -1,36 +1,16 @@
 edges
-| test.c:20:15:20:23 | array to pointer conversion | test.c:21:8:21:16 | (const char *)... |
-| test.c:20:15:20:23 | array to pointer conversion | test.c:21:8:21:16 | file_name |
-| test.c:20:15:20:23 | array to pointer conversion | test.c:21:8:21:16 | file_name indirection |
-| test.c:20:15:20:23 | file_name | test.c:21:8:21:16 | (const char *)... |
-| test.c:20:15:20:23 | file_name | test.c:21:8:21:16 | file_name |
 | test.c:20:15:20:23 | file_name | test.c:21:8:21:16 | file_name indirection |
-| test.c:20:15:20:23 | scanf output argument | test.c:21:8:21:16 | (const char *)... |
-| test.c:20:15:20:23 | scanf output argument | test.c:21:8:21:16 | file_name |
 | test.c:20:15:20:23 | scanf output argument | test.c:21:8:21:16 | file_name indirection |
-| test.c:45:15:45:23 | array to pointer conversion | test.c:46:29:46:37 | (LPCTSTR)... |
-| test.c:45:15:45:23 | array to pointer conversion | test.c:46:29:46:37 | file_name |
-| test.c:45:15:45:23 | array to pointer conversion | test.c:46:29:46:37 | file_name indirection |
-| test.c:45:15:45:23 | file_name | test.c:46:29:46:37 | (LPCTSTR)... |
-| test.c:45:15:45:23 | file_name | test.c:46:29:46:37 | file_name |
 | test.c:45:15:45:23 | file_name | test.c:46:29:46:37 | file_name indirection |
-| test.c:45:15:45:23 | scanf output argument | test.c:46:29:46:37 | (LPCTSTR)... |
-| test.c:45:15:45:23 | scanf output argument | test.c:46:29:46:37 | file_name |
 | test.c:45:15:45:23 | scanf output argument | test.c:46:29:46:37 | file_name indirection |
-subpaths
 nodes
-| test.c:20:15:20:23 | array to pointer conversion | semmle.label | array to pointer conversion |
 | test.c:20:15:20:23 | file_name | semmle.label | file_name |
 | test.c:20:15:20:23 | scanf output argument | semmle.label | scanf output argument |
-| test.c:21:8:21:16 | (const char *)... | semmle.label | (const char *)... |
-| test.c:21:8:21:16 | file_name | semmle.label | file_name |
 | test.c:21:8:21:16 | file_name indirection | semmle.label | file_name indirection |
-| test.c:45:15:45:23 | array to pointer conversion | semmle.label | array to pointer conversion |
 | test.c:45:15:45:23 | file_name | semmle.label | file_name |
 | test.c:45:15:45:23 | scanf output argument | semmle.label | scanf output argument |
-| test.c:46:29:46:37 | (LPCTSTR)... | semmle.label | (LPCTSTR)... |
-| test.c:46:29:46:37 | file_name | semmle.label | file_name |
 | test.c:46:29:46:37 | file_name indirection | semmle.label | file_name indirection |
+subpaths
 #select
-| test.c:21:8:21:16 | file_name | test.c:20:15:20:23 | file_name | test.c:21:8:21:16 | file_name | This argument to a file access function is derived from $@ and then passed to func(file_name), which calls fopen((unnamed parameter 0)) | test.c:20:15:20:23 | file_name | user input (scanf) |
-| test.c:46:29:46:37 | file_name | test.c:45:15:45:23 | file_name | test.c:46:29:46:37 | file_name | This argument to a file access function is derived from $@ and then passed to CreateFile(lpFileName) | test.c:45:15:45:23 | file_name | user input (scanf) |
+| test.c:21:8:21:16 | file_name | test.c:20:15:20:23 | file_name | test.c:21:8:21:16 | file_name indirection | This argument to a file access function is derived from $@ and then passed to func(file_name), which calls fopen((unnamed parameter 0)). | test.c:20:15:20:23 | file_name | user input (scanf) |
+| test.c:46:29:46:37 | file_name | test.c:45:15:45:23 | file_name | test.c:46:29:46:37 | file_name indirection | This argument to a file access function is derived from $@ and then passed to CreateFile(lpFileName). | test.c:45:15:45:23 | file_name | user input (scanf) |

change_notes/2022-12-06-remove-use-of-default-taint-tracking.md

@@ -0,0 +1,2 @@
+ - `FIO32-C` - `DoNotPerformFileOperationsOnDevices.ql`:
+   - The query was rewritten to no longer depend of the `DefaultTaintTracking` library, which will be deprecated.