Merge pull request #118 from esben-semmle/js/request-forgery

Approved by asger-semmle

Author: semmle-qlci

change-notes/1.18/analysis-javascript.md

@@ -19,11 +19,13 @@
 * Type inference for simple function calls has been improved. This may give additional results for queries that rely on type inference.
 
 * Support for popular libraries has been improved. Consequently, queries may produce more results on code bases that use the following libraries:
+  - [axios](https://github.com/axios/axios)
   - [bluebird](https://bluebirdjs.com)
   - [browserid-crypto](https://github.com/mozilla/browserid-crypto)
   - [compose-function](https://github.com/stoeffel/compose-function)
   - [cookie-parser](https://github.com/expressjs/cookie-parser)
   - [cookie-session](https://github.com/expressjs/cookie-session)
+  - [cross-fetch](https://github.com/lquixada/cross-fetch)
   - [crypto-js](https://github.com/https://github.com/brix/crypto-js)
   - [deep-assign](https://github.com/sindresorhus/deep-assign)
   - [deep-extend](https://github.com/unclechu/node-deep-extend)
@@ -45,9 +47,11 @@
   - [fast-json-parse](https://github.com/mcollina/fast-json-parse)
   - [forge](https://github.com/digitalbazaar/forge)
   - [format-util](https://github.com/tmpfs/format-util)
+  - [got](https://github.com/sindresorhus/got)
   - [global](https://github.com/Raynos/global)
   - [he](https://github.com/mathiasbynens/he)
   - [html-entities](https://github.com/mdevils/node-html-entities)
+  - [isomorphic-fetch](https://github.com/matthew-andrews/isomorphic-fetch)
   - [jquery](https://jquery.com)
   - [js-extend](https://github.com/vmattos/js-extend)
   - [json-parse-better-errors](https://github.com/zkat/json-parse-better-errors)
@@ -63,6 +67,7 @@
   - [mixin-object](https://github.com/jonschlinkert/mixin-object)
   - [MySQL2](https://github.com/sidorares/node-mysql2)
   - [node.extend](https://github.com/dreamerslab/node.extend)
+  - [node-fetch](https://github.com/bitinn/node-fetch)
   - [object-assign](https://github.com/sindresorhus/object-assign)
   - [object.assign](https://github.com/ljharb/object.assign)
   - [object.defaults](https://github.com/jonschlinkert/object.defaults)
@@ -71,13 +76,18 @@
   - [printj](https://github.com/SheetJS/printj)
   - [q](https://documentup.com/kriskowal/q/)
   - [ramda](https://ramdajs.com)
+  - [request](https://github.com/request/request)
+  - [request-promise](https://github.com/request/request-promise)
+  - [request-promise-any](https://github.com/request/request-promise-any)
+  - [request-promise-native](https://github.com/request/request-promise-native)
   - [React Native](https://facebook.github.io/react-native/)
   - [safe-json-parse](https://github.com/Raynos/safe-json-parse)
   - [sanitize](https://github.com/pocketly/node-sanitize)
   - [sanitizer](https://github.com/theSmaw/Caja-HTML-Sanitizer)
   - [smart-extend](https://github.com/danielkalen/smart-extend)
   - [sprintf.js](https://github.com/alexei/sprintf.js)
   - [string-template](https://github.com/Matt-Esch/string-template)
+  - [superagent](https://github.com/visionmedia/superagent)
   - [underscore](https://underscorejs.org)
   - [util-extend](https://github.com/isaacs/util-extend)
   - [utils-merge](https://github.com/jaredhanson/utils-merge)
@@ -94,6 +104,7 @@
 | Clear-text logging of sensitive information (`js/clear-text-logging`) | security, external/cwe/cwe-312, external/cwe/cwe-315, external/cwe/cwe-359 | Highlights logging of sensitive information, indicating a violation of [CWE-312](https://cwe.mitre.org/data/definitions/312.html). Results shown on LGTM by default. |
 | Disabling Electron webSecurity (`js/disabling-electron-websecurity`) | security, frameworks/electron | Highlights Electron browser objects that are created with the `webSecurity` property set to false. Results shown on LGTM by default. |
 | Enabling Electron allowRunningInsecureContent (`js/enabling-electron-insecure-content`) | security, frameworks/electron | Highlights Electron browser objects that are created with the `allowRunningInsecureContent` property set to true. Results shown on LGTM by default. |
+| Uncontrolled data used in remote request (`js/request-forgery`) | security, external/cwe/cwe-918 | Highlights remote requests that are built from unsanitized user input, indicating a violation of [CWE-918](https://cwe.mitre.org/data/definitions/918.html). Results are not shown on LGTM by default. |
 | Use of externally-controlled format string (`js/tainted-format-string`) | security, external/cwe/cwe-134 | Highlights format strings containing user-provided data, indicating a violation of [CWE-134](https://cwe.mitre.org/data/definitions/134.html). Results shown on LGTM by default. |
 
 ## Changes to existing queries

javascript/config/suites/javascript/security

@@ -29,3 +29,4 @@
 + semmlecode-javascript-queries/Security/CWE-807/DifferentKindsComparisonBypass.ql: /Security/CWE/CWE-807
 + semmlecode-javascript-queries/Security/CWE-843/TypeConfusionThroughParameterTampering.ql: /Security/CWE/CWE-834
 + semmlecode-javascript-queries/Security/CWE-916/InsufficientPasswordHash.ql: /Security/CWE/CWE-916
++ semmlecode-javascript-queries/Security/CWE-918/RequestForgery.ql: /Security/CWE/CWE-918

javascript/ql/src/Security/CWE-918/RequestForgery.qhelp

@@ -0,0 +1,79 @@
+<!DOCTYPE qhelp PUBLIC
+"-//Semmle//qhelp//EN"
+"qhelp.dtd">
+<qhelp>
+
+	<overview>
+		<p>
+
+			Directly incorporating user input into an HTTP request
+			without validating the input can facilitate different kinds of request
+			forgery attacks, where the attacker essentially controls the request.
+
+			If the vulnerable request is in server-side code, then security
+			mechanisms, such as external firewalls, can be bypassed.
+
+			If the vulnerable request is in client-side code, then unsuspecting
+			users can send malicious requests to other servers, potentially
+			resulting in a DDOS attack.
+
+		</p>
+	</overview>
+
+	<recommendation>
+
+		<p>
+
+			To guard against request forgery, it is advisable to avoid
+			putting user input directly into a remote request. If a flexible
+			remote request mechanism is required, it is recommended to maintain a
+			list of authorized request targets and choose from that list based on
+			the user input provided.
+
+		</p>
+
+	</recommendation>
+
+	<example>
+
+		<p>
+
+			The following example shows an HTTP request parameter
+			being used directly in a URL request without validating the input,
+			which facilitates an SSRF attack. The request
+			<code>http.get(...)</code> is vulnerable since attackers can choose
+			the value of <code>target</code> to be anything they want. For
+			instance, the attacker can choose
+			<code>"internal.example.com/#"</code> as the target, causing the URL
+			used in the request to be
+			<code>"https://internal.example.com/#.example.com/data"</code>.
+
+		</p>
+
+		<p>
+
+			A request to <code>https://internal.example.com</code> may
+			be problematic if that server is not meant to be
+			directly accessible from the attacker's machine.
+
+		</p>
+
+		<sample src="examples/RequestForgeryBad.js"/>
+
+		<p>
+
+			One way to remedy the problem is to use the user input to
+			select a known fixed string before performing the request:
+
+		</p>
+
+        <sample src="examples/RequestForgeryGood.js"/>
+
+	</example>
+
+	<references>
+
+		<li>OWASP: <a href="https://www.owasp.org/index.php/Server_Side_Request_Forgery">SSRF</a></li>
+
+	</references>
+</qhelp>

javascript/ql/src/Security/CWE-918/RequestForgery.ql

@@ -0,0 +1,18 @@
+/**
+ * @name Uncontrolled data used in remote request
+ * @description Sending remote requests with user-controlled data allows for request forgery attacks.
+ * @kind problem
+ * @problem.severity error
+ * @precision medium
+ * @id js/request-forgery
+ * @tags security
+ *       external/cwe/cwe-918
+ */
+
+import javascript
+import semmle.javascript.security.dataflow.RequestForgery::RequestForgery
+
+from Configuration cfg, DataFlow::Node source, Sink sink, DataFlow::Node request
+where cfg.hasFlow(source, sink) and
+      request = sink.getARequest()
+select request, "The $@ of this request depends on $@.", sink, sink.getKind(), source, "a user-provided value"

javascript/ql/src/Security/CWE-918/examples/RequestForgeryBad.js

@@ -0,0 +1,12 @@
+import http from 'http';
+import url from 'url';
+
+var server = http.createServer(function(req, res) {
+    var target = url.parse(request.url, true).query.target;
+
+    // BAD: `target` is controlled by the attacker
+    http.get('https://' + target + ".example.com/data/", res => {
+        // process request response ...
+    });
+
+});

javascript/ql/src/Security/CWE-918/examples/RequestForgeryGood.js

@@ -0,0 +1,19 @@
+import http from 'http';
+import url from 'url';
+
+var server = http.createServer(function(req, res) {
+    var target = url.parse(request.url, true).query.target;
+
+    var subdomain;
+    if (target === 'EU') {
+        subdomain = "europe"
+    } else {
+        subdomain = "world"
+    }
+
+    // GOOD: `subdomain` is controlled by the server
+    http.get('https://' + subdomain + ".example.com/data/", res => {
+        // process request response ...
+    });
+
+});

javascript/ql/src/javascript.qll

@@ -54,6 +54,7 @@ import semmle.javascript.frameworks.AWS
 import semmle.javascript.frameworks.Azure
 import semmle.javascript.frameworks.Babel
 import semmle.javascript.frameworks.ComposedFunctions
+import semmle.javascript.frameworks.ClientRequests
 import semmle.javascript.frameworks.Credentials
 import semmle.javascript.frameworks.CryptoLibraries
 import semmle.javascript.frameworks.DigitalOcean