Merge pull request #246 from microsoft/powershell-commandinjection-invokesinkfix

MathiasVP · web-flow · commit 52ff5d3fbc9e · 2025-07-04T18:17:09.000+01:00
InvokeSink fix
diff --git a/powershell/ql/lib/semmle/code/powershell/security/CommandInjectionCustomizations.qll b/powershell/ql/lib/semmle/code/powershell/security/CommandInjectionCustomizations.qll
@@ -142,8 +142,10 @@ module CommandInjection {
   class InvokeSink extends Sink {
     InvokeSink() {
       exists(InvokeMemberExpr ie |
-        this.asExpr().getExpr() = ie.getCallee() or
-        this.asExpr().getExpr() = ie.getQualifier().getAChild*()
+        this.asExpr().getExpr() = ie.getCallee()
+        or
+        ie.getAName() = "Invoke" and
+        ie.getQualifier().(MemberExprReadAccess).getMemberExpr() = this.asExpr().getExpr()
       )
     }
 
diff --git a/powershell/ql/src/queries/security/cwe-078/CommandInjection.qhelp b/powershell/ql/src/queries/security/cwe-078/CommandInjection.qhelp
@@ -58,5 +58,6 @@ Injection Hunter:
 <!--  LocalWords:  CWE untrusted unsanitized Runtime
  -->
 
+
 </references>
 </qhelp>
diff --git a/powershell/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.expected b/powershell/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.expected
@@ -3,9 +3,7 @@ edges
 | test.ps1:9:11:9:20 | userinput | test.ps1:10:9:10:38 | Get-Process -Name $UserInput | provenance |  |
 | test.ps1:15:11:15:20 | userinput | test.ps1:16:50:16:79 | Get-Process -Name $UserInput | provenance |  |
 | test.ps1:21:11:21:20 | userinput | test.ps1:22:41:22:70 | Get-Process -Name $UserInput | provenance |  |
-| test.ps1:21:11:21:20 | userinput | test.ps1:22:60:22:69 | UserInput | provenance |  |
 | test.ps1:27:11:27:20 | userinput | test.ps1:28:38:28:67 | Get-Process -Name $UserInput | provenance |  |
-| test.ps1:27:11:27:20 | userinput | test.ps1:28:57:28:66 | UserInput | provenance |  |
 | test.ps1:33:11:33:20 | userinput | test.ps1:34:14:34:46 | public class Foo { $UserInput } | provenance |  |
 | test.ps1:39:11:39:20 | userinput | test.ps1:40:30:40:62 | public class Foo { $UserInput } | provenance |  |
 | test.ps1:45:11:45:20 | userinput | test.ps1:48:30:48:34 | code | provenance |  |
@@ -16,7 +14,7 @@ edges
 | test.ps1:104:11:104:20 | userinput | test.ps1:108:58:108:87 | Get-Process -Name $UserInput | provenance |  |
 | test.ps1:114:11:114:20 | userinput | test.ps1:116:34:116:43 | UserInput | provenance |  |
 | test.ps1:121:11:121:20 | userinput | test.ps1:123:28:123:37 | UserInput | provenance |  |
-| test.ps1:128:11:128:20 | userinput | test.ps1:130:28:130:37 | UserInput | provenance |  |
+| test.ps1:129:11:129:20 | userinput | test.ps1:131:28:131:37 | UserInput | provenance |  |
 | test.ps1:136:11:136:20 | userinput | test.ps1:139:50:139:59 | UserInput | provenance |  |
 | test.ps1:144:11:144:20 | userinput | test.ps1:147:63:147:72 | UserInput | provenance |  |
 | test.ps1:152:10:152:32 | Call to read-host | test.ps1:154:46:154:51 | input | provenance | Src:MaD:0  |
@@ -52,7 +50,7 @@ edges
 | test.ps1:167:41:167:46 | input | test.ps1:104:11:104:20 | userinput | provenance |  |
 | test.ps1:168:36:168:41 | input | test.ps1:114:11:114:20 | userinput | provenance |  |
 | test.ps1:169:36:169:41 | input | test.ps1:121:11:121:20 | userinput | provenance |  |
-| test.ps1:170:36:170:41 | input | test.ps1:128:11:128:20 | userinput | provenance |  |
+| test.ps1:170:36:170:41 | input | test.ps1:129:11:129:20 | userinput | provenance |  |
 | test.ps1:172:42:172:47 | input | test.ps1:136:11:136:20 | userinput | provenance |  |
 | test.ps1:173:42:173:47 | input | test.ps1:144:11:144:20 | userinput | provenance |  |
 nodes
@@ -64,10 +62,8 @@ nodes
 | test.ps1:16:50:16:79 | Get-Process -Name $UserInput | semmle.label | Get-Process -Name $UserInput |
 | test.ps1:21:11:21:20 | userinput | semmle.label | userinput |
 | test.ps1:22:41:22:70 | Get-Process -Name $UserInput | semmle.label | Get-Process -Name $UserInput |
-| test.ps1:22:60:22:69 | UserInput | semmle.label | UserInput |
 | test.ps1:27:11:27:20 | userinput | semmle.label | userinput |
 | test.ps1:28:38:28:67 | Get-Process -Name $UserInput | semmle.label | Get-Process -Name $UserInput |
-| test.ps1:28:57:28:66 | UserInput | semmle.label | UserInput |
 | test.ps1:33:11:33:20 | userinput | semmle.label | userinput |
 | test.ps1:34:14:34:46 | public class Foo { $UserInput } | semmle.label | public class Foo { $UserInput } |
 | test.ps1:39:11:39:20 | userinput | semmle.label | userinput |
@@ -88,8 +84,8 @@ nodes
 | test.ps1:116:34:116:43 | UserInput | semmle.label | UserInput |
 | test.ps1:121:11:121:20 | userinput | semmle.label | userinput |
 | test.ps1:123:28:123:37 | UserInput | semmle.label | UserInput |
-| test.ps1:128:11:128:20 | userinput | semmle.label | userinput |
-| test.ps1:130:28:130:37 | UserInput | semmle.label | UserInput |
+| test.ps1:129:11:129:20 | userinput | semmle.label | userinput |
+| test.ps1:131:28:131:37 | UserInput | semmle.label | UserInput |
 | test.ps1:136:11:136:20 | userinput | semmle.label | userinput |
 | test.ps1:139:50:139:59 | UserInput | semmle.label | UserInput |
 | test.ps1:144:11:144:20 | userinput | semmle.label | userinput |
@@ -119,9 +115,7 @@ subpaths
 | test.ps1:10:9:10:38 | Get-Process -Name $UserInput | test.ps1:152:10:152:32 | Call to read-host | test.ps1:10:9:10:38 | Get-Process -Name $UserInput | This command depends on a $@. | test.ps1:152:10:152:32 | Call to read-host | user-provided value |
 | test.ps1:16:50:16:79 | Get-Process -Name $UserInput | test.ps1:152:10:152:32 | Call to read-host | test.ps1:16:50:16:79 | Get-Process -Name $UserInput | This command depends on a $@. | test.ps1:152:10:152:32 | Call to read-host | user-provided value |
 | test.ps1:22:41:22:70 | Get-Process -Name $UserInput | test.ps1:152:10:152:32 | Call to read-host | test.ps1:22:41:22:70 | Get-Process -Name $UserInput | This command depends on a $@. | test.ps1:152:10:152:32 | Call to read-host | user-provided value |
-| test.ps1:22:60:22:69 | UserInput | test.ps1:152:10:152:32 | Call to read-host | test.ps1:22:60:22:69 | UserInput | This command depends on a $@. | test.ps1:152:10:152:32 | Call to read-host | user-provided value |
 | test.ps1:28:38:28:67 | Get-Process -Name $UserInput | test.ps1:152:10:152:32 | Call to read-host | test.ps1:28:38:28:67 | Get-Process -Name $UserInput | This command depends on a $@. | test.ps1:152:10:152:32 | Call to read-host | user-provided value |
-| test.ps1:28:57:28:66 | UserInput | test.ps1:152:10:152:32 | Call to read-host | test.ps1:28:57:28:66 | UserInput | This command depends on a $@. | test.ps1:152:10:152:32 | Call to read-host | user-provided value |
 | test.ps1:34:14:34:46 | public class Foo { $UserInput } | test.ps1:152:10:152:32 | Call to read-host | test.ps1:34:14:34:46 | public class Foo { $UserInput } | This command depends on a $@. | test.ps1:152:10:152:32 | Call to read-host | user-provided value |
 | test.ps1:40:30:40:62 | public class Foo { $UserInput } | test.ps1:152:10:152:32 | Call to read-host | test.ps1:40:30:40:62 | public class Foo { $UserInput } | This command depends on a $@. | test.ps1:152:10:152:32 | Call to read-host | user-provided value |
 | test.ps1:48:30:48:34 | code | test.ps1:152:10:152:32 | Call to read-host | test.ps1:48:30:48:34 | code | This command depends on a $@. | test.ps1:152:10:152:32 | Call to read-host | user-provided value |
@@ -132,6 +126,6 @@ subpaths
 | test.ps1:108:58:108:87 | Get-Process -Name $UserInput | test.ps1:152:10:152:32 | Call to read-host | test.ps1:108:58:108:87 | Get-Process -Name $UserInput | This command depends on a $@. | test.ps1:152:10:152:32 | Call to read-host | user-provided value |
 | test.ps1:116:34:116:43 | UserInput | test.ps1:152:10:152:32 | Call to read-host | test.ps1:116:34:116:43 | UserInput | This command depends on a $@. | test.ps1:152:10:152:32 | Call to read-host | user-provided value |
 | test.ps1:123:28:123:37 | UserInput | test.ps1:152:10:152:32 | Call to read-host | test.ps1:123:28:123:37 | UserInput | This command depends on a $@. | test.ps1:152:10:152:32 | Call to read-host | user-provided value |
-| test.ps1:130:28:130:37 | UserInput | test.ps1:152:10:152:32 | Call to read-host | test.ps1:130:28:130:37 | UserInput | This command depends on a $@. | test.ps1:152:10:152:32 | Call to read-host | user-provided value |
+| test.ps1:131:28:131:37 | UserInput | test.ps1:152:10:152:32 | Call to read-host | test.ps1:131:28:131:37 | UserInput | This command depends on a $@. | test.ps1:152:10:152:32 | Call to read-host | user-provided value |
 | test.ps1:139:50:139:59 | UserInput | test.ps1:152:10:152:32 | Call to read-host | test.ps1:139:50:139:59 | UserInput | This command depends on a $@. | test.ps1:152:10:152:32 | Call to read-host | user-provided value |
 | test.ps1:147:63:147:72 | UserInput | test.ps1:152:10:152:32 | Call to read-host | test.ps1:147:63:147:72 | UserInput | This command depends on a $@. | test.ps1:152:10:152:32 | Call to read-host | user-provided value |
diff --git a/powershell/ql/test/query-tests/security/cwe-078/CommandInjection/test.ps1 b/powershell/ql/test/query-tests/security/cwe-078/CommandInjection/test.ps1
@@ -1,51 +1,51 @@
 function Invoke-InvokeExpressionInjection1
 {
     param($UserInput)
-    Invoke-Expression "Get-Process -Name $UserInput"
+    Invoke-Expression "Get-Process -Name $UserInput" # BAD
 }
 
 function Invoke-InvokeExpressionInjection2
 {
     param($UserInput)
-    iex "Get-Process -Name $UserInput"
+    iex "Get-Process -Name $UserInput" # BAD
 }
 
 function Invoke-InvokeExpressionInjection3
 {
     param($UserInput)
-    $executionContext.InvokeCommand.InvokeScript("Get-Process -Name $UserInput")
+    $executionContext.InvokeCommand.InvokeScript("Get-Process -Name $UserInput") # BAD
 }
 
 function Invoke-InvokeExpressionInjection4
 {
     param($UserInput)
-    $host.Runspace.CreateNestedPipeline("Get-Process -Name $UserInput", $false).Invoke()
+    $host.Runspace.CreateNestedPipeline("Get-Process -Name $UserInput", $false).Invoke() # BAD
 }
 
 function Invoke-InvokeExpressionInjection5
 {
     param($UserInput)
-    [PowerShell]::Create().AddScript("Get-Process -Name $UserInput").Invoke()
+    [PowerShell]::Create().AddScript("Get-Process -Name $UserInput").Invoke() # BAD
 }
 
 function Invoke-InvokeExpressionInjection6
 {
     param($UserInput)
-    Add-Type "public class Foo { $UserInput }"
+    Add-Type "public class Foo { $UserInput }" # BAD
 }
 
 function Invoke-InvokeExpressionInjection7
 {
     param($UserInput)
-    Add-Type -TypeDefinition "public class Foo { $UserInput }"
+    Add-Type -TypeDefinition "public class Foo { $UserInput }" # BAD
 }
 
 function Invoke-InvokeExpressionInjection8
 {
     param($UserInput)
 
     $code = "public class Foo { $UserInput }"
-    Add-Type -TypeDefinition $code
+    Add-Type -TypeDefinition $code # BAD
 }
 
 function Invoke-InvokeExpressionInjectionFP
@@ -72,21 +72,21 @@ function Invoke-ExploitableCommandInjection1
 {
     param($UserInput)
 
-    powershell -command "Get-Process -Name $UserInput"
+    powershell -command "Get-Process -Name $UserInput" # BAD
 }
 
 function Invoke-ExploitableCommandInjection2
 {
     param($UserInput)
 
-    powershell "Get-Process -Name $UserInput"
+    powershell "Get-Process -Name $UserInput" # BAD
 }
 
 function Invoke-ExploitableCommandInjection3
 {
     param($UserInput)
 
-    cmd /c "ping $UserInput"
+    cmd /c "ping $UserInput" # BAD
 }
 
 function Invoke-ScriptBlockInjection1
@@ -95,7 +95,7 @@ function Invoke-ScriptBlockInjection1
 
     ## Often used when making remote connections
 
-    $sb = [ScriptBlock]::Create("Get-Process -Name $UserInput")
+    $sb = [ScriptBlock]::Create("Get-Process -Name $UserInput") # BAD
     Invoke-Command RemoteServer $sb
 }
 
@@ -105,46 +105,46 @@ function Invoke-ScriptBlockInjection2
 
     ## Often used when making remote connections
 
-    $sb = $executionContext.InvokeCommand.NewScriptBlock("Get-Process -Name $UserInput")
+    $sb = $executionContext.InvokeCommand.NewScriptBlock("Get-Process -Name $UserInput") # BAD
     Invoke-Command RemoteServer $sb
 }
 
 function Invoke-MethodInjection1
 {
     param($UserInput)
 
-    Get-Process | Foreach-Object $UserInput
+    Get-Process | Foreach-Object $UserInput # BAD
 }
 
 function Invoke-MethodInjection2
 {
     param($UserInput)
 
-    (Get-Process -Id $pid).$UserInput()
+    (Get-Process -Id $pid).$UserInput() # BAD
 }
 
