PS: Add flow through string subexpressions and accept test changes.

Author: MathiasVP

powershell/ql/lib/semmle/code/powershell/controlflow/CfgNodes.qll

@@ -965,7 +965,7 @@ module ExprNodes {
 
     override ExpandableSubExpr getExpr() { result = e }
 
-    ExprCfgNode getSubExpr() { e.hasCfgChild(e.getExpr(), this, result) }
+    StmtNodes::StmtBlockCfgNode getSubExpr() { e.hasCfgChild(e.getExpr(), this, result) }
   }
 
   private class UsingExprChildMapping extends ExprChildMapping, UsingExpr {

powershell/ql/lib/semmle/code/powershell/dataflow/internal/TaintTrackingPrivate.qll

@@ -4,6 +4,7 @@ private import TaintTrackingPublic
 private import semmle.code.powershell.Cfg
 private import semmle.code.powershell.dataflow.DataFlow
 private import FlowSummaryImpl as FlowSummaryImpl
+private import PipelineReturns as PipelineReturns
 
 /**
  * Holds if `node` should be a sanitizer in all global taint flow configurations
@@ -21,6 +22,44 @@ predicate defaultImplicitTaintRead(DataFlow::Node node, DataFlow::ContentSet c)
   c.isAnyPositional()
 }
 
+private module ExpandableSubExprInput implements PipelineReturns::InputSig {
+  additional predicate isSourceImpl(
+    CfgNodes::ExprNodes::ExpandableSubExprCfgNode expandableSubExpr, CfgNodes::AstCfgNode source
+  ) {
+    source = expandableSubExpr.getSubExpr()
+  }
+
+  predicate isSource(CfgNodes::AstCfgNode source) { isSourceImpl(_, source) }
+}
+
+/**
+ * Holds if `nodeFrom` flows to `nodeTo` via string interpolation.
+ */
+predicate stringInterpolationTaintStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
+  // flow from $x to "Hello $x"
+  exists(CfgNodes::ExprNodes::ExpandableStringExprCfgNode es |
+    nodeFrom.asExpr() = es.getAnExpr() and
+    nodeTo.asExpr() = es
+  )
+  or
+  // Flow from $x to $($x)
+  exists(
+    CfgNodes::ExprNodes::ExpandableSubExprCfgNode es,
+    CfgNodes::StmtNodes::StmtBlockCfgNode blockStmt
+  |
+    ExpandableSubExprInput::isSourceImpl(es, blockStmt) and
+    nodeFrom.asExpr() = PipelineReturns::Make<ExpandableSubExprInput>::getAReturn(blockStmt) and
+    nodeTo.asExpr() = es
+  )
+}
+
+predicate operationTaintStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
+  exists(CfgNodes::ExprNodes::OperationCfgNode op |
+    op = nodeTo.asExpr() and
+    op.getAnOperand() = nodeFrom.asExpr()
+  )
+}
+
 cached
 private module Cached {
   private import semmle.code.powershell.dataflow.internal.DataFlowImplCommon as DataFlowImplCommon
@@ -36,16 +75,10 @@ private module Cached {
   predicate defaultAdditionalTaintStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo, string model) {
     (
       // Flow from an operand to an operation
-      exists(CfgNodes::ExprNodes::OperationCfgNode op |
-        op = nodeTo.asExpr() and
-        op.getAnOperand() = nodeFrom.asExpr()
-      )
+      operationTaintStep(nodeFrom, nodeTo)
       or
       // Flow through string interpolation
-      exists(CfgNodes::ExprNodes::ExpandableStringExprCfgNode es |
-        nodeFrom.asExpr() = es.getAnExpr() and
-        nodeTo.asExpr() = es
-      )
+      stringInterpolationTaintStep(nodeFrom, nodeTo)
       or
       // Although flow through collections is modeled precisely using stores/reads, we still
       // allow flow out of a _tainted_ collection. This is needed in order to support taint-

powershell/ql/lib/semmle/code/powershell/dataflow/internal/TaintTrackingPublic.qll

@@ -1,5 +1,5 @@
 private import powershell
-private import TaintTrackingPrivate
+private import TaintTrackingPrivate as Private
 private import semmle.code.powershell.Cfg
 private import semmle.code.powershell.dataflow.DataFlow
 
@@ -19,4 +19,8 @@ predicate localExprTaint(CfgNodes::ExprCfgNode e1, CfgNodes::ExprCfgNode e2) {
   localTaintStep*(DataFlow::exprNode(e1), DataFlow::exprNode(e2))
 }
 
-predicate localTaintStep = localTaintStepCached/2;
+predicate stringInterpolationTaintStep = Private::stringInterpolationTaintStep/2;
+
+predicate operationTaintStep = Private::operationTaintStep/2;
+
+predicate localTaintStep = Private::localTaintStepCached/2;

powershell/ql/test/query-tests/security/cwe-089/SqlInjection.expected

@@ -4,12 +4,15 @@ edges
 | test.ps1:1:1:1:10 | userinput | test.ps1:17:24:17:76 | SELECT * FROM MyTable WHERE MyColumn = '$userinput' | provenance |  |
 | test.ps1:1:1:1:10 | userinput | test.ps1:28:24:28:76 | SELECT * FROM MyTable WHERE MyColumn = '$userinput' | provenance |  |
 | test.ps1:1:1:1:10 | userinput | test.ps1:78:13:78:59 | SELECT * FROM Customers WHERE id = $userinput | provenance |  |
+| test.ps1:1:1:1:10 | userinput | test.ps1:128:28:128:37 | userinput | provenance |  |
 | test.ps1:1:14:1:45 | Call to read-host | test.ps1:1:1:1:10 | userinput | provenance | Src:MaD:0  |
 | test.ps1:4:1:4:6 | query | test.ps1:5:72:5:77 | query | provenance |  |
 | test.ps1:8:1:8:6 | query | test.ps1:9:72:9:77 | query | provenance |  |
 | test.ps1:72:1:72:11 | QueryConn2 [element Query] | test.ps1:81:15:81:25 | QueryConn2 | provenance |  |
 | test.ps1:72:15:79:1 | ${...} [element Query] | test.ps1:72:1:72:11 | QueryConn2 [element Query] | provenance |  |
 | test.ps1:78:13:78:59 | SELECT * FROM Customers WHERE id = $userinput | test.ps1:72:15:79:1 | ${...} [element Query] | provenance |  |
+| test.ps1:121:9:121:56 | unvalidated | test.ps1:125:92:125:143 | SELECT * FROM Customers where id = $($unvalidated) | provenance |  |
+| test.ps1:128:28:128:37 | userinput | test.ps1:121:9:121:56 | unvalidated | provenance |  |
 nodes
 | test.ps1:1:1:1:10 | userinput | semmle.label | userinput |
 | test.ps1:1:14:1:45 | Call to read-host | semmle.label | Call to read-host |
@@ -23,10 +26,14 @@ nodes
 | test.ps1:72:15:79:1 | ${...} [element Query] | semmle.label | ${...} [element Query] |
 | test.ps1:78:13:78:59 | SELECT * FROM Customers WHERE id = $userinput | semmle.label | SELECT * FROM Customers WHERE id = $userinput |
 | test.ps1:81:15:81:25 | QueryConn2 | semmle.label | QueryConn2 |
+| test.ps1:121:9:121:56 | unvalidated | semmle.label | unvalidated |
+| test.ps1:125:92:125:143 | SELECT * FROM Customers where id = $($unvalidated) | semmle.label | SELECT * FROM Customers where id = $($unvalidated) |
+| test.ps1:128:28:128:37 | userinput | semmle.label | userinput |
 subpaths
 #select
 | test.ps1:5:72:5:77 | query | test.ps1:1:14:1:45 | Call to read-host | test.ps1:5:72:5:77 | query | This SQL query depends on a $@. | test.ps1:1:14:1:45 | Call to read-host | read from stdin |
 | test.ps1:9:72:9:77 | query | test.ps1:1:14:1:45 | Call to read-host | test.ps1:9:72:9:77 | query | This SQL query depends on a $@. | test.ps1:1:14:1:45 | Call to read-host | read from stdin |
 | test.ps1:17:24:17:76 | SELECT * FROM MyTable WHERE MyColumn = '$userinput' | test.ps1:1:14:1:45 | Call to read-host | test.ps1:17:24:17:76 | SELECT * FROM MyTable WHERE MyColumn = '$userinput' | This SQL query depends on a $@. | test.ps1:1:14:1:45 | Call to read-host | read from stdin |
 | test.ps1:28:24:28:76 | SELECT * FROM MyTable WHERE MyColumn = '$userinput' | test.ps1:1:14:1:45 | Call to read-host | test.ps1:28:24:28:76 | SELECT * FROM MyTable WHERE MyColumn = '$userinput' | This SQL query depends on a $@. | test.ps1:1:14:1:45 | Call to read-host | read from stdin |
 | test.ps1:81:15:81:25 | QueryConn2 | test.ps1:1:14:1:45 | Call to read-host | test.ps1:81:15:81:25 | QueryConn2 | This SQL query depends on a $@. | test.ps1:1:14:1:45 | Call to read-host | read from stdin |
+| test.ps1:125:92:125:143 | SELECT * FROM Customers where id = $($unvalidated) | test.ps1:1:14:1:45 | Call to read-host | test.ps1:125:92:125:143 | SELECT * FROM Customers where id = $($unvalidated) | This SQL query depends on a $@. | test.ps1:1:14:1:45 | Call to read-host | read from stdin |