Merge pull request #258 from microsoft/fix-call-operator-bug

PS: Fix bug in `CallOperator::getCommand`

Author: chanel-y

powershell/ql/lib/semmle/code/powershell/ast/internal/Command.qll

@@ -76,14 +76,14 @@ class CmdCall extends CallExpr, TCmd {
 class CallOperator extends CmdCall {
   CallOperator() { getRawAst(this) instanceof Raw::CallOperator }
 
-  Expr getCommand() { result = this.getArgument(0) }
+  Expr getCommand() { result = this.getCallee() }
 }
 
 /** A call to the dot-sourcing `.`. */
 class DotSourcingOperator extends CmdCall {
   DotSourcingOperator() { getRawAst(this) instanceof Raw::DotSourcingOperator }
 
-  Expr getPath() { result = this.getArgument(0) }
+  Expr getCommand() { result = this.getCallee() }
 }
 
 class JoinPath extends CmdCall {

powershell/ql/lib/semmle/code/powershell/controlflow/CfgNodes.qll

@@ -605,7 +605,22 @@ module ExprNodes {
 
     override CallOperator getExpr() { result = e }
 
-    ExprCfgNode getCommand() { result = this.getArgument(0) }
+    ExprCfgNode getCommand() { result = this.getCallee() }
+  }
+
+  private class DotSourcingOperatorChildMapping extends CallExprChildMapping instanceof DotSourcingOperator
+  {
+    override predicate relevantChild(Ast child) { super.relevantChild(child) }
+  }
+
+  class DotSourcingOperatorCfgNode extends CallExprCfgNode {
+    override string getAPrimaryQlClass() { result = "DotSourcingOperatorCfgNode" }
+
+    override DotSourcingOperatorChildMapping e;
+
+    override DotSourcingOperator getExpr() { result = e }
+
+    ExprCfgNode getCommand() { result = this.getCallee() }
   }
 
   private class ToStringCallChildmapping extends CallExprChildMapping instanceof ToStringCall {

powershell/ql/lib/semmle/code/powershell/dataflow/internal/DataFlowPublic.qll

@@ -540,11 +540,18 @@ class CallNode extends ExprNode {
   Node getCallee() { result.asExpr() = call.getCallee() }
 }
 
-/** A call to operator `&`, viwed as a node in a data flow graph. */
+/** A call to operator `&`, viewed as a node in a data flow graph. */
 class CallOperatorNode extends CallNode {
   override CfgNodes::ExprNodes::CallOperatorCfgNode call;
 
-  Node getCommand() { result.asExpr() = call.getCommand() } // TODO: Alternatively, we could remap calls to & as command expressions.
+  Node getCommand() { result.asExpr() = call.getCommand() }
+}
+
+/** A call to operator `.`, viewed as a node in a data flow graph. */
+class DotSourcingOperatorNode extends CallNode {
+  override CfgNodes::ExprNodes::DotSourcingOperatorCfgNode call;
+
+  Node getCommand() { result.asExpr() = call.getCommand() }
 }
 
 /**

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

@@ -46,8 +46,10 @@ module CommandInjection {
         call.getAnArgument() = this
       )
       or
-      // Or the call command itself in case it's a use of operator &.
+      // Or the call command itself in case it's a use of "operator &" or "operator .".
       any(DataFlow::CallOperatorNode call).getCommand() = this
+      or
+      any(DataFlow::DotSourcingOperatorNode call).getCommand() = this
     }
 
     override string getSinkType() { result = "call to Invoke-Expression" }

powershell/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.expected

@@ -53,6 +53,7 @@ edges
 | test.ps1:170:36:170:41 | input | test.ps1:129:11:129:20 | userinput | provenance |  |
 | test.ps1:172:42:172:47 | input | test.ps1:136:11:136:20 | userinput | provenance |  |
 | test.ps1:173:42:173:47 | input | test.ps1:144:11:144:20 | userinput | provenance |  |
+| test.ps1:214:10:214:32 | Call to read-host | test.ps1:217:7:217:10 | $o | provenance | Src:MaD:0  |
 nodes
 | test.ps1:3:11:3:20 | userinput | semmle.label | userinput |
 | test.ps1:4:23:4:52 | Get-Process -Name $UserInput | semmle.label | Get-Process -Name $UserInput |
@@ -109,6 +110,8 @@ nodes
 | test.ps1:170:36:170:41 | input | semmle.label | input |
 | test.ps1:172:42:172:47 | input | semmle.label | input |
 | test.ps1:173:42:173:47 | input | semmle.label | input |
+| test.ps1:214:10:214:32 | Call to read-host | semmle.label | Call to read-host |
+| test.ps1:217:7:217:10 | $o | semmle.label | $o |
 subpaths
 #select
 | test.ps1:4:23:4:52 | Get-Process -Name $UserInput | test.ps1:152:10:152:32 | Call to read-host | test.ps1:4:23:4:52 | Get-Process -Name $UserInput | This command depends on a $@. | test.ps1:152:10:152:32 | Call to read-host | user-provided value |
@@ -129,3 +132,4 @@ subpaths
 | test.ps1:131:28:131:37 | UserInput | test.ps1:152:10:152:32 | Call to read-host | test.ps1:131:28:131:37 | UserInput | This command depends on a $@. | test.ps1:152:10:152:32 | Call to read-host | user-provided value |
 | test.ps1:139:50:139:59 | UserInput | test.ps1:152:10:152:32 | Call to read-host | test.ps1:139:50:139:59 | UserInput | This command depends on a $@. | test.ps1:152:10:152:32 | Call to read-host | user-provided value |
 | test.ps1:147:63:147:72 | UserInput | test.ps1:152:10:152:32 | Call to read-host | test.ps1:147:63:147:72 | UserInput | This command depends on a $@. | test.ps1:152:10:152:32 | Call to read-host | user-provided value |
+| test.ps1:217:7:217:10 | $o | test.ps1:214:10:214:32 | Call to read-host | test.ps1:217:7:217:10 | $o | This command depends on a $@. | test.ps1:214:10:214:32 | Call to read-host | user-provided value |

powershell/ql/test/query-tests/security/cwe-078/CommandInjection/test.ps1

@@ -207,4 +207,12 @@ function Invoke-InvokeExpressionInjectionSafe4
 Invoke-InvokeExpressionInjectionSafe1 -UserInput $input 
 Invoke-InvokeExpressionInjectionSafe2 -UserInput $input 
 Invoke-InvokeExpressionInjectionSafe3 -UserInput $input 
-Invoke-InvokeExpressionInjectionSafe4 -UserInput $input 
+Invoke-InvokeExpressionInjectionSafe4 -UserInput $input 
+
+function false-positive-in-call-operator($d)
+{
+    $o = Read-Host "enter input"
+    & unzip -o "$o" -d $d # GOOD
+
+    . "$o" # BAD
+}