Merge pull request #124 from microsoft/powershell-more-correct-value-from-property-name

PS: Allow for `ValueFromPipelineByPropertyName` to also read off an `ElementContent`

Author: MathiasVP

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

@@ -7,6 +7,8 @@ class NamedAttributeArgument extends @named_attribute_argument, Ast {
 
   string getName() { named_attribute_argument(this, result, _) }
 
+  predicate hasName(string s) { this.getName() = s }
+
   Expr getValue() { named_attribute_argument(this, _, result) }
 }
 

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

@@ -60,4 +60,12 @@ class ProcessBlock extends NamedBlock {
   ProcessBlock() { scriptBlock.getProcessBlock() = this }
 
   ScriptBlock getScriptBlock() { result = scriptBlock }
+
+  PipelineParameter getPipelineParameter() {
+    result = scriptBlock.getEnclosingFunction().getAParameter()
+  }
+
+  PipelineByPropertyNameParameter getAPipelineByPropertyNameParameter() {
+    result = scriptBlock.getEnclosingFunction().getAParameter()
+  }
 }

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

@@ -178,14 +178,27 @@ private class ThisParameter extends ParameterImpl, TThisParameter {
   final override predicate isPipelineByPropertyName() { none() }
 }
 
+private predicate isPipelineIteratorVariable(ParameterImpl p, ProcessBlock pb) {
+  p.isPipeline() and
+  pb.getEnclosingScope() = p.getEnclosingScope()
+}
+
+private predicate isPipelineByPropertyNameIteratorVariable(ParameterImpl p, ProcessBlock pb) {
+  p.isPipelineByPropertyName() and
+  pb.getEnclosingScope() = p.getEnclosingScope()
+}
+
 private newtype TVariable =
   TLocalVariable(string name, Scope scope) {
     not isParameterImpl(name, scope) and
     not name = "this" and // This is modeled as a parameter
     exists(VarAccess va | va.getUserPath() = name and scope = va.getEnclosingScope())
   } or
   TParameter(ParameterImpl p) or
