Merge pull request #15988 from MathiasVP/clean-up-missing-check-scanf

C++: Rewrite 'cpp/missing-check-scanf' to use standard dataflow configs

Author: MathiasVP

cpp/ql/src/Critical/MissingCheckScanf.ql

@@ -21,14 +21,6 @@ import semmle.code.cpp.ir.IR
 import semmle.code.cpp.ir.ValueNumbering
 import ScanfChecks
 
-/** Holds if `n` reaches an argument  to a call to a `scanf`-like function. */
-pragma[nomagic]
-predicate revFlow0(Node n) {
-  isSink(_, _, n, _)
-  or
-  exists(Node succ | revFlow0(succ) | localFlowStep(n, succ))
-}
-
 /**
  * Holds if `n` represents an uninitialized stack-allocated variable, or a
  * newly (and presumed uninitialized) heap allocation.
@@ -38,30 +30,45 @@ predicate isUninitialized(Node n) {
   n.asIndirectExpr(1) instanceof AllocationExpr
 }
 
-pragma[nomagic]
-predicate fwdFlow0(Node n) {
-  revFlow0(n) and
-  (
-    isUninitialized(n)
-    or
-    exists(Node prev |
-      fwdFlow0(prev) and
-      localFlowStep(prev, n)
-    )
-  )
-}
-
 predicate isSink(ScanfFunctionCall call, int index, Node n, Expr input) {
   input = call.getOutputArgument(index) and
   n.asIndirectExpr() = input
 }
 
+/**
+ * A configuration to track a uninitialized data flowing to a `scanf`-like
+ * output parameter position.
+ *
+ * This is meant to be a simple flow to rule out cases like:
+ * ```
+ * int x = 0;
+ * scanf(..., &x);
+ * use(x);
+ * ```
+ * since `x` is already initialized it's not a security concern that `x` is
+ * used without checking the return value of `scanf`.
+ *
+ * Since this flow is meant to be simple, we disable field flow and require the
+ * source and the sink to be in the same callable.
+ */
+module UninitializedToScanfConfig implements ConfigSig {
+  predicate isSource(Node source) { isUninitialized(source) }
+
+  predicate isSink(Node sink) { isSink(_, _, sink, _) }
+
+  FlowFeature getAFeature() { result instanceof FeatureEqualSourceSinkCallContext }
+
+  int accessPathLimit() { result = 0 }
+}
+
+module UninitializedToScanfFlow = Global<UninitializedToScanfConfig>;
+
 /**
  * Holds if `call` is a `scanf`-like call and `output` is the `index`'th
  * argument that has not been previously initialized.
  */
 predicate isRelevantScanfCall(ScanfFunctionCall call, int index, Expr output) {
-  exists(Node n | fwdFlow0(n) and isSink(call, index, n, output)) and
+  exists(Node n | UninitializedToScanfFlow::flowTo(n) and isSink(call, index, n, output)) and
   // Exclude results from incorrectky checked scanf query
   not incorrectlyCheckedScanf(call)
 }
@@ -77,31 +84,6 @@ predicate isSource(ScanfFunctionCall call, int index, Node n, Expr output) {
   n.asDefiningArgument() = output
 }
 
-/**
- * Holds if `n` is reachable from an output argument of a relevant call to
- * a `scanf`-like function.
- */
-pragma[nomagic]
-predicate fwdFlow(Node n) {
-  isSource(_, _, n, _)
-  or
-  exists(Node prev |
-    fwdFlow(prev) and
-    localFlowStep(prev, n) and
-    not isSanitizerOut(prev)
-  )
-}
-
-/** Holds if `n` should not have outgoing flow. */
-predicate isSanitizerOut(Node n) {
-  // We disable flow out of sinks to reduce result duplication
-  isSink(n, _)
-  or
-  // If the node is being passed to a function it may be
-  // modified, and thus it's safe to later read the value.
-  exists(n.asIndirectArgument())
-}
-
 /**
  * Holds if `n` is a node such that `n.asExpr() = e` and `e` is not an
  * argument of a deallocation expression.
@@ -112,39 +94,33 @@ predicate isSink(Node n, Expr e) {
 }
 
 /**
- * Holds if `n` is part of a path from a call to a `scanf`-like function
- * to a use of the written variable.
+ * A configuration to track flow from the output argument of a call to a
+ * `scanf`-like function, and to a use of the defined variable.
  */
-pragma[nomagic]
-predicate revFlow(Node n) {
-  fwdFlow(n) and
-  (
+module ScanfToUseConfig implements ConfigSig {
+  predicate isSource(Node source) { isSource(_, _, source, _) }
+
+  predicate isSink(Node sink) { isSink(sink, _) }
+
+  predicate isBarrierOut(Node n) {
+    // We disable flow out of sinks to reduce result duplication
     isSink(n, _)
     or
-    exists(Node succ |
-      revFlow(succ) and
-      localFlowStep(n, succ) and
-      not isSanitizerOut(n)
-    )
-  )
-}
-
-/** A local flow step, restricted to relevant dataflow nodes. */
-private predicate step(Node n1, Node n2) {
-  revFlow(n1) and
-  revFlow(n2) and
-  localFlowStep(n1, n2)
+    // If the node is being passed to a function it may be
+    // modified, and thus it's safe to later read the value.
+    exists(n.asIndirectArgument())
+  }
 }
 
-predicate hasFlow(Node n1, Node n2) = fastTC(step/2)(n1, n2)
+module ScanfToUseFlow = Global<ScanfToUseConfig>;
 
 /**
  * Holds if `source` is the `index`'th argument to the `scanf`-like call `call`, and `sink` is
  * a dataflow node that represents the expression `e`.
  */
 predicate hasFlow(Node source, ScanfFunctionCall call, int index, Node sink, Expr e) {
   isSource(call, index, source, _) and
-  hasFlow(source, sink) and
+  ScanfToUseFlow::flow(source, sink) and
   isSink(sink, e)
 }