+
 function Invoke-MethodInjection3
 {
     param($UserInput)
 
-    (Get-Process -Id $pid).$UserInput.Invoke()
+    (Get-Process -Id $pid).$UserInput.Invoke() # BAD
 }
 
-#TODO: currently a FN
 function Invoke-ExpandStringInjection1
 {
     param($UserInput)
 
     ## Used to attempt a variable resolution
-    $executionContext.InvokeCommand.ExpandString($UserInput)
+    $executionContext.InvokeCommand.ExpandString($UserInput) # BAD
 }
 
 function Invoke-ExpandStringInjection2
 {
     param($UserInput)
 
     ## Used to attempt a variable resolution
-    $executionContext.SessionState.InvokeCommand.ExpandString($UserInput)
+    $executionContext.SessionState.InvokeCommand.ExpandString($UserInput) # BAD
 }
 
 

Original file line number	Diff line number	Diff line change
`@@ -142,8 +142,10 @@ module CommandInjection {`
`142`	`142`	`class InvokeSink extends Sink {`
`143`	`143`	`InvokeSink() {`
`144`	`144`	`exists(InvokeMemberExpr ie \|`
`145`		`- this.asExpr().getExpr() = ie.getCallee() or`
`146`		`- this.asExpr().getExpr() = ie.getQualifier().getAChild*()`
	`145`	`+ this.asExpr().getExpr() = ie.getCallee()`
	`146`	`+ or`
	`147`	`+ ie.getAName() = "Invoke" and`
	`148`	`+ ie.getQualifier().(MemberExprReadAccess).getMemberExpr() = this.asExpr().getExpr()`
