Skip to content

InvokeSink fix #246

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

chanel-y
Copy link

@chanel-y chanel-y commented Jun 12, 2025

More restricted definition

TODO: figure out this case:

function Invoke-MethodInjection3
{
    param($UserInput)

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

Comment on lines 144 to +147
exists(InvokeMemberExpr ie |
this.asExpr().getExpr() = ie.getCallee() or
this.asExpr().getExpr() = ie.getQualifier().getAChild*()
)
this.asExpr().getExpr() = ie.getCallee() or
this.asExpr().getExpr() = ie.getQualifier()
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants