C++: Rewrite cpp/tainted-permissions-check to not use DefaultTaintTracking

Author: jketema

cpp/ql/src/Security/CWE/CWE-807/TaintedCondition.ql

@@ -12,30 +12,83 @@
  *       external/cwe/cwe-807
  */
 
-import semmle.code.cpp.ir.dataflow.internal.DefaultTaintTrackingImpl
-import TaintedWithPath
+import cpp
+import semmle.code.cpp.security.Security
+import semmle.code.cpp.security.FlowSources
+import semmle.code.cpp.ir.dataflow.TaintTracking
+import semmle.code.cpp.ir.IR
+import Flow::PathGraph
+
+Expr getExprWithoutNot(Expr expr) {
+  result = expr and not expr instanceof NotExpr
+  or
+  result = getExprWithoutNot(expr.(NotExpr).getOperand()) and expr instanceof NotExpr
+}
 
 predicate sensitiveCondition(Expr condition, Expr raise) {
   raisesPrivilege(raise) and
   exists(IfStmt ifstmt |
-    ifstmt.getCondition() = condition and
+    getExprWithoutNot(ifstmt.getCondition()) = condition and
     raise.getEnclosingStmt().getParentStmt*() = ifstmt
   )
 }
 
-class Configuration extends TaintTrackingConfiguration {
-  override predicate isSink(Element tainted) { sensitiveCondition(tainted, _) }
+private predicate constantInstruction(Instruction instr) {
+  instr instanceof ConstantInstruction
+  or
+  instr instanceof StringConstantInstruction
+  or
+  constantInstruction(instr.(UnaryInstruction).getUnary())
+}
+
+predicate isSource(FlowSource source, string sourceType) { sourceType = source.getSourceType() }
+
+module Config implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node node) { isSource(node, _) }
+
+  predicate isSink(DataFlow::Node node) {
+    sensitiveCondition([node.asExpr(), node.asIndirectExpr()], _)
+  }
+
+  predicate isBarrier(DataFlow::Node node) {
+    // Block flow into binary instructions if both operands are non-constant
+    exists(BinaryInstruction iTo |
+      iTo = node.asInstruction() and
+      not constantInstruction(iTo.getLeft()) and
+      not constantInstruction(iTo.getRight()) and
+      // propagate taint from either the pointer or the offset, regardless of constant-ness
+      not iTo instanceof PointerArithmeticInstruction
+    )
+    or
+    // Block flow through calls to pure functions if two or more operands are non-constant
+    exists(Instruction iFrom1, Instruction iFrom2, CallInstruction iTo |
+      iTo = node.asInstruction() and
+      isPureFunction(iTo.getStaticCallTarget().getName()) and
+      iFrom1 = iTo.getAnArgument() and
+      iFrom2 = iTo.getAnArgument() and
+      not constantInstruction(iFrom1) and
+      not constantInstruction(iFrom2) and
+      iFrom1 != iFrom2
+    )
+  }
 }
 
+module Flow = TaintTracking::Global<Config>;
+
 /*
  * Produce an alert if there is an 'if' statement whose condition `condition`
  * is influenced by tainted data `source`, and the body contains
  * `raise` which escalates privilege.
  */
 
-from Expr source, Expr condition, Expr raise, PathNode sourceNode, PathNode sinkNode
+from
+  Expr raise, string sourceType, DataFlow::Node source, DataFlow::Node sink,
+  Flow::PathNode sourceNode, Flow::PathNode sinkNode
 where
-  taintedWithPath(source, condition, sourceNode, sinkNode) and
-  sensitiveCondition(condition, raise)
-select condition, sourceNode, sinkNode, "Reliance on untrusted input $@ to raise privilege at $@.",
-  source, source.toString(), raise, raise.toString()
+  source = sourceNode.getNode() and
+  sink = sinkNode.getNode() and
+  isSource(source, sourceType) and
+  sensitiveCondition([sink.asExpr(), sink.asIndirectExpr()], raise) and
+  Flow::flowPath(sourceNode, sinkNode)
+select sink, sourceNode, sinkNode, "Reliance on $@ to raise privilege at $@.", source, sourceType,
+  raise, raise.toString()

cpp/ql/test/query-tests/Security/CWE/CWE-807/semmle/TaintedCondition/TaintedCondition.expected

@@ -1,13 +1,11 @@
 edges
-| test.cpp:20:29:20:34 | call to getenv | test.cpp:24:10:24:35 | ! ... |
-| test.cpp:20:29:20:34 | call to getenv | test.cpp:24:11:24:16 | call to strcmp |
-| test.cpp:20:29:20:47 | call to getenv | test.cpp:24:10:24:35 | ! ... |
 | test.cpp:20:29:20:47 | call to getenv | test.cpp:24:11:24:16 | call to strcmp |
-subpaths
+| test.cpp:20:29:20:47 | call to getenv indirection | test.cpp:24:11:24:16 | call to strcmp |
 nodes
-| test.cpp:20:29:20:34 | call to getenv | semmle.label | call to getenv |
 | test.cpp:20:29:20:47 | call to getenv | semmle.label | call to getenv |
-| test.cpp:24:10:24:35 | ! ... | semmle.label | ! ... |
+| test.cpp:20:29:20:47 | call to getenv indirection | semmle.label | call to getenv indirection |
 | test.cpp:24:11:24:16 | call to strcmp | semmle.label | call to strcmp |
+subpaths
 #select
-| test.cpp:24:10:24:35 | ! ... | test.cpp:20:29:20:34 | call to getenv | test.cpp:24:10:24:35 | ! ... | Reliance on untrusted input $@ to raise privilege at $@. | test.cpp:20:29:20:34 | call to getenv | call to getenv | test.cpp:25:9:25:27 | ... = ... | ... = ... |
+| test.cpp:24:11:24:16 | call to strcmp | test.cpp:20:29:20:47 | call to getenv | test.cpp:24:11:24:16 | call to strcmp | Reliance on $@ to raise privilege at $@. | test.cpp:20:29:20:47 | call to getenv | an environment variable | test.cpp:25:9:25:27 | ... = ... | ... = ... |
+| test.cpp:24:11:24:16 | call to strcmp | test.cpp:20:29:20:47 | call to getenv indirection | test.cpp:24:11:24:16 | call to strcmp | Reliance on $@ to raise privilege at $@. | test.cpp:20:29:20:47 | call to getenv indirection | an environment variable | test.cpp:25:9:25:27 | ... = ... | ... = ... |