`147`	`149`	`)`
`148`	`150`	`}`
`149`	`151`
Original file line number	Diff line number	Diff line change
`@@ -1,51 +1,51 @@`
`1`	`1`	`function Invoke-InvokeExpressionInjection1`
`2`	`2`	`{`
`3`	`3`	`param($UserInput)`
`4`		`- Invoke-Expression "Get-Process -Name $UserInput"`
	`4`	`+ Invoke-Expression "Get-Process -Name $UserInput" # BAD`
`5`	`5`	`}`
`6`	`6`
`7`	`7`	`function Invoke-InvokeExpressionInjection2`
`8`	`8`	`{`
`9`	`9`	`param($UserInput)`
`10`		`- iex "Get-Process -Name $UserInput"`
	`10`	`+ iex "Get-Process -Name $UserInput" # BAD`
`11`	`11`	`}`
`12`	`12`
`13`	`13`	`function Invoke-InvokeExpressionInjection3`
`14`	`14`	`{`
`15`	`15`	`param($UserInput)`
`16`		`- $executionContext.InvokeCommand.InvokeScript("Get-Process -Name $UserInput")`
	`16`	`+ $executionContext.InvokeCommand.InvokeScript("Get-Process -Name $UserInput") # BAD`
`17`	`17`	`}`
`18`	`18`
`19`	`19`	`function Invoke-InvokeExpressionInjection4`
`20`	`20`	`{`
`21`	`21`	`param($UserInput)`
`22`		`- $host.Runspace.CreateNestedPipeline("Get-Process -Name $UserInput", $false).Invoke()`
	`22`	`+ $host.Runspace.CreateNestedPipeline("Get-Process -Name $UserInput", $false).Invoke() # BAD`
