From 5e2051bdeac6153085ec09bb6eb53cd31cb6a86b Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Thu, 26 Sep 2024 18:44:40 +0100 Subject: [PATCH 1/4] PS: Add test. --- .../library-tests/dataflow/params/test.ps1 | 35 ++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/powershell/ql/test/library-tests/dataflow/params/test.ps1 b/powershell/ql/test/library-tests/dataflow/params/test.ps1 index 0fe835d8bee..c217580d723 100644 --- a/powershell/ql/test/library-tests/dataflow/params/test.ps1 +++ b/powershell/ql/test/library-tests/dataflow/params/test.ps1 @@ -3,4 +3,37 @@ function Foo($a) { } $x = Source "1" -Foo $x \ No newline at end of file +Foo $x + +function ThreeArgs($x, $y, $z) { + Sink $x # $ MISSING: hasValueFlow=x + Sink $y # $ MISSING: hasValueFlow=y + Sink $z # $ MISSING: hasValueFlow=z +} + +$first = Source "x" +$second = Source "y" +$third = Source "z" + +ThreeArgs $first $second $third +ThreeArgs $second -x $first $third +ThreeArgs -x $first $second $third +ThreeArgs $first -y $second $third +ThreeArgs -y $second $first $third +ThreeArgs $second -x $first -z $third +ThreeArgs -x $first $second -z $third +ThreeArgs $first -y $second -z $third +ThreeArgs -y $second $first -z $third +ThreeArgs $second -x $first -z $third +ThreeArgs -x $first -z $third $second +ThreeArgs $first -y $second -z $third +ThreeArgs -y $second -z $third $first +ThreeArgs $second -z $third -x $first +ThreeArgs -x $first -z $third $second +ThreeArgs $first -z $third -y $second +ThreeArgs -y $second -z $third $first +ThreeArgs -z $third -y $second $first +ThreeArgs -z $third $second -x $first +ThreeArgs -z $third -x $first $second +ThreeArgs -z $third $first -y $second +ThreeArgs -z $third -y $second $first \ No newline at end of file From 7f25caf3f6a0241507a59a2b7adc816c81898d51 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Thu, 26 Sep 2024 18:34:50 +0100 Subject: [PATCH 2/4] PS: Add various helper predicates. --- .../ql/lib/semmle/code/powershell/Command.qll | 34 +++++++++++++++++-- .../lib/semmle/code/powershell/Variable.qll | 19 +++++++++-- .../code/powershell/controlflow/CfgNodes.qll | 24 ++++++++++++- .../internal/ControlFlowGraphImpl.qll | 2 +- 4 files changed, 71 insertions(+), 8 deletions(-) diff --git a/powershell/ql/lib/semmle/code/powershell/Command.qll b/powershell/ql/lib/semmle/code/powershell/Command.qll index a81e6c2686c..4b1283fd19c 100644 --- a/powershell/ql/lib/semmle/code/powershell/Command.qll +++ b/powershell/ql/lib/semmle/code/powershell/Command.qll @@ -19,8 +19,14 @@ class Cmd extends @command, CmdBase { StringConstExpr getCmdName() { result = this.getElement(0) } - Expr getAnArgument() { result = this.getArgument(_) or result = this.getNamedArgument(_) } + /** Gets any argument to this command. */ + Expr getAnArgument() { result = this.getArgument(_) } + /** + * Gets the `i`th argument to this command. + * + * The argument may be named or positional. + */ Expr getArgument(int i) { result = rank[i + 1](CmdElement e, int j | @@ -32,6 +38,18 @@ class Cmd extends @command, CmdBase { ) } + /** Gets the `i`th positional argument to this command. */ + Expr getPositionalArgument(int i) { + result = + rank[i + 1](Argument e, int j | + e = this.getArgument(j) and + e instanceof PositionalArgument + | + e order by j + ) + } + + /** Gets the named argument with the given name. */ Expr getNamedArgument(string name) { exists(int i, CmdParameter p | this.getElement(i) = p and @@ -53,11 +71,21 @@ class Cmd extends @command, CmdBase { class Argument extends Expr { Cmd cmd; - Argument() { cmd.getArgument(_) = this or cmd.getNamedArgument(_) = this } + Argument() { cmd.getAnArgument() = this } Cmd getCmd() { result = cmd } - int getIndex() { cmd.getArgument(result) = this } + int getPosition() { cmd.getPositionalArgument(result) = this } string getName() { cmd.getNamedArgument(result) = this } } + +/** A positional argument to a command. */ +class PositionalArgument extends Argument { + PositionalArgument() { not this instanceof NamedArgument } +} + +/** A named argument to a command. */ +class NamedArgument extends Argument { + NamedArgument() { this = cmd.getNamedArgument(_) } +} diff --git a/powershell/ql/lib/semmle/code/powershell/Variable.qll b/powershell/ql/lib/semmle/code/powershell/Variable.qll index 5a66ea5eebf..c916a78e722 100644 --- a/powershell/ql/lib/semmle/code/powershell/Variable.qll +++ b/powershell/ql/lib/semmle/code/powershell/Variable.qll @@ -14,10 +14,10 @@ private predicate hasParameterBlockImpl(Internal::Parameter p, ParamBlock block, /** * Gets the enclosing scope of `p`. - * + * * For a function parameter, this is the function body. For a parameter from a * parameter block, this is the enclosing scope of the parameter block. - * + * * In both of the above cases, the enclosing scope is the function body. */ private Scope getEnclosingScopeImpl(Internal::Parameter p) { @@ -176,6 +176,8 @@ class Parameter extends AbstractLocalScopeVariable, TParameter { override string getName() { result = p.getName() } + final predicate hasName(string name) { name = p.getName() } + final override Scope getDeclaringScope() { result = p.getEnclosingScope() } predicate hasParameterBlock(ParamBlock block, int i) { p.hasParameterBlock(block, i) } @@ -186,7 +188,18 @@ class Parameter extends AbstractLocalScopeVariable, TParameter { predicate hasDefaultValue() { exists(this.getDefaultValue()) } - int getIndex() { this.isFunctionParameter(_, result) } + /** + * Gets the index of this parameter. + * + * The parameter may be in a parameter block or a function parameter. + */ + int getIndex() { result = this.getFunctionIndex() or result = this.getBlockIndex() } + + /** Gets the index of this parameter in the parameter block, if any. */ + int getBlockIndex() { this.hasParameterBlock(_, result) } + + /** Gets the index of this parameter in the function, if any. */ + int getFunctionIndex() { this.isFunctionParameter(_, result) } Function getFunction() { result.getBody() = this.getDeclaringScope() } } diff --git a/powershell/ql/lib/semmle/code/powershell/controlflow/CfgNodes.qll b/powershell/ql/lib/semmle/code/powershell/controlflow/CfgNodes.qll index 1d4759430b0..6b589fb6783 100644 --- a/powershell/ql/lib/semmle/code/powershell/controlflow/CfgNodes.qll +++ b/powershell/ql/lib/semmle/code/powershell/controlflow/CfgNodes.qll @@ -174,6 +174,14 @@ module ExprNodes { override Argument e; final override Argument getExpr() { result = super.getExpr() } + + /** Gets the position of this argument, if any. */ + int getPosition() { result = e.getPosition() } + + /** Gets the name of this argument, if any. */ + string getName() { result = e.getName() } + + StmtNodes::CmdCfgNode getCmd() { result.getAnArgument() = this } } private class InvokeMemberChildMapping extends ExprChildMapping, InvokeMemberExpr { @@ -253,7 +261,7 @@ module ExprNodes { module StmtNodes { private class CmdChildMapping extends NonExprChildMapping, Cmd { - override predicate relevantChild(Ast n) { n = this.getElement(_) } + override predicate relevantChild(Ast n) { n = this.getAnArgument() or n = this.getCommand() } } /** A control-flow node that wraps a `Cmd` AST expression. */ @@ -263,6 +271,20 @@ module StmtNodes { override CmdChildMapping s; override Cmd getStmt() { result = super.getStmt() } + + ExprCfgNode getArgument(int i) { s.hasCfgChild(s.getArgument(i), this, result) } + + ExprCfgNode getPositionalArgument(int i) { + s.hasCfgChild(s.getPositionalArgument(i), this, result) + } + + ExprCfgNode getNamedArgument(string name) { + s.hasCfgChild(s.getNamedArgument(name), this, result) + } + + ExprCfgNode getAnArgument() { s.hasCfgChild(s.getAnArgument(), this, result) } + + ExprCfgNode getCommand() { s.hasCfgChild(s.getCommand(), this, result) } } private class AssignStmtChildMapping extends NonExprChildMapping, AssignStmt { diff --git a/powershell/ql/lib/semmle/code/powershell/controlflow/internal/ControlFlowGraphImpl.qll b/powershell/ql/lib/semmle/code/powershell/controlflow/internal/ControlFlowGraphImpl.qll index b60d53a6922..f435dd65bcf 100644 --- a/powershell/ql/lib/semmle/code/powershell/controlflow/internal/ControlFlowGraphImpl.qll +++ b/powershell/ql/lib/semmle/code/powershell/controlflow/internal/ControlFlowGraphImpl.qll @@ -176,7 +176,7 @@ module Trees { exists(Parameter p | p = rank[i + 1](Parameter cand, int j | - cand.hasDefaultValue() and j = cand.getIndex() + cand.hasDefaultValue() and j = cand.getFunctionIndex() | cand order by j ) and From e4c702ef148692e5bd1207361f1cea0f8401251f Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Thu, 26 Sep 2024 18:38:23 +0100 Subject: [PATCH 3/4] PS: Represent sets of parameter names. --- .../dataflow/internal/DataFlowPrivate.qll | 60 +++++++++++++++++++ 1 file changed, 60 insertions(+) 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 ceff0a6c6e3..6c1f3c9b780 100644 --- a/powershell/ql/lib/semmle/code/powershell/dataflow/internal/DataFlowPrivate.qll +++ b/powershell/ql/lib/semmle/code/powershell/dataflow/internal/DataFlowPrivate.qll @@ -229,6 +229,66 @@ class SsaInputNode extends SsaNode { override CfgScope getCfgScope() { result = node.getDefinitionExt().getBasicBlock().getScope() } } +private string getANamedArgument(Cmd c) { exists(c.getNamedArgument(result)) } + +private module NamedSetModule = QlBuiltins::InternSets; + +private newtype NamedSet0 = + TEmptyNamedSet() or + TNonEmptyNamedSet(NamedSetModule::Set ns) + +/** A (possiby empty) set of argument names. */ +class NamedSet extends NamedSet0 { + /** Gets the non-empty set of names, if any. */ + NamedSetModule::Set asNonEmpty() { this = TNonEmptyNamedSet(result) } + + /** Holds if this is the empty set. */ + predicate isEmpty() { this = TEmptyNamedSet() } + + /** Gets a name in this set. */ + string getAName() { this.asNonEmpty().contains(result) } + + /** Gets the textual representation of this set. */ + string toString() { + result = "{" + strictconcat(this.getAName(), ", ") + "}" + or + this.isEmpty() and + result = "{}" + } + + /** + * Gets a `Cmd` that provides a named parameter for every name in `this`. + * + * NOTE: The `Cmd` may also provide more names. + */ + Cmd getABindingCall() { + forex(string name | name = this.getAName() | exists(result.getNamedArgument(name))) + or + this.isEmpty() and + exists(result) + } + + /** + * Gets a `Cmd` that provides exactly the named parameters represented by + * this set. + */ + Cmd getAnExactBindingCall() { + forex(string name | name = this.getAName() | exists(result.getNamedArgument(name))) and + forex(string name | exists(result.getNamedArgument(name)) | name = this.getAName()) + or + this.isEmpty() and + not exists(result.getNamedArgument(_)) + } + + /** Gets a function that has a parameter for each name in this set. */ + Function getAFunction() { + forex(string name | name = this.getAName() | result.getAParameter().hasName(name)) + or + this.isEmpty() and + exists(result) + } +} + private module ParameterNodes { abstract class ParameterNodeImpl extends NodeImpl { abstract Parameter getParameter(); From b6019655ced3c1d2ffd47e0067978298d3633061 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Thu, 26 Sep 2024 18:36:35 +0100 Subject: [PATCH 4/4] PS: Use named sets to model parameter and argument matching. --- .../dataflow/internal/DataFlowDispatch.qll | 57 ++++++++++++++++--- .../dataflow/internal/DataFlowPrivate.qll | 53 +++++++++++++---- 2 files changed, 90 insertions(+), 20 deletions(-) diff --git a/powershell/ql/lib/semmle/code/powershell/dataflow/internal/DataFlowDispatch.qll b/powershell/ql/lib/semmle/code/powershell/dataflow/internal/DataFlowDispatch.qll index 8ec7d8f1911..10837843346 100644 --- a/powershell/ql/lib/semmle/code/powershell/dataflow/internal/DataFlowDispatch.qll +++ b/powershell/ql/lib/semmle/code/powershell/dataflow/internal/DataFlowDispatch.qll @@ -170,34 +170,75 @@ private module Cached { cached newtype TArgumentPosition = - TPositionalArgumentPosition(int pos) { exists(Cmd c | exists(c.getArgument(pos))) } + TKeywordArgumentPosition(string name) { name = any(CmdParameter p).getName() } or + TPositionalArgumentPosition(int pos, NamedSet ns) { + exists(Cmd cmd | + cmd = ns.getABindingCall() and + exists(cmd.getArgument(pos)) + ) + } cached - newtype TParameterPosition = TPositionalParameterPosition(int pos) { none() /* TODO */ } + newtype TParameterPosition = + TKeywordParameter(string name) { name = any(CmdParameter p).getName() } or + TPositionalParameter(int pos, NamedSet ns) { + exists(Cmd cmd | + cmd = ns.getABindingCall() and + exists(cmd.getArgument(pos)) + ) + } } import Cached /** A parameter position. */ class ParameterPosition extends TParameterPosition { - /** Holds if this position represents a positional parameter at position `pos`. */ - predicate isPositional(int pos) { this = TPositionalParameterPosition(pos) } + /** + * Holds if this position represents a positional parameter at position `pos` + * with function is called with exactly the named parameters from the set `ns` + */ + predicate isPositional(int pos, NamedSet ns) { this = TPositionalParameter(pos, ns) } + + /** Holds if this parameter is a keyword parameter with `name`. */ + predicate isKeyword(string name) { this = TKeywordParameter(name) } /** Gets a textual representation of this position. */ - string toString() { exists(int pos | this.isPositional(pos) and result = "position " + pos) } + string toString() { + exists(int pos, NamedSet ns | + this.isPositional(pos, ns) and result = "pos(" + pos + ", " + ns.toString() + ")" + ) + or + exists(string name | this.isKeyword(name) and result = "kw(" + name + ")") + } } /** An argument position. */ class ArgumentPosition extends TArgumentPosition { /** Holds if this position represents a positional argument at position `pos`. */ - predicate isPositional(int pos) { this = TPositionalArgumentPosition(pos) } + predicate isPositional(int pos, NamedSet ns) { this = TPositionalArgumentPosition(pos, ns) } + + predicate isKeyword(string name) { this = TKeywordArgumentPosition(name) } /** Gets a textual representation of this position. */ - string toString() { exists(int pos | this.isPositional(pos) and result = "position " + pos) } + string toString() { + exists(int pos, NamedSet ns | + this.isPositional(pos, ns) and result = "pos(" + pos + ", " + ns.toString() + ")" + ) + or + exists(string name | this.isKeyword(name) and result = "kw(" + name + ")") + } } /** Holds if arguments at position `apos` match parameters at position `ppos`. */ pragma[nomagic] predicate parameterMatch(ParameterPosition ppos, ArgumentPosition apos) { - exists(int pos | ppos.isPositional(pos) and apos.isPositional(pos)) + exists(string name | + ppos.isKeyword(name) and + apos.isKeyword(name) + ) + or + exists(int pos, NamedSet ns | + ppos.isPositional(pos, ns) and + apos.isPositional(pos, ns) + ) } 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 6c1f3c9b780..4034e724a39 100644 --- a/powershell/ql/lib/semmle/code/powershell/dataflow/internal/DataFlowPrivate.qll +++ b/powershell/ql/lib/semmle/code/powershell/dataflow/internal/DataFlowPrivate.qll @@ -294,13 +294,6 @@ private module ParameterNodes { abstract Parameter getParameter(); abstract predicate isParameterOf(DataFlowCallable c, ParameterPosition pos); - - final predicate isSourceParameterOf(CfgScope c, ParameterPosition pos) { - exists(DataFlowCallable callable | - this.isParameterOf(callable, pos) and - c = callable.asCfgScope() - ) - } } /** @@ -315,8 +308,29 @@ private module ParameterNodes { override Parameter getParameter() { result = parameter } override predicate isParameterOf(DataFlowCallable c, ParameterPosition pos) { - exists(CfgScope callable, int i | - callable = c.asCfgScope() and pos.isPositional(i) and callable.getParameter(i) = parameter + exists(CfgScope callable | callable = c.asCfgScope() | + pos.isKeyword(parameter.getName()) + or + // Given a function f with parameters x, y we map + // x to the positions: + // 1. keyword(x) + // 2. position(0, {y}) + // 3. position(0, {}) + // Likewise, y is mapped to the positions: + // 1. keyword(y) + // 2. position(0, {x}) + // 3. position(1, {}) + // The interpretation of `position(i, S)` is the position of the i'th unnamed parameter when the + // keywords in S are specified. + exists(int i, int j, string name, NamedSet ns, Function f | + pos.isPositional(j, ns) and + parameter.getIndex() = i and + f = parameter.getFunction() and + f = ns.getAFunction() and + name = parameter.getName() and + not name = ns.getAName() and + j = i - count(int k | k < i and f.getParameter(k).getName() = ns.getAName()) + ) ) } @@ -335,14 +349,29 @@ abstract class ArgumentNode extends Node { /** Holds if this argument occurs at the given position in the given call. */ abstract predicate argumentOf(DataFlowCall call, ArgumentPosition pos); - abstract predicate sourceArgumentOf(CfgNodes::StmtNodes::CmdCfgNode call, ArgumentPosition pos); - /** Gets the call in which this node is an argument. */ final DataFlowCall getCall() { this.argumentOf(result, _) } } module ArgumentNodes { - // TODO + class ExplicitArgumentNode extends ArgumentNode { + CfgNodes::ExprNodes::ArgumentCfgNode arg; + + ExplicitArgumentNode() { this.asExpr() = arg } + + override predicate argumentOf(DataFlowCall call, ArgumentPosition pos) { + arg.getCmd() = call.asCall() and + ( + pos.isKeyword(arg.getName()) + or + exists(NamedSet ns, int i | + i = arg.getPosition() and + ns.getAnExactBindingCall() = call.asCall().getStmt() and + pos.isPositional(i, ns) + ) + ) + } + } } import ArgumentNodes