diff --git a/powershell/ql/src/queries/security/cwe-078/CommandInjectionCritical.ql b/powershell/ql/src/queries/security/cwe-078/CommandInjectionCritical.ql index 0b965c885ff8..0908a98c04ba 100644 --- a/powershell/ql/src/queries/security/cwe-078/CommandInjectionCritical.ql +++ b/powershell/ql/src/queries/security/cwe-078/CommandInjectionCritical.ql @@ -23,6 +23,13 @@ abstract class CriticalSource extends DataFlow::Node { abstract string getSourceType(); } +/** + * Holds if `sink` is a dot-source operator command sink. + */ +private predicate isDotSourceSink(DataFlow::Node sink) { + any(DataFlow::DotSourcingOperatorNode call).getCommand() = sink +} + class CmdletBindingParam extends CriticalSource { CmdletBindingParam(){ exists(Attribute a, Function f | @@ -54,6 +61,12 @@ import CommandInjectionCriticalFlow::PathGraph from CommandInjectionCriticalFlow::PathNode source, CommandInjectionCriticalFlow::PathNode sink, CriticalSource sourceNode where CommandInjectionCriticalFlow::flowPath(source, sink) and - sourceNode = source.getNode() + sourceNode = source.getNode() and + // For dot-source sinks, verify the parameter actually reaches the command + // via intra-procedural taint flow. This eliminates FPs where the parameter + // is in scope but doesn't flow to the dot-sourced expression. + (isDotSourceSink(sink.getNode()) implies + TaintTracking::localTaint(sourceNode, sink.getNode()) + ) select sink.getNode(), source, sink, "This command depends on a $@.", sourceNode, sourceNode.getSourceType() 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 index 4a2e32fc5ce4..3436c33be1ad 100644 --- a/powershell/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.expected +++ b/powershell/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.expected @@ -1,3 +1,26 @@ +#select +| test.ps1:4:23:4:52 | Get-Process -Name $UserInput | test.ps1:164:10:164:32 | Call to read-host | test.ps1:4:23:4:52 | Get-Process -Name $UserInput | This command depends on a $@. | test.ps1:164:10:164:32 | Call to read-host | user-provided value | +| test.ps1:10:9:10:38 | Get-Process -Name $UserInput | test.ps1:164:10:164:32 | Call to read-host | test.ps1:10:9:10:38 | Get-Process -Name $UserInput | This command depends on a $@. | test.ps1:164:10:164:32 | Call to read-host | user-provided value | +| test.ps1:16:50:16:79 | Get-Process -Name $UserInput | test.ps1:164:10:164:32 | Call to read-host | test.ps1:16:50:16:79 | Get-Process -Name $UserInput | This command depends on a $@. | test.ps1:164:10:164:32 | Call to read-host | user-provided value | +| test.ps1:22:41:22:70 | Get-Process -Name $UserInput | test.ps1:164:10:164:32 | Call to read-host | test.ps1:22:41:22:70 | Get-Process -Name $UserInput | This command depends on a $@. | test.ps1:164:10:164:32 | Call to read-host | user-provided value | +| test.ps1:28:38:28:67 | Get-Process -Name $UserInput | test.ps1:164:10:164:32 | Call to read-host | test.ps1:28:38:28:67 | Get-Process -Name $UserInput | This command depends on a $@. | test.ps1:164:10:164:32 | Call to read-host | user-provided value | +| test.ps1:34:14:34:46 | public class Foo { $UserInput } | test.ps1:164:10:164:32 | Call to read-host | test.ps1:34:14:34:46 | public class Foo { $UserInput } | This command depends on a $@. | test.ps1:164:10:164:32 | Call to read-host | user-provided value | +| test.ps1:40:30:40:62 | public class Foo { $UserInput } | test.ps1:164:10:164:32 | Call to read-host | test.ps1:40:30:40:62 | public class Foo { $UserInput } | This command depends on a $@. | test.ps1:164:10:164:32 | Call to read-host | user-provided value | +| test.ps1:48:30:48:34 | code | test.ps1:164:10:164:32 | Call to read-host | test.ps1:48:30:48:34 | code | This command depends on a $@. | test.ps1:164:10:164:32 | Call to read-host | user-provided value | +| test.ps1:75:25:75:54 | Get-Process -Name $UserInput | test.ps1:164:10:164:32 | Call to read-host | test.ps1:75:25:75:54 | Get-Process -Name $UserInput | This command depends on a $@. | test.ps1:164:10:164:32 | Call to read-host | user-provided value | +| test.ps1:82:16:82:45 | Get-Process -Name $UserInput | test.ps1:164:10:164:32 | Call to read-host | test.ps1:82:16:82:45 | Get-Process -Name $UserInput | This command depends on a $@. | test.ps1:164:10:164:32 | Call to read-host | user-provided value | +| test.ps1:89:12:89:28 | ping $UserInput | test.ps1:164:10:164:32 | Call to read-host | test.ps1:89:12:89:28 | ping $UserInput | This command depends on a $@. | test.ps1:164:10:164:32 | Call to read-host | user-provided value | +| test.ps1:98:33:98:62 | Get-Process -Name $UserInput | test.ps1:164:10:164:32 | Call to read-host | test.ps1:98:33:98:62 | Get-Process -Name $UserInput | This command depends on a $@. | test.ps1:164:10:164:32 | Call to read-host | user-provided value | +| test.ps1:108:58:108:87 | Get-Process -Name $UserInput | test.ps1:164:10:164:32 | Call to read-host | test.ps1:108:58:108:87 | Get-Process -Name $UserInput | This command depends on a $@. | test.ps1:164:10:164:32 | Call to read-host | user-provided value | +| test.ps1:116:34:116:43 | UserInput | test.ps1:164:10:164:32 | Call to read-host | test.ps1:116:34:116:43 | UserInput | This command depends on a $@. | test.ps1:164:10:164:32 | Call to read-host | user-provided value | +| test.ps1:123:28:123:37 | UserInput | test.ps1:164:10:164:32 | Call to read-host | test.ps1:123:28:123:37 | UserInput | This command depends on a $@. | test.ps1:164:10:164:32 | Call to read-host | user-provided value | +| test.ps1:131:28:131:37 | UserInput | test.ps1:164:10:164:32 | Call to read-host | test.ps1:131:28:131:37 | UserInput | This command depends on a $@. | test.ps1:164:10:164:32 | Call to read-host | user-provided value | +| test.ps1:139:50:139:59 | UserInput | test.ps1:164:10:164:32 | Call to read-host | test.ps1:139:50:139:59 | UserInput | This command depends on a $@. | test.ps1:164:10:164:32 | Call to read-host | user-provided value | +| test.ps1:147:63:147:72 | UserInput | test.ps1:164:10:164:32 | Call to read-host | test.ps1:147:63:147:72 | UserInput | This command depends on a $@. | test.ps1:164:10:164:32 | Call to read-host | user-provided value | +| test.ps1:154:23:154:52 | Get-Process -Name $UserInput | test.ps1:164:10:164:32 | Call to read-host | test.ps1:154:23:154:52 | Get-Process -Name $UserInput | This command depends on a $@. | test.ps1:164:10:164:32 | Call to read-host | user-provided value | +| test.ps1:160:29:160:38 | UserInput | test.ps1:164:10:164:32 | Call to read-host | test.ps1:160:29:160:38 | UserInput | This command depends on a $@. | test.ps1:164:10:164:32 | Call to read-host | user-provided value | +| test.ps1:257:7:257:10 | $o | test.ps1:254:10:254:32 | Call to read-host | test.ps1:257:7:257:10 | $o | This command depends on a $@. | test.ps1:254:10:254:32 | Call to read-host | user-provided value | +| test.ps1:269:7:269:10 | $y | test.ps1:265:14:265:36 | Call to read-host | test.ps1:269:7:269:10 | $y | This command depends on a $@. | test.ps1:265:14:265:36 | Call to read-host | user-provided value | edges | test.ps1:3:11:3:20 | userinput | test.ps1:4:23:4:52 | Get-Process -Name $UserInput | provenance | | | test.ps1:9:11:9:20 | userinput | test.ps1:10:9:10:38 | Get-Process -Name $UserInput | provenance | | @@ -145,26 +168,3 @@ nodes | test.ps1:268:5:268:6 | y | semmle.label | y | | test.ps1:269:7:269:10 | $y | semmle.label | $y | subpaths -#select -| test.ps1:4:23:4:52 | Get-Process -Name $UserInput | test.ps1:164:10:164:32 | Call to read-host | test.ps1:4:23:4:52 | Get-Process -Name $UserInput | This command depends on a $@. | test.ps1:164:10:164:32 | Call to read-host | user-provided value | -| test.ps1:10:9:10:38 | Get-Process -Name $UserInput | test.ps1:164:10:164:32 | Call to read-host | test.ps1:10:9:10:38 | Get-Process -Name $UserInput | This command depends on a $@. | test.ps1:164:10:164:32 | Call to read-host | user-provided value | -| test.ps1:16:50:16:79 | Get-Process -Name $UserInput | test.ps1:164:10:164:32 | Call to read-host | test.ps1:16:50:16:79 | Get-Process -Name $UserInput | This command depends on a $@. | test.ps1:164:10:164:32 | Call to read-host | user-provided value | -| test.ps1:22:41:22:70 | Get-Process -Name $UserInput | test.ps1:164:10:164:32 | Call to read-host | test.ps1:22:41:22:70 | Get-Process -Name $UserInput | This command depends on a $@. | test.ps1:164:10:164:32 | Call to read-host | user-provided value | -| test.ps1:28:38:28:67 | Get-Process -Name $UserInput | test.ps1:164:10:164:32 | Call to read-host | test.ps1:28:38:28:67 | Get-Process -Name $UserInput | This command depends on a $@. | test.ps1:164:10:164:32 | Call to read-host | user-provided value | -| test.ps1:34:14:34:46 | public class Foo { $UserInput } | test.ps1:164:10:164:32 | Call to read-host | test.ps1:34:14:34:46 | public class Foo { $UserInput } | This command depends on a $@. | test.ps1:164:10:164:32 | Call to read-host | user-provided value | -| test.ps1:40:30:40:62 | public class Foo { $UserInput } | test.ps1:164:10:164:32 | Call to read-host | test.ps1:40:30:40:62 | public class Foo { $UserInput } | This command depends on a $@. | test.ps1:164:10:164:32 | Call to read-host | user-provided value | -| test.ps1:48:30:48:34 | code | test.ps1:164:10:164:32 | Call to read-host | test.ps1:48:30:48:34 | code | This command depends on a $@. | test.ps1:164:10:164:32 | Call to read-host | user-provided value | -| test.ps1:75:25:75:54 | Get-Process -Name $UserInput | test.ps1:164:10:164:32 | Call to read-host | test.ps1:75:25:75:54 | Get-Process -Name $UserInput | This command depends on a $@. | test.ps1:164:10:164:32 | Call to read-host | user-provided value | -| test.ps1:82:16:82:45 | Get-Process -Name $UserInput | test.ps1:164:10:164:32 | Call to read-host | test.ps1:82:16:82:45 | Get-Process -Name $UserInput | This command depends on a $@. | test.ps1:164:10:164:32 | Call to read-host | user-provided value | -| test.ps1:89:12:89:28 | ping $UserInput | test.ps1:164:10:164:32 | Call to read-host | test.ps1:89:12:89:28 | ping $UserInput | This command depends on a $@. | test.ps1:164:10:164:32 | Call to read-host | user-provided value | -| test.ps1:98:33:98:62 | Get-Process -Name $UserInput | test.ps1:164:10:164:32 | Call to read-host | test.ps1:98:33:98:62 | Get-Process -Name $UserInput | This command depends on a $@. | test.ps1:164:10:164:32 | Call to read-host | user-provided value | -| test.ps1:108:58:108:87 | Get-Process -Name $UserInput | test.ps1:164:10:164:32 | Call to read-host | test.ps1:108:58:108:87 | Get-Process -Name $UserInput | This command depends on a $@. | test.ps1:164:10:164:32 | Call to read-host | user-provided value | -| test.ps1:116:34:116:43 | UserInput | test.ps1:164:10:164:32 | Call to read-host | test.ps1:116:34:116:43 | UserInput | This command depends on a $@. | test.ps1:164:10:164:32 | Call to read-host | user-provided value | -| test.ps1:123:28:123:37 | UserInput | test.ps1:164:10:164:32 | Call to read-host | test.ps1:123:28:123:37 | UserInput | This command depends on a $@. | test.ps1:164:10:164:32 | Call to read-host | user-provided value | -| test.ps1:131:28:131:37 | UserInput | test.ps1:164:10:164:32 | Call to read-host | test.ps1:131:28:131:37 | UserInput | This command depends on a $@. | test.ps1:164:10:164:32 | Call to read-host | user-provided value | -| test.ps1:139:50:139:59 | UserInput | test.ps1:164:10:164:32 | Call to read-host | test.ps1:139:50:139:59 | UserInput | This command depends on a $@. | test.ps1:164:10:164:32 | Call to read-host | user-provided value | -| test.ps1:147:63:147:72 | UserInput | test.ps1:164:10:164:32 | Call to read-host | test.ps1:147:63:147:72 | UserInput | This command depends on a $@. | test.ps1:164:10:164:32 | Call to read-host | user-provided value | -| test.ps1:154:23:154:52 | Get-Process -Name $UserInput | test.ps1:164:10:164:32 | Call to read-host | test.ps1:154:23:154:52 | Get-Process -Name $UserInput | This command depends on a $@. | test.ps1:164:10:164:32 | Call to read-host | user-provided value | -| test.ps1:160:29:160:38 | UserInput | test.ps1:164:10:164:32 | Call to read-host | test.ps1:160:29:160:38 | UserInput | This command depends on a $@. | test.ps1:164:10:164:32 | Call to read-host | user-provided value | -| test.ps1:257:7:257:10 | $o | test.ps1:254:10:254:32 | Call to read-host | test.ps1:257:7:257:10 | $o | This command depends on a $@. | test.ps1:254:10:254:32 | Call to read-host | user-provided value | -| test.ps1:269:7:269:10 | $y | test.ps1:265:14:265:36 | Call to read-host | test.ps1:269:7:269:10 | $y | This command depends on a $@. | test.ps1:265:14:265:36 | Call to read-host | user-provided value | diff --git a/powershell/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjectionCritical.expected b/powershell/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjectionCritical.expected deleted file mode 100644 index b55cefb43dc7..000000000000 --- a/powershell/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjectionCritical.expected +++ /dev/null @@ -1,8 +0,0 @@ -edges -| test.ps1:153:11:153:20 | userinput | test.ps1:154:23:154:52 | Get-Process -Name $UserInput | provenance | | -nodes -| test.ps1:153:11:153:20 | userinput | semmle.label | userinput | -| test.ps1:154:23:154:52 | Get-Process -Name $UserInput | semmle.label | Get-Process -Name $UserInput | -subpaths -#select -| test.ps1:154:23:154:52 | Get-Process -Name $UserInput | test.ps1:153:11:153:20 | userinput | test.ps1:154:23:154:52 | Get-Process -Name $UserInput | This command depends on a $@. | test.ps1:153:11:153:20 | userinput | param to CmdletBinding function | diff --git a/powershell/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjectionCritical.qlref b/powershell/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjectionCritical.qlref deleted file mode 100644 index 5c564e56c99b..000000000000 --- a/powershell/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjectionCritical.qlref +++ /dev/null @@ -1 +0,0 @@ -queries/security/cwe-078/CommandInjectionCritical.ql \ No newline at end of file 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 index 60662cc86584..d65e5a5a535d 100644 --- a/powershell/ql/test/query-tests/security/cwe-078/CommandInjection/test.ps1 +++ b/powershell/ql/test/query-tests/security/cwe-078/CommandInjection/test.ps1 @@ -267,4 +267,4 @@ function flow-through-env-var() { $y = $env:bar . "$y" # $ Alert # but we have flow through them -} \ No newline at end of file +} diff --git a/powershell/ql/test/query-tests/security/cwe-078/CommandInjectionCritical/CommandInjectionCritical.expected b/powershell/ql/test/query-tests/security/cwe-078/CommandInjectionCritical/CommandInjectionCritical.expected new file mode 100644 index 000000000000..6b5e3c85d645 --- /dev/null +++ b/powershell/ql/test/query-tests/security/cwe-078/CommandInjectionCritical/CommandInjectionCritical.expected @@ -0,0 +1,26 @@ +#select +| test.ps1:6:23:6:52 | Get-Process -Name $UserInput | test.ps1:5:11:5:20 | userinput | test.ps1:6:23:6:52 | Get-Process -Name $UserInput | This command depends on a $@. | test.ps1:5:11:5:20 | userinput | param to CmdletBinding function | +| test.ps1:14:7:14:17 | ScriptPath | test.ps1:13:11:13:21 | scriptpath | test.ps1:14:7:14:17 | ScriptPath | This command depends on a $@. | test.ps1:13:11:13:21 | scriptpath | param to CmdletBinding function | +| test.ps1:23:7:23:11 | path | test.ps1:21:11:21:21 | scriptpath | test.ps1:23:7:23:11 | path | This command depends on a $@. | test.ps1:21:11:21:21 | scriptpath | param to CmdletBinding function | +| test.ps1:31:23:31:52 | Get-Process -Name $UserInput | test.ps1:30:11:30:20 | userinput | test.ps1:31:23:31:52 | Get-Process -Name $UserInput | This command depends on a $@. | test.ps1:30:11:30:20 | userinput | param to CmdletBinding function | +| test.ps1:50:23:50:52 | Get-Process -Name $UserInput | test.ps1:49:11:49:20 | userinput | test.ps1:50:23:50:52 | Get-Process -Name $UserInput | This command depends on a $@. | test.ps1:49:11:49:20 | userinput | param to CmdletBinding function | +edges +| test.ps1:5:11:5:20 | userinput | test.ps1:6:23:6:52 | Get-Process -Name $UserInput | provenance | | +| test.ps1:13:11:13:21 | scriptpath | test.ps1:14:7:14:17 | ScriptPath | provenance | | +| test.ps1:21:11:21:21 | scriptpath | test.ps1:22:5:22:9 | path | provenance | | +| test.ps1:22:5:22:9 | path | test.ps1:23:7:23:11 | path | provenance | | +| test.ps1:30:11:30:20 | userinput | test.ps1:31:23:31:52 | Get-Process -Name $UserInput | provenance | | +| test.ps1:49:11:49:20 | userinput | test.ps1:50:23:50:52 | Get-Process -Name $UserInput | provenance | | +nodes +| test.ps1:5:11:5:20 | userinput | semmle.label | userinput | +| test.ps1:6:23:6:52 | Get-Process -Name $UserInput | semmle.label | Get-Process -Name $UserInput | +| test.ps1:13:11:13:21 | scriptpath | semmle.label | scriptpath | +| test.ps1:14:7:14:17 | ScriptPath | semmle.label | ScriptPath | +| test.ps1:21:11:21:21 | scriptpath | semmle.label | scriptpath | +| test.ps1:22:5:22:9 | path | semmle.label | path | +| test.ps1:23:7:23:11 | path | semmle.label | path | +| test.ps1:30:11:30:20 | userinput | semmle.label | userinput | +| test.ps1:31:23:31:52 | Get-Process -Name $UserInput | semmle.label | Get-Process -Name $UserInput | +| test.ps1:49:11:49:20 | userinput | semmle.label | userinput | +| test.ps1:50:23:50:52 | Get-Process -Name $UserInput | semmle.label | Get-Process -Name $UserInput | +subpaths diff --git a/powershell/ql/test/query-tests/security/cwe-078/CommandInjectionCritical/CommandInjectionCritical.qlref b/powershell/ql/test/query-tests/security/cwe-078/CommandInjectionCritical/CommandInjectionCritical.qlref new file mode 100644 index 000000000000..a1b0edee5d99 --- /dev/null +++ b/powershell/ql/test/query-tests/security/cwe-078/CommandInjectionCritical/CommandInjectionCritical.qlref @@ -0,0 +1,2 @@ +query: queries/security/cwe-078/CommandInjectionCritical.ql +postprocess: utils/test/InlineExpectationsTestQuery.ql diff --git a/powershell/ql/test/query-tests/security/cwe-078/CommandInjectionCritical/test.ps1 b/powershell/ql/test/query-tests/security/cwe-078/CommandInjectionCritical/test.ps1 new file mode 100644 index 000000000000..0e1b345ec47f --- /dev/null +++ b/powershell/ql/test/query-tests/security/cwe-078/CommandInjectionCritical/test.ps1 @@ -0,0 +1,52 @@ +# CmdletBinding param flows to Invoke-Expression +function Invoke-InvokeExpressionInjectionCmdletBinding +{ + [CmdletBinding()] + param($UserInput) # $ Source + Invoke-Expression "Get-Process -Name $UserInput" # $ Alert +} + +# CmdletBinding param flows directly to dot-source operator +function Invoke-DotSourceDirect +{ + [CmdletBinding()] + param($ScriptPath) # $ Source + . $ScriptPath # $ Alert +} + +# CmdletBinding param flows through a variable to dot-source operator +function Invoke-DotSourceIndirect +{ + [CmdletBinding()] + param($ScriptPath) # $ Source + $path = $ScriptPath + . $path # $ Alert +} + +# CmdletBinding param flows to Invoke-Expression (not dot-source) +function Invoke-InvokeExpressionCmdletBinding +{ + [CmdletBinding()] + param($UserInput) # $ Source + Invoke-Expression "Get-Process -Name $UserInput" # $ Alert +} + +# FP filtered: CmdletBinding param used elsewhere, unrelated dot-source of different variable +function Invoke-DotSourceUnrelatedParam +{ + [CmdletBinding()] + param($UserInput) + Write-Host $UserInput + $safePath = "C:\scripts\helper.ps1" + . $safePath # GOOD - param doesn't flow to dot-source +} + +# FP filtered: CmdletBinding param does NOT flow to the dot-sourced expression. +# The param is in scope but a hardcoded path is dot-sourced instead. +function Invoke-DotSourceNoFlow +{ + [CmdletBinding()] + param($UserInput) # $ Source + Invoke-Expression "Get-Process -Name $UserInput" # $ Alert + . "C:\safe\script.ps1" # GOOD - hardcoded, param doesn't flow here +}