-  TPipelineIteratorVariable(ProcessBlock pb)
+  TPipelineIteratorVariable(ProcessBlock pb) { isPipelineIteratorVariable(_, pb) } or
+  TPipelineByPropertyNameIteratorVariable(ParameterImpl p) {
+    isPipelineByPropertyNameIteratorVariable(p, _)
+  }
 
 private class AbstractVariable extends TVariable {
   abstract Location getLocation();
@@ -296,12 +309,22 @@ class Parameter extends AbstractLocalScopeVariable, TParameter {
 
 class PipelineParameter extends Parameter {
   PipelineParameter() { this.isPipeline() }
+
+  PipelineIteratorVariable getIteratorVariable() {
+    result.getProcessBlock().getEnclosingScope() = p.getEnclosingScope()
+  }
+}
+
+class PipelineByPropertyNameParameter extends Parameter {
+  PipelineByPropertyNameParameter() { this.isPipelineByPropertyName() }
+
+  PipelineByPropertyNameIteratorVariable getIteratorVariable() { result.getParameter() = this }
 }
 
 /**
  * The variable that represents the value of a pipeline during a process block.
  *
- * That is, it is _not_ the pipeline variable, but the value that is obtained by reading
+ * That is, it is _not_ the `ValueFromPipeline` variable, but the value that is obtained by reading
  * from the pipeline.
  */
 class PipelineIteratorVariable extends AbstractLocalScopeVariable, TPipelineIteratorVariable {
@@ -317,3 +340,28 @@ class PipelineIteratorVariable extends AbstractLocalScopeVariable, TPipelineIter
 
   ProcessBlock getProcessBlock() { result = pb }
 }
+
+/**
+ * The variable that represents the value of a pipeline that picks out a
+ * property specific property during a process block.
+ *
+ * That is, it is _not_ the `PipelineByPropertyName` variable, but the value that is obtained by reading
+ * from the pipeline.
+ */
+class PipelineByPropertyNameIteratorVariable extends AbstractLocalScopeVariable,
+  TPipelineByPropertyNameIteratorVariable
+{
+  private ParameterImpl p;
+
+  PipelineByPropertyNameIteratorVariable() { this = TPipelineByPropertyNameIteratorVariable(p) }
+
+  override Location getLocation() { result = p.getLocation() }
+
+  override string getName() { result = "pipeline iterator for " + p.toString() }
+
+  final override Scope getDeclaringScope() { result = p.getEnclosingScope() }
+
+  Parameter getParameter() { result = TParameter(p) }
+
+  ProcessBlock getProcessBlock() { isPipelineByPropertyNameIteratorVariable(p, result) }
+}

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

@@ -198,7 +198,11 @@ class ProcessBlockCfgNode extends NamedBlockCfgNode {
 
   override ProcessBlock getBlock() { result = block }
 
-  PipelineParameter getPipelineParameter() { result = block.getEnclosingFunction().getAParameter() }
+  PipelineParameter getPipelineParameter() { result = block.getPipelineParameter() }
+
+  PipelineByPropertyNameParameter getAPipelineByPropertyNameParameter() {
+    result = block.getAPipelineByPropertyNameParameter()
+  }
 }
 
 private class StmtBlockChildMapping extends NonExprChildMapping, StmtBlock {

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

@@ -76,17 +76,16 @@ module SsaFlow {
     or
     result.(Impl::ExprNode).getExpr() = n.asStmt()
     or
-    result.(Impl::ExprNode).getExpr() = n.(ProcessNode).getProcessBlock()
+    result.(Impl::ExprNode).getExpr() =
+      [n.(ProcessNode).getProcessBlock(), n.(ProcessPropertyByNameNode).getProcessBlock()]
     or
     result.(Impl::ExprPostUpdateNode).getExpr() = n.(PostUpdateNode).getPreUpdateNode().asExpr()
     or
     n = toParameterNode(result.(Impl::ParameterNode).getParameter())
   }
 
   predicate localFlowStep(SsaImpl::DefinitionExt def, Node nodeFrom, Node nodeTo, boolean 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
+    Impl::localFlowStep(def, asNode(nodeFrom), asNode(nodeTo), isUseStep)
   }
 
   predicate localMustFlowStep(SsaImpl::DefinitionExt def, Node nodeFrom, Node nodeTo) {
@@ -146,6 +145,12 @@ module VariableCapture {
   // TODO
 }
 
+private predicate isProcessPropertyByNameNode(
+  PipelineByPropertyNameIteratorVariable iter, ProcessBlock pb
+) {
+  pb.getEnclosingScope() = iter.getDeclaringScope()
+}
+
 /** A collection of cached types and predicates to be evaluated in the same stage. */
 cached
 private module Cached {
@@ -172,7 +177,10 @@ private module Cached {
     TPreReturnNodeImpl(CfgNodes::AstCfgNode n, Boolean isArray) { isMultiReturned(n) } or
     TImplicitWrapNode(CfgNodes::AstCfgNode n, Boolean shouldWrap) { isMultiReturned(n) } or
     TReturnNodeImpl(CfgScope scope) or
-    TProcessNode(ProcessBlock process)
+    TProcessNode(ProcessBlock process) or
+    TProcessPropertyByNameNode(PipelineByPropertyNameIteratorVariable iter) {
+      isProcessPropertyByNameNode(iter, _)
+    }
 
   cached
   Location getLocation(NodeImpl n) { result = n.getLocationImpl() }
@@ -501,8 +509,8 @@ private module ParameterNodes {
     override string toStringImpl() { result = parameter.toString() }
   }
 
-  class PipelineByPropertyNameParameter extends NormalParameterNode {
-    PipelineByPropertyNameParameter() { this.getParameter().isPipelineByPropertyName() }
+  class PipelineByPropertyNameParameterNode extends NormalParameterNode {
+    PipelineByPropertyNameParameterNode() { this.getParameter().isPipelineByPropertyName() }
 
     string getPropretyName() { result = this.getParameter().getName() }
   }
@@ -753,15 +761,30 @@ predicate readStep(Node node1, ContentSet c, Node node2) {
   )
   or
   c.isAnyElement() and
-  exists(SsaImpl::DefinitionExt def |
-    node1.(ProcessNode).getIteratorVariable() = def.getSourceVariable() and
+  exists(SsaImpl::DefinitionExt def, ProcessNode processNode, LocalScopeVariable iterator |
+    processNode = node1 and iterator = def.getSourceVariable()
+  |
+    processNode.getIteratorVariable() = iterator and
+    SsaImpl::firstRead(def, node2.asExpr())
+    or
+    processNode.getPropertyNameIteratorVariable() = iterator and
+    node2 = TProcessPropertyByNameNode(def.getSourceVariable())
+  )
+  or
+  exists(
+    Content::KnownElementContent ec, PipelineByPropertyNameParameter p, SsaImpl::DefinitionExt def
+  |
+    c.isSingleton(ec) and
+    p.getIteratorVariable() = node1.(ProcessPropertyByNameNode).getVariable() and
+    p.getName() = ec.getIndex().asString() and
+    def.getSourceVariable() = p.getIteratorVariable() 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
+    node1.(PipelineByPropertyNameParameterNode).getPropretyName() = ec.getIndex().asString() and
+    def.getSourceVariable() = node1.(PipelineByPropertyNameParameterNode).getParameter() and
     SsaImpl::firstRead(def, node2.asExpr())
   )
 }
@@ -792,11 +815,6 @@ predicate expectsContent(Node n, ContentSet c) {
   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 {
@@ -925,15 +943,39 @@ private class ProcessNode extends TProcessNode, NodeImpl {
 
   override Location getLocationImpl() { result = process.getLocation() }
 
-  override string toStringImpl() { result = process.toString() }
+  override string toStringImpl() { result = "process node for " + process.toString() }
 
   override predicate nodeIsHidden() { any() }
 
   PipelineIteratorVariable getIteratorVariable() { result.getProcessBlock() = process }
 
+  PipelineByPropertyNameIteratorVariable getPropertyNameIteratorVariable() {
+    result.getProcessBlock() = process
+  }
+
   CfgNodes::ProcessBlockCfgNode getProcessBlock() { result.getAstNode() = process }
 }
 
+private class ProcessPropertyByNameNode extends TProcessPropertyByNameNode, NodeImpl {
+  private PipelineByPropertyNameIteratorVariable iter;
+
+  ProcessPropertyByNameNode() { this = TProcessPropertyByNameNode(iter) }
+
+  PipelineByPropertyNameIteratorVariable getVariable() { result = iter }
+
+  override CfgScope getCfgScope() { result = iter.getDeclaringScope() }
+
+  override Location getLocationImpl() { result = iter.getLocation() }
+
+  override string toStringImpl() { result = "process node for " + iter.toString() }
+
+  override predicate nodeIsHidden() { any() }
+
+  CfgNodes::ProcessBlockCfgNode getProcessBlock() {
+    isProcessPropertyByNameNode(iter, result.getAstNode())
+  }
+}
+
 /** A node that performs a type cast. */
 class CastNode extends Node {
   CastNode() { none() }

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

@@ -68,10 +68,11 @@ predicate uninitializedWrite(Cfg::EntryBasicBlock bb, int i, LocalVariable v) {
   i = -1
 }
 
-predicate pipelineIteratorWrite(Cfg::BasicBlock bb, int i, PipelineIteratorVariable v) {
-  exists(ProcessBlockCfgNode process |
-    process = bb.getNode(i) and
-    v.getProcessBlock() = process.getAstNode()
+predicate pipelineIteratorWrite(Cfg::BasicBlock bb, int i, LocalScopeVariable v) {
+  exists(ProcessBlockCfgNode process | process = bb.getNode(i) |
+    v.(PipelineIteratorVariable).getProcessBlock() = process.getAstNode()
+    or
+    v.(PipelineByPropertyNameIteratorVariable).getProcessBlock() = process.getAstNode()
   )
 }
 
@@ -114,12 +115,19 @@ predicate parameterWrite(Cfg::EntryBasicBlock bb, int i, Parameter p) {
   )
 }
 
-private PipelineIteratorVariable getPipelineIterator(PipelineParameter pipelineParam) {
-  result.getProcessBlock().getScriptBlock() = pipelineParam.getDeclaringScope()
+private LocalScopeVariable getPipelineIterator(LocalScopeVariable pipelineParam) {
+  result.(PipelineIteratorVariable).getProcessBlock().getScriptBlock() =
+    pipelineParam.(PipelineParameter).getDeclaringScope()
+  or
+  result.(PipelineByPropertyNameIteratorVariable).getParameter() =
+    pipelineParam.(PipelineByPropertyNameParameter)
 }
 
 private predicate isPipelineIteratorVarAccess(VarAccessCfgNode va) {
-  va.getVariable() instanceof PipelineParameter and
+  (
+    va.getVariable() instanceof PipelineParameter or
+    va.getVariable() instanceof PipelineByPropertyNameParameter
+  ) and
   va.getAstNode().getParent*() instanceof ProcessBlock
 }
 
@@ -133,9 +141,9 @@ private predicate variableReadActual(Cfg::BasicBlock bb, int i, SsaInput::Source
 }
 
 private predicate pipelineRead(Cfg::BasicBlock bb, int i, SsaInput::SourceVariable v) {
-  exists(ProcessBlockCfgNode process |
-    process = bb.getNode(i) and
-    v = process.getPipelineParameter()
+  exists(ProcessBlockCfgNode process | process = bb.getNode(i) |
+    v = process.getPipelineParameter() or
+    v = process.getAPipelineByPropertyNameParameter()
   )
 }