`23`	`23`	`}`
`24`	`24`
`25`	`25`	`function Invoke-InvokeExpressionInjection5`
`26`	`26`	`{`
`27`	`27`	`param($UserInput)`
`28`		`- [PowerShell]::Create().AddScript("Get-Process -Name $UserInput").Invoke()`
	`28`	`+ [PowerShell]::Create().AddScript("Get-Process -Name $UserInput").Invoke() # BAD`
`29`	`29`	`}`
`30`	`30`
`31`	`31`	`function Invoke-InvokeExpressionInjection6`
`32`	`32`	`{`
`33`	`33`	`param($UserInput)`
`34`		`- Add-Type "public class Foo { $UserInput }"`
	`34`	`+ Add-Type "public class Foo { $UserInput }" # BAD`
`35`	`35`	`}`
`36`	`36`
`37`	`37`	`function Invoke-InvokeExpressionInjection7`
`38`	`38`	`{`
`39`	`39`	`param($UserInput)`
`40`		`- Add-Type -TypeDefinition "public class Foo { $UserInput }"`
	`40`	`+ Add-Type -TypeDefinition "public class Foo { $UserInput }" # BAD`
`41`	`41`	`}`
`42`	`42`
`43`	`43`	`function Invoke-InvokeExpressionInjection8`
`44`	`44`	`{`
`45`	`45`	`param($UserInput)`
`46`	`46`
`47`	`47`	`$code = "public class Foo { $UserInput }"`
`48`		`- Add-Type -TypeDefinition $code`
	`48`	`+ Add-Type -TypeDefinition $code # BAD`
