InvokeSink fix

[chanel-y]

More restricted definition

TODO: figure out this case:

function Invoke-MethodInjection3
{
    param($UserInput)

    (Get-Process -Id $pid).$UserInput.Invoke()
}

[MathiasVP]

Note that InvokeMemberExpr doesn't just cover calls to Invoke. InvokeMember is the name for any call to a member method. So we would still alert on stuff like:

$x = Source
$x.SubString(...)

since there's taint-flow from Source to the qualifier of a method call. So I think we should still have some constraint like ie.getAName() = "Invoke", right?

Also: Should we call InvokeMemberExpr something else to not confuse future us? The name is just taken from the PowerShell AST documentation, but it's a rather non-standard name. Should we call it MethodCall or something instead?

[chanel-y]

These are the cases I'm trying to cover:

(Get-Process -Id $pid).$UserInput()

(Get-Process -Id $pid).$UserInput.Invoke()

First one by .getCallee, second previously by the .getQualifier.getAChild() , since .getQualifier returned the whole "(Get-Process -Id $pid).$UserInput" as an Expr

I don't mind keeping the second case a FN for now, but would

class InvokeSink extends Sink {
    InvokeSink() {
      exists(InvokeMemberExpr ie |
        this.asExpr().getExpr() = ie.getCallee()
      ) 

still have the issue?

[chanel-y]

I do like MethodCall but that's because I'm a regular citizen of csharp query authoring land

[MathiasVP]

Oh! I see the problem.... Hm... I'll give it a couple of hours of thinking 🤔!

[MathiasVP]

I think covering (Get-Process -Id $pid).$UserInput() by flow to .getCallee() sounds good! And for the (Get-Process -Id $pid).$UserInput.Invoke() case we could we do something like:

exists(DataFlow::CallNode call |
  call.getAName() = "Invoke" and
  call.getQualifier().asExpr().getExpr().(MemberExprReadAccess).getMemberExpr() = sink.asExpr().getExpr()
)

That covers (Get-Process -Id $pid).$UserInput.Invoke(). Would that work?