Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JS: add sanitizer support for ~whitelist.indexOf(x) #83

Merged
merged 2 commits into from Aug 21, 2018
Merged

JS: add sanitizer support for ~whitelist.indexOf(x) #83

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
be8a32b
JS: add sanitizer support for `~whitelist.indexOf(x)`
Aug 20, 2018
2d63524
JS: explain sanitizer equivalence
Aug 21, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions change-notes/1.18/analysis-javascript.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
20 changes: 20 additions & 0 deletions javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -612,6 +612,26 @@ module TaintTracking {

}

/** A check of the form `if(~o.indexOf(x))`, which sanitizes `x` in its "then" branch. */
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be helpful to add an example here to explain why it's x that is sanitised and not o. (In fact, it seems like there are uses of this idiom that sanitise o, but they are perhaps more difficult to capture.)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps all that's needed is to rename o to whitelist. But please also explain that this is equivalent to if(whitelist.indexOf(x) !== -1).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. The o in the o.indexOf(x) !== -1 case has also been renamed to whitelist.

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 {
Expand Down
2 changes: 2 additions & 0 deletions javascript/ql/test/library-tests/TaintBarriers/SanitizingGuard.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
3 changes: 3 additions & 0 deletions javascript/ql/test/library-tests/TaintBarriers/TaintedSink.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() |
2 changes: 2 additions & 0 deletions javascript/ql/test/library-tests/TaintBarriers/isBarrier.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
18 changes: 18 additions & 0 deletions javascript/ql/test/library-tests/TaintBarriers/tst.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

}