`49`	`49`	`}`
`50`	`50`
`51`	`51`	`function Invoke-InvokeExpressionInjectionFP`
`@@ -72,21 +72,21 @@ function Invoke-ExploitableCommandInjection1`
`72`	`72`	`{`
`73`	`73`	`param($UserInput)`
`74`	`74`
`75`		`- powershell -command "Get-Process -Name $UserInput"`
	`75`	`+ powershell -command "Get-Process -Name $UserInput" # BAD`
`76`	`76`	`}`
`77`	`77`
`78`	`78`	`function Invoke-ExploitableCommandInjection2`
`79`	`79`	`{`
`80`	`80`	`param($UserInput)`
`81`	`81`
`82`		`- powershell "Get-Process -Name $UserInput"`
	`82`	`+ powershell "Get-Process -Name $UserInput" # BAD`
`83`	`83`	`}`
`84`	`84`
`85`	`85`	`function Invoke-ExploitableCommandInjection3`
`86`	`86`	`{`
`87`	`87`	`param($UserInput)`
`88`	`88`
`89`		`- cmd /c "ping $UserInput"`
	`89`	`+ cmd /c "ping $UserInput" # BAD`
`90`	`90`	`}`
`91`	`91`
`92`	`92`	`function Invoke-ScriptBlockInjection1`
`@@ -95,7 +95,7 @@ function Invoke-ScriptBlockInjection1`
`95`	`95`
`96`	`96`	`## Often used when making remote connections`
`97`	`97`
`98`		`- $sb = [ScriptBlock]::Create("Get-Process -Name $UserInput")`
	`98`	`+ $sb = [ScriptBlock]::Create("Get-Process -Name $UserInput") # BAD`
