Merge pull request #555 from xiemaisi/js/invalid-dynamic-method-call

Approved by esben-semmle

Author: semmle-qlci

change-notes/1.19/analysis-javascript.md

@@ -24,11 +24,12 @@
 | Enabling Node.js integration for Electron web content renderers (`js/enabling-electron-renderer-node-integration`) | security, frameworks/electron, external/cwe/cwe-094  | Highlights Electron web content renderer preferences with Node.js integration enabled, indicating a violation of [CWE-94](https://cwe.mitre.org/data/definitions/94.html). Results are not shown on LGTM by default. |
 | File data in outbound network request | security, external/cwe/cwe-200 | Highlights locations where file data is sent in a network request. Results are not shown on LGTM by default. |
 | Host header poisoning in email generation |  security, external/cwe/cwe-640 | Highlights code that generates emails with links that can be hijacked by HTTP host header poisoning, indicating a violation of [CWE-640](https://cwe.mitre.org/data/definitions/640.html). Results shown on LGTM by default.  |
-| Unsafe dynamic method access (`js/unsafe-dynamic-method-access` ) | security, external/cwe/cwe-094 | Highlights code that invokes a user-controlled method on an object with unsafe methods. Results are shown on LGTM by default. |
 | Replacement of a substring with itself (`js/identity-replacement`) | correctness, security, external/cwe/cwe-116 | Highlights string replacements that replace a string with itself, which usually indicates a mistake. Results shown on LGTM by default. |
 | Stored cross-site scripting (`js/stored-xss`) | security, external/cwe/cwe-079, external/cwe/cwe-116 | Highlights uncontrolled stored values flowing into HTML content, indicating a violation of [CWE-079](https://cwe.mitre.org/data/definitions/79.html). Results shown on LGTM by default. |
 | Unclear precedence of nested operators (`js/unclear-operator-precedence`) | maintainability, correctness, external/cwe/cwe-783 | Highlights nested binary operators whose relative precedence is easy to misunderstand. Results shown on LGTM by default. |
 | Unneeded defensive code | correctness, external/cwe/cwe-570, external/cwe/cwe-571 | Highlights locations where defensive code is not needed. Results are shown on LGTM by default. |
+| Unsafe dynamic method access (`js/unsafe-dynamic-method-access` ) | security, external/cwe/cwe-094 | Highlights code that invokes a user-controlled method on an object with unsafe methods. Results are shown on LGTM by default. |
+| Unvalidated dynamic method access (`js/unvalidated-dynamic-method-call` ) | security, external/cwe/cwe-754 | Highlights code that invokes a user-controlled method without guarding against exceptional circumstances. Results are shown on LGTM by default. |
 | Useless assignment to property | maintainability | Highlights property assignments whose value is always overwritten. Results are shown on LGTM by default. |
 | User-controlled data in file | security, external/cwe/cwe-912 | Highlights locations where user-controlled data is written to a file. Results are not shown on LGTM by default. |
 
@@ -44,11 +45,11 @@
 | Duplicate 'if' condition | Lower severity | The severity of this rule has been revised to "warning". |
 | Duplicate switch case | Lower severity | The severity of this rule has been revised to "warning". |
 | Information exposure through a stack trace | More results | This rule now also flags cases where the entire exception object (including the stack trace) may be exposed. |
-| Missing CSRF middleware | Fewer false-positive results | This rule now recognizes additional CSRF protection middlewares. |
 | Missing 'this' qualifier | Fewer false-positive results | This rule now recognizes additional intentional calls to global functions. | 
+| Missing CSRF middleware | Fewer false-positive results | This rule now recognizes additional CSRF protection middlewares. |
 | Missing variable declaration | Lower severity | The severity of this rule has been revised to "warning". |
 | Regular expression injection | Fewer false-positive results | This rule now identifies calls to `String.prototype.search` with more precision. |
-| Remote property injection | Fewer results | The precision of this rule has been revised to "medium". Results are no longer shown on LGTM by default. |
+| Remote property injection | Fewer results | The precision of this rule has been revised to "medium". Furthermore, it no longer flags dynamic method calls, which are now handled by two new queries. Results are no longer shown on LGTM by default. |
 | Self assignment | Fewer false-positive results | This rule now ignores self-assignments preceded by a JSDoc comment with a `@type` tag. |
 | Server-side URL redirect | Fewer false-positive results | This rule now recognizes safe redirects in more cases. |
 | Server-side URL redirect | More results | This rule now recognizes redirection calls in more cases. |

javascript/config/suites/javascript/security

@@ -26,6 +26,7 @@
 + semmlecode-javascript-queries/Security/CWE-640/HostHeaderPoisoningInEmailGeneration.ql: /Security/CWE/CWE-640
 + semmlecode-javascript-queries/Security/CWE-643/XpathInjection.ql: /Security/CWE/CWE-643
 + semmlecode-javascript-queries/Security/CWE-730/RegExpInjection.ql: /Security/CWE/CWE-730
++ semmlecode-javascript-queries/Security/CWE-754/UnvalidatedDynamicMethodCall.ql: /Security/CWE/CWE-754
 + semmlecode-javascript-queries/Security/CWE-770/MissingRateLimiting.ql: /Security/CWE/CWE-770
 + semmlecode-javascript-queries/Security/CWE-776/XmlBomb.ql: /Security/CWE/CWE-776
 + semmlecode-javascript-queries/Security/CWE-798/HardcodedCredentials.ql: /Security/CWE/CWE-798

javascript/ql/src/Security/CWE-400/RemotePropertyInjection.qhelp

@@ -6,89 +6,80 @@
 <overview>
 	<p>
 		Dynamically computing object property names from untrusted input
-		may have multiple undesired consequences. For example, 
-		if the property access is used as part of a write, an 
-		attacker may overwrite vital properties of objects, such as 
-		<code>__proto__</code>. This attack is known as <i>prototype 
-		pollution attack</i> and may serve as a vehicle for denial-of-service 
-		attacks. A similar attack vector, is to replace the 
-		<code>toString</code> property of an object with a primitive. 
-		Whenever <code>toString</code> is then called on that object, either 
-		explicitly or implicitly as part of a type coercion, an exception 
+		may have multiple undesired consequences. For example,
+		if the property access is used as part of a write, an
+		attacker may overwrite vital properties of objects, such as
+		<code>__proto__</code>. This attack is known as <i>prototype
+		pollution attack</i> and may serve as a vehicle for denial-of-service
+		attacks. A similar attack vector, is to replace the
+		<code>toString</code> property of an object with a primitive.
+		Whenever <code>toString</code> is then called on that object, either
+		explicitly or implicitly as part of a type coercion, an exception
 		will be raised.
 	</p>
 
 	<p>
-		Moreover, if the dynamically computed property is 
-		used as part of a method call, the attacker may trigger 
-		the execution of unwanted functions such as the 
-		<code>Function</code> constructor or the 
-		<code>eval</code> method, which can be used 
-		for code injection.  
-	</p>
-	
-	<p>
-		Additionally, if the name of an HTTP header is user-controlled,
-		an attacker may exploit this to overwrite security-critical headers 
-		such as <code>Access-Control-Allow-Origin</code> or 
+		Moreover, if the name of an HTTP header is user-controlled,
+		an attacker may exploit this to overwrite security-critical headers
+		such as <code>Access-Control-Allow-Origin</code> or
 		<code>Content-Security-Policy</code>.
 	</p>
 </overview>
 
 <recommendation>
 	<p>
 		The most common case in which prototype pollution vulnerabilities arise
-		is when JavaScript objects are used for implementing map data 
-		structures. This case should be avoided whenever possible by using the 
-		ECMAScript 2015 <code>Map</code> instead. When this is not possible, an 
-		alternative fix is to prepend untrusted input with a marker character 
-		such as <code>$</code>, before using it in properties accesses. In this way, 
-		the attacker does not	have access to built-in properties which do not 
-		start with the chosen character.   
+		is when JavaScript objects are used for implementing map data
+		structures. This case should be avoided whenever possible by using the
+		ECMAScript 2015 <code>Map</code> instead. When this is not possible, an
+		alternative fix is to prepend untrusted input with a marker character
+		such as <code>$</code>, before using it in properties accesses. In this way,
+		the attacker does not	have access to built-in properties which do not
+		start with the chosen character.
 	</p>
 	<p>
-		When using user input as part of header or method names, a sanitization
-		step should be performed on the input to ensure that the name does not 
-		clash with existing property and header names such as 
-		<code>__proto__</code> or <code>Content-Security-Policy</code>.    
+		When using user input as part of a header name, a sanitization
+		step should be performed on the input to ensure that the name does not
+		clash with existing header names such as
+		<code>Content-Security-Policy</code>.
 	</p>
 </recommendation>
 
 <example>
 	<p>
-		In the example below, the dynamically computed property 
-		<code>prop</code> is accessed on <code>myObj</code> using a 
+		In the example below, the dynamically computed property
+		<code>prop</code> is accessed on <code>myObj</code> using a
 		user-controlled value.
 	</p>
 
 	<sample src="examples/RemotePropertyInjection.js"/>
 
 	<p>
-		This is not secure since an attacker may exploit this code to 
+		This is not secure since an attacker may exploit this code to
 		overwrite the property <code>__proto__</code> with an empty function.
-		If this happens, the concatenation in the <code>console.log</code> 
-		argument will fail with a confusing message such as 
+		If this happens, the concatenation in the <code>console.log</code>
+		argument will fail with a confusing message such as
 		"Function.prototype.toString is not	generic". If the application does
 		not properly handle this error, this scenario may result in a serious
-		denial-of-service attack. The fix is to prepend the user-controlled 
-		string with a marker character such as <code>$</code> which will 
-		prevent arbitrary property names from being overwritten. 
+		denial-of-service attack. The fix is to prepend the user-controlled
+		string with a marker character such as <code>$</code> which will
+		prevent arbitrary property names from being overwritten.
 	</p>
 
 	<sample src="examples/RemotePropertyInjection_fixed.js"/>
 </example>
 
 <references>
-	<li>Prototype pollution attacks: 
+	<li>Prototype pollution attacks:
 		<a href="https://github.com/electron/electron/pull/9287">electron</a>,
 		<a href="https://hackerone.com/reports/310443">lodash</a>,
 		<a href="https://nodesecurity.io/advisories/566">hoek</a>.
 	</li>
-	<li> Penetration testing report: 
+	<li> Penetration testing report:
 		<a href="http://seclists.org/pen-test/2009/Mar/67">
 			header name injection attack</a>
 	</li>
-	<li> npm blog post: 
+	<li> npm blog post:
 		<a href="https://blog.liftsecurity.io/2015/01/14/the-dangers-of-square-bracket-notation#lift-security">
 			dangers of square bracket notation</a>
 	</li>

javascript/ql/src/Security/CWE-400/RemotePropertyInjection.ql

@@ -1,8 +1,7 @@
 /**
  * @name Remote property injection
- * @description Allowing writes to arbitrary properties or calls to arbitrary 
- *       methods of an object may lead to denial-of-service attacks. 
- *   
+ * @description Allowing writes to arbitrary properties of an object may lead to
+ *              denial-of-service attacks.
  * @kind path-problem
  * @problem.severity warning
  * @precision medium

javascript/ql/src/Security/CWE-754/UnvalidatedDynamicMethodCall.qhelp

@@ -0,0 +1,86 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+JavaScript makes it easy to look up object properties dynamically at runtime. In particular, methods
+can be looked up by name and then called. However, if he method name is user controlled, an attacker
+could choose a name that makes the application invoke an unexpected method, which may cause a runtime
+exception. If this exception is not handled, it could be used to mount a denial-of-service attack.
+</p>
+<p>
+For example, there might not be a method of the given name or the result of the lookup might not be
+a function, which would cause the method call to throw a <code>TypeError</code> at runtime.
+</p>
+<p>
+Another, more subtle example is where the result of the lookup is a standard library method from
+<code>Object.prototype</code>, which most objects have on their prototype chain. Examples of such
+methods include <code>valueOf</code>, <code>hasOwnProperty</code> and <code>__defineSetter__</code>.
+If the method call passes the wrong number or kind of arguments to these methods, they will
+throw an exception.
+</p>
+</overview>
+
+<recommendation>
+<p>
+It is best to avoid dynamic method lookup involving user-controlled names altogether, for instance
+by using a <code>Map</code> instead of a plain object.
+</p>
+<p>
+If the dynamic method lookup cannot be avoided, consider whitelisting permitted method names. At
+the very least, check that the method is an own property and not inherited from the prototype object.
+If the object on which the method is looked up contains properties that are not methods, you
+should additionally check that the result of the lookup is a function. Even if the object only
+contains methods it is still a good idea to perform this check in case other properties are
+added to the object later on.
+</p>
+</recommendation>
+
+<example>
+<p>
+In the following example, an HTTP request parameter <code>action</code> property is used to dynamically
+look up a function in the <code>actions</code> map, which is then invoked with the <code>payload</code>
+parameter as its argument.
+</p>
+
+<sample src="examples/UnvalidatedDynamicMethodCall.js" />
+
+<p>
+The intention is to allow clients to invoke the <code>play</code> or <code>pause</code> method, but there
+is no check that <code>action</code> is actually the name of a method stored in <code>actions</code>.
+If, for example, <code>action</code> is <code>rewind</code>, <code>action</code> will be <code>undefined</code>
+and the call will result in a runtime error.
+</p>
+
+<p>
+The easiest way to prevent this is to turn <code>actions</code> into a <code>Map</code> and using
+<code>Map.prototype.has</code> to check whether the method name is valid before looking it up.
+</p>
+
+<sample src="examples/UnvalidatedDynamicMethodCallGood.js" />
+
+<p>
+If <code>actions</code> cannot be turned into a <code>Map</code>, a <code>hasOwnProperty</code>
+check should be added to validate the method name:
+</p>
+
+<sample src="examples/UnvalidatedDynamicMethodCallGood2.js" />
+
+</example>
+
+<references>
+<li>
+OWASP:
+<a href="https://www.owasp.org/index.php/Denial_of_Service">Denial of Service</a>.
+</li>
+<li>
+MDN: <a href="https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map">Map</a>.
+</li>
+<li>
+MDN: <a href="https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/prototype">Object.prototype</a>.
+</li>
+</references>
+
+</qhelp>

javascript/ql/src/Security/CWE-754/UnvalidatedDynamicMethodCall.ql

@@ -0,0 +1,21 @@
+/**
+ * @name Unvalidated dynamic method call
+ * @description Calling a method with a user-controlled name may dispatch to
+ *              an unexpected target, which could cause an exception.
+ * @kind path-problem
+ * @problem.severity warning
+ * @precision high
+ * @id js/unvalidated-dynamic-method-call
+ * @tags security
+ *       external/cwe/cwe-754
+ */
+
+import javascript
+import semmle.javascript.security.dataflow.UnvalidatedDynamicMethodCall::UnvalidatedDynamicMethodCall
+import DataFlow::PathGraph
+
+from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
+where cfg.hasFlowPath(source, sink)
+select sink.getNode(), source, sink,
+       "Invocation of method with $@ name may dispatch to unexpected target and cause an exception.",
+       source.getNode(), "user-controlled"

javascript/ql/src/Security/CWE-754/examples/UnvalidatedDynamicMethodCall.js

@@ -0,0 +1,17 @@
+var express = require('express');
+var app = express();
+
+var actions = {
+  play(data) {
+    // ...
+  },
+  pause(data) {
+    // ...
+  }
+}
+
+app.get('/perform/:action/:payload', function(req, res) {
+  let action = actions[req.params.action];
+  // BAD: `action` may not be a function
+  res.end(action(req.params.payload));
+});

javascript/ql/src/Security/CWE-754/examples/UnvalidatedDynamicMethodCallGood.js

@@ -0,0 +1,20 @@
+var express = require('express');
+var app = express();
+
+var actions = new Map();
+actions.put("play", function play(data) {
+  // ...
+});
+actions.put("pause", function pause(data) {
+  // ...
+});
+
+app.get('/perform/:action/:payload', function(req, res) {
+  if (actions.has(req.params.action)) {
+    let action = actions.get(req.params.action);
+    // GOOD: `action` is either the `play` or the `pause` function from above
+    res.end(action(req.params.payload));
+  } else {
+    res.end("Unsupported action.");
+  }
+});

javascript/ql/src/Security/CWE-754/examples/UnvalidatedDynamicMethodCallGood2.js

@@ -0,0 +1,23 @@
+var express = require('express');
+var app = express();
+
+var actions = {
+  play(data) {
+    // ...
+  },
+  pause(data) {
+    // ...
+  }
+}
+
+app.get('/perform/:action/:payload', function(req, res) {
+  if (actions.hasOwnProperty(req.params.action)) {
+    let action = actions[req.params.action];
+    if (typeof action === 'function') {
+      // GOOD: `action` is an own method of `actions`
+      res.end(action(req.params.payload));
+      return;
+    }
+  }
+  res.end("Unsupported action.");
+});

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

@@ -36,4 +36,12 @@ module PropertyInjection {
     // Assume that a value that is invoked can refer to a function.
     exists (node.getAnInvocation())
   }
+
+  /**
+   * Holds if the `node` is of form `Object.create(null)` and so it has no prototype.
+   */
+  predicate isPrototypeLessObject(DataFlow::MethodCallNode node) {
+    node = DataFlow::globalVarRef("Object").getAMethodCall("create") and
+    node.getArgument(0).asExpr() instanceof NullLiteral
+  }
 }