Merge pull request #6767 from aschackmull/dataflow/callback-postupdate

Dataflow: Support side-effects for callbacks in summaries.

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

@@ -186,10 +186,17 @@ module Private {
     TArgumentSummaryComponent(int i) { parameterPosition(i) } or
     TReturnSummaryComponent(ReturnKind rk)
 
+  private TSummaryComponent thisParam() {
+    result = TParameterSummaryComponent(instanceParameterPosition())
+  }
+
   newtype TSummaryComponentStack =
     TSingletonSummaryComponentStack(SummaryComponent c) or
     TConsSummaryComponentStack(SummaryComponent head, SummaryComponentStack tail) {
       tail.(RequiredSummaryComponentStack).required(head)
+      or
+      tail.(RequiredSummaryComponentStack).required(TParameterSummaryComponent(_)) and
+      head = thisParam()
     }
 
   pragma[nomagic]
@@ -198,21 +205,63 @@ module Private {
     boolean preservesValue
   ) {
     c.propagatesFlow(input, output, preservesValue)
+    or
+    // observe side effects of callbacks on input arguments
+    c.propagatesFlow(output, input, preservesValue) and
+    preservesValue = true and
+    isCallbackParameter(input) and
+    isContentOfArgument(output)
+    or
+    // flow from the receiver of a callback into the instance-parameter
+    exists(SummaryComponentStack s, SummaryComponentStack callbackRef |
+      c.propagatesFlow(s, _, _) or c.propagatesFlow(_, s, _)
+    |
+      callbackRef = s.drop(_) and
+      (isCallbackParameter(callbackRef) or callbackRef.head() = TReturnSummaryComponent(_)) and
+      input = callbackRef.tail() and
+      output = TConsSummaryComponentStack(thisParam(), input) and
+      preservesValue = true
+    )
+  }
+
+  private predicate isCallbackParameter(SummaryComponentStack s) {
+    s.head() = TParameterSummaryComponent(_) and exists(s.tail())
+  }
+
+  private predicate isContentOfArgument(SummaryComponentStack s) {
+    s.head() = TContentSummaryComponent(_) and isContentOfArgument(s.tail())
+    or
+    s = TSingletonSummaryComponentStack(TArgumentSummaryComponent(_))
+  }
+
+  private predicate outputState(SummarizedCallable c, SummaryComponentStack s) {
+    summary(c, _, s, _)
+    or
+    exists(SummaryComponentStack out |
+      outputState(c, out) and
+      out.head() = TContentSummaryComponent(_) and
+      s = out.tail()
+    )
+    or
+    // Add the argument node corresponding to the requested post-update node
+    inputState(c, s) and isCallbackParameter(s)
+  }
+
+  private predicate inputState(SummarizedCallable c, SummaryComponentStack s) {
+    summary(c, s, _, _)
+    or
+    exists(SummaryComponentStack inp | inputState(c, inp) and s = inp.tail())
+    or
+    exists(SummaryComponentStack out |
+      outputState(c, out) and
+      out.head() = TParameterSummaryComponent(_) and
+      s = out.tail()
+    )
   }
 
   private newtype TSummaryNodeState =
-    TSummaryNodeInputState(SummaryComponentStack s) {
-      exists(SummaryComponentStack input |
-        summary(_, input, _, _) and
-        s = input.drop(_)
-      )
-    } or
-    TSummaryNodeOutputState(SummaryComponentStack s) {
-      exists(SummaryComponentStack output |
-        summary(_, _, output, _) and
-        s = output.drop(_)
-      )
-    }
+    TSummaryNodeInputState(SummaryComponentStack s) { inputState(_, s) } or
+    TSummaryNodeOutputState(SummaryComponentStack s) { outputState(_, s) }
 
   /**
    * A state used to break up (complex) flow summaries into atomic flow steps.
@@ -238,20 +287,14 @@ module Private {
     pragma[nomagic]
     predicate isInputState(SummarizedCallable c, SummaryComponentStack s) {
       this = TSummaryNodeInputState(s) and
-      exists(SummaryComponentStack input |
-        summary(c, input, _, _) and
-        s = input.drop(_)
-      )
+      inputState(c, s)
     }
 
     /** Holds if this state is a valid output state for `c`. */
     pragma[nomagic]
     predicate isOutputState(SummarizedCallable c, SummaryComponentStack s) {
       this = TSummaryNodeOutputState(s) and
-      exists(SummaryComponentStack output |
-        summary(c, _, output, _) and
-        s = output.drop(_)
-      )
+      outputState(c, s)
     }
 
     /** Gets a textual representation of this state. */
@@ -331,19 +374,12 @@ module Private {
     receiver = summaryNodeInputState(c, s.drop(1))
   }
 
-  private Node pre(Node post) {
-    summaryPostUpdateNode(post, result)
-    or
-    not summaryPostUpdateNode(post, _) and
-    result = post
-  }
-
   private predicate callbackInput(
     SummarizedCallable c, SummaryComponentStack s, Node receiver, int i
   ) {
     any(SummaryNodeState state).isOutputState(c, s) and
     s.head() = TParameterSummaryComponent(i) and
-    receiver = pre(summaryNodeOutputState(c, s.drop(1)))
+    receiver = summaryNodeInputState(c, s.drop(1))
   }
 
   /** Holds if a call targeting `receiver` should be synthesized inside `c`. */
@@ -395,7 +431,7 @@ module Private {
         or
         exists(int i | head = TParameterSummaryComponent(i) |
           result =
-            getCallbackParameterType(getNodeType(summaryNodeOutputState(pragma[only_bind_out](c),
+            getCallbackParameterType(getNodeType(summaryNodeInputState(pragma[only_bind_out](c),
                   s.drop(1))), i)
         )
       )
@@ -421,10 +457,16 @@ module Private {
   }
 
   /** Holds if summary node `post` is a post-update node with pre-update node `pre`. */
-  predicate summaryPostUpdateNode(Node post, ParamNode pre) {
+  predicate summaryPostUpdateNode(Node post, Node pre) {
     exists(SummarizedCallable c, int i |
       isParameterPostUpdate(post, c, i) and
-      pre.isParameterOf(c, i)
+      pre.(ParamNode).isParameterOf(c, i)
+    )
+    or
+    exists(SummarizedCallable callable, SummaryComponentStack s |
+      callbackInput(callable, s, _, _) and
+      pre = summaryNodeOutputState(callable, s) and
+      post = summaryNodeInputState(callable, s)
     )
   }
 
@@ -462,7 +504,11 @@ module Private {
       // for `StringBuilder.append(x)` with a specified value flow from qualifier to
       // return value and taint flow from argument 0 to the qualifier, then this
       // allows us to infer taint flow from argument 0 to the return value.
-      summaryPostUpdateNode(pred, succ) and preservesValue = true
+      succ instanceof ParamNode and summaryPostUpdateNode(pred, succ) and preservesValue = true
+      or
+      // Similarly we would like to chain together summaries where values get passed
+      // into callbacks along the way.
+      pred instanceof ArgNode and summaryPostUpdateNode(succ, pred) and preservesValue = true
     }
 
     /**

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

@@ -16,6 +16,9 @@ private module FlowSummaries {
 /** Holds is `i` is a valid parameter position. */
 predicate parameterPosition(int i) { i in [-1 .. any(Parameter p).getPosition()] }
 
+/** Gets the parameter position of the instance parameter. */
+int instanceParameterPosition() { result = -1 }
+
 /** Gets the synthesized summary data-flow node for the given values. */
 Node summaryNode(SummarizedCallable c, SummaryNodeState state) { result = getSummaryNode(c, state) }
 
@@ -37,6 +40,8 @@ DataFlowType getReturnType(SummarizedCallable c, ReturnKind rk) {
  */
 DataFlowType getCallbackParameterType(DataFlowType t, int i) {
   result = getErasedRepr(t.(FunctionalInterface).getRunMethod().getParameterType(i))
+  or
+  result = getErasedRepr(t.(FunctionalInterface)) and i = -1
 }
 
 /**

java/ql/test/library-tests/dataflow/callback-dispatch/A.java

@@ -1,5 +1,7 @@
 package my.callback.qltest;
 
+import java.util.*;
+
 public class A {
   public interface Consumer1 {
     void eat(Object o);
@@ -28,6 +30,20 @@ public class A {
     // con.eat(x);
   }
 
+  static <T> T applyConsumer3_ret_postup(Consumer3<T> con) {
+    // summary:
+    // x = new T();
+    // con.eat(x);
+    // return x;
+    return null;
+  }
+
+  static <T> void forEach(T[] xs, Consumer3<T> con) {
+    // summary:
+    // x = xs[..];
+    // con.eat(x);
+  }
+
   public interface Producer1<T> {
     T make();
   }
@@ -38,6 +54,14 @@ public class A {
     return null;
   }
 
+  static <T> T produceConsume(Producer1<T> prod, Consumer3<T> con) {
+    // summary:
+    // x = prod.make();
+    // con.eat(x);
+    // return x;
+    return null;
+  }
+
   public interface Converter1<T1,T2> {
     T2 conv(T1 x);
   }
@@ -109,5 +133,40 @@ public class A {
     };
     applyConsumer3(new Integer[] { (Integer)source(12) }, pc);
     sink(applyProducer1(pc)[0]); // $ flow=11
+
+    Integer res = applyProducer1(new Producer1<Integer>() {
+      private Integer ii = (Integer)source(13);
+      @Override public Integer make() {
+        return this.ii;
+      }
+    });
+    sink(res); // $ flow=13
+
+    ArrayList<Object> list = new ArrayList<>();
+    applyConsumer3(list, l -> l.add(source(14)));
+    sink(list.get(0)); // $ flow=14
+
+    Consumer3<ArrayList<Object>> tainter = l -> l.add(source(15));
+    sink(applyConsumer3_ret_postup(tainter).get(0)); // $ flow=15
+
+    forEach(new Object[] {source(16)}, x -> sink(x)); // $ flow=16
+
+    // Spurious flow from 17 is reasonable as it would likely
+    // also occur if the lambda body was inlined in a for loop.
+    // It occurs from the combination of being able to observe
+    // the side-effect of the callback on the other argument and
+    // being able to chain summaries that update/read arguments,
+    // e.g. fluent apis.
+    // Spurious flow from 18 is due to not matching call targets
+    // in a return-from-call-to-enter-call flow sequence.
+    forEach(new Object[2][], xs -> { sink(xs[0]); xs[0] = source(17); }); // $ SPURIOUS: flow=17 flow=18
+
+    Object[][] xss = new Object[][] { { null } };
+    forEach(xss, x -> {x[0] = source(18);});
+    sink(xss[0][0]); // $ flow=18
+
+    Object res2 = produceConsume(() -> source(19), A::sink); // $ flow=19
+    sink(res2); // $ flow=19
   }
+
 }

java/ql/test/library-tests/dataflow/callback-dispatch/test.ql

@@ -10,7 +10,11 @@ class SummaryModelTest extends SummaryModelCsv {
         "my.callback.qltest;A;false;applyConsumer1;(Object,Consumer1);;Argument[0];Parameter[0] of Argument[1];value",
         "my.callback.qltest;A;false;applyConsumer2;(Object,Consumer2);;Argument[0];Parameter[0] of Argument[1];value",
         "my.callback.qltest;A;false;applyConsumer3;(Object,Consumer3);;Argument[0];Parameter[0] of Argument[1];value",
+        "my.callback.qltest;A;false;applyConsumer3_ret_postup;(Consumer3);;Parameter[0] of Argument[0];ReturnValue;value",
+        "my.callback.qltest;A;false;forEach;(Object[],Consumer3);;ArrayElement of Argument[0];Parameter[0] of Argument[1];value",
         "my.callback.qltest;A;false;applyProducer1;(Producer1);;ReturnValue of Argument[0];ReturnValue;value",
+        "my.callback.qltest;A;false;produceConsume;(Producer1,Consumer3);;ReturnValue of Argument[0];Parameter[0] of Argument[1];value",
+        "my.callback.qltest;A;false;produceConsume;(Producer1,Consumer3);;Parameter[0] of Argument[1];ReturnValue;value",
         "my.callback.qltest;A;false;applyConverter1;(Object,Converter1);;Argument[0];Parameter[0] of Argument[1];value",
         "my.callback.qltest;A;false;applyConverter1;(Object,Converter1);;ReturnValue of Argument[1];ReturnValue;value"
       ]