From 70bc32a542b4064218dc30749ac04d1d93e981ab Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Wed, 16 Oct 2024 16:39:30 +0100 Subject: [PATCH] PS: Make sure we handle pipeline-value-from-property-name variables when passed an array. --- .../semmle/code/powershell/ScriptBlock.qll | 8 +++ .../lib/semmle/code/powershell/Variable.qll | 52 +++++++++++++- .../code/powershell/controlflow/CfgNodes.qll | 6 +- .../dataflow/internal/DataFlowPrivate.qll | 68 +++++++++++++++---- .../powershell/dataflow/internal/SsaImpl.qll | 28 +++++--- 5 files changed, 136 insertions(+), 26 deletions(-) diff --git a/powershell/ql/lib/semmle/code/powershell/ScriptBlock.qll b/powershell/ql/lib/semmle/code/powershell/ScriptBlock.qll index 66cb7eca1d4..79691dbb6e5 100644 --- a/powershell/ql/lib/semmle/code/powershell/ScriptBlock.qll +++ b/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() + } } diff --git a/powershell/ql/lib/semmle/code/powershell/Variable.qll b/powershell/ql/lib/semmle/code/powershell/Variable.qll index 2d041ca939d..18117db52b5 100644 --- a/powershell/ql/lib/semmle/code/powershell/Variable.qll +++ b/powershell/ql/lib/semmle/code/powershell/Variable.qll @@ -178,6 +178,16 @@ 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 @@ -185,7 +195,10 @@ private newtype TVariable = 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) } +} diff --git a/powershell/ql/lib/semmle/code/powershell/controlflow/CfgNodes.qll b/powershell/ql/lib/semmle/code/powershell/controlflow/CfgNodes.qll index f60389e8aeb..0183167dbb7 100644 --- a/powershell/ql/lib/semmle/code/powershell/controlflow/CfgNodes.qll +++ b/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 { diff --git a/powershell/ql/lib/semmle/code/powershell/dataflow/internal/DataFlowPrivate.qll b/powershell/ql/lib/semmle/code/powershell/dataflow/internal/DataFlowPrivate.qll index fa97be39ee5..67ee749b771 100644 --- a/powershell/ql/lib/semmle/code/powershell/dataflow/internal/DataFlowPrivate.qll +++ b/powershell/ql/lib/semmle/code/powershell/dataflow/internal/DataFlowPrivate.qll @@ -76,7 +76,8 @@ 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 @@ -84,9 +85,7 @@ module SsaFlow { } 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 PipelineByPropertyNameParameterNode + 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() } @@ -753,8 +761,23 @@ 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 @@ -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.(PipelineByPropertyNameParameterNode).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() } diff --git a/powershell/ql/lib/semmle/code/powershell/dataflow/internal/SsaImpl.qll b/powershell/ql/lib/semmle/code/powershell/dataflow/internal/SsaImpl.qll index 0b1d80acdbc..05d8128d035 100644 --- a/powershell/ql/lib/semmle/code/powershell/dataflow/internal/SsaImpl.qll +++ b/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() ) }