Merge pull request #123 from microsoft/flow-through-ValueFromPipelineByPropertyName

PS: Flow through `ValueFromPipelineByPropertyName` parameters

Author: MathiasVP

powershell/ql/lib/semmle/code/powershell/Function.qll

@@ -51,10 +51,10 @@ abstract private class AbstractFunction extends Ast {
     result.getIndex() = i
   }
 
-  final Parameter getParameterExcludingPipline(int i) {
+  final Parameter getParameterExcludingPiplines(int i) {
     result = this.getFunctionParameter(i)
     or
-    result = this.getBody().getParamBlock().getParameterExcludingPipline(i)
+    result = this.getBody().getParamBlock().getParameterExcludingPiplines(i)
   }
 
   final Parameter getThisParameter() {

powershell/ql/lib/semmle/code/powershell/NamedAttributeArgument.qll

@@ -13,3 +13,7 @@ class NamedAttributeArgument extends @named_attribute_argument, Ast {
 class ValueFromPipelineAttribute extends NamedAttributeArgument {
   ValueFromPipelineAttribute() { this.getName() = "ValueFromPipeline" }
 }
+
+class ValueFromPipelineByPropertyName extends NamedAttributeArgument {
+  ValueFromPipelineByPropertyName() { this.getName() = "ValueFromPipelineByPropertyName" }
+}

powershell/ql/lib/semmle/code/powershell/ParamBlock.qll

@@ -15,7 +15,7 @@ class ParamBlock extends @param_block, Ast {
 
   Parameter getParameter(int i) { result.hasParameterBlock(this, i) }
 
-  Parameter getParameterExcludingPipline(int i) { result.hasParameterBlockExcludingPipeline(this, i) }
+  Parameter getParameterExcludingPiplines(int i) { result.hasParameterBlockExcludingPipelines(this, i) }
 
   Parameter getAParameter() { result = this.getParameter(_) }
 }

powershell/ql/lib/semmle/code/powershell/Variable.qll

@@ -10,14 +10,16 @@ private predicate hasParameterBlockImpl(Internal::Parameter p, ParamBlock block,
   param_block_parameter(block, i, p)
 }
 
-private predicate hasParameterBlockExcludingPipelineImpl(
+private predicate hasParameterBlockExcludingPipelinesImpl(
   Internal::Parameter p, ParamBlock block, int i
 ) {
   p =
     rank[i + 1](Internal::Parameter cand, int j |
       hasParameterBlockImpl(cand, block, j) and
       not cand.getAnAttribute().(Attribute).getANamedArgument() instanceof
-        ValueFromPipelineAttribute
+        ValueFromPipelineAttribute and
+      not cand.getAnAttribute().(Attribute).getANamedArgument() instanceof
+        ValueFromPipelineByPropertyName
     |
       cand order by j
     )
@@ -69,7 +71,7 @@ private class ParameterImpl extends TParameterImpl {
 
   predicate hasParameterBlock(ParamBlock block, int i) { none() }
 
-  predicate hasParameterBlockExcludingPipeline(ParamBlock block, int i) { none() }
+  predicate hasParameterBlockExcludingPipelines(ParamBlock block, int i) { none() }
 
   predicate isFunctionParameter(Function f, int i) { none() }
 
@@ -84,6 +86,8 @@ private class ParameterImpl extends TParameterImpl {
   }
 
   abstract predicate isPipeline();
+
+  abstract predicate isPipelineByPropertyName();
 }
 
 private class InternalParameter extends ParameterImpl, TInternalParameter {
@@ -101,8 +105,8 @@ private class InternalParameter extends ParameterImpl, TInternalParameter {
     hasParameterBlockImpl(p, block, i)
   }
 
-  override predicate hasParameterBlockExcludingPipeline(ParamBlock block, int i) {
-    hasParameterBlockExcludingPipelineImpl(p, block, i)
+  override predicate hasParameterBlockExcludingPipelines(ParamBlock block, int i) {
+    hasParameterBlockExcludingPipelinesImpl(p, block, i)
   }
 
   override predicate isFunctionParameter(Function f, int i) { isFunctionParameterImpl(p, f, i) }
@@ -114,6 +118,10 @@ private class InternalParameter extends ParameterImpl, TInternalParameter {
   override predicate isPipeline() {
     this.getAnAttribute().getANamedArgument() instanceof ValueFromPipelineAttribute
   }
+
+  override predicate isPipelineByPropertyName() {
+    this.getAnAttribute().getANamedArgument() instanceof ValueFromPipelineByPropertyName
+  }
 }
 
 /**
@@ -147,6 +155,8 @@ private class Underscore extends ParameterImpl, TUnderscore {
 
   final override predicate isPipeline() { any() }
 
+  final override predicate isPipelineByPropertyName() { none() }
+
   final override predicate isFunctionParameter(Function f, int i) { f.getBody() = scope and i = -1 }
 }
 
@@ -164,6 +174,8 @@ private class ThisParameter extends ParameterImpl, TThisParameter {
   final override Attribute getAnAttribute() { none() }
 
   final override predicate isPipeline() { none() }
+
+  final override predicate isPipelineByPropertyName() { none() }
 }
 
 private newtype TVariable =
@@ -241,8 +253,8 @@ class Parameter extends AbstractLocalScopeVariable, TParameter {
 
   predicate hasParameterBlock(ParamBlock block, int i) { p.hasParameterBlock(block, i) }
 
-  predicate hasParameterBlockExcludingPipeline(ParamBlock block, int i) {
-    p.hasParameterBlockExcludingPipeline(block, i)
+  predicate hasParameterBlockExcludingPipelines(ParamBlock block, int i) {
+    p.hasParameterBlockExcludingPipelines(block, i)
   }
 
   predicate isFunctionParameter(Function f, int i) { p.isFunctionParameter(f, i) }
@@ -261,14 +273,14 @@ class Parameter extends AbstractLocalScopeVariable, TParameter {
    */
   int getIndex() { result = this.getFunctionIndex() or result = this.getBlockIndex() }
 
-  int getIndexExcludingPipeline() {
-    result = this.getFunctionIndex() or result = this.getBlockIndexExcludingPipeline()
+  int getIndexExcludingPipelines() {
+    result = this.getFunctionIndex() or result = this.getBlockIndexExcludingPipelines()
   }
 
   /** Gets the index of this parameter in the parameter block, if any. */
   int getBlockIndex() { this.hasParameterBlock(_, result) }
 
-  int getBlockIndexExcludingPipeline() { this.hasParameterBlockExcludingPipeline(_, result) }
+  int getBlockIndexExcludingPipelines() { this.hasParameterBlockExcludingPipelines(_, result) }
 
   /** Gets the index of this parameter in the function, if any. */
   int getFunctionIndex() { this.isFunctionParameter(_, result) }
@@ -278,6 +290,8 @@ class Parameter extends AbstractLocalScopeVariable, TParameter {
   Attribute getAnAttribute() { result = p.getAnAttribute() }
 
   predicate isPipeline() { p.isPipeline() }
+
+  predicate isPipelineByPropertyName() { p.isPipelineByPropertyName() }
 }
 
 class PipelineParameter extends Parameter {

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

@@ -84,7 +84,9 @@ module SsaFlow {
   }
 
   predicate localFlowStep(SsaImpl::DefinitionExt def, Node nodeFrom, Node nodeTo, boolean isUseStep) {
-    Impl::localFlowStep(def, asNode(nodeFrom), asNode(nodeTo), isUseStep)
+    Impl::localFlowStep(def, asNode(nodeFrom), asNode(nodeTo), isUseStep) and
+    // Flow out of property name parameter nodes are covered by `readStep`.
+    not nodeFrom instanceof PipelineByPropertyNameParameter
   }
 
   predicate localMustFlowStep(SsaImpl::DefinitionExt def, Node nodeFrom, Node nodeTo) {
@@ -471,7 +473,7 @@ private module ParameterNodes {
         // keywords in S are specified.
         exists(int i, int j, string name, NamedSet ns, Function f |
           pos.isPositional(j, ns) and
-          parameter.getIndexExcludingPipeline() = i and
+          parameter.getIndexExcludingPipelines() = i and
           f = parameter.getFunction() and
           f = ns.getAFunction() and
           name = parameter.getName() and
@@ -480,12 +482,12 @@ private module ParameterNodes {
             i -
               count(int k, Parameter p |
                 k < i and
-                p = f.getParameterExcludingPipline(k) and
+                p = f.getParameterExcludingPiplines(k) and
                 p.getName() = ns.getAName()
               )
         )
         or
-        parameter.isPipeline() and
+        (parameter.isPipeline() or parameter.isPipelineByPropertyName()) and
         pos.isPipeline()
       )
     }
@@ -498,6 +500,12 @@ private module ParameterNodes {
 
     override string toStringImpl() { result = parameter.toString() }
   }
+
+  class PipelineByPropertyNameParameter extends NormalParameterNode {
+    PipelineByPropertyNameParameter() { this.getParameter().isPipelineByPropertyName() }
+
+    string getPropretyName() { result = this.getParameter().getName() }
+  }
 }
 
 import ParameterNodes
@@ -741,14 +749,21 @@ predicate readStep(Node node1, ContentSet c, Node node2) {
   exists(CfgNode cfgNode |
     node1 = TPreReturnNodeImpl(cfgNode, true) and
     node2 = TImplicitWrapNode(cfgNode, true) and
-    c.isSingleton(any(Content::KnownElementContent ec))
+    c.isSingleton(any(Content::KnownElementContent ec | exists(ec.getIndex().asInt())))
   )
   or
   c.isAnyElement() and
   exists(SsaImpl::DefinitionExt def |
     node1.(ProcessNode).getIteratorVariable() = def.getSourceVariable() and
     SsaImpl::firstRead(def, node2.asExpr())
   )
+  or
+  exists(Content::KnownElementContent ec, SsaImpl::DefinitionExt def |
+    c.isSingleton(ec) and
+    node1.(PipelineByPropertyNameParameter).getPropretyName() = ec.getIndex().asString() and
+    def.getSourceVariable() = node1.(PipelineByPropertyNameParameter).getParameter() and
+    SsaImpl::firstRead(def, node2.asExpr())
+  )
 }
 
 /**
@@ -770,13 +785,18 @@ predicate clearsContent(Node n, ContentSet c) {
  */
 predicate expectsContent(Node n, ContentSet c) {
   n = TPreReturnNodeImpl(_, true) and
-  c.isKnownOrUnknownElement(_)
+  c.isKnownOrUnknownElement(any(Content::KnownElementContent ec | exists(ec.getIndex().asInt())))
   or
   n = TImplicitWrapNode(_, false) and
   c.isSingleton(any(Content::UnknownElementContent ec))
   or
   n instanceof ProcessNode and
   c.isAnyElement()
+  or
+  exists(Content::KnownElementContent ec |
+    ec.getIndex().asString() = n.(PipelineByPropertyNameParameter).getPropretyName() and
+    c.isSingleton(ec)
+  )
 }
 
 class DataFlowType extends TDataFlowType {

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

@@ -340,7 +340,7 @@ import Cached
  * Only intended for internal use.
  */
 class DefinitionExt extends Impl::DefinitionExt {
-  VarReadAccessCfgNode getARead() { result = getARead(this) }
+  AstCfgNode getARead() { result = getARead(this) }
 
   override string toString() { result = this.(Ssa::Definition).toString() }
 

powershell/ql/test/library-tests/dataflow/pipeline/test.expected

@@ -30,6 +30,11 @@ edges
 | test.ps1:31:1:31:7 | ...,... [element 1] | test.ps1:25:14:25:16 | _ [element 1] | provenance |  |
 | test.ps1:31:1:31:7 | ...,... [element 1] | test.ps1:31:1:31:7 | ...,... [element 1] | provenance |  |
 | test.ps1:31:5:31:7 | y | test.ps1:31:1:31:7 | ...,... [element 1] | provenance |  |
+| test.ps1:34:11:34:58 | x [element x] | test.ps1:36:10:36:12 | x | provenance |  |
+| test.ps1:39:6:39:17 | Source | test.ps1:40:23:40:25 | x | provenance |  |
+| test.ps1:40:1:40:26 | [...]... [element x] | test.ps1:34:11:34:58 | x [element x] | provenance |  |
+| test.ps1:40:17:40:26 | ${...} [element x] | test.ps1:40:1:40:26 | [...]... [element x] | provenance |  |
+| test.ps1:40:23:40:25 | x | test.ps1:40:17:40:26 | ${...} [element x] | provenance |  |
 nodes
 | test.ps1:2:10:2:21 | Source | semmle.label | Source |
 | test.ps1:3:10:3:21 | Source | semmle.label | Source |
@@ -63,6 +68,12 @@ nodes
 | test.ps1:31:1:31:7 | ...,... [element 1] | semmle.label | ...,... [element 1] |
 | test.ps1:31:1:31:7 | ...,... [element 1] | semmle.label | ...,... [element 1] |
 | test.ps1:31:5:31:7 | y | semmle.label | y |
+| test.ps1:34:11:34:58 | x [element x] | semmle.label | x [element x] |
+| test.ps1:36:10:36:12 | x | semmle.label | x |
+| test.ps1:39:6:39:17 | Source | semmle.label | Source |
+| test.ps1:40:1:40:26 | [...]... [element x] | semmle.label | [...]... [element x] |
+| test.ps1:40:17:40:26 | ${...} [element x] | semmle.label | ${...} [element x] |
+| test.ps1:40:23:40:25 | x | semmle.label | x |
 subpaths
 testFailures
 #select
@@ -73,3 +84,4 @@ testFailures
 | test.ps1:13:14:13:16 | x | test.ps1:20:6:20:17 | Source | test.ps1:13:14:13:16 | x | $@ | test.ps1:20:6:20:17 | Source | Source |
 | test.ps1:25:14:25:16 | _ | test.ps1:29:6:29:17 | Source | test.ps1:25:14:25:16 | _ | $@ | test.ps1:29:6:29:17 | Source | Source |
 | test.ps1:25:14:25:16 | _ | test.ps1:30:6:30:17 | Source | test.ps1:25:14:25:16 | _ | $@ | test.ps1:30:6:30:17 | Source | Source |
+| test.ps1:36:10:36:12 | x | test.ps1:39:6:39:17 | Source | test.ps1:36:10:36:12 | x | $@ | test.ps1:39:6:39:17 | Source | Source |

powershell/ql/test/library-tests/dataflow/pipeline/test.ps1

@@ -28,4 +28,13 @@ function consume2 {
 
 $x = Source "21"
 $y = Source "22"
-$x, $y | consume2
+$x, $y | consume2
+
+function consumeValueFromPipelineByPropertyName {
+    Param([Parameter(ValueFromPipelineByPropertyName)] $x)
+
+    Sink $x # $ hasValueFlow=23
+}
+
+$x = Source "23"
+[pscustomobject]@{x = $x} | consumeValueFromPipelineByPropertyName