Merge pull request #278 from microsoft/powershell-cmd-injection-updates

Powershell command injection updates

Author: MathiasVP

powershell/ql/lib/semmle/code/powershell/security/CommandInjectionCustomizations.qll

@@ -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
+        this.asParameter() = p
+      )
+    }
+  }
+
   class SingleQuoteSanitizer extends Sanitizer {
     SingleQuoteSanitizer() {
       exists(ExpandableStringExpr e, VarReadAccess v |

powershell/ql/src/queries/security/cwe-078/CommandInjectionCritical.qhelp

@@ -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>

powershell/ql/src/queries/security/cwe-078/CommandInjectionCritical.ql

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