Merge pull request #230 from esben-semmle/js/ad-hoc-whitelisting

Approved by xiemaisi

change-notes/1.19/analysis-javascript.md

@@ -4,6 +4,8 @@
 
 * Modelling of taint flow through array operations has been improved. This may give additional results for the security queries.
 
+* The taint tracking library now recognizes additional sanitization patterns. This may give fewer false-positive results for the security queries.
+
 * Support for popular libraries has been improved. Consequently, queries may produce more results on code bases that use the following features:
   - file system access, for example through [fs-extra](https://github.com/jprichardson/node-fs-extra) or [globby](https://www.npmjs.com/package/globby)
 

javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll

@@ -614,6 +614,26 @@ module TaintTracking {
 
   }
 
+  /**
+   * A check of the form `if(<isWhitelisted>(x))`, which sanitizes `x` in its "then" branch.
+   *
+   * `<isWhitelisted>` is a call with callee name 'safe', 'whitelist', 'allow', or similar.
+   *
+   * This sanitizer is not enabled by default.
+   */
+  class AdHocWhitelistCheckSanitizer extends SanitizerGuardNode, DataFlow::CallNode {
+    AdHocWhitelistCheckSanitizer() {
+      getCalleeName().regexpMatch("(?i).*((?<!un)safe|whitelist|allow|(?<!un)auth(?!or\\b)).*") and
+      getNumArgument() = 1
+    }
+
+    override predicate sanitizes(boolean outcome, Expr e) {
+      outcome = true and
+      e = getArgument(0).asExpr()
+    }
+
+  }
+
   /** A check of the form `if(x in o)`, which sanitizes `x` in its "then" branch. */
   class InSanitizer extends AdditionalSanitizerGuardNode, DataFlow::ValueNode {
 

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

@@ -49,6 +49,11 @@ module CorsMisconfigurationForCredentials {
       super.isSanitizer(node) or
       node instanceof Sanitizer
     }
+
+    override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode guard) {
+      guard instanceof TaintTracking::AdHocWhitelistCheckSanitizer
+    }
+
   }
 
   /** A source of remote user input, considered as a flow source for CORS misconfiguration. */

javascript/ql/test/library-tests/TaintBarriers/ExampleConfiguration.qll

@@ -23,4 +23,9 @@ class ExampleConfiguration extends TaintTracking::Configuration {
     )
   }
 
+  override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode guard) {
+    // add additional generic sanitizers
+    guard instanceof TaintTracking::AdHocWhitelistCheckSanitizer
+  }
+
 }

javascript/ql/test/library-tests/TaintBarriers/SanitizingGuard.expected

@@ -36,3 +36,5 @@
 | tst.js:214:9:214:24 | o.indexOf(v) < 0 | ExampleConfiguration | false | tst.js:214:19:214:19 | v |
 | tst.js:220:9:220:25 | o.indexOf(v) > -1 | ExampleConfiguration | true | tst.js:220:19:220:19 | v |
 | tst.js:226:9:226:26 | -1 >= o.indexOf(v) | ExampleConfiguration | false | tst.js:226:25:226:25 | v |
+| tst.js:236:9:236:24 | isWhitelisted(v) | ExampleConfiguration | true | tst.js:236:23:236:23 | v |
+| tst.js:240:9:240:28 | config.allowValue(v) | ExampleConfiguration | true | tst.js:240:27:240:27 | v |

javascript/ql/test/library-tests/TaintBarriers/TaintedSink.expected

@@ -34,3 +34,5 @@
 | tst.js:215:14:215:14 | v | tst.js:199:13:199:20 | SOURCE() |
 | tst.js:223:14:223:14 | v | tst.js:199:13:199:20 | SOURCE() |
 | tst.js:227:14:227:14 | v | tst.js:199:13:199:20 | SOURCE() |
+| tst.js:239:14:239:14 | v | tst.js:235:13:235:20 | SOURCE() |
+| tst.js:243:14:243:14 | v | tst.js:235:13:235:20 | SOURCE() |

javascript/ql/test/library-tests/TaintBarriers/isBarrier.expected

@@ -29,3 +29,5 @@
 | tst.js:217:14:217:14 | v | ExampleConfiguration |
 | tst.js:221:14:221:14 | v | ExampleConfiguration |
 | tst.js:229:14:229:14 | v | ExampleConfiguration |
+| tst.js:237:14:237:14 | v | ExampleConfiguration |
+| tst.js:241:14:241:14 | v | ExampleConfiguration |

javascript/ql/test/library-tests/TaintBarriers/tst.js

@@ -230,3 +230,16 @@ function RelationalIndexOfCheckSanitizer () {
     }
 
 }
+
+function adhocWhitelisting() {
+    var v = SOURCE();
+    if (isWhitelisted(v))
+        SINK(v);
+    else
+        SINK(v);
+    if (config.allowValue(v))
+        SINK(v);
+    else
+        SINK(v);
+
+}