From be8a32bb18349d65ea462ffb9ca3592c62a98aca Mon Sep 17 00:00:00 2001 From: Esben Sparre Andreasen Date: Mon, 20 Aug 2018 13:28:19 +0200 Subject: [PATCH 1/2] JS: add sanitizer support for `~whitelist.indexOf(x)` --- change-notes/1.18/analysis-javascript.md | 2 ++ .../javascript/dataflow/TaintTracking.qll | 20 +++++++++++++++++++ .../TaintBarriers/SanitizingGuard.expected | 2 ++ .../TaintBarriers/TaintedSink.expected | 3 +++ .../TaintBarriers/isBarrier.expected | 2 ++ .../test/library-tests/TaintBarriers/tst.js | 18 +++++++++++++++++ 6 files changed, 47 insertions(+) diff --git a/change-notes/1.18/analysis-javascript.md b/change-notes/1.18/analysis-javascript.md index 8341116f24ac..ff694e503590 100644 --- a/change-notes/1.18/analysis-javascript.md +++ b/change-notes/1.18/analysis-javascript.md @@ -10,6 +10,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 index 99cf41b31376..d6f61c64714b 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll @@ -612,6 +612,26 @@ module TaintTracking { } + /** A check of the form `if(~o.indexOf(x))`, which sanitizes `x` in its "then" branch. */ + 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 index 2b045a189b68..6ccd938c13c7 100644 --- 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 index 9f6c6a24b025..f13dba89283d 100644 --- 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 index 9ccb4123ec66..901d75d70b81 100644 --- 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 index eabdae5241b4..16f81aad3f04 100644 --- 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); + } + +} From 2d63524f83a4c7cc5760c14ea48983024920ec72 Mon Sep 17 00:00:00 2001 From: Esben Sparre Andreasen Date: Tue, 21 Aug 2018 09:38:05 +0200 Subject: [PATCH 2/2] JS: explain sanitizer equivalence --- .../src/semmle/javascript/dataflow/TaintTracking.qll | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll b/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll index d6f61c64714b..b656aaeb2681 100644 --- 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,7 +612,11 @@ module TaintTracking { } - /** A check of the form `if(~o.indexOf(x))`, which sanitizes `x` in its "then" branch. */ + /** + * 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;