Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

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?

Copy link
Author

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

}

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.
*/
Expand All @@ -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 |
Expand Down Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The 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!

Copy link
Collaborator

Choose a reason for hiding this comment

The 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 |
Expand Down
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>&amp;</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
Copy link
Collaborator

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 on Attribute yet.

Copy link
Collaborator

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.

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()
Loading
Loading