Merge pull request #696 from esben-semmle/js/host-request-forgery

semmle-qlci · web-flow · commit b11b714152be · 2018-12-19T15:04:08.000Z
Approved by asger-semmle
diff --git a/change-notes/1.20/analysis-javascript.md b/change-notes/1.20/analysis-javascript.md
@@ -26,6 +26,7 @@
 |--------------------------------------------|------------------------------|------------------------------------------------------------------------------|
 | Client-side cross-site scripting           | More true-positive results, fewer false-positive results.                 | This rule now recognizes WinJS functions that are vulnerable to HTML injection, and no longer flags certain safe uses of jQuery. |
 | Insecure randomness | More results | This rule now flags insecure uses of `crypto.pseudoRandomBytes`. |
+| Uncontrolled data used in network request  | More results                 | This rule now recognizes host values that are vulnerable to injection. |
 | Unused parameter                           | Fewer false-positive results | This rule no longer flags parameters with leading underscore. |
 | Unused variable, import, function or class | Fewer false-positive results | This rule now flags fewer variables that are implictly used by JSX elements, and no longer flags variables with leading underscore. |
 
diff --git a/javascript/ql/src/semmle/javascript/frameworks/ClientRequests.qll b/javascript/ql/src/semmle/javascript/frameworks/ClientRequests.qll
@@ -21,6 +21,11 @@ abstract class CustomClientRequest extends DataFlow::InvokeNode {
    */
   abstract DataFlow::Node getUrl();
 
+  /**
+   * Gets the host of the request.
+   */
+  abstract DataFlow::Node getHost();
+
   /**
    * Gets a node that contributes to the data-part this request.
    */
@@ -50,6 +55,13 @@ class ClientRequest extends DataFlow::InvokeNode {
     result = custom.getUrl()
   }
 
+  /**
+   * Gets the host of the request.
+   */
+  DataFlow::Node getHost() {
+    result = custom.getHost()
+  }
+
   /**
    * Gets a node that contributes to the data-part this request.
    */
@@ -67,7 +79,7 @@ private string httpMethodName() {
 }
 
 /**
- * Gets the name of a property that likely contains a  URL value.
+ * Gets the name of a property that likely contains a URL value.
  */
 private string urlPropertyName() {
   result = "uri" or
@@ -93,16 +105,17 @@ private class RequestUrlRequest extends CustomClientRequest {
       (
         callee = DataFlow::moduleImport(moduleName) or
         callee = DataFlow::moduleMember(moduleName, httpMethodName())
-      ) and
-      (
-        url = getArgument(0) or
-        url = getOptionArgument(0, urlPropertyName())
       )
     )
   }
 
   override DataFlow::Node getUrl() {
-    result = url
+    result = getArgument(0) or
+    result = getOptionArgument(0, urlPropertyName())
+  }
+
+  override DataFlow::Node getHost() {
+    none()
   }
 
   override DataFlow::Node getADataNode() {
@@ -129,10 +142,18 @@ private class AxiosUrlRequest extends CustomClientRequest {
     )
   }
 
+  private DataFlow::Node getOptionArgument(string name) {
+    // depends on the method name and the call arity, over-approximating slightly in the name of simplicity
+    result = getOptionArgument([0..2], name)
+  }
+
   override DataFlow::Node getUrl() {
     result = getArgument(0) or
-    // depends on the method name and the call arity, over-approximating slightly in the name of simplicity
-    result = getOptionArgument([0..2], urlPropertyName())
+    result = getOptionArgument(urlPropertyName())
+  }
+
+  override DataFlow::Node getHost() {
+    result = getOptionArgument("host")
   }
 
   override DataFlow::Node getADataNode() {
@@ -179,6 +200,8 @@ private class FetchUrlRequest extends CustomClientRequest {
     result = url
   }
 
+  override DataFlow::Node getHost() { none() }
+
   override DataFlow::Node getADataNode() {
     exists (string name |
       name = "headers" or name = "body" |
@@ -209,6 +232,14 @@ private class GotUrlRequest extends CustomClientRequest {
     not exists (getOptionArgument(1, "baseUrl"))
   }
 
+  override DataFlow::Node getHost() {
+    exists (string name |
+      name = "host" or
+      name = "hostname" |
+      result = getOptionArgument(1, name)
+    )
+  }
+
   override DataFlow::Node getADataNode() {
     exists (string name |
       name = "headers" or name = "body" or name = "query" |
@@ -238,6 +269,8 @@ private class SuperAgentUrlRequest extends CustomClientRequest {
     result = url
   }
 
+  override DataFlow::Node getHost() { none() }
+
   override DataFlow::Node getADataNode() {
     exists (string name |
       name = "set" or name = "send" or name = "query" |
@@ -255,5 +288,6 @@ private class XMLHttpRequest extends CustomClientRequest {
 
   override DataFlow::Node getUrl() { result = getAMethodCall("open").getArgument(1) }
 
+  override DataFlow::Node getHost() { none() }
   override DataFlow::Node getADataNode() { result = getAMethodCall("send").getArgument(0) }
 }
diff --git a/javascript/ql/src/semmle/javascript/frameworks/Electron.qll b/javascript/ql/src/semmle/javascript/frameworks/Electron.qll
@@ -64,6 +64,14 @@ module Electron {
       result = getOptionArgument(0, "url")
     }
 
+    override DataFlow::Node getHost() {
+      exists (string name |
+        name = "host" or
+        name = "hostname" |
+        result = getOptionArgument(0, name)
+      )
+    }
+
     override DataFlow::Node getADataNode() {
       exists (string name |
         name = "write" or name = "end" |
diff --git a/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll b/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll
@@ -737,6 +737,14 @@ module NodeJSLib {
       result = url
     }
 
+    override DataFlow::Node getHost() {
+      exists (string name |
+        name = "host" or
+        name = "hostname" |
+        result = getOptionArgument(1, name)
+      )
+    }
+
     override DataFlow::Node getADataNode() {
       exists (string name |
         name = "write" or name = "end" |
diff --git a/javascript/ql/src/semmle/javascript/frameworks/jQuery.qll b/javascript/ql/src/semmle/javascript/frameworks/jQuery.qll
@@ -359,5 +359,7 @@ private class JQueryClientRequest extends CustomClientRequest {
     result = getOptionArgument([0 .. 1], "url")
   }
 
+  override DataFlow::Node getHost() { none() }
+
   override DataFlow::Node getADataNode() { result = getOptionArgument([0 .. 1], "data") }
 }
diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/RequestForgery.qll b/javascript/ql/src/semmle/javascript/security/dataflow/RequestForgery.qll
@@ -72,16 +72,20 @@ module RequestForgery {
 
     ClientRequest request;
 
+    string kind;
+
     ClientRequestUrlAsSink() {
-      this = request.getUrl()
+      this = request.getUrl() and kind = "URL" or
+      this = request.getHost() and kind = "host"
     }
 
     override DataFlow::Node getARequest() {
       result = request
     }
 
     override string getKind() {
-      result = "URL"
+      result = kind
     }
+
   }
 }
diff --git a/javascript/ql/test/library-tests/frameworks/ClientRequests/ClientRequest.expected b/javascript/ql/test/library-tests/frameworks/ClientRequests/ClientRequest.expected
@@ -32,3 +32,7 @@
 | tst.js:77:5:77:32 | $.getJS ...  data}) |
 | tst.js:78:5:78:38 | $.getJS ...  data}) |
 | tst.js:80:15:80:34 | new XMLHttpRequest() |
+| tst.js:87:5:87:39 | http.ge ...  host}) |
+| tst.js:89:5:89:23 | axios({host: host}) |
+| tst.js:91:5:91:34 | got(rel ...  host}) |
+| tst.js:93:5:93:35 | net.req ... host }) |
diff --git a/javascript/ql/test/library-tests/frameworks/ClientRequests/ClientRequest_getHost.expected b/javascript/ql/test/library-tests/frameworks/ClientRequests/ClientRequest_getHost.expected
@@ -0,0 +1,4 @@
+| tst.js:87:5:87:39 | http.ge ...  host}) | tst.js:87:34:87:37 | host |
+| tst.js:89:5:89:23 | axios({host: host}) | tst.js:89:18:89:21 | host |
+| tst.js:91:5:91:34 | got(rel ...  host}) | tst.js:91:29:91:32 | host |
+| tst.js:93:5:93:35 | net.req ... host }) | tst.js:93:29:93:32 | host |
diff --git a/javascript/ql/test/library-tests/frameworks/ClientRequests/ClientRequest_getHost.ql b/javascript/ql/test/library-tests/frameworks/ClientRequests/ClientRequest_getHost.ql
@@ -0,0 +1,4 @@
+import javascript
+
+from ClientRequest r
+select r, r.getHost()
diff --git a/javascript/ql/test/library-tests/frameworks/ClientRequests/ClientRequest_getUrl.expected b/javascript/ql/test/library-tests/frameworks/ClientRequests/ClientRequest_getUrl.expected
@@ -38,3 +38,7 @@
 | tst.js:78:5:78:38 | $.getJS ...  data}) | tst.js:78:15:78:37 | {url: u ... : data} |
 | tst.js:78:5:78:38 | $.getJS ...  data}) | tst.js:78:21:78:23 | url |
 | tst.js:80:15:80:34 | new XMLHttpRequest() | tst.js:81:17:81:19 | url |
