Merge pull request #912 from calumgrant/cs/ef

C#: Model EntityFrameworkCore

change-notes/1.20/analysis-csharp.md

@@ -30,5 +30,9 @@
 
 * The class `TrivialProperty` now includes library properties determined to be trivial using CIL analysis. This may increase the number of results for all queries that use data flow.
 * Taint-tracking steps have been added for the `Json.NET` package. This will improve results for queries that use taint-tracking.
+* Support has been added for EntityFrameworkCore, including
+  - Stored data flow sources
+  - Sinks for SQL expressions
+  - Data flow through fields that are mapped to the database.
 
 ## Changes to the autobuilder

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

@@ -9,6 +9,7 @@ module DataFlow {
   private import semmle.code.csharp.dataflow.CallContext
   private import semmle.code.csharp.dataflow.DelegateDataFlow
   private import semmle.code.csharp.dataflow.LibraryTypeDataFlow
+  private import semmle.code.csharp.frameworks.EntityFramework
   private import Internal::Cached
   private import dotnet
   private import cil
@@ -117,6 +118,15 @@ module DataFlow {
 
   predicate localFlowStep = Internal::LocalFlow::step/2;
 
+  /**
+   * A dataflow node that jumps between callables. This can be extended in framework code
+   * to add additional dataflow steps.
+   */
+  abstract class NonLocalJumpNode extends Node {
+    /** Gets a successor node that is potentially in another callable. */
+    abstract Node getAJumpSuccessor(boolean preservesValue);
+  }
+
   /**
    * A data flow node augmented with a call context and a configuration. Only
    * nodes that are reachable from a source, and which can reach a sink, are
@@ -700,7 +710,7 @@ module DataFlow {
       CilCall() {
         call = this and
         // No need to include calls that are compiled from source
-        not call.getImplementation().getMethod().compiledFromSource()
+        cilCallWithoutSource(call)
       }
 
       override DotNet::Callable getARuntimeTarget() {
@@ -1084,13 +1094,20 @@ module DataFlow {
         flowThroughCallableLibraryOutRef(_, pred, succ, true)
       }
 
+      pragma[noinline]
+      private predicate localFlowStep0(Node pred, Node succ, Configuration config, DotNet::Callable c) {
+        config.isAdditionalFlowStep(pred, succ) and
+        pred.getEnclosingCallable() = c
+      }
+
       /**
        * Holds if data may flow in one local step from `pred` to `succ`.
        */
       bindingset[config]
       predicate localFlowStep(Node pred, Node succ, Configuration config) {
-        localFlowStepNoConfig(pred, succ) or
-        config.isAdditionalFlowStep(pred, succ)
+        localFlowStepNoConfig(pred, succ)
+        or
+        localFlowStep0(pred, succ, config, succ.getEnclosingCallable())
       }
 
       /**
@@ -1101,7 +1118,7 @@ module DataFlow {
         Pruning::nodeCand(node, config) and
         (
           config.isSource(node) or
-          jumpStep(_, node) or
+          jumpStep(_, node, config) or
           node instanceof ParameterNode or
           node instanceof OutNode
         )
@@ -1114,7 +1131,7 @@ module DataFlow {
       predicate localFlowExit(Node node, Configuration config) {
         Pruning::nodeCand(node, config) and
         (
-          jumpStep(node, _) or
+          jumpStep(node, _, config) or
           node instanceof ArgumentNode or
           node instanceof ReturnNode or
           config.isSink(node)
@@ -1155,6 +1172,26 @@ module DataFlow {
       }
     }
 
+    /**
+     * Holds if the additional step from `node1` to `node2` jumps between callables.
+     */
+    pragma[noinline]
+    private predicate additionalJumpStep(Node node1, Node node2, Configuration config) {
+      config.isAdditionalFlowStep(node1, node2) and
+      node1.getEnclosingCallable() != node2.getEnclosingCallable()
+    }
+
+    /**
+     * Holds if `pred` can flow to `succ`, by jumping from one callable to
+     * another.
+     */
+    bindingset[config]
+    private predicate jumpStep(Node node1, Node node2, Configuration config) {
+      additionalJumpStep(node1, node2, config)
+      or
+      jumpStepNoConfig(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
@@ -1172,7 +1209,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) | jumpStep(mid, node, config))
           or
           exists(ArgumentNode arg | nodeCandFwd1(arg, config) |
             flowIntoCallableStep(_, arg, node, _, config)
@@ -1195,7 +1232,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) | jumpStep(node, mid, config))
           or
           exists(ParameterNode p | nodeCand1(p, config) |
             flowIntoCallableStep(_, node, p, _, config)
@@ -1250,7 +1287,7 @@ module DataFlow {
       pragma[noinline]
       private predicate jumpStepCand1(Node pred, Node succ, Configuration config) {
         nodeCand1(succ, config) and
-        jumpStep(pred, succ)
+        jumpStep(pred, succ, config)
       }
 
       pragma[noinline]
@@ -1339,7 +1376,7 @@ module DataFlow {
         or
         nodeCandFwd2(node, _, config) and
         exists(Node mid | nodeCand2(mid, _, config) |
-          jumpStep(node, mid) and
+          jumpStep(node, mid, config) and
           isReturned = false
         )
         or
@@ -1394,16 +1431,32 @@ module DataFlow {
 
       /**
        * Holds if `pred` can flow to `succ`, by jumping from one callable to
-       * another.
+       * another. Additional steps specified by the configuration are *not* taken into account.
        */
       cached
-      predicate jumpStep(ExprNode pred, ExprNode succ) {
-        exists(FieldLike fl, FieldLikeRead flr | fl.isStatic() |
-          fl.getAnAssignedValue() = pred.getExpr() and
+      predicate jumpStepNoConfig(ExprNode pred, ExprNode succ) {
+        pred.(NonLocalJumpNode).getAJumpSuccessor(true) = succ
+      }
+
+      /** A dataflow node that has field-like dataflow. */
+      private class FieldLikeJumpNode extends NonLocalJumpNode, ExprNode {
+        FieldLike fl;
+
+        FieldLikeRead flr;
+
+        ExprNode succ;
+
+        FieldLikeJumpNode() {
+          fl.isStatic() and
+          fl.getAnAssignedValue() = this.getExpr() and
           fl.getAnAccess() = flr and
           flr = succ.getExpr() and
           hasNonlocalValue(flr)
-        )
+        }
+
+        override ExprNode getAJumpSuccessor(boolean preservesValue) {
+          result = succ and preservesValue = true
+        }
       }
 
       /**
@@ -1486,6 +1539,11 @@ module DataFlow {
         ret.flowsOut(out.getDefinition()) and
         call.asExpr() = out.getCall()
       }
+
+      cached
+      predicate cilCallWithoutSource(CIL::Call call) {
+        not call.getImplementation().getMethod().compiledFromSource()
+      }
     }
 
     private newtype TContext =
@@ -1548,9 +1606,7 @@ module DataFlow {
     /**
      * A data flow context describing flow into a callable via a call argument.
      */
-    abstract private class ArgumentContext extends Context {
-      abstract DotNet::Expr getCall();
-    }
+    abstract private class ArgumentContext extends Context { abstract DotNet::Expr getCall(); }
 
     /**
      * A data flow context describing flow into a callable via an explicit call argument.
@@ -1691,7 +1747,7 @@ module DataFlow {
       ctx = mid.getContext() and
       LocalFlow::localFlowBigStep(mid.getNode(), node, mid.getConfiguration())
       or
-      jumpStep(mid.getNode(), node) and
+      jumpStep(mid.getNode(), node, mid.getConfiguration()) and
       ctx instanceof NoContext
       or
       flowIntoCallable(mid, node, ctx)

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

@@ -172,7 +172,7 @@ private predicate flowsFrom(
   // Flow through static field or property
   exists(DataFlow::Node mid |
     flowsFrom(sink, mid, _, _) and
-    Cached::jumpStep(node, mid) and
+    Cached::jumpStepNoConfig(node, mid) and
     isReturned = false and
     lastCall instanceof EmptyCallContext
   )

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

@@ -0,0 +1,193 @@
+/**
+ * Classes modelling EntityFramework and EntityFrameworkCore.
+ */
+
+import csharp
+private import semmle.code.csharp.frameworks.system.data.Entity
+private import semmle.code.csharp.frameworks.system.collections.Generic
+private import semmle.code.csharp.frameworks.Sql
+private import semmle.code.csharp.dataflow.LibraryTypeDataFlow
+
+module DataAnnotations {
+  class NotMappedAttribute extends Attribute {
+    NotMappedAttribute() {
+      this
+          .getType()
+          .hasQualifiedName("System.ComponentModel.DataAnnotations.Schema.NotMappedAttribute")
+    }
+  }
+}
+
+module EntityFramework {
+  /** An EF6 or EFCore namespace. */
+  class EFNamespace extends Namespace {
+    EFNamespace() {
+      this.getQualifiedName() = "Microsoft.EntityFrameworkCore"
+      or
+      this.getQualifiedName() = "System.Data.Entity"
+    }
+  }
+
+  /** A taint source where the data has come from a mapped property stored in the database. */
+  class StoredFlowSource extends DataFlow::Node {
+    StoredFlowSource() {
+      this.asExpr() = any(PropertyRead read | read.getTarget() instanceof MappedProperty)
+    }
+  }
+
+  private class EFClass extends Class {
+    EFClass() { this.getDeclaringNamespace() instanceof EFNamespace }
+  }
+
+  /** The class `Microsoft.EntityFrameworkCore.DbContext` or `System.Data.Entity.DbContext`. */
+  class DbContext extends EFClass {
+    DbContext() { this.getName() = "DbContext" }
+
+    Method getAFindMethod() {
+      result = this.getAMethod("Find")
+      or
+      result = this.getAMethod("FindAsync")
+    }
+
+    Method getAnUpdateMethod() { result = this.getAMethod("Update") }
+  }
+
+  /** The class `Microsoft.EntityFrameworkCore.DbSet<>` or `System.Data.Entity.DbSet<>`. */
+  class DbSet extends EFClass, UnboundGenericClass { DbSet() { this.getName() = "DbSet<>" } }
+
+  /** The class `Microsoft.EntityFrameworkCore.DbQuery<>` or `System.Data.Entity.DbQuery<>`. */
+  class DbQuery extends EFClass, UnboundGenericClass { DbQuery() { this.hasName("DbQuery<>") } }
+
+  /** A generic type or method that takes a mapped type as its type argument. */
+  private predicate usesMappedType(UnboundGeneric g) {
+    g instanceof DbSet
+    or
+    g instanceof DbQuery
+    or
+    exists(DbContext db |
+      g = db.getAnUpdateMethod()
+      or
+      g = db.getAFindMethod()
+    )
+  }
+
+  /** A type that is mapped to database table, or used as a query. */
+  class MappedType extends ValueOrRefType {
+    MappedType() {
+      not this instanceof ObjectType and
+      not this instanceof StringType and
+      not this instanceof ValueType and
+      (
+        exists(UnboundGeneric g | usesMappedType(g) |
+          this = g.getAConstructedGeneric().getATypeArgument()
+        )
+        or
+        this.getASubType() instanceof MappedType
+      )
+    }
+  }
+
+  /** A property that is potentially stored and retrieved from a database. */
+  class MappedProperty extends Property {
+    MappedProperty() {
+      this = any(MappedType t).getAMember() and
+      this.isPublic() and
+      not this.getAnAttribute() instanceof DataAnnotations::NotMappedAttribute
+    }
+  }
+
+  /** The struct `Microsoft.EntityFrameworkCore.RawSqlString`. */
+  class RawSqlStringStruct extends Struct, LibraryTypeDataFlow {
+    RawSqlStringStruct() { this.getQualifiedName() = "Microsoft.EntityFrameworkCore.RawSqlString" }
+
+    override predicate callableFlow(
+      CallableFlowSource source, CallableFlowSink sink, SourceDeclarationCallable c,
+      boolean preservesValue
+    ) {
+      c = this.getAConstructor() and
+      source.(CallableFlowSourceArg).getArgumentIndex() = 0 and
+      sink instanceof CallableFlowSinkReturn and
+      preservesValue = true
+      or
+      c = this.getAConversionTo() and
+      source.(CallableFlowSourceArg).getArgumentIndex() = 0 and
+      sink instanceof CallableFlowSinkReturn and
+      preservesValue = true
+    }
+
+    ConversionOperator getAConversionTo() {
+      result = this.getAMember() and
+      result.getTargetType() instanceof RawSqlStringStruct and
+      result.getSourceType() instanceof StringType
+    }
+  }
+
+  /**
+   * A parameter that accepts raw SQL. Parameters of type `System.FormattableString`
+   * are not included as they are not vulnerable to SQL injection.
+   */
+  private class SqlParameter extends Parameter {
+    SqlParameter() {
+      this.getType() instanceof StringType and
+      (
+        exists(Callable c | this = c.getParameter(0) | c.getName().matches("%Sql"))
+        or
+        this.getName() = "sql"
+      ) and
+      this.getCallable().getDeclaringType().getDeclaringNamespace().getParentNamespace*() instanceof
+        EFNamespace
+      or
+      this.getType() instanceof RawSqlStringStruct
+      or
+      this = any(RawSqlStringStruct s).getAConstructor().getAParameter()
+      or
+      this = any(RawSqlStringStruct s).getAConversionTo().getAParameter()
+    }
+  }
+
+  /** A call to a method in EntityFrameworkCore that executes SQL. */
+  class EntityFrameworkCoreSqlSink extends SqlExpr, Call {
+    SqlParameter sqlParam;
+
+    EntityFrameworkCoreSqlSink() { this.getTarget().getAParameter() = sqlParam }
+
+    override Expr getSql() { result = this.getArgumentForParameter(sqlParam) }
+  }
+
+  /** A call to `System.Data.Entity.DbSet.SqlQuery`. */
+  class SystemDataEntityDbSetSqlExpr extends SqlExpr, MethodCall {
+    SystemDataEntityDbSetSqlExpr() {
+      this.getTarget() = any(SystemDataEntity::DbSet dbSet).getSqlQueryMethod()
+    }
+
+    override Expr getSql() { result = this.getArgumentForName("sql") }
+  }
+
+  /** A call to a method in `System.Data.Entity.Database` that executes SQL. */
+  class SystemDataEntityDatabaseSqlExpr extends SqlExpr, MethodCall {
+    SystemDataEntityDatabaseSqlExpr() {
+      exists(SystemDataEntity::Database db |
+        this.getTarget() = db.getSqlQueryMethod() or
+        this.getTarget() = db.getExecuteSqlCommandMethod() or
+        this.getTarget() = db.getExecuteSqlCommandAsyncMethod()
+      )
+    }
+
+    override Expr getSql() { result = this.getArgumentForName("sql") }
+  }
+
+  /**
+   * A dataflow node whereby data flows from a property write to a property read
+   * via some database. The assumption is that all writes can flow to all reads.
+   */
+  class MappedPropertyJumpNode extends DataFlow::NonLocalJumpNode {
+    MappedProperty property;
+
+    MappedPropertyJumpNode() { this.asExpr() = property.getAnAssignedValue() }
+
+    override DataFlow::Node getAJumpSuccessor(boolean preservesValue) {
+      result.asExpr().(PropertyRead).getTarget() = property and
+      preservesValue = false
+    }
+  }
+}

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

@@ -2,8 +2,8 @@
 
 import csharp
 private import semmle.code.csharp.frameworks.system.Data
-private import semmle.code.csharp.frameworks.system.data.Entity
 private import semmle.code.csharp.frameworks.system.data.SqlClient
+private import semmle.code.csharp.frameworks.EntityFramework
 
 /** An expression containing a SQL command. */
 abstract class SqlExpr extends Expr {
@@ -22,7 +22,7 @@ class CommandTextAssignmentSqlExpr extends SqlExpr, AssignExpr {
     )
   }
 
-  override Expr getSql() { result = getRValue() }
+  override Expr getSql() { result = this.getRValue() }
 }
 
 /** A construction of an `IDbCommand` object. */
@@ -34,7 +34,7 @@ class IDbCommandConstructionSqlExpr extends SqlExpr, ObjectCreation {
     )
   }
 
-  override Expr getSql() { result = getArgument(0) }
+  override Expr getSql() { result = this.getArgument(0) }
 }
 
 /** A construction of an `SqlDataAdapter` object. */
@@ -47,7 +47,7 @@ class SqlDataAdapterConstructionSqlExpr extends SqlExpr, ObjectCreation {
     )
   }
 
-  override Expr getSql() { result = getArgument(0) }
+  override Expr getSql() { result = this.getArgument(0) }
 }
 
 /** A `MySql.Data.MySqlClient.MySqlHelper` method. */
@@ -83,25 +83,3 @@ class MicrosoftSqlHelperMethodCallSqlExpr extends SqlExpr, MethodCall {
     )
   }
 }
-
-/** A call to `System.Data.Entity.DbSet.SqlQuery`. */
-class SystemDataEntityDbSetSqlExpr extends SqlExpr, MethodCall {
-  SystemDataEntityDbSetSqlExpr() {
-    this.getTarget() = any(SystemDataEntity::DbSet dbSet).getSqlQueryMethod()
-  }
-
-  override Expr getSql() { result = getArgumentForName("sql") }
-}
-
-/** A call to a method in `System.Data.Entity.Database` that executes SQL. */
-class SystemDataEntityDatabaseSqlExpr extends SqlExpr, MethodCall {
-  SystemDataEntityDatabaseSqlExpr() {
-    exists(SystemDataEntity::Database db |
-      this.getTarget() = db.getSqlQueryMethod() or
-      this.getTarget() = db.getExecuteSqlCommandMethod() or
-      this.getTarget() = db.getExecuteSqlCommandAsyncMethod()
-    )
-  }
-
-  override Expr getSql() { result = getArgumentForName("sql") }
-}

csharp/ql/src/semmle/code/csharp/security/dataflow/flowsources/Stored.qll

@@ -5,18 +5,12 @@
 import csharp
 private import semmle.code.csharp.frameworks.system.data.Common
 private import semmle.code.csharp.frameworks.system.data.Entity
