-
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
Changes from all commits
a5a632f
b3dbe20
5e0ef92
e66ae68
72f8680
44ed048
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,12 +34,19 @@ module CommandInjection { | |
| class FlowSourceAsSource extends Source { | ||
| FlowSourceAsSource() { | ||
| this instanceof SourceNode and | ||
| not this instanceof EnvironmentVariableSource | ||
| not this instanceof EnvironmentVariableSource and | ||
| not this instanceof InvokeWebRequest | ||
| } | ||
|
|
||
| override string getSourceType() { result = "user-provided value" } | ||
| } | ||
|
|
||
| class InvokeWebRequest extends DataFlow::CallNode { | ||
| InvokeWebRequest(){ | ||
| this.matchesName("Invoke-WebRequest") | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * A command argument to a function that initiates an operating system command. | ||
| */ | ||
|
|
@@ -60,6 +67,16 @@ module CommandInjection { | |
| override string getSinkType() { result = "call to Invoke-Expression" } | ||
| } | ||
|
|
||
| class StartProcessSink extends Sink { | ||
| StartProcessSink(){ | ||
| exists(DataFlow::CallNode call | | ||
| call.matchesName("Start-Process") and | ||
| call.getAnArgument() = this | ||
| ) | ||
| } | ||
| override string getSinkType(){ result = "call to Start-Process"} | ||
| } | ||
|
|
||
| class AddTypeSink extends Sink { | ||
| AddTypeSink() { | ||
| exists(DataFlow::CallNode call | | ||
|
|
@@ -218,6 +235,17 @@ module CommandInjection { | |
| } | ||
| } | ||
|
|
||
| class ValidateAttributeSanitizer extends Sanitizer { | ||
| ValidateAttributeSanitizer() { | ||
| exists(Function f, Attribute a, Parameter p | | ||
| p = f.getAParameter() and | ||
| p.getAnAttribute() = a and | ||
| a.getName() = ["ValidateScript", "ValidateSet", "ValidatePattern"] and | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again: I'll do another PR to make this case insensitive. Thanks for adding this!
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed here. |
||
| this.asParameter() = p | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| class SingleQuoteSanitizer extends Sanitizer { | ||
| SingleQuoteSanitizer() { | ||
| exists(ExpandableStringExpr e, VarReadAccess v | | ||
|
|
||
| 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> |
| 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 | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've fixed this here. |
||
| 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() | ||
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.
Should we only remove it when it's given a constant string literal as a source? Or do you think it's better to totally remove it like you're doing here?
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 think totally remove it. It could be a case if there's flow from user input -> InvokeWebRequest -> command call, but that's more of an SSRF vuln first, which we can model as a separate query