+| tst.js:87:5:87:39 | http.ge ...  host}) | tst.js:87:14:87:24 | relativeUrl |
+| tst.js:89:5:89:23 | axios({host: host}) | tst.js:89:11:89:22 | {host: host} |
+| tst.js:91:5:91:34 | got(rel ...  host}) | tst.js:91:9:91:19 | relativeUrl |
+| tst.js:93:5:93:35 | net.req ... host }) | tst.js:93:17:93:34 | { hostname: host } |
diff --git a/javascript/ql/test/library-tests/frameworks/ClientRequests/tst.js b/javascript/ql/test/library-tests/frameworks/ClientRequests/tst.js
@@ -81,3 +81,15 @@ import {ClientRequest, net} from 'electron';
     xhr.open(_, url);
     xhr.send(data);
 });
+
+(function() {
+
+    http.get(relativeUrl, {host: host});
+
+    axios({host: host});
+
+    got(relativeUrl, {host: host});
+
+    net.request({ hostname: host });
+
+});
diff --git a/javascript/ql/test/query-tests/Security/CWE-918/RequestForgery.expected b/javascript/ql/test/query-tests/Security/CWE-918/RequestForgery.expected
@@ -13,13 +13,15 @@ nodes
 | tst.js:26:36:26:42 | tainted |
 | tst.js:28:13:28:43 | "http:/ ... tainted |
 | tst.js:28:37:28:43 | tainted |