+private import semmle.code.csharp.frameworks.EntityFramework
 private import semmle.code.csharp.frameworks.Sql
 
 /** A data flow source of stored user input. */
 abstract class StoredFlowSource extends DataFlow::Node { }
 
-/** An access of an Entity Framework `Entity` property that may hold stored data. */
-class EntityPropertyStoredFlowSource extends StoredFlowSource {
-  EntityPropertyStoredFlowSource() {
-    this.asExpr().(PropertyAccess).getTarget() = any(SystemDataEntity::Entity e).getAProperty()
-  }
-}
-
 /**
  * An expression that has a type of `DbRawSqlQuery`, representing the result of an Entity Framework
  * SqlQuery.
@@ -37,9 +31,7 @@ class DbDataReaderStoredFlowSource extends StoredFlowSource {
   }
 }
 
-/**
- * An expression that accesses a method of `DbDataReader` or a sub-class.
- */
+/** An expression that accesses a method of `DbDataReader` or a sub-class. */
 class DbDataReaderMethodStoredFlowSource extends StoredFlowSource {
   DbDataReaderMethodStoredFlowSource() {
     this.asExpr().(MethodCall).getTarget().getDeclaringType() = any(SystemDataCommon::DbDataReader dataReader
@@ -47,12 +39,15 @@ class DbDataReaderMethodStoredFlowSource extends StoredFlowSource {
   }
 }
 
-/**
- * An expression that accesses a property of `DbDataReader` or a sub-class.
- */
+/** An expression that accesses a property of `DbDataReader` or a sub-class. */
 class DbDataReaderPropertyStoredFlowSource extends StoredFlowSource {
   DbDataReaderPropertyStoredFlowSource() {
     this.asExpr().(PropertyAccess).getTarget().getDeclaringType() = any(SystemDataCommon::DbDataReader dataReader
       ).getASubType*()
   }
 }
+
+/** A read of a mapped property. */
+class EntityFrameworkMappedProperty extends StoredFlowSource {
+  EntityFrameworkMappedProperty() { this instanceof EntityFramework::StoredFlowSource }
+}

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

@@ -0,0 +1,7 @@
+| EntityFramework.cs:52:18:52:24 | access to property Name | EntityFramework.cs:47:34:47:42 | "tainted" |
+| EntityFramework.cs:53:18:53:34 | access to property Name | EntityFramework.cs:47:34:47:42 | "tainted" |
+| EntityFrameworkCore.cs:50:18:50:28 | access to local variable taintSource | EntityFrameworkCore.cs:47:31:47:39 | "tainted" |
+| EntityFrameworkCore.cs:51:18:51:46 | (...) ... | EntityFrameworkCore.cs:47:31:47:39 | "tainted" |
+| EntityFrameworkCore.cs:52:18:52:42 | (...) ... | EntityFrameworkCore.cs:47:31:47:39 | "tainted" |
+| EntityFrameworkCore.cs:60:18:60:24 | access to property Name | EntityFrameworkCore.cs:47:31:47:39 | "tainted" |
+| EntityFrameworkCore.cs:61:18:61:34 | access to property Name | EntityFrameworkCore.cs:47:31:47:39 | "tainted" |

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

@@ -0,0 +1,16 @@
+import csharp
+import semmle.code.csharp.dataflow.TaintTracking
+
+class MyConfiguration extends TaintTracking::Configuration {
+  MyConfiguration() { this = "EntityFramework dataflow" }
+
+  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()
+  }
+}
+
+from MyConfiguration config, DataFlow::Node source, DataFlow::Node sink
+where config.hasFlow(source, sink)
+select sink, source

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

@@ -0,0 +1,64 @@
+// semmle-extractor-options: /r:System.Data.dll /r:System.ComponentModel.Primitives.dll /r:System.ComponentModel.TypeConverter.dll ${testdir}/../../../resources/stubs/EntityFramework.cs ${testdir}/../../../resources/stubs/System.Data.cs /r:System.ComponentModel.TypeConverter.dll /r:System.Data.Common.dll
+
+using System.Data.Entity;
+using System.ComponentModel.DataAnnotations.Schema;
+using System.Data;
+using System.Data.Common;
+
+namespace EFTests
+{
+    class Person
+    {
+        public int Id { get; set; }
+        public string Name { get; set; }
+
+        [NotMapped]
+        public int Age { get; set; }
+    }
+
+    class MyContext : DbContext
+    {
+        DbSet<Person> person { get; set; }
+
+        void FlowSources()
+        {
+            var p = new Person();
+            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 = "";  // SqlExpr
+
+            // System.Data.SqlClient.SqlCommand.SqlCommand
+            new System.Data.SqlClient.SqlCommand("");  // SqlExpr
+
+            this.Database.ExecuteSqlCommand("");  // SqlExpr
+            await this.Database.ExecuteSqlCommandAsync("");  // SqlExpr
+        }
+
+        void TestDataFlow()
+        {
+            string taintSource = "tainted";
+
+            // Tainted via database, even though technically there were no reads or writes to the database in this particular case.
+            var p1 = new Person { Name = taintSource };
+            var p2 = new Person();
+            Sink(p2.Name);  // Tainted
+            Sink(new Person().Name);  // Tainted
+
+            p1.Age = int.Parse(taintSource);
+            Sink(p2.Age);  // Not tainted due to NotMappedAttribute
+        }
+
+        void Sink(object @object)
+        {
+        }
+    }
+
+}

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

@@ -0,0 +1,71 @@
+using Microsoft.EntityFrameworkCore;
+using System;
+using System.ComponentModel.DataAnnotations.Schema;
+using System.Data.Common;
+
+namespace EFCoreTests
+{
+    class Person
+    {
+      public int Id { get; set; }
+      public string Name { get; set; }
+    
+      [NotMapped]
+      public int Age { get; set; }
+    }
+
+    class MyContext : DbContext
+    {
+        DbSet<Person> person;
+
+        void FlowSources()
+        {
+            var p = new Person();
+            var id = p.Id;  // Remote flow source
+            var name = p.Name;  // Remote flow source
+            var age = p.Age;  // Not a remote flow source
+        }
+
+        Microsoft.EntityFrameworkCore.Storage.IRawSqlCommandBuilder builder;
+
+        async void SqlExprs()
+        {
+            // Microsoft.EntityFrameworkCore.RelationalDatabaseFacadeExtensions.ExecuteSqlCommand
+            this.Database.ExecuteSqlCommand("");  // SqlExpr
+            await this.Database.ExecuteSqlCommandAsync("");  // SqlExpr
+
+            // Microsoft.EntityFrameworkCore.Storage.IRawSqlCommandBuilder.Build
+            builder.Build("");  // SqlExpr
+
+            // Microsoft.EntityFrameworkCore.RawSqlString
+            new RawSqlString("");  // SqlExpr
+            RawSqlString str = "";  // SqlExpr
+        }
+
+        void TestDataFlow()
+        {
+            var taintSource = "tainted";
+            var untaintedSource = "untainted";
+
+            Sink(taintSource);  // Tainted
+            Sink(new RawSqlString(taintSource));  // Tainted
+            Sink((RawSqlString)taintSource);  // Tainted
+            Sink((RawSqlString)(FormattableString)$"{taintSource}");  // Not tainted
+
+            // Tainted via database, even though technically there were no reads or writes to the database in this particular case.
+            var p1 = new Person { Name = taintSource };
+            p1.Name = untaintedSource;
+            var p2 = new Person();
+
+            Sink(p2.Name);  // Tainted
+            Sink(new Person().Name);  // Tainted
+
+            p1.Age = int.Parse(taintSource);
+            Sink(p2.Age);  // Not tainted due to NotMappedAttribute
+        }
+
+        void Sink(object @object)
+        {
+        }
+    }
+}

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

@@ -0,0 +1,4 @@
+| EntityFramework.cs:12:20:12:21 | Id |
+| EntityFramework.cs:13:23:13:26 | Name |
+| EntityFrameworkCore.cs:10:18:10:19 | Id |
+| EntityFrameworkCore.cs:11:21:11:24 | Name |

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

@@ -0,0 +1,5 @@
+import csharp
+import semmle.code.csharp.frameworks.EntityFramework
+
+from EntityFramework::MappedProperty property
+select property

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

@@ -0,0 +1,11 @@
+| EntityFramework.cs:36:13:36:36 | ... = ... |
+| EntityFramework.cs:39:13:39:52 | object creation of type SqlCommand |
+| EntityFramework.cs:41:13:41:47 | call to method ExecuteSqlCommand |
+| EntityFramework.cs:42:19:42:58 | call to method ExecuteSqlCommandAsync |
+| EntityFrameworkCore.cs:34:13:34:47 | call to method ExecuteSqlCommand |
+| EntityFrameworkCore.cs:35:19:35:58 | call to method ExecuteSqlCommandAsync |
+| EntityFrameworkCore.cs:38:13:38:29 | call to method Build |
+| EntityFrameworkCore.cs:41:13:41:32 | object creation of type RawSqlString |
+| EntityFrameworkCore.cs:42:32:42:33 | call to operator implicit conversion |
+| EntityFrameworkCore.cs:51:18:51:46 | object creation of type RawSqlString |
+| EntityFrameworkCore.cs:52:18:52:42 | call to operator implicit conversion |

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

@@ -0,0 +1,5 @@
+import csharp
+import semmle.code.csharp.frameworks.Sql
+
+from SqlExpr expr
+select expr

csharp/ql/test/library-tests/frameworks/EntityFramework/StoredFlowSources.expected

@@ -0,0 +1,8 @@
+| EntityFramework.cs:26:22:26:25 | access to property Id |
+| EntityFramework.cs:27:24:27:29 | access to property Name |
+| EntityFramework.cs:52:18:52:24 | access to property Name |
+| EntityFramework.cs:53:18:53:34 | access to property Name |
+| EntityFrameworkCore.cs:24:22:24:25 | access to property Id |
+| EntityFrameworkCore.cs:25:24:25:29 | access to property Name |
+| EntityFrameworkCore.cs:60:18:60:24 | access to property Name |
+| EntityFrameworkCore.cs:61:18:61:34 | access to property Name |

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

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

csharp/ql/test/library-tests/security/dataflow/flowsources/entity.cs

@@ -32,14 +32,14 @@ public class BloggingContext : DbContext
         DbRawSqlQuery<Blog> blogs = Database.SqlQuery<Blog>("SELECT * FROM Blogs");
         foreach (var blog in blogs)
         {
-            // This will be a sink because it is an access of an entity property
+            // This will be a source because it is an access of an entity property
             Console.WriteLine(blog.Name);
         }
 
         DbRawSqlQuery<string> blogNames = Database.SqlQuery<string>("SELECT Name FROM Blogs");
         foreach (var blogName in blogNames)
         {
-            // This will be a sink because it is returned from an SqlQuery
+            // This will be a source because it is returned from an SqlQuery
             Console.WriteLine(blogName);
         }
     }

