Merge pull request github#2087 from calumgrant/cs/localexprflow

C#: Implement localExprFlow and localExprTaint

Author: hvitved

change-notes/1.23/analysis-csharp.md

@@ -40,5 +40,8 @@ The following changes in version 1.23 affect C# analysis in all applications.
   overriding `int explorationLimit()`.
 * `foreach` statements where the body is guaranteed to be executed at least once, such as `foreach (var x in new string[]{ "a", "b", "c" }) { ... }`, are now recognized by all analyses based on the control flow graph (such as SSA, data flow and taint tracking).
 * Fixed the control flow graph for `switch` statements where the `default` case was not the last case. This had caused the remaining cases to be unreachable. `SwitchStmt.getCase(int i)` now puts the `default` case last.
+* There is now a `DataFlow::localExprFlow` predicate and a
+  `TaintTracking::localExprTaint` predicate to make it easy to use the most
+  common case of local data flow and taint: from one `Expr` to another.
 
 ## Changes to autobuilder

csharp/ql/src/Bad Practices/Implementation Hiding/ExposeRepresentation.ql

@@ -32,7 +32,7 @@ predicate returnsCollection(Callable c, Field f) {
 predicate mayWriteToCollection(Expr modified) {
   modified instanceof CollectionModificationAccess
   or
-  exists(Expr mid | mayWriteToCollection(mid) | localFlow(exprNode(modified), exprNode(mid)))
+  exists(Expr mid | mayWriteToCollection(mid) | localExprFlow(modified, mid))
   or
   exists(MethodCall mid, Callable c | mayWriteToCollection(mid) |
     mid.getTarget() = c and

csharp/ql/src/Dead Code/DeadStoreOfLocal.ql

@@ -62,7 +62,7 @@ predicate nonEscapingCall(Call c) {
 predicate mayEscape(LocalVariable v) {
   exists(Callable c, Expr e, Expr succ | c = getACapturingCallableAncestor(v) |
     e = getADelegateExpr(c) and
-    DataFlow::localFlow(DataFlow::exprNode(e), DataFlow::exprNode(succ)) and
+    DataFlow::localExprFlow(e, succ) and
     not succ = any(DelegateCall dc).getDelegateExpr() and
     not succ = any(Cast cast).getExpr() and
     not succ = any(Call call | nonEscapingCall(call)).getAnArgument() and

csharp/ql/src/Security Features/CWE-119/LocalUnvalidatedArithmetic.ql

@@ -22,11 +22,9 @@ where
   // `add` is performing pointer arithmetic
   add.getType() instanceof PointerType and
   // one of the operands comes, in zero or more steps, from a virtual method call
-  DataFlow::localFlow(DataFlow::exprNode(taintSrc), DataFlow::exprNode(add.getAnOperand())) and
+  DataFlow::localExprFlow(taintSrc, add.getAnOperand()) and
   // virtual method call result has not been validated
-  not exists(Expr check, ComparisonOperation cmp |
-    DataFlow::localFlow(DataFlow::exprNode(taintSrc), DataFlow::exprNode(check))
-  |
+  not exists(Expr check, ComparisonOperation cmp | DataFlow::localExprFlow(taintSrc, check) |
     cmp.getAnOperand() = check and
     add.getAnOperand().(GuardedExpr).isGuardedBy(cmp, check, _)
   )

csharp/ql/src/semmle/code/csharp/Property.qll

@@ -520,7 +520,7 @@ class IndexerProperty extends Property {
   pragma[nomagic]
   private IndexerCall getAnIndexerCall0() {
     exists(Expr qualifier | qualifier = result.getQualifier() |
-      DataFlow::localFlow(DataFlow::exprNode(this.getAnAccess()), DataFlow::exprNode(qualifier))
+      DataFlow::localExprFlow(this.getAnAccess(), qualifier)
     )
   }
 

csharp/ql/src/semmle/code/csharp/dataflow/internal/DataFlowPublic.qll

@@ -164,6 +164,12 @@ predicate localFlowStep(Node nodeFrom, Node nodeTo) {
  */
 predicate localFlow(Node source, Node sink) { localFlowStep*(source, sink) }
 
+/**
+ * Holds if data can flow from `e1` to `e2` in zero or more
+ * local (intra-procedural) steps.
+ */
+predicate localExprFlow(Expr e1, Expr e2) { localFlow(exprNode(e1), exprNode(e2)) }
+
 /**
  * A data flow node that jumps between callables. This can be extended in
  * framework code to add additional data flow steps.

csharp/ql/src/semmle/code/csharp/dataflow/internal/TaintTrackingPublic.qll

@@ -7,6 +7,14 @@ private import TaintTrackingPrivate
  */
 predicate localTaint(DataFlow::Node source, DataFlow::Node sink) { localTaintStep*(source, sink) }
 
+/**
+ * Holds if taint can flow from `e1` to `e2` in zero or more
+ * local (intra-procedural) steps.
+ */
+predicate localExprTaint(Expr e1, Expr e2) {
+  localTaint(DataFlow::exprNode(e1), DataFlow::exprNode(e2))
+}
+
 /** A member (property or field) that is tainted if its containing object is tainted. */
 abstract class TaintedMember extends AssignableMember { }
 

csharp/ql/src/semmle/code/csharp/frameworks/system/Xml.qll

@@ -156,7 +156,7 @@ class XmlReaderSettingsCreation extends ObjectCreation {
     p = this.getType().(RefType).getAProperty() and
     exists(PropertyCall set, Expr arg |
       set.getTarget() = p.getSetter() and
-      DataFlow::localFlow(DataFlow::exprNode(this), DataFlow::exprNode(set.getQualifier())) and
+      DataFlow::localExprFlow(this, set.getQualifier()) and
       arg = set.getAnArgument() and
       result = getBitwiseOrOperand*(arg)
     )

csharp/ql/src/semmle/code/csharp/frameworks/system/text/RegularExpressions.qll

@@ -71,7 +71,7 @@ class RegexOperation extends Call {
       |
         // e.g. `new Regex(...).Match(...)`
         // or   `var r = new Regex(...); r.Match(...)`
-        DataFlow::localFlow(DataFlow::exprNode(this), DataFlow::exprNode(call.getQualifier()))
+        DataFlow::localExprFlow(this, call.getQualifier())
         or
         // e.g. `private string r = new Regex(...); public void foo() { r.Match(...); }`
         call.getQualifier().(FieldAccess).getTarget().getInitializer() = this

csharp/ql/src/semmle/code/csharp/security/dataflow/ZipSlip.qll

@@ -142,7 +142,7 @@ module ZipSlip {
       // not yet been resolved.
       not exists(MethodCall combineCall |
         combineCall.getTarget().hasQualifiedName("System.IO.Path", "Combine") and
-        DataFlow::localFlow(DataFlow::exprNode(combineCall), DataFlow::exprNode(q))
+        DataFlow::localExprFlow(combineCall, q)
       )
     }