From 0bb6692c198e2be638c04cd22027989ccfba2b8e Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 10 Jan 2019 12:50:18 +0000 Subject: [PATCH 1/3] JS: add 'this' as possible access path root --- .../dataflow/internal/AccessPaths.qll | 15 ++++++++++--- .../TaintTracking/BasicTaintTracking.expected | 2 ++ .../TaintTracking/BasicTaintTracking.ql | 12 +++++++++++ .../TaintTracking/sanitizer-guards.js | 21 +++++++++++++++++++ 4 files changed, 47 insertions(+), 3 deletions(-) create mode 100644 javascript/ql/test/library-tests/TaintTracking/sanitizer-guards.js diff --git a/javascript/ql/src/semmle/javascript/dataflow/internal/AccessPaths.qll b/javascript/ql/src/semmle/javascript/dataflow/internal/AccessPaths.qll index 9e24c73a8d0..49279a60f77 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/internal/AccessPaths.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/internal/AccessPaths.qll @@ -44,7 +44,8 @@ private PropertyName getPropertyName(PropAccess pacc) { * where each property name is either constant or itself an SSA variable. */ private newtype TAccessPath = - MkRoot(SsaVariable var) or + MkSsaRoot(SsaVariable var) or + MkThisRoot(Function function) { function.getThisBinder() = function } or MkAccessStep(AccessPath base, PropertyName name) { exists(PropAccess pacc | pacc.getBase() = base.getAnInstance() and @@ -62,10 +63,16 @@ class AccessPath extends TAccessPath { */ Expr getAnInstanceIn(BasicBlock bb) { exists(SsaVariable var | - this = MkRoot(var) and + this = MkSsaRoot(var) and result = var.getAUseIn(bb) ) or + exists(ThisExpr this_ | + this = MkThisRoot(this_.getBinder()) and + result = this_ and + this_.getBasicBlock() = bb + ) + or exists(PropertyName name | result = getABaseInstanceIn(bb, name) and getPropertyName(result) = name @@ -96,7 +103,9 @@ class AccessPath extends TAccessPath { * Gets a textual representation of this access path. */ string toString() { - exists(SsaVariable var | this = MkRoot(var) | result = var.getSourceVariable().getName()) + exists(SsaVariable var | this = MkSsaRoot(var) | result = var.getSourceVariable().getName()) + or + this = MkThisRoot(_) and result = "this" or exists(AccessPath base, PropertyName name, string rest | rest = "." + any(string s | name = StaticPropertyName(s)) diff --git a/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected b/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected index dca429252ad..a59ec7948c6 100644 --- a/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected +++ b/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected @@ -12,6 +12,8 @@ | partialCalls.js:4:17:4:24 | source() | partialCalls.js:51:14:51:14 | x | | promise.js:4:24:4:31 | source() | promise.js:4:8:4:32 | Promise ... urce()) | | promise.js:5:25:5:32 | source() | promise.js:5:8:5:33 | bluebir ... urce()) | +| sanitizer-guards.js:2:11:2:18 | source() | sanitizer-guards.js:4:8:4:8 | x | +| sanitizer-guards.js:13:14:13:21 | source() | sanitizer-guards.js:15:10:15:15 | this.x | | thisAssignments.js:4:17:4:24 | source() | thisAssignments.js:5:10:5:18 | obj.field | | thisAssignments.js:7:19:7:26 | source() | thisAssignments.js:8:10:8:20 | this.field2 | | tst.js:2:13:2:20 | source() | tst.js:4:10:4:10 | x | diff --git a/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.ql b/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.ql index 7d591e1b48b..9251129ee54 100644 --- a/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.ql +++ b/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.ql @@ -8,6 +8,18 @@ class BasicConfig extends TaintTracking::Configuration { override predicate isSource(DataFlow::Node node) { node = getACall("source") } override predicate isSink(DataFlow::Node node) { node = getACall("sink").getAnArgument() } + + override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode node) { + node instanceof BasicSanitizerGuard + } +} + +class BasicSanitizerGuard extends TaintTracking::SanitizerGuardNode, DataFlow::CallNode { + BasicSanitizerGuard() { this = getACall("isSafe") } + + override predicate sanitizes(boolean outcome, Expr e) { + outcome = true and e = getArgument(0).asExpr() + } } from BasicConfig cfg, DataFlow::Node src, DataFlow::Node sink diff --git a/javascript/ql/test/library-tests/TaintTracking/sanitizer-guards.js b/javascript/ql/test/library-tests/TaintTracking/sanitizer-guards.js new file mode 100644 index 00000000000..688c70bbff8 --- /dev/null +++ b/javascript/ql/test/library-tests/TaintTracking/sanitizer-guards.js @@ -0,0 +1,21 @@ +function test() { + let x = source(); + + sink(x); // NOT OK + + if (isSafe(x)) { + sink(x); // OK + } +} + +class C { + method() { + this.x = source(); + + sink(this.x); // NOT OK + + if (isSafe(this.x)) { + sink(this.x); // OK + } + } +} From 78bd76048ada41e3452dcd891aae652d79ad0099 Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 10 Jan 2019 12:53:53 +0000 Subject: [PATCH 2/3] JS: add test with closures --- .../TaintTracking/BasicTaintTracking.expected | 2 ++ .../test/library-tests/TaintTracking/sanitizer-guards.js | 8 ++++++++ 2 files changed, 10 insertions(+) diff --git a/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected b/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected index a59ec7948c6..50a291c160d 100644 --- a/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected +++ b/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected @@ -14,6 +14,8 @@ | promise.js:5:25:5:32 | source() | promise.js:5:8:5:33 | bluebir ... urce()) | | sanitizer-guards.js:2:11:2:18 | source() | sanitizer-guards.js:4:8:4:8 | x | | sanitizer-guards.js:13:14:13:21 | source() | sanitizer-guards.js:15:10:15:15 | this.x | +| sanitizer-guards.js:13:14:13:21 | source() | sanitizer-guards.js:21:14:21:19 | this.x | +| sanitizer-guards.js:13:14:13:21 | source() | sanitizer-guards.js:26:9:26:14 | this.x | | thisAssignments.js:4:17:4:24 | source() | thisAssignments.js:5:10:5:18 | obj.field | | thisAssignments.js:7:19:7:26 | source() | thisAssignments.js:8:10:8:20 | this.field2 | | tst.js:2:13:2:20 | source() | tst.js:4:10:4:10 | x | diff --git a/javascript/ql/test/library-tests/TaintTracking/sanitizer-guards.js b/javascript/ql/test/library-tests/TaintTracking/sanitizer-guards.js index 688c70bbff8..e112603a74f 100644 --- a/javascript/ql/test/library-tests/TaintTracking/sanitizer-guards.js +++ b/javascript/ql/test/library-tests/TaintTracking/sanitizer-guards.js @@ -16,6 +16,14 @@ class C { if (isSafe(this.x)) { sink(this.x); // OK + + addEventListener('hey', () => { + sink(this.x); // OK - but still flagged + }); } + + addEventListener('hey', () => { + sink(this.x); // NOT OK + }); } } From 107ec3b68735d59e93f5ebac86223c998107dc06 Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 10 Jan 2019 13:24:55 +0000 Subject: [PATCH 3/3] JS: add test with self=this variable --- .../library-tests/TaintTracking/sanitizer-guards.js | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/javascript/ql/test/library-tests/TaintTracking/sanitizer-guards.js b/javascript/ql/test/library-tests/TaintTracking/sanitizer-guards.js index e112603a74f..494e828460d 100644 --- a/javascript/ql/test/library-tests/TaintTracking/sanitizer-guards.js +++ b/javascript/ql/test/library-tests/TaintTracking/sanitizer-guards.js @@ -25,5 +25,16 @@ class C { addEventListener('hey', () => { sink(this.x); // NOT OK }); + + let self = this; + if (isSafe(self.x)) { + sink(self.x); // OK + } + + addEventListener('hey', function() { + if (isSafe(self.x)) { + sink(self.x); // OK + } + }); } }