C#: Add preservesValue to NonLocalJumpNode.getAJumpSuccessor. Allow DataFlow::Configuration::isAdditionalFlowStep to jump between callables.

Author: calumgrant

change-notes/1.20/analysis-csharp.md

@@ -31,6 +31,6 @@
 * Support has been added for EntityFrameworkCore, including
   - Stored data flow sources
   - Sinks for SQL expressions
-  - Dataflow through fields that are mapped to the database.
+  - Data flow through fields that are mapped to the database.
 
 ## Changes to the autobuilder

csharp/ql/src/semmle/code/csharp/dataflow/DataFlow.qll

@@ -124,7 +124,7 @@ module DataFlow {
    */
   abstract class NonLocalJumpNode extends Node {
     /** Gets a successor node that is potentially in another callable. */
-    abstract Node getAJumpSuccessor();
+    abstract Node getAJumpSuccessor(boolean preservesValue);
   }
 
   /**
@@ -1072,8 +1072,10 @@ module DataFlow {
        */
       bindingset[config]
       predicate localFlowStep(Node pred, Node succ, Configuration config) {
-        localFlowStepNoConfig(pred, succ) or
-        config.isAdditionalFlowStep(pred, succ)
+        localFlowStepNoConfig(pred, succ)
+        or
+        config.isAdditionalFlowStep(pred, succ) and
+        pred.getEnclosingCallable() = succ.getEnclosingCallable()
       }
 
       /**
@@ -1084,7 +1086,7 @@ module DataFlow {
         Pruning::nodeCand(node, config) and
         (
           config.isSource(node) or
-          jumpStep(_, node) or
+          additionalJumpStep(_, node, config) or
           node instanceof ParameterNode or
           node instanceof OutNode
         )
@@ -1097,7 +1099,7 @@ module DataFlow {
       predicate localFlowExit(Node node, Configuration config) {
         Pruning::nodeCand(node, config) and
         (
-          jumpStep(node, _) or
+          additionalJumpStep(node, _, config) or
           node instanceof ArgumentNode or
           node instanceof ReturnNode or
           config.isSink(node)
@@ -1138,6 +1140,17 @@ module DataFlow {
       }
     }
 
+      
+      /**
+       * Holds if the additional step from `node1` to `node2` jumps between callables.
+       */
+      private predicate additionalJumpStep(Node node1, Node node2, Configuration config) {
+        config.isAdditionalFlowStep(node1, node2) and
+        node1.getEnclosingCallable() != node2.getEnclosingCallable()
+        or
+        jumpStep(node1, node2)
+      }
+
     /**
      * Provides predicates for pruning the data flow graph, by only including
      * nodes that may potentially be reached in flow from some source to some
@@ -1155,7 +1168,7 @@ module DataFlow {
           or
           exists(Node mid | nodeCandFwd1(mid, config) | LocalFlow::localFlowStep(mid, node, config))
           or
-          exists(Node mid | nodeCandFwd1(mid, config) | jumpStep(mid, node))
+          exists(Node mid | nodeCandFwd1(mid, config) | additionalJumpStep(mid, node, config))
           or
           exists(ArgumentNode arg | nodeCandFwd1(arg, config) |
             flowIntoCallableStep(_, arg, node, _, config)
@@ -1178,7 +1191,7 @@ module DataFlow {
           or
           exists(Node mid | nodeCand1(mid, config) | LocalFlow::localFlowStep(node, mid, config))
           or
-          exists(Node mid | nodeCand1(mid, config) | jumpStep(node, mid))
+          exists(Node mid | nodeCand1(mid, config) | additionalJumpStep(node, mid, config))
           or
           exists(ParameterNode p | nodeCand1(p, config) |
             flowIntoCallableStep(_, node, p, _, config)
@@ -1261,7 +1274,7 @@ module DataFlow {
         or
         nodeCand1(node, config) and
         exists(Node mid | nodeCandFwd2(mid, _, config) |
-          jumpStep(mid, node) and
+          additionalJumpStep(mid, node, config) and
           fromArg = false
         )
         or
@@ -1317,7 +1330,7 @@ module DataFlow {
         or
         nodeCandFwd2(node, _, config) and
         exists(Node mid | nodeCand2(mid, _, config) |
-          jumpStep(node, mid) and
+          additionalJumpStep(node, mid, config) and
           isReturned = false
         )
         or
@@ -1376,7 +1389,7 @@ module DataFlow {
        */
       cached
       predicate jumpStep(ExprNode pred, ExprNode succ) {
-        pred.(NonLocalJumpNode).getAJumpSuccessor() = succ
+        pred.(NonLocalJumpNode).getAJumpSuccessor(true) = succ
       }
 
       /** A dataflow node that has field-like dataflow. */
@@ -1395,7 +1408,9 @@ module DataFlow {
           hasNonlocalValue(flr)
         }
 
-        override ExprNode getAJumpSuccessor() { result = succ }
+        override ExprNode getAJumpSuccessor(boolean preservesValue) {
+          result = succ and preservesValue = true
+        }
       }
 
       /**
@@ -1681,7 +1696,7 @@ module DataFlow {
       ctx = mid.getContext() and
       LocalFlow::localFlowBigStep(mid.getNode(), node, mid.getConfiguration())
       or
-      jumpStep(mid.getNode(), node) and
+      additionalJumpStep(mid.getNode(), node, mid.getConfiguration()) and
       ctx instanceof NoContext
       or
       flowIntoCallable(mid, node, ctx)

csharp/ql/src/semmle/code/csharp/dataflow/TaintTracking.qll

@@ -80,9 +80,13 @@ module TaintTracking {
     predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) { none() }
 
     final override predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ) {
-      isAdditionalTaintStep(pred, succ) or
-      localAdditionalTaintStep(pred, succ) or
+      isAdditionalTaintStep(pred, succ)
+      or
+      localAdditionalTaintStep(pred, succ)
+      or
       DataFlow::Internal::flowThroughCallableLibraryOutRef(_, pred, succ, false)
+      or
+      succ = pred.(DataFlow::NonLocalJumpNode).getAJumpSuccessor(false)
     }
 
     final override predicate isAdditionalFlowStepIntoCall(

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

@@ -185,8 +185,9 @@ module EntityFramework {
 
     MappedPropertyJumpNode() { this.asExpr() = property.getAnAssignedValue() }
 
-    override DataFlow::Node getAJumpSuccessor() {
-      result.asExpr().(PropertyRead).getTarget() = property
+    override DataFlow::Node getAJumpSuccessor(boolean preservesValue) {
+      result.asExpr().(PropertyRead).getTarget() = property and
+      preservesValue = false
     }
   }
 }

csharp/ql/test/library-tests/frameworks/EntityFramework/Dataflow.ql

@@ -1,14 +1,11 @@
 import csharp
 import semmle.code.csharp.dataflow.TaintTracking
 
-class MyConfiguration extends TaintTracking::Configuration
-{
+class MyConfiguration extends TaintTracking::Configuration {
   MyConfiguration() { this = "EntityFramework dataflow" }
-  
-  override predicate isSource(DataFlow::Node node) {
-    node.asExpr().getValue() = "tainted"
-  }
-  
+
+  override predicate isSource(DataFlow::Node node) { node.asExpr().getValue() = "tainted" }
+
   override predicate isSink(DataFlow::Node node) {
     node.asExpr() = any(MethodCall c | c.getTarget().hasName("Sink")).getAnArgument()
   }

csharp/ql/test/library-tests/frameworks/EntityFramework/EntityFramework.cs

@@ -23,23 +23,23 @@ class MyContext : DbContext
         void FlowSources()
         {
             var p = new Person();
-            var id = p.Id;
-            var name = p.Name;
-            var age = p.Age;
+            var id = p.Id;  // Remote flow source
+            var name = p.Name;  // Remote flow source
+            var age = p.Age;  // Not a remote flow source
         }
 
         DbCommand command;
 
         async void SqlSinks()
         {
             // System.Data.Common.DbCommand.set_CommandText
-            command.CommandText = "";
-            
+            command.CommandText = "";  // SqlExpr
+
             // System.Data.SqlClient.SqlCommand.SqlCommand
-            new System.Data.SqlClient.SqlCommand("");
-            
-            this.Database.ExecuteSqlCommand("");
-            await this.Database.ExecuteSqlCommandAsync("");
+            new System.Data.SqlClient.SqlCommand("");  // SqlExpr
+
+            this.Database.ExecuteSqlCommand("");  // SqlExpr
+            await this.Database.ExecuteSqlCommandAsync("");  // SqlExpr
         }
 
         void TestDataFlow()
@@ -59,7 +59,6 @@ void TestDataFlow()
         void Sink(object @object)
         {
         }
-
     }
 
 }

csharp/ql/test/library-tests/frameworks/EntityFramework/MappedProperties.ql

@@ -3,4 +3,3 @@ import semmle.code.csharp.frameworks.EntityFramework
 
 from EntityFramework::MappedProperty property
 select property
-

csharp/ql/test/library-tests/frameworks/EntityFramework/SqlExprs.ql

@@ -2,4 +2,4 @@ import csharp
 import semmle.code.csharp.frameworks.Sql
 
 from SqlExpr expr
-select expr
+select expr

…rks/EntityFramework/FlowSources.expected …tityFramework/StoredFlowSources.expectedcsharp/ql/test/library-tests/frameworks/EntityFramework/FlowSources.expected renamed to csharp/ql/test/library-tests/frameworks/EntityFramework/StoredFlowSources.expected csharp/ql/test/library-tests/frameworks/EntityFramework/FlowSources.expected renamed to csharp/ql/test/library-tests/frameworks/EntityFramework/StoredFlowSources.expected

@@ -1,5 +1,4 @@
 import csharp
-
 import semmle.code.csharp.security.dataflow.flowsources.Stored
 
 from StoredFlowSource source