+| tst.js:32:34:32:40 | tainted |
 edges
 | tst.js:12:9:12:52 | tainted | tst.js:16:13:16:19 | tainted |
 | tst.js:12:9:12:52 | tainted | tst.js:18:17:18:23 | tainted |
 | tst.js:12:9:12:52 | tainted | tst.js:21:19:21:25 | tainted |
 | tst.js:12:9:12:52 | tainted | tst.js:24:25:24:31 | tainted |
 | tst.js:12:9:12:52 | tainted | tst.js:26:36:26:42 | tainted |
 | tst.js:12:9:12:52 | tainted | tst.js:28:37:28:43 | tainted |
+| tst.js:12:9:12:52 | tainted | tst.js:32:34:32:40 | tainted |
 | tst.js:12:19:12:42 | url.par ... , true) | tst.js:12:19:12:48 | url.par ... ).query |
 | tst.js:12:19:12:48 | url.par ... ).query | tst.js:12:19:12:52 | url.par ... ery.url |
 | tst.js:12:19:12:52 | url.par ... ery.url | tst.js:12:9:12:52 | tainted |
@@ -34,3 +36,4 @@ edges
 | tst.js:24:5:24:32 | request ... ainted) | tst.js:12:29:12:35 | req.url | tst.js:24:13:24:31 | "http://" + tainted | The $@ of this request depends on $@. | tst.js:24:13:24:31 | "http://" + tainted | URL | tst.js:12:29:12:35 | req.url | a user-provided value |
 | tst.js:26:5:26:43 | request ... ainted) | tst.js:12:29:12:35 | req.url | tst.js:26:13:26:42 | "http:/ ... tainted | The $@ of this request depends on $@. | tst.js:26:13:26:42 | "http:/ ... tainted | URL | tst.js:12:29:12:35 | req.url | a user-provided value |
 | tst.js:28:5:28:44 | request ... ainted) | tst.js:12:29:12:35 | req.url | tst.js:28:13:28:43 | "http:/ ... tainted | The $@ of this request depends on $@. | tst.js:28:13:28:43 | "http:/ ... tainted | URL | tst.js:12:29:12:35 | req.url | a user-provided value |
+| tst.js:32:5:32:42 | http.ge ... inted}) | tst.js:12:29:12:35 | req.url | tst.js:32:34:32:40 | tainted | The $@ of this request depends on $@. | tst.js:32:34:32:40 | tainted | host | tst.js:12:29:12:35 | req.url | a user-provided value |
diff --git a/javascript/ql/test/query-tests/Security/CWE-918/tst.js b/javascript/ql/test/query-tests/Security/CWE-918/tst.js
@@ -28,4 +28,6 @@ var server = http.createServer(function(req, res) {
     request("http://example.com/" + tainted); // NOT OK
 
     request("http://example.com/?" + tainted); // OK
+
+    http.get(relativeUrl, {host: tainted}); // NOT OK
 })

Original file line number	Diff line number	Diff line change
`@@ -359,5 +359,7 @@ private class JQueryClientRequest extends CustomClientRequest {`
`359`	`359`	`result = getOptionArgument([0 .. 1], "url")`
`360`	`360`	`}`
`361`	`361`
	`362`	`+ override DataFlow::Node getHost() { none() }`
	`363`	`+`
`362`	`364`	`override DataFlow::Node getADataNode() { result = getOptionArgument([0 .. 1], "data") }`
`363`	`365`	`}`
Original file line number	Diff line number	Diff line change
`@@ -72,16 +72,20 @@ module RequestForgery {`
`72`	`72`
`73`	`73`	`ClientRequest request;`
`74`	`74`
	`75`	`+ string kind;`
	`76`	`+`
`75`	`77`	`ClientRequestUrlAsSink() {`
`76`		`- this = request.getUrl()`
	`78`	`+ this = request.getUrl() and kind = "URL" or`
	`79`	`+ this = request.getHost() and kind = "host"`
`77`	`80`	`}`
`78`	`81`
`79`	`82`	`override DataFlow::Node getARequest() {`
`80`	`83`	`result = request`
`81`	`84`	`}`
`82`	`85`
`83`	`86`	`override string getKind() {`
`84`		`- result = "URL"`
	`87`	`+ result = kind`
`85`	`88`	`}`
	`89`	`+`
`86`	`90`	`}`
`87`	`91`	`}`