`99`	`99`	`Invoke-Command RemoteServer $sb`
`100`	`100`	`}`
`101`	`101`
`@@ -105,46 +105,46 @@ function Invoke-ScriptBlockInjection2`
`105`	`105`
`106`	`106`	`## Often used when making remote connections`
`107`	`107`
`108`		`- $sb = $executionContext.InvokeCommand.NewScriptBlock("Get-Process -Name $UserInput")`
	`108`	`+ $sb = $executionContext.InvokeCommand.NewScriptBlock("Get-Process -Name $UserInput") # BAD`
`109`	`109`	`Invoke-Command RemoteServer $sb`
`110`	`110`	`}`
`111`	`111`
`112`	`112`	`function Invoke-MethodInjection1`
`113`	`113`	`{`
`114`	`114`	`param($UserInput)`
`115`	`115`
`116`		`- Get-Process \| Foreach-Object $UserInput`
	`116`	`+ Get-Process \| Foreach-Object $UserInput # BAD`
`117`	`117`	`}`
`118`	`118`
`119`	`119`	`function Invoke-MethodInjection2`
`120`	`120`	`{`
`121`	`121`	`param($UserInput)`
`122`	`122`
`123`		`- (Get-Process -Id $pid).$UserInput()`
	`123`	`+ (Get-Process -Id $pid).$UserInput() # BAD`
`124`	`124`	`}`
`125`	`125`
	`126`	`+`
`126`	`127`	`function Invoke-MethodInjection3`
`127`	`128`	`{`
`128`	`129`	`param($UserInput)`
`129`	`130`
`130`		`- (Get-Process -Id $pid).$UserInput.Invoke()`
	`131`	`+ (Get-Process -Id $pid).$UserInput.Invoke() # BAD`
`131`	`132`	`}`
`132`	`133`
`133`		`-#TODO: currently a FN`
`134`	`134`	`function Invoke-ExpandStringInjection1`
`135`	`135`	`{`
`136`	`136`	`param($UserInput)`
`137`	`137`
`138`	`138`	`## Used to attempt a variable resolution`
`139`		`- $executionContext.InvokeCommand.ExpandString($UserInput)`
	`139`	`+ $executionContext.InvokeCommand.ExpandString($UserInput) # BAD`
`140`	`140`	`}`
`141`	`141`
`142`	`142`	`function Invoke-ExpandStringInjection2`
`143`	`143`	`{`
`144`	`144`	`param($UserInput)`
`145`	`145`
`146`	`146`	`## Used to attempt a variable resolution`
`147`		`- $executionContext.SessionState.InvokeCommand.ExpandString($UserInput)`
	`147`	`+ $executionContext.SessionState.InvokeCommand.ExpandString($UserInput) # BAD`
`148`	`148`	`}`
`149`	`149`
`150`	`150`