Merge pull request #91 from esben-semmle/js/additional-indexof-sanitizers

Approved by xiemaisi

Author: semmle-qlci

javascript/ql/src/semmle/javascript/Expr.qll

@@ -1545,6 +1545,14 @@ class RelationalComparison extends Comparison {
   Expr getGreaterOperand() {
     result = getAnOperand() and result != getLesserOperand()
   }
+
+  /**
+   * Holds if this is a comparison with `<=` or `>=`.
+   */
+  predicate isInclusive() {
+    this instanceof LEExpr or
+    this instanceof GEExpr
+  }
 }
 
 /** A (pre or post) increment expression. */

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

@@ -643,12 +643,56 @@ module TaintTracking {
 
   }
 
+  /** 
+   * A check of the form `if(whitelist.indexOf(x) >= 0)`, which sanitizes `x` in its "then" branch. 
+   *
+   * Similar relational checks are also supported.
+   */
+  private class RelationalIndexOfSanitizer extends AdditionalSanitizerGuardNode, DataFlow::ValueNode {
+    MethodCallExpr indexOf;
+    override RelationalComparison astNode;
+    boolean polarity;
+
+    RelationalIndexOfSanitizer() {
+      exists (Expr lesser, Expr greater |
+        astNode.getLesserOperand() = lesser and
+        astNode.getGreaterOperand() = greater and
+        indexOf.getMethodName() = "indexOf" |
+        polarity = true and
+        greater = indexOf and
+        (
+          lesser.getIntValue() >= 0
+          or
+          lesser.getIntValue() = -1 and not astNode.isInclusive()
+        )
+        or 
+        polarity = false and
+        lesser = indexOf and
+        (
+          greater.getIntValue() = -1
+          or
+          greater.getIntValue() = 0 and not astNode.isInclusive()
+        )
+      )
+    }
+
+    override predicate sanitizes(boolean outcome, Expr e) {
+      outcome = polarity and
+      e = indexOf.getArgument(0)
+    }
+
+    override predicate appliesTo(Configuration cfg) {
+      any()
+    }
+
+  }
+
   /** 
    * A check of the form `if(~whitelist.indexOf(x))`, which sanitizes `x` in its "then" branch.
    * 
    * This sanitizer is equivalent to `if(whitelist.indexOf(x) != -1)`, since `~n = 0` iff `n = -1`.
    */
-  class BitwiseIndexOfSanitizer extends AdditionalSanitizerGuardNode, DataFlow::ValueNode {
+  private class BitwiseIndexOfSanitizer extends AdditionalSanitizerGuardNode, DataFlow::ValueNode {
     MethodCallExpr indexOf;
     override BitNotExpr astNode;
 

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

@@ -31,3 +31,8 @@
 | tst.js:166:9:166:16 | v == !!0 | ExampleConfiguration | true | tst.js:166:9:166:9 | v |
 | tst.js:184:9:184:21 | ~o.indexOf(v) | ExampleConfiguration | true | tst.js:184:20:184:20 | v |
 | tst.js:190:10:190:22 | ~o.indexOf(v) | ExampleConfiguration | true | tst.js:190:21:190:21 | v |
+| tst.js:202:9:202:26 | o.indexOf(v) <= -1 | ExampleConfiguration | false | tst.js:202:19:202:19 | v |
+| tst.js:208:9:208:25 | o.indexOf(v) >= 0 | ExampleConfiguration | true | tst.js:208:19:208:19 | v |
+| 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 |

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

@@ -28,3 +28,9 @@
 | tst.js:182:10:182:10 | v | tst.js:181:13:181:20 | SOURCE() |
 | tst.js:187:14:187:14 | v | tst.js:181:13:181:20 | SOURCE() |
 | tst.js:191:14:191:14 | v | tst.js:181:13:181:20 | SOURCE() |
+| tst.js:200:10:200:10 | v | tst.js:199:13:199:20 | SOURCE() |
+| tst.js:203:14:203:14 | v | tst.js:199:13:199:20 | SOURCE() |
+| tst.js:211:14:211:14 | v | tst.js:199:13:199:20 | SOURCE() |
+| 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() |

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

@@ -24,3 +24,8 @@
 | tst.js:176:18:176:18 | v | ExampleConfiguration |
 | tst.js:185:14:185:14 | v | ExampleConfiguration |
 | tst.js:193:14:193:14 | v | ExampleConfiguration |
+| tst.js:205:14:205:14 | v | ExampleConfiguration |
+| tst.js:209:14:209:14 | v | ExampleConfiguration |
+| tst.js:217:14:217:14 | v | ExampleConfiguration |
+| tst.js:221:14:221:14 | v | ExampleConfiguration |
+| tst.js:229:14:229:14 | v | ExampleConfiguration |

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

@@ -194,3 +194,39 @@ function BitwiseIndexOfCheckSanitizer () {
     }
 
 }
+
+function RelationalIndexOfCheckSanitizer () {
+    var v = SOURCE();
+    SINK(v);
+
+    if (o.indexOf(v) <= -1) {
+        SINK(v);
+    } else {
+        SINK(v);
+    }
+
+    if (o.indexOf(v) >= 0) {
+        SINK(v);
+    } else {
+        SINK(v);
+    }
+
+    if (o.indexOf(v) < 0) {
+        SINK(v);
+    } else {
+        SINK(v);
+    }
+
+    if (o.indexOf(v) > -1) {
+        SINK(v);
+    } else {
+        SINK(v);
+    }
+
+    if (-1 >= o.indexOf(v)) {
+        SINK(v);
+    } else {
+        SINK(v);
+    }
+
+}

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

@@ -51,3 +51,19 @@ probalyAServer.on('request', (req, res) => {
     res.setHeader("Access-Control-Allow-Origin", null); // NOT OK (but not detected)
     res.setHeader("Access-Control-Allow-Credentials", true);
 });
+
+server.on('request', (req, res) => {
+    let origin = url.parse(req.url, true).query.origin;
+    if (ALLOWED_ORIGINS.indexOf(origin) !== -1) {
+        res.setHeader("Access-Control-Allow-Origin", origin); // OK, sanitized origin
+        res.setHeader("Access-Control-Allow-Credentials", true);
+    }
+});
+
+server.on('request', (req, res) => {
+    let origin = url.parse(req.url, true).query.origin;
+    if (ALLOWED_ORIGINS.indexOf(origin) >= 0) {
+        res.setHeader("Access-Control-Allow-Origin", origin); // OK, sanitized origin
+        res.setHeader("Access-Control-Allow-Credentials", true);
+    }
+});