Merge pull request #6931 from hvitved/dataflow/restrict-derived-summaries

Data flow: Restrict derived flow summaries

Author: aschackmull

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

@@ -85,6 +85,9 @@ module Public {
     /** Holds if this stack contains summary component `c`. */
     predicate contains(SummaryComponent c) { c = this.drop(_).head() }
 
+    /** Gets the bottom element of this stack. */
+    SummaryComponent bottom() { result = this.drop(this.length() - 1).head() }
+
     /** Gets a textual representation of this stack. */
     string toString() {
       exists(SummaryComponent head, SummaryComponentStack tail |
@@ -197,6 +200,8 @@ module Private {
       or
       tail.(RequiredSummaryComponentStack).required(TParameterSummaryComponent(_)) and
       head = thisParam()
+      or
+      derivedFluentFlowPush(_, _, _, head, tail, _)
     }
 
   pragma[nomagic]
@@ -210,7 +215,7 @@ module Private {
     c.propagatesFlow(output, input, preservesValue) and
     preservesValue = true and
     isCallbackParameter(input) and
-    isContentOfArgument(output)
+    isContentOfArgument(output, _)
     or
     // flow from the receiver of a callback into the instance-parameter
     exists(SummaryComponentStack s, SummaryComponentStack callbackRef |
@@ -222,16 +227,81 @@ module Private {
       output = TConsSummaryComponentStack(thisParam(), input) and
       preservesValue = true
     )
+    or
+    exists(SummaryComponentStack arg, SummaryComponentStack return |
+      derivedFluentFlow(c, input, arg, return, preservesValue)
+    |
+      arg.length() = 1 and
+      output = return
+      or
+      exists(SummaryComponent head, SummaryComponentStack tail |
+        derivedFluentFlowPush(c, input, arg, head, tail, 0) and
+        output = SummaryComponentStack::push(head, tail)
+      )
+    )
+    or
+    // Chain together summaries where values get passed into callbacks along the way
+    exists(SummaryComponentStack mid, boolean preservesValue1, boolean preservesValue2 |
+      c.propagatesFlow(input, mid, preservesValue1) and
+      c.propagatesFlow(mid, output, preservesValue2) and
+      mid.drop(mid.length() - 2) =
+        SummaryComponentStack::push(TParameterSummaryComponent(_),
+          SummaryComponentStack::singleton(TArgumentSummaryComponent(_))) and
+      preservesValue = preservesValue1.booleanAnd(preservesValue2)
+    )
+  }
+
+  /**
+   * Holds if `c` has a flow summary from `input` to `arg`, where `arg`
+   * writes to (contents of) the `i`th argument, and `c` has a
+   * value-preserving flow summary from the `i`th argument to a return value
+   * (`return`).
+   *
+   * In such a case, we derive flow from `input` to (contents of) the return
+   * value.
+   *
+   * As an example, this simplifies modeling of fluent methods:
+   * 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.
+   */
+  pragma[nomagic]
+  private predicate derivedFluentFlow(
+    SummarizedCallable c, SummaryComponentStack input, SummaryComponentStack arg,
+    SummaryComponentStack return, boolean preservesValue
+  ) {
+    exists(int i |
+      summary(c, input, arg, preservesValue) and
+      isContentOfArgument(arg, i) and
+      summary(c, SummaryComponentStack::singleton(TArgumentSummaryComponent(i)), return, true) and
+      return.bottom() = TReturnSummaryComponent(_)
+    )
+  }
+
+  pragma[nomagic]
+  private predicate derivedFluentFlowPush(
+    SummarizedCallable c, SummaryComponentStack input, SummaryComponentStack arg,
+    SummaryComponent head, SummaryComponentStack tail, int i
+  ) {
+    derivedFluentFlow(c, input, arg, tail, _) and
+    head = arg.drop(i).head() and
+    i = arg.length() - 2
+    or
+    exists(SummaryComponent head0, SummaryComponentStack tail0 |
+      derivedFluentFlowPush(c, input, arg, head0, tail0, i + 1) and
+      head = arg.drop(i).head() and
+      tail = SummaryComponentStack::push(head0, tail0)
+    )
   }
 
   private predicate isCallbackParameter(SummaryComponentStack s) {
     s.head() = TParameterSummaryComponent(_) and exists(s.tail())
   }
 
-  private predicate isContentOfArgument(SummaryComponentStack s) {
-    s.head() = TContentSummaryComponent(_) and isContentOfArgument(s.tail())
+  private predicate isContentOfArgument(SummaryComponentStack s, int i) {
+    s.head() = TContentSummaryComponent(_) and isContentOfArgument(s.tail(), i)
     or
-    s = TSingletonSummaryComponentStack(TArgumentSummaryComponent(_))
+    s = TSingletonSummaryComponentStack(TArgumentSummaryComponent(i))
   }
 
   private predicate outputState(SummarizedCallable c, SummaryComponentStack s) {
@@ -508,9 +578,14 @@ module Private {
    * node, and back out to `p`.
    */
   predicate summaryAllowParameterReturnInSelf(ParamNode p) {
-    exists(SummarizedCallable c, int i |
-      c.clearsContent(i, _) and
-      p.isParameterOf(c, i)
+    exists(SummarizedCallable c, int i | p.isParameterOf(c, i) |
+      c.clearsContent(i, _)
+      or
+      exists(SummaryComponentStack inputContents, SummaryComponentStack outputContents |
+        summary(c, inputContents, outputContents, _) and
+        inputContents.bottom() = pragma[only_bind_into](TArgumentSummaryComponent(i)) and
+        outputContents.bottom() = pragma[only_bind_into](TArgumentSummaryComponent(i))
+      )
     )
   }
 
@@ -534,22 +609,6 @@ module Private {
         preservesValue = false and not summary(c, inputContents, outputContents, true)
       )
       or
-      // If flow through a method updates a parameter from some input A, and that
-      // parameter also is returned through B, then we'd like a combined flow from A
-      // to B as well. As an example, this simplifies modeling of fluent methods:
-      // 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.
-      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
-      or
       exists(SummarizedCallable c, int i |
         pred.(ParamNode).isParameterOf(c, i) and
         succ = summaryNode(c, TSummaryNodeClearsContentState(i, _)) and

csharp/ql/lib/semmle/code/csharp/frameworks/EntityFramework.qll

@@ -174,32 +174,34 @@ module EntityFramework {
     }
   }
 
-  private class RawSqlStringSummarizedCallable extends EFSummarizedCallable {
-    private SummaryComponentStack input_;
-    private SummaryComponentStack output_;
-    private boolean preservesValue_;
-
-    RawSqlStringSummarizedCallable() {
+  private class RawSqlStringConstructorSummarizedCallable extends EFSummarizedCallable {
+    RawSqlStringConstructorSummarizedCallable() {
       exists(RawSqlStringStruct s |
         this = s.getAConstructor() and
-        input_ = SummaryComponentStack::argument(0) and
-        this.getNumberOfParameters() > 0 and
-        output_ = SummaryComponentStack::return() and
-        preservesValue_ = false
-        or
-        this = s.getAConversionTo() and
-        input_ = SummaryComponentStack::argument(0) and
-        output_ = SummaryComponentStack::return() and
-        preservesValue_ = false
+        this.getNumberOfParameters() > 0
       )
     }
 
     override predicate propagatesFlow(
       SummaryComponentStack input, SummaryComponentStack output, boolean preservesValue
     ) {
-      input = input_ and
-      output = output_ and
-      preservesValue = preservesValue_
+      input = SummaryComponentStack::argument(0) and
+      output = SummaryComponentStack::return() and
+      preservesValue = false
+    }
+  }
+
+  private class RawSqlStringConversionSummarizedCallable extends EFSummarizedCallable {
+    RawSqlStringConversionSummarizedCallable() {
+      exists(RawSqlStringStruct s | this = s.getAConversionTo())
+    }
+
+    override predicate propagatesFlow(
+      SummaryComponentStack input, SummaryComponentStack output, boolean preservesValue
+    ) {
+      input = SummaryComponentStack::argument(0) and
+      output = SummaryComponentStack::return() and
+      preservesValue = false
     }
   }
 

csharp/ql/test/library-tests/dataflow/external-models/ExternalFlow.cs

@@ -33,7 +33,7 @@ void M4()
 
         void M5()
         {
-            this.StepFieldSetter(new object());
+            Sink(((D)this.StepFieldSetter(new object()).Field2).Field);
             Sink(this.Field);
         }
 
@@ -102,7 +102,7 @@ void M15()
                 Sink(d);
             }, d1, d2);
             Sink(d1.Field);
-            Sink(d2.Field2); // SPURIOUS FLOW
+            Sink(d2.Field2);
         }
 
         object StepArgRes(object x) { return null; }
@@ -120,7 +120,7 @@ void StepQualArg(object @out) { }
 
         object StepFieldGetter() => throw null;
 
-        void StepFieldSetter(object value) => throw null;
+        D StepFieldSetter(object value) => throw null;
 
         object Property { get; set; }
 

csharp/ql/test/library-tests/dataflow/external-models/ExternalFlow.expected

@@ -11,8 +11,12 @@ edges
 | ExternalFlow.cs:30:13:30:16 | [post] this access [field Field] : Object | ExternalFlow.cs:31:18:31:21 | this access [field Field] : Object |
 | ExternalFlow.cs:30:26:30:37 | object creation of type Object : Object | ExternalFlow.cs:30:13:30:16 | [post] this access [field Field] : Object |
 | ExternalFlow.cs:31:18:31:21 | this access [field Field] : Object | ExternalFlow.cs:31:18:31:39 | call to method StepFieldGetter |
-| ExternalFlow.cs:36:13:36:16 | [post] this access [field Field] : Object | ExternalFlow.cs:37:18:37:21 | this access [field Field] : Object |
-| ExternalFlow.cs:36:34:36:45 | object creation of type Object : Object | ExternalFlow.cs:36:13:36:16 | [post] this access [field Field] : Object |
+| ExternalFlow.cs:36:19:36:62 | (...) ... [field Field] : Object | ExternalFlow.cs:36:18:36:69 | access to field Field |
+| ExternalFlow.cs:36:22:36:25 | [post] this access [field Field] : Object | ExternalFlow.cs:37:18:37:21 | this access [field Field] : Object |
+| ExternalFlow.cs:36:22:36:55 | call to method StepFieldSetter [field Field2, field Field] : Object | ExternalFlow.cs:36:22:36:62 | access to field Field2 [field Field] : Object |
+| ExternalFlow.cs:36:22:36:62 | access to field Field2 [field Field] : Object | ExternalFlow.cs:36:19:36:62 | (...) ... [field Field] : Object |
+| ExternalFlow.cs:36:43:36:54 | object creation of type Object : Object | ExternalFlow.cs:36:22:36:25 | [post] this access [field Field] : Object |
+| ExternalFlow.cs:36:43:36:54 | object creation of type Object : Object | ExternalFlow.cs:36:22:36:55 | call to method StepFieldSetter [field Field2, field Field] : Object |
 | ExternalFlow.cs:37:18:37:21 | this access [field Field] : Object | ExternalFlow.cs:37:18:37:27 | access to field Field |
 | ExternalFlow.cs:42:13:42:16 | [post] this access [property Property] : Object | ExternalFlow.cs:43:18:43:21 | this access [property Property] : Object |
 | ExternalFlow.cs:42:29:42:40 | object creation of type Object : Object | ExternalFlow.cs:42:13:42:16 | [post] this access [property Property] : Object |
@@ -48,10 +52,7 @@ edges
 | ExternalFlow.cs:98:24:98:35 | object creation of type Object : Object | ExternalFlow.cs:98:13:98:14 | [post] access to local variable d1 [field Field] : Object |
 | ExternalFlow.cs:100:20:100:20 | d : Object | ExternalFlow.cs:102:22:102:22 | access to parameter d |
 | ExternalFlow.cs:103:16:103:17 | access to local variable d1 [field Field] : Object | ExternalFlow.cs:100:20:100:20 | d : Object |
-| ExternalFlow.cs:103:16:103:17 | access to local variable d1 [field Field] : Object | ExternalFlow.cs:103:20:103:21 | [post] access to local variable d2 [field Field2] : Object |
-| ExternalFlow.cs:103:20:103:21 | [post] access to local variable d2 [field Field2] : Object | ExternalFlow.cs:105:18:105:19 | access to local variable d2 [field Field2] : Object |
 | ExternalFlow.cs:104:18:104:19 | access to local variable d1 [field Field] : Object | ExternalFlow.cs:104:18:104:25 | access to field Field |
-| ExternalFlow.cs:105:18:105:19 | access to local variable d2 [field Field2] : Object | ExternalFlow.cs:105:18:105:26 | access to field Field2 |
 nodes
 | ExternalFlow.cs:9:27:9:38 | object creation of type Object : Object | semmle.label | object creation of type Object : Object |
 | ExternalFlow.cs:10:18:10:33 | call to method StepArgRes | semmle.label | call to method StepArgRes |
@@ -69,8 +70,12 @@ nodes
 | ExternalFlow.cs:30:26:30:37 | object creation of type Object : Object | semmle.label | object creation of type Object : Object |
 | ExternalFlow.cs:31:18:31:21 | this access [field Field] : Object | semmle.label | this access [field Field] : Object |
 | ExternalFlow.cs:31:18:31:39 | call to method StepFieldGetter | semmle.label | call to method StepFieldGetter |
-| ExternalFlow.cs:36:13:36:16 | [post] this access [field Field] : Object | semmle.label | [post] this access [field Field] : Object |
-| ExternalFlow.cs:36:34:36:45 | object creation of type Object : Object | semmle.label | object creation of type Object : Object |
+| ExternalFlow.cs:36:18:36:69 | access to field Field | semmle.label | access to field Field |
+| ExternalFlow.cs:36:19:36:62 | (...) ... [field Field] : Object | semmle.label | (...) ... [field Field] : Object |
+| ExternalFlow.cs:36:22:36:25 | [post] this access [field Field] : Object | semmle.label | [post] this access [field Field] : Object |
+| ExternalFlow.cs:36:22:36:55 | call to method StepFieldSetter [field Field2, field Field] : Object | semmle.label | call to method StepFieldSetter [field Field2, field Field] : Object |
+| ExternalFlow.cs:36:22:36:62 | access to field Field2 [field Field] : Object | semmle.label | access to field Field2 [field Field] : Object |
+| ExternalFlow.cs:36:43:36:54 | object creation of type Object : Object | semmle.label | object creation of type Object : Object |
 | ExternalFlow.cs:37:18:37:21 | this access [field Field] : Object | semmle.label | this access [field Field] : Object |
 | ExternalFlow.cs:37:18:37:27 | access to field Field | semmle.label | access to field Field |
 | ExternalFlow.cs:42:13:42:16 | [post] this access [property Property] : Object | semmle.label | [post] this access [property Property] : Object |
@@ -116,11 +121,8 @@ nodes
 | ExternalFlow.cs:100:20:100:20 | d : Object | semmle.label | d : Object |
 | ExternalFlow.cs:102:22:102:22 | access to parameter d | semmle.label | access to parameter d |
 | ExternalFlow.cs:103:16:103:17 | access to local variable d1 [field Field] : Object | semmle.label | access to local variable d1 [field Field] : Object |
-| ExternalFlow.cs:103:20:103:21 | [post] access to local variable d2 [field Field2] : Object | semmle.label | [post] access to local variable d2 [field Field2] : Object |
 | ExternalFlow.cs:104:18:104:19 | access to local variable d1 [field Field] : Object | semmle.label | access to local variable d1 [field Field] : Object |
 | ExternalFlow.cs:104:18:104:25 | access to field Field | semmle.label | access to field Field |
-| ExternalFlow.cs:105:18:105:19 | access to local variable d2 [field Field2] : Object | semmle.label | access to local variable d2 [field Field2] : Object |
-| ExternalFlow.cs:105:18:105:26 | access to field Field2 | semmle.label | access to field Field2 |
 subpaths
 invalidModelRow
 #select
@@ -129,7 +131,8 @@ invalidModelRow
 | ExternalFlow.cs:18:18:18:24 | access to local variable argOut1 | ExternalFlow.cs:16:30:16:41 | object creation of type Object : Object | ExternalFlow.cs:18:18:18:24 | access to local variable argOut1 | $@ | ExternalFlow.cs:16:30:16:41 | object creation of type Object : Object | object creation of type Object : Object |
 | ExternalFlow.cs:25:18:25:21 | this access | ExternalFlow.cs:23:27:23:38 | object creation of type Object : Object | ExternalFlow.cs:25:18:25:21 | this access | $@ | ExternalFlow.cs:23:27:23:38 | object creation of type Object : Object | object creation of type Object : Object |
 | ExternalFlow.cs:31:18:31:39 | call to method StepFieldGetter | ExternalFlow.cs:30:26:30:37 | object creation of type Object : Object | ExternalFlow.cs:31:18:31:39 | call to method StepFieldGetter | $@ | ExternalFlow.cs:30:26:30:37 | object creation of type Object : Object | object creation of type Object : Object |
-| ExternalFlow.cs:37:18:37:27 | access to field Field | ExternalFlow.cs:36:34:36:45 | object creation of type Object : Object | ExternalFlow.cs:37:18:37:27 | access to field Field | $@ | ExternalFlow.cs:36:34:36:45 | object creation of type Object : Object | object creation of type Object : Object |
+| ExternalFlow.cs:36:18:36:69 | access to field Field | ExternalFlow.cs:36:43:36:54 | object creation of type Object : Object | ExternalFlow.cs:36:18:36:69 | access to field Field | $@ | ExternalFlow.cs:36:43:36:54 | object creation of type Object : Object | object creation of type Object : Object |
+| ExternalFlow.cs:37:18:37:27 | access to field Field | ExternalFlow.cs:36:43:36:54 | object creation of type Object : Object | ExternalFlow.cs:37:18:37:27 | access to field Field | $@ | ExternalFlow.cs:36:43:36:54 | object creation of type Object : Object | object creation of type Object : Object |
 | ExternalFlow.cs:43:18:43:42 | call to method StepPropertyGetter | ExternalFlow.cs:42:29:42:40 | object creation of type Object : Object | ExternalFlow.cs:43:18:43:42 | call to method StepPropertyGetter | $@ | ExternalFlow.cs:42:29:42:40 | object creation of type Object : Object | object creation of type Object : Object |
 | ExternalFlow.cs:49:18:49:30 | access to property Property | ExternalFlow.cs:48:37:48:48 | object creation of type Object : Object | ExternalFlow.cs:49:18:49:30 | access to property Property | $@ | ExternalFlow.cs:48:37:48:48 | object creation of type Object : Object | object creation of type Object : Object |
 | ExternalFlow.cs:55:18:55:41 | call to method StepElementGetter | ExternalFlow.cs:54:36:54:47 | object creation of type Object : Object | ExternalFlow.cs:55:18:55:41 | call to method StepElementGetter | $@ | ExternalFlow.cs:54:36:54:47 | object creation of type Object : Object | object creation of type Object : Object |
@@ -141,4 +144,3 @@ invalidModelRow
 | ExternalFlow.cs:92:18:92:18 | (...) ... | ExternalFlow.cs:90:21:90:34 | object creation of type String : String | ExternalFlow.cs:92:18:92:18 | (...) ... | $@ | ExternalFlow.cs:90:21:90:34 | object creation of type String : String | object creation of type String : String |
 | ExternalFlow.cs:102:22:102:22 | access to parameter d | ExternalFlow.cs:98:24:98:35 | object creation of type Object : Object | ExternalFlow.cs:102:22:102:22 | access to parameter d | $@ | ExternalFlow.cs:98:24:98:35 | object creation of type Object : Object | object creation of type Object : Object |
 | ExternalFlow.cs:104:18:104:25 | access to field Field | ExternalFlow.cs:98:24:98:35 | object creation of type Object : Object | ExternalFlow.cs:104:18:104:25 | access to field Field | $@ | ExternalFlow.cs:98:24:98:35 | object creation of type Object : Object | object creation of type Object : Object |
-| ExternalFlow.cs:105:18:105:26 | access to field Field2 | ExternalFlow.cs:98:24:98:35 | object creation of type Object : Object | ExternalFlow.cs:105:18:105:26 | access to field Field2 | $@ | ExternalFlow.cs:98:24:98:35 | object creation of type Object : Object | object creation of type Object : Object |

csharp/ql/test/library-tests/dataflow/external-models/ExternalFlow.ql

@@ -17,6 +17,7 @@ class SummaryModelTest extends SummaryModelCsv {
         "My.Qltest;D;false;StepArgQual;(System.Object);;Argument[0];Argument[-1];taint",
         "My.Qltest;D;false;StepFieldGetter;();;Field[My.Qltest.D.Field] of Argument[-1];ReturnValue;value",
         "My.Qltest;D;false;StepFieldSetter;(System.Object);;Argument[0];Field[My.Qltest.D.Field] of Argument[-1];value",
+        "My.Qltest;D;false;StepFieldSetter;(System.Object);;Argument[-1];Field[My.Qltest.D.Field2] of ReturnValue;value",
         "My.Qltest;D;false;StepPropertyGetter;();;Property[My.Qltest.D.Property] of Argument[-1];ReturnValue;value",
         "My.Qltest;D;false;StepPropertySetter;(System.Object);;Argument[0];Property[My.Qltest.D.Property] of Argument[-1];value",
         "My.Qltest;D;false;StepElementGetter;();;Element of Argument[-1];ReturnValue;value",