csharp/ql/test/resources/stubs/EntityFramework.cs

@@ -1,6 +1,8 @@
 using System.Collections.Generic;
 using System.Collections;
 using System.ComponentModel;
+using System.Threading.Tasks;
+using System;
 
 namespace System.Data.Entity
 {
@@ -16,6 +18,7 @@ namespace System.Data.Entity
     {
         public int ExecuteSqlQuery(string sql, params object[] parameters) => 0;
         public int ExecuteSqlCommand(string sql, params object[] parameters) => 0;
+        public async Task ExecuteSqlCommandAsync(string sql, params object[] parameters) => throw null;
         public Infrastructure.DbRawSqlQuery<T> SqlQuery<T>(string sql, params object[] parameters) => null;
     }
 
@@ -41,3 +44,52 @@ namespace System.Data.Entity.Infrastructure
         IList IListSource.GetList() => null;
     }
 }
+
+namespace Microsoft.EntityFrameworkCore
+{
+    public class DbSet<T>
+    {
+    }
+
+    public class DbContext : IDisposable
+    {
+        public void Dispose() { }
+        public virtual Infrastructure.DatabaseFacade Database => null;
+        // public Infrastructure.DbRawSqlQuery<TElement> SqlQuery<TElement>(string sql, params object[] parameters) => null;
+    }
+
+    namespace Infrastructure
+    {
+      public class DatabaseFacade
+      {
+      }
+    }
+
+    public static class RelationalDatabaseFacaseExtensions
+    {
+      public static void ExecuteSqlCommand(this Infrastructure.DatabaseFacade db, string sql, params object[] parameters) {}
+      public static Task ExecuteSqlCommandAsync(this Infrastructure.DatabaseFacade db, string sql, params object[] parameters) => throw null;
+    }
+
+    struct RawSqlString
+    {
+        public RawSqlString(string str) { }
+        public static implicit operator Microsoft.EntityFrameworkCore.RawSqlString (FormattableString fs) => throw null;
+        public static implicit operator Microsoft.EntityFrameworkCore.RawSqlString (string s) => throw null;
+    }
+}
+
+namespace System.ComponentModel.DataAnnotations.Schema
+{
+    class NotMappedAttribute : Attribute
+    {
+    }
+}
+
+namespace Microsoft.EntityFrameworkCore.Storage
+{
+    interface IRawSqlCommandBuilder
+    {
+        void Build(string sql);
+    }
+}

csharp/ql/test/resources/stubs/System.Data.cs

@@ -71,6 +71,7 @@ namespace System.Data
         IDataReader ExecuteReader();
         CommandType CommandType { get; set; }
         IDataParameterCollection Parameters { get; set; }
+        string CommandText { get; set; }
     }
 
     public interface IDataReader
@@ -117,13 +118,14 @@ namespace System.Data.Common
         public virtual string GetString(int i) => "";
     }
 
-    public class DbCommand : IDbCommand, IDisposable
+    public abstract class DbCommand : IDbCommand, IDisposable
     {
         public DbDataReader ExecuteReader() => null;
         public CommandType CommandType { get; set; }
         public IDataParameterCollection Parameters { get; set; }
         IDataReader IDbCommand.ExecuteReader() => null;
         public void Dispose() { }
+        public string CommandText { get; set; }
     }
 
     public class DbDataAdapter : IDataAdapter, IDbDataAdapter