forked from github/codeql
-
Notifications
You must be signed in to change notification settings - Fork 20
Powershell command injection updates #278
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
Merged
Merged
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
a5a632f
added cmdletbinding case to tests
chanel-y b3dbe20
adding powershell cmd injection critical query, updated unit test res…
chanel-y 5e0ef92
adding Start-Process as sink
chanel-y e66ae68
add ValidateAttribute case
chanel-y 72f8680
remove Invoke-WebRequest as source
chanel-y 44ed048
Merge branch 'main' into powershell-cmd-injection-updates
MathiasVP File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
63 changes: 63 additions & 0 deletions
63
powershell/ql/src/queries/security/cwe-078/CommandInjectionCritical.qhelp
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| <!DOCTYPE qhelp PUBLIC | ||
| "-//Semmle//qhelp//EN" | ||
| "qhelp.dtd"> | ||
| <qhelp> | ||
| <overview> | ||
| <p>Code that passes user input directly to | ||
| <code>Invoke-Expression</code>, <code>&</code>, or some other library | ||
| routine that executes a command, allows the user to execute malicious | ||
| code.</p> | ||
|
|
||
| <p>The following are considered dangerous sinks: </p> | ||
| <ul> | ||
| <li>Invoke-Expression</li> | ||
| <li>InvokeScript</li> | ||
| <li>CreateNestedPipeline</li> | ||
| <li>AddScript</li> | ||
| <li>powershell</li> | ||
| <li>cmd</li> | ||
| <li>Foreach-Object</li> | ||
| <li>Invoke</li> | ||
| <li>CreateScriptBlock</li> | ||
| <li>NewScriptBlock</li> | ||
| <li>ExpandString</li> | ||
| </ul> | ||
|
|
||
| </overview> | ||
| <recommendation> | ||
|
|
||
| <p>If possible, use hard-coded string literals to specify the command to run | ||
| or library to load. Instead of passing the user input directly to the | ||
| process or library function, examine the user input and then choose | ||
| among hard-coded string literals.</p> | ||
|
|
||
| <p>If the applicable libraries or commands cannot be determined at | ||
| compile time, then add code to verify that the user input string is | ||
| safe before using it.</p> | ||
|
|
||
| </recommendation> | ||
| <example> | ||
|
|
||
| <p>The following example shows code that takes a shell script that can be changed | ||
| maliciously by a user, and passes it straight to <code>Invoke-Expression</code> | ||
| without examining it first.</p> | ||
|
|
||
| <sample src="examples/command_injection.ps1" /> | ||
|
|
||
| </example> | ||
| <references> | ||
|
|
||
| <li> | ||
| OWASP: | ||
| <a href="https://www.owasp.org/index.php/Command_Injection">Command Injection</a>. | ||
| </li> | ||
| <li> | ||
| Injection Hunter: | ||
| <a href="https://devblogs.microsoft.com/powershell/powershell-injection-hunter-security-auditing-for-powershell-scripts/">PowerShell Injection Hunter: Security Auditing for PowerShell Scripts</a>. | ||
| </li> | ||
| <!-- LocalWords: CWE untrusted unsanitized Runtime | ||
| --> | ||
|
|
||
|
|
||
| </references> | ||
| </qhelp> |
59 changes: 59 additions & 0 deletions
59
powershell/ql/src/queries/security/cwe-078/CommandInjectionCritical.ql
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| /** | ||
| * @name Uncontrolled command line from param to CmdletBinding | ||
| * @description Using externally controlled strings in a command line may allow a malicious | ||
| * user to change the meaning of the command. | ||
| * @kind path-problem | ||
| * @problem.severity error | ||
| * @security-severity 9.8 | ||
| * @precision high | ||
| * @id powershell/microsoft/public/command-injection-critical | ||
| * @tags correctness | ||
| * security | ||
| * external/cwe/cwe-078 | ||
| * external/cwe/cwe-088 | ||
| */ | ||
|
|
||
| import powershell | ||
| import semmle.code.powershell.security.CommandInjectionCustomizations::CommandInjection | ||
| import semmle.code.powershell.dataflow.TaintTracking | ||
| import semmle.code.powershell.dataflow.DataFlow | ||
|
|
||
| abstract class CriticalSource extends DataFlow::Node { | ||
| /** Gets a string that describes the type of this flow source. */ | ||
| abstract string getSourceType(); | ||
| } | ||
|
|
||
| class CmdletBindingParam extends CriticalSource { | ||
| CmdletBindingParam(){ | ||
| exists(Attribute a, Function f | | ||
| a.getName() = "CmdletBinding" and | ||
| f = a.getEnclosingFunction() and | ||
| this.asParameter() = f.getAParameter() | ||
| ) | ||
| } | ||
| override string getSourceType(){ | ||
| result = "param to CmdletBinding function" | ||
| } | ||
| } | ||
|
|
||
|
|
||
| private module Config implements DataFlow::ConfigSig { | ||
| predicate isSource(DataFlow::Node source) { source instanceof CriticalSource } | ||
|
|
||
| predicate isSink(DataFlow::Node sink) { sink instanceof Sink } | ||
|
|
||
| predicate isBarrier(DataFlow::Node node) { node instanceof Sanitizer } | ||
| } | ||
|
|
||
| /** | ||
| * Taint-tracking for reasoning about command-injection vulnerabilities. | ||
| */ | ||
| module CommandInjectionCriticalFlow = TaintTracking::Global<Config>; | ||
| import CommandInjectionCriticalFlow::PathGraph | ||
|
|
||
| from CommandInjectionCriticalFlow::PathNode source, CommandInjectionCriticalFlow::PathNode sink, CriticalSource sourceNode | ||
| where | ||
| CommandInjectionCriticalFlow::flowPath(source, sink) and | ||
| sourceNode = source.getNode() | ||
| select sink.getNode(), source, sink, "This command depends on a $@.", sourceNode, | ||
| sourceNode.getSourceType() | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably to case insensitive matching since I'm sure PowerShell happily accepts
cMdlEtBinDinG. But I'll fix that in another since we don't have an API for this onAttributeyet.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've fixed this here.