Java: Prevent accidental recursion through AdditionalValueStep.

Author: aschackmull

java/ql/lib/semmle/code/java/dataflow/internal/DataFlowUtil.qll

@@ -100,14 +100,15 @@ predicate hasNonlocalValue(FieldRead fr) {
 /**
  * Holds if data can flow from `node1` to `node2` in one local step.
  */
+cached
 predicate localFlowStep(Node node1, Node node2) {
-  simpleLocalFlowStep(node1, node2)
+  simpleLocalFlowStep0(node1, node2)
   or
   adjacentUseUse(node1.asExpr(), node2.asExpr())
   or
   // Simple flow through library code is included in the exposed local
   // step relation, even though flow is technically inter-procedural
-  FlowSummaryImpl::Private::Steps::summaryThroughStep(node1, node2, true)
+  FlowSummaryImpl::Private::Steps::summaryThroughStepValue(node1, node2)
 }
 
 /**
@@ -118,6 +119,16 @@ predicate localFlowStep(Node node1, Node node2) {
  */
 cached
 predicate simpleLocalFlowStep(Node node1, Node node2) {
+  simpleLocalFlowStep0(node1, node2)
+  or
+  any(AdditionalValueStep a).step(node1, node2) and
+  pragma[only_bind_out](node1.getEnclosingCallable()) =
+    pragma[only_bind_out](node2.getEnclosingCallable()) and
+  // prevent recursive call
+  (any() or strictcount(Node n1, Node n2, AdditionalValueStep a | a.step(n1, n2)) < 0)
+}
+
+private predicate simpleLocalFlowStep0(Node node1, Node node2) {
   TaintTrackingUtil::forceCachingInSameStage() and
   // Variable flow steps through adjacent def-use and use-use pairs.
   exists(SsaExplicitUpdate upd |
@@ -166,10 +177,6 @@ predicate simpleLocalFlowStep(Node node1, Node node2) {
   )
   or
   FlowSummaryImpl::Private::Steps::summaryLocalStep(node1, node2, true)
-  or
-  any(AdditionalValueStep a).step(node1, node2) and
-  pragma[only_bind_out](node1.getEnclosingCallable()) =
-    pragma[only_bind_out](node2.getEnclosingCallable())
 }
 
 private newtype TContent =

java/ql/lib/semmle/code/java/dataflow/internal/FlowSummaryImpl.qll

@@ -791,15 +791,30 @@ module Private {
     }
 
     /**
-     * Holds if `arg` flows to `out` using a simple flow summary, that is, a flow
-     * summary without reads and stores.
+     * Holds if `arg` flows to `out` using a simple value-preserving flow
+     * summary, that is, a flow summary without reads and stores.
      *
      * NOTE: This step should not be used in global data-flow/taint-tracking, but may
      * be useful to include in the exposed local data-flow/taint-tracking relations.
      */
-    predicate summaryThroughStep(ArgNode arg, Node out, boolean preservesValue) {
+    predicate summaryThroughStepValue(ArgNode arg, Node out) {
+      exists(ReturnKind rk, ReturnNode ret, DataFlowCall call |
+        summaryLocalStep(summaryArgParam0(call, arg), ret, true) and
+        ret.getKind() = rk and
+        out = getAnOutNode(call, rk)
+      )
+    }
+
+    /**
+     * Holds if `arg` flows to `out` using a simple flow summary involving taint
+     * step, that is, a flow summary without reads and stores.
+     *
+     * NOTE: This step should not be used in global data-flow/taint-tracking, but may
+     * be useful to include in the exposed local data-flow/taint-tracking relations.
+     */
+    predicate summaryThroughStepTaint(ArgNode arg, Node out) {
       exists(ReturnNodeExt ret |
-        summaryLocalStep(summaryArgParam(arg, ret, out), ret, preservesValue)
+        summaryLocalStep(summaryArgParam(arg, ret, out), ret, false)
       )
     }
 

java/ql/lib/semmle/code/java/dataflow/internal/TaintTrackingUtil.qll

@@ -51,7 +51,7 @@ private module Cached {
     or
     // Simple flow through library code is included in the exposed local
     // step relation, even though flow is technically inter-procedural
-    FlowSummaryImpl::Private::Steps::summaryThroughStep(src, sink, false)
+    FlowSummaryImpl::Private::Steps::summaryThroughStepTaint(src, sink)
     or
     // Treat container flow as taint for the local taint flow relation
     exists(DataFlow::Content c | containerContent(c) |