fix a number of FPs in js/exception-xss

Author: erik-krogh

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,39 @@ 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
+    result = "indexOf" or
+    result = "stringify" or
+    result = "assign" or
+    // fs methods. (The callback argument to the async functions are vulnerable, but its unlikely that the callback is the user-controlled part).
+    result = "existsSync" or
+    result = "exists" or
+    result = "writeFileSync" or
+    result = "writeFile" or
+    result = "appendFile" or
+    result = "appendFileSync" or
+    result = "pick" or
+    // log.info etc.
+    result = "info" or
+    result = "warn" or
+    result = "error" or
+    result = "join" or
+    result = "val" or // $.val
+    result = "parse" or // JSON.parse
+    result = "push" or // Array.prototype.push
+    result = "test" // RegExp.prototype.test
+  }
+
   /**
    * 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 +62,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 +104,15 @@ module ExceptionXss {
     }
 
     /**
-     * Get the parameter in the callback that contains an error. 
+     * Get 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 +126,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

@@ -51,6 +51,24 @@ module Shared {
       )
     }
   }
+  
+  /**
+   * 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"
+      )
+    }
+  }
 }
 
 /** Provides classes and predicates for the DOM-based XSS query. */
@@ -269,6 +287,8 @@ module DomBasedXss {
   private class MetacharEscapeSanitizer extends Sanitizer, Shared::MetacharEscapeSanitizer { }
 
   private class UriEncodingSanitizer extends Sanitizer, Shared::UriEncodingSanitizer { }
+  
+  private class SafePropertyReadSanitizer extends Sanitizer, Shared::SafePropertyReadSanitizer {}
 }
 
 /** Provides classes and predicates for the reflected XSS query. */

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)
+}