Merge pull request #83 from esben-semmle/js/bitwise-indexof-sanitizer

Approved by xiemaisi
github · Aug 21, 2018 · 6969466 · 6969466
2 parents ea9bff0 + 2d63524
commit 6969466
Show file tree

Hide file tree

Showing 6 changed files with 53 additions and 2 deletions.
diff --git a/change-notes/1.18/analysis-javascript.md b/change-notes/1.18/analysis-javascript.md
@@ -12,6 +12,8 @@
 
 * Modelling of taint flow through the array operations `map` and `join` 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 libraries:
   - [bluebird](http://bluebirdjs.com)
   - [browserid-crypto](https://github.com/mozilla/browserid-crypto)

diff --git a/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll b/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll
@@ -587,14 +587,14 @@ module TaintTracking {
 
   }
 
-  /** A check of the form `if(o.indexOf(x) != -1)`, which sanitizes `x` in its "then" branch. */
+  /** A check of the form `if(whitelist.indexOf(x) != -1)`, which sanitizes `x` in its "then" branch. */
   class IndexOfSanitizer extends AdditionalSanitizerGuardNode, DataFlow::ValueNode {
     MethodCallExpr indexOf;
     override EqualityTest astNode;
 
     IndexOfSanitizer() {
       exists (Expr index | astNode.hasOperands(indexOf, index) |
-        // one operand is of the form `o.indexOf(x)`
+        // one operand is of the form `whitelist.indexOf(x)`
         indexOf.getMethodName() = "indexOf" and
         // and the other one is -1
         index.getIntValue() = -1
@@ -612,6 +612,30 @@ module TaintTracking {
 
   }
 
+  /** 
+   * 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 {
+    MethodCallExpr indexOf;
+    override BitNotExpr astNode;
+
+    BitwiseIndexOfSanitizer() {
+      astNode.getOperand() = indexOf and
+      indexOf.getMethodName() = "indexOf"
+    }
+
+    override predicate sanitizes(boolean outcome, Expr e) {
+      outcome = true and
+      e = indexOf.getArgument(0)
+    }
+
+    override predicate appliesTo(Configuration cfg) {
+      any()
+    }
+
+  }
 
   /** A check of the form `if(x == 'some-constant')`, which sanitizes `x` in its "then" branch. */
   class ConstantComparison extends AdditionalSanitizerGuardNode, DataFlow::ValueNode {

diff --git a/javascript/ql/test/library-tests/TaintBarriers/SanitizingGuard.expected b/javascript/ql/test/library-tests/TaintBarriers/SanitizingGuard.expected
@@ -29,3 +29,5 @@
 | tst.js:160:9:160:30 | v === " ... sted-1" | ExampleConfiguration | true | tst.js:160:9:160:9 | v |
 | tst.js:160:35:160:56 | v === " ... sted-2" | ExampleConfiguration | true | tst.js:160:35:160:35 | v |
 | 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 |
diff --git a/javascript/ql/test/library-tests/TaintBarriers/TaintedSink.expected b/javascript/ql/test/library-tests/TaintBarriers/TaintedSink.expected
@@ -25,3 +25,6 @@
 | tst.js:155:14:155:14 | v | tst.js:145:13:145:20 | SOURCE() |
 | tst.js:163:14:163:14 | v | tst.js:145:13:145:20 | SOURCE() |
 | tst.js:169:14:169:14 | v | tst.js:145:13:145:20 | SOURCE() |
+| 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() |
diff --git a/javascript/ql/test/library-tests/TaintBarriers/isBarrier.expected b/javascript/ql/test/library-tests/TaintBarriers/isBarrier.expected
@@ -22,3 +22,5 @@
 | tst.js:160:35:160:56 | v | ExampleConfiguration |
 | tst.js:167:14:167:14 | v | ExampleConfiguration |
 | tst.js:176:18:176:18 | v | ExampleConfiguration |
+| tst.js:185:14:185:14 | v | ExampleConfiguration |
+| tst.js:193:14:193:14 | v | ExampleConfiguration |
diff --git a/javascript/ql/test/library-tests/TaintBarriers/tst.js b/javascript/ql/test/library-tests/TaintBarriers/tst.js
@@ -176,3 +176,21 @@ function customSanitizer() {
     v = SANITIZE(v);
     SINK(v);
 }
+
+function BitwiseIndexOfCheckSanitizer () {
+    var v = SOURCE();
+    SINK(v);
+
+    if (~o.indexOf(v)) {
+        SINK(v);
+    } else {
+        SINK(v);
+    }
+
+    if (!~o.indexOf(v)) {
+        SINK(v);
+    } else {
+        SINK(v);
+    }
+
+}