Merge pull request #2653 from erik-krogh/exceptionFPs

Approved by esbena

Author: semmle-qlci

javascript/ql/src/semmle/javascript/frameworks/Logging.qll

@@ -17,7 +17,7 @@ abstract class LoggerCall extends DataFlow::CallNode {
 /**
  * Gets a log level name that is used in RFC5424, `npm`, `console`.
  */
-private string getAStandardLoggerMethodName() {
+string getAStandardLoggerMethodName() {
   result = "crit" or
   result = "debug" or
   result = "error" or

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

@@ -21,15 +21,6 @@ module DomBasedXss {
     override predicate isSanitizer(DataFlow::Node node) {
       super.isSanitizer(node)
       or
-      exists(PropAccess pacc | pacc = node.asExpr() |
-        isSafeLocationProperty(pacc)
-        or
-        // `$(location.hash)` is a fairly common and safe idiom
-        // (because `location.hash` always starts with `#`),
-        // so we mark `hash` as safe for the purposes of this query
-        pacc.getPropertyName() = "hash"
-      )
-      or
       node instanceof Sanitizer
     }
   }

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

@@ -12,15 +12,33 @@ module ExceptionXss {
   import Xss as Xss
   private import semmle.javascript.dataflow.InferredTypes
 
+  /**
+   * Gets the name of a method that does not leak taint from its arguments if an exception is thrown by the method.
+   */
+  private string getAnUnlikelyToThrowMethodName() {
+    result = "getElementById" or // document.getElementById
+    result = "indexOf" or // String.prototype.indexOf
+    result = "assign" or // Object.assign
+    result = "pick" or // _.pick
+    result = getAStandardLoggerMethodName() or // log.info etc.
+    result = "val" or // $.val
+    result = "parse" or // JSON.parse
+    result = "stringify" or // JSON.stringify
+    result = "test" or // RegExp.prototype.test
+    result = "setItem" or // localStorage.setItem
+    result = "existsSync" or
+    // the "fs" methods are a mix of "this is safe" and "you have bigger problems".
+    exists(ExternalMemberDecl decl | decl.hasQualifiedName("fs", result)) or
+    // Array methods are generally exception safe.
+    exists(ExternalMemberDecl decl | decl.hasQualifiedName("Array", result))
+  }
+
   /**
    * Holds if `node` is unlikely to cause an exception containing sensitive information to be thrown.
    */
   private predicate isUnlikelyToThrowSensitiveInformation(DataFlow::Node node) {
-    node = any(DataFlow::CallNode call | call.getCalleeName() = "getElementById").getAnArgument()
-    or
-    node = any(DataFlow::CallNode call | call.getCalleeName() = "indexOf").getAnArgument()
-    or
-    node = any(DataFlow::CallNode call | call.getCalleeName() = "stringify").getAnArgument()
+    node = any(DataFlow::CallNode call | call.getCalleeName() = getAnUnlikelyToThrowMethodName())
+          .getAnArgument()
     or
     node = DataFlow::globalVarRef("console").getAMemberCall(_).getAnArgument()
   }
@@ -38,6 +56,7 @@ module ExceptionXss {
    */
   predicate canThrowSensitiveInformation(DataFlow::Node node) {
     not isUnlikelyToThrowSensitiveInformation(node) and
+    not node instanceof Xss::Shared::Sink and // removes duplicates from js/xss.
     (
       // in the case of reflective calls the below ensures that both InvokeNodes have no known callee.
       forex(DataFlow::InvokeNode call | call.getAnArgument() = node | not exists(call.getACallee()))
@@ -79,15 +98,15 @@ module ExceptionXss {
     }
 
     /**
-     * Get the parameter in the callback that contains an error. 
+     * Gets the parameter in the callback that contains an error.
      * In the current implementation this is always the first parameter.
      */
     DataFlow::Node getErrorParam() { result = errorParameter }
   }
 
   /**
-   * Gets the error parameter for a callback that is supplied to the same call as `pred` is an argument to. 
-   * For example: `outerCall(foo, <pred>, bar, (<result>, val) => { ... })`. 
+   * Gets the error parameter for a callback that is supplied to the same call as `pred` is an argument to.
+   * For example: `outerCall(foo, <pred>, bar, (<result>, val) => { ... })`.
    */
   DataFlow::Node getCallbackErrorParam(DataFlow::Node pred) {
     exists(DataFlow::CallNode call, Callback callback |
@@ -101,8 +120,8 @@ module ExceptionXss {
   /**
    * Gets the data-flow node to which any exceptions thrown by
    * this expression will propagate.
-   * This predicate adds, on top of `Expr::getExceptionTarget`, exceptions 
-   * propagated by callbacks. 
+   * This predicate adds, on top of `Expr::getExceptionTarget`, exceptions
+   * propagated by callbacks.
    */
   private DataFlow::Node getExceptionTarget(DataFlow::Node pred) {
     result = pred.asExpr().getExceptionTarget()

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

@@ -259,6 +259,24 @@ module DomBasedXss {
     }
   }
 
+  /**
+   * A property read from a safe property is considered a sanitizer.
+   */
+  class SafePropertyReadSanitizer extends Sanitizer, DataFlow::Node {
+    SafePropertyReadSanitizer() {
+      exists(PropAccess pacc | pacc = this.asExpr() |
+        isSafeLocationProperty(pacc)
+        or
+        // `$(location.hash)` is a fairly common and safe idiom
+        // (because `location.hash` always starts with `#`),
+        // so we mark `hash` as safe for the purposes of this query
+        pacc.getPropertyName() = "hash"
+        or
+        pacc.getPropertyName() = "length"
+      )
+    }
+  }
+
   /**
    * A regexp replacement involving an HTML meta-character, viewed as a sanitizer for
    * XSS vulnerabilities.

javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss.expected

@@ -9,6 +9,9 @@ nodes
 | etherpad.js:9:16:9:53 | req.que ... e + ")" |
 | etherpad.js:11:12:11:19 | response |
 | etherpad.js:11:12:11:19 | response |
+| exception-xss.js:190:12:190:24 | req.params.id |
+| exception-xss.js:190:12:190:24 | req.params.id |
+| exception-xss.js:190:12:190:24 | req.params.id |
 | formatting.js:4:9:4:29 | evil |
 | formatting.js:4:16:4:29 | req.query.evil |
 | formatting.js:4:16:4:29 | req.query.evil |
@@ -77,6 +80,7 @@ edges
 | etherpad.js:9:16:9:30 | req.query.jsonp | etherpad.js:9:16:9:53 | req.que ... e + ")" |
 | etherpad.js:9:16:9:30 | req.query.jsonp | etherpad.js:9:16:9:53 | req.que ... e + ")" |
 | etherpad.js:9:16:9:53 | req.que ... e + ")" | etherpad.js:9:5:9:53 | response |
+| exception-xss.js:190:12:190:24 | req.params.id | exception-xss.js:190:12:190:24 | req.params.id |
 | formatting.js:4:9:4:29 | evil | formatting.js:6:43:6:46 | evil |
 | formatting.js:4:9:4:29 | evil | formatting.js:7:49:7:52 | evil |
 | formatting.js:4:16:4:29 | req.query.evil | formatting.js:4:9:4:29 | evil |
@@ -131,6 +135,7 @@ edges
 #select
 | ReflectedXss.js:8:14:8:45 | "Unknow ... rams.id | ReflectedXss.js:8:33:8:45 | req.params.id | ReflectedXss.js:8:14:8:45 | "Unknow ... rams.id | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:8:33:8:45 | req.params.id | user-provided value |
 | etherpad.js:11:12:11:19 | response | etherpad.js:9:16:9:30 | req.query.jsonp | etherpad.js:11:12:11:19 | response | Cross-site scripting vulnerability due to $@. | etherpad.js:9:16:9:30 | req.query.jsonp | user-provided value |
+| exception-xss.js:190:12:190:24 | req.params.id | exception-xss.js:190:12:190:24 | req.params.id | exception-xss.js:190:12:190:24 | req.params.id | Cross-site scripting vulnerability due to $@. | exception-xss.js:190:12:190:24 | req.params.id | user-provided value |
 | formatting.js:6:14:6:47 | util.fo ... , evil) | formatting.js:4:16:4:29 | req.query.evil | formatting.js:6:14:6:47 | util.fo ... , evil) | Cross-site scripting vulnerability due to $@. | formatting.js:4:16:4:29 | req.query.evil | user-provided value |
 | formatting.js:7:14:7:53 | require ... , evil) | formatting.js:4:16:4:29 | req.query.evil | formatting.js:7:14:7:53 | require ... , evil) | Cross-site scripting vulnerability due to $@. | formatting.js:4:16:4:29 | req.query.evil | user-provided value |
 | partial.js:10:14:10:18 | x + y | partial.js:13:42:13:48 | req.url | partial.js:10:14:10:18 | x + y | Cross-site scripting vulnerability due to $@. | partial.js:13:42:13:48 | req.url | user-provided value |

javascript/ql/test/query-tests/Security/CWE-079/ReflectedXssWithCustomSanitizer.expected

@@ -1,4 +1,5 @@
 | ReflectedXss.js:8:14:8:45 | "Unknow ... rams.id | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:8:33:8:45 | req.params.id | user-provided value |
+| exception-xss.js:190:12:190:24 | req.params.id | Cross-site scripting vulnerability due to $@. | exception-xss.js:190:12:190:24 | req.params.id | user-provided value |
 | formatting.js:6:14:6:47 | util.fo ... , evil) | Cross-site scripting vulnerability due to $@. | formatting.js:4:16:4:29 | req.query.evil | user-provided value |
 | formatting.js:7:14:7:53 | require ... , evil) | Cross-site scripting vulnerability due to $@. | formatting.js:4:16:4:29 | req.query.evil | user-provided value |
 | partial.js:10:14:10:18 | x + y | Cross-site scripting vulnerability due to $@. | partial.js:13:42:13:48 | req.url | user-provided value |

javascript/ql/test/query-tests/Security/CWE-079/exception-xss.js

@@ -183,4 +183,34 @@ app.get('/user/:id', function (req, res) {
 		}
 		$('myId').html(res); // NOT OK!
 	});
-});
+});
+
+app.get('/user/:id', function (req, res) {
+	try {
+		res.send(req.params.id);
+	} catch(err) {
+		res.send(err); // OK (the above `res.send()` is already reported by js/xss)
+	}
+});
+
+var fs = require("fs");
+
+(function () {
+	var foo = document.location.search;
+
+	try {
+		// A series of functions does not throw tainted exceptions.
+		Object.assign(foo, foo)
+		_.pick(foo, foo);
+		[foo, foo].join(join);
+		$.val(foo);
+		JSON.parse(foo); 
+		/bla/.test(foo);
+		console.log(foo);
+		log.info(foo);
+		localStorage.setItem(foo);
+	} catch (e) {
+		$('myId').html(e); // OK
+	}
+	
+})();

javascript/ql/test/query-tests/Security/CWE-079/tst.js

@@ -318,3 +318,10 @@ function basicExceptions() {
 function handlebarsSafeString() {
 	return new Handlebars.SafeString(location); // NOT OK!	
 }
+
+function test2() {
+  var target = document.location.search
+
+  // OK
+  $('myId').html(target.length)
+}