From e2b0ec5696111ab5183d291de61dbd5b81b5ce3e Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 31 Oct 2019 16:17:17 +0000 Subject: [PATCH 1/4] JS: Handle multiple and/or operators in SanitizerFunction --- .../javascript/dataflow/TaintTracking.qll | 24 +++++++++++-- .../TaintTracking/BasicTaintTracking.expected | 2 ++ .../TaintTracking/DataFlowTracking.expected | 5 +++ .../TaintTracking/sanitizer-function.js | 35 +++++++++++++++++++ 4 files changed, 64 insertions(+), 2 deletions(-) create mode 100644 javascript/ql/test/library-tests/TaintTracking/sanitizer-function.js diff --git a/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll b/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll index 06a0d9042613..41afdb653f75 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll @@ -846,6 +846,26 @@ module TaintTracking { override predicate appliesTo(Configuration cfg) { any() } } + /** + * Gets an operand of the given `&&` operator. + * + * We use this to construct the transitive closure over a relation + * that does not include all of `BinaryExpr.getAnOperand`. + */ + private Expr getALogicalAndOperand(LogAndExpr e) { + result = e.getAnOperand() + } + + /** + * Gets an operand of the given `||` operator. + * + * We use this to construct the transitive closure over a relation + * that does not include all of `BinaryExpr.getAnOperand`. + */ + private Expr getALogicalOrOperand(LogOrExpr e) { + result = e.getAnOperand() + } + /** * A function that returns the result of a sanitizer check. */ @@ -860,10 +880,10 @@ module TaintTracking { returnExpr = sanitizer.asExpr() or // ad hoc support for conjunctions: - returnExpr.(LogAndExpr).getAnOperand() = sanitizer.asExpr() and sanitizerOutcome = true + getALogicalAndOperand+(returnExpr) = sanitizer.asExpr() and sanitizerOutcome = true or // ad hoc support for disjunctions: - returnExpr.(LogOrExpr).getAnOperand() = sanitizer.asExpr() and sanitizerOutcome = false + getALogicalOrOperand+(returnExpr) = sanitizer.asExpr() and sanitizerOutcome = false | exists(SsaExplicitDefinition ssa | ssa.getDef().getSource() = returnExpr and diff --git a/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected b/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected index 2722f67d6fdf..df1d6306feb8 100644 --- a/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected +++ b/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected @@ -69,6 +69,8 @@ typeInferenceMismatch | promise.js:5:25:5:32 | source() | promise.js:5:8:5:33 | bluebir ... urce()) | | promise.js:10:24:10:31 | source() | promise.js:10:8:10:32 | Promise ... urce()) | | promise.js:12:20:12:27 | source() | promise.js:13:8:13:23 | resolver.promise | +| sanitizer-function.js:12:17:12:24 | source() | sanitizer-function.js:14:10:14:14 | taint | +| sanitizer-function.js:12:17:12:24 | source() | sanitizer-function.js:33:14:33:18 | taint | | 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 | diff --git a/javascript/ql/test/library-tests/TaintTracking/DataFlowTracking.expected b/javascript/ql/test/library-tests/TaintTracking/DataFlowTracking.expected index 6a589e3e0d93..f3023ad44daa 100644 --- a/javascript/ql/test/library-tests/TaintTracking/DataFlowTracking.expected +++ b/javascript/ql/test/library-tests/TaintTracking/DataFlowTracking.expected @@ -41,6 +41,11 @@ | partialCalls.js:4:17:4:24 | source() | partialCalls.js:30:14:30:20 | x.value | | partialCalls.js:4:17:4:24 | source() | partialCalls.js:41:10:41:18 | id(taint) | | partialCalls.js:4:17:4:24 | source() | partialCalls.js:51:14:51:14 | x | +| sanitizer-function.js:12:17:12:24 | source() | sanitizer-function.js:14:10:14:14 | taint | +| sanitizer-function.js:12:17:12:24 | source() | sanitizer-function.js:17:14:17:18 | taint | +| sanitizer-function.js:12:17:12:24 | source() | sanitizer-function.js:21:14:21:18 | taint | +| sanitizer-function.js:12:17:12:24 | source() | sanitizer-function.js:25:14:25:18 | taint | +| sanitizer-function.js:12:17:12:24 | source() | sanitizer-function.js:33:14:33:18 | taint | | 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 | diff --git a/javascript/ql/test/library-tests/TaintTracking/sanitizer-function.js b/javascript/ql/test/library-tests/TaintTracking/sanitizer-function.js new file mode 100644 index 000000000000..6df74cf29eec --- /dev/null +++ b/javascript/ql/test/library-tests/TaintTracking/sanitizer-function.js @@ -0,0 +1,35 @@ +function test() { + function myCheck1(x) { + return x === "a" && something() && somethingElse(); + } + function myCheck2(x) { + return something() && x === "a" && somethingElse(); + } + function myCheck3(x) { + return something() && somethingElse() && x === "a"; + } + + let taint = source(); + + sink(taint); // NOT OK + + if (myCheck1(taint)) { + sink(taint); // OK + } + + if (myCheck2(taint)) { + sink(taint); // OK + } + + if (myCheck3(taint)) { + sink(taint); // OK + } + + function badCheck(x) { + return something && x + isSafe(x) != null; + } + + if (badCheck(taint)) { + sink(taint); // NOT OK + } +} From d6158427c5cdecea8569441c71c59bb892304380 Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 31 Oct 2019 22:33:36 +0000 Subject: [PATCH 2/4] JS: Generalize SanitizerFunction to data flow configs and flow labels --- .../javascript/dataflow/Configuration.qll | 169 +++++++++++++++--- .../javascript/dataflow/TaintTracking.qll | 89 +-------- 2 files changed, 145 insertions(+), 113 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll b/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll index 568c1c92eab6..68771406006e 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll @@ -147,7 +147,7 @@ abstract class Configuration extends string { */ predicate isBarrier(DataFlow::Node node) { exists(BarrierGuardNode guard | - isBarrierGuard(guard) and + isBarrierGuardInternal(guard) and guard.internalBlocks(node, "") ) } @@ -181,7 +181,7 @@ abstract class Configuration extends string { */ predicate isLabeledBarrier(DataFlow::Node node, FlowLabel lbl) { exists(BarrierGuardNode guard | - isBarrierGuard(guard) and + isBarrierGuardInternal(guard) and guard.internalBlocks(node, lbl) ) or @@ -198,6 +198,12 @@ abstract class Configuration extends string { */ predicate isBarrierGuard(BarrierGuardNode guard) { none() } + private predicate isBarrierGuardInternal(BarrierGuardNode guard) { + isBarrierGuard(guard) + or + guard.(AdditionalBarrierGuardNode).appliesTo(this) + } + /** * Holds if data may flow from `source` to `sink` for this configuration. */ @@ -304,7 +310,7 @@ abstract class BarrierGuardNode extends DataFlow::Node { forex(SsaVariable input | input = ref.getAnInput() | asExpr() = ref.getGuard().getTest() and outcome = ref.getGuard().(ConditionGuardNode).getOutcome() and - internalBlocksExpr(outcome, input.getAUse(), label) + barrierGuardBlocksExpr(this, outcome, input.getAUse(), label) ) ) or @@ -313,33 +319,11 @@ abstract class BarrierGuardNode extends DataFlow::Node { nd = DataFlow::valueNode(p.getAnInstanceIn(bb)) and asExpr() = cond.getTest() and outcome = cond.getOutcome() and - internalBlocksAccessPath(outcome, p, label) and + barrierGuardBlocksAccessPath(this, outcome, p, label) and cond.dominates(bb) ) } - /** - * Holds if data flow node `nd` acts as a barrier for data flow. - * - * `label` is bound to the blocked label, or the empty string if all labels should be blocked. - */ - private predicate internalBlocksExpr(boolean outcome, Expr test, string label) { - blocks(outcome, test) and label = "" - or - blocks(outcome, test, label) - } - - /** - * Holds if data flow node `nd` acts as a barrier for data flow due to aliasing through - * an access path. - * - * `label` is bound to the blocked label, or the empty string if all labels should be blocked. - */ - pragma[noinline] - private predicate internalBlocksAccessPath(boolean outcome, AccessPath ap, string label) { - internalBlocksExpr(outcome, ap.getAnInstance(), label) - } - /** * Holds if this node blocks expression `e` provided it evaluates to `outcome`. * @@ -353,6 +337,32 @@ abstract class BarrierGuardNode extends DataFlow::Node { predicate blocks(boolean outcome, Expr e, FlowLabel label) { none() } } +/** + * Holds if data flow node `nd` acts as a barrier for data flow. + * + * `label` is bound to the blocked label, or the empty string if all labels should be blocked. + */ +private predicate barrierGuardBlocksExpr(BarrierGuardNode guard, boolean outcome, Expr test, string label) { + guard.blocks(outcome, test) and label = "" + or + guard.blocks(outcome, test, label) + or + // Handle labelled barrier guard functions specially, to avoid negative recursion + // through the non-abstract 3-argument version of blocks(). + guard.(AdditionalBarrierGuardCall).internalBlocksLabel(outcome, test, label) +} + +/** + * Holds if data flow node `nd` acts as a barrier for data flow due to aliasing through + * an access path. + * + * `label` is bound to the blocked label, or the empty string if all labels should be blocked. + */ +pragma[noinline] +private predicate barrierGuardBlocksAccessPath(BarrierGuardNode guard, boolean outcome, AccessPath ap, string label) { + barrierGuardBlocksExpr(guard, outcome, ap.getAnInstance(), label) +} + /** * A guard node that only blocks specific labels. */ @@ -1186,3 +1196,110 @@ module PathGraph { not pred = finalMidNode(succ) } } + + + +/** + * Gets an operand of the given `&&` operator. + * + * We use this to construct the transitive closure over a relation + * that does not include all of `BinaryExpr.getAnOperand`. + */ +private Expr getALogicalAndOperand(LogAndExpr e) { + result = e.getAnOperand() +} + +/** + * Gets an operand of the given `||` operator. + * + * We use this to construct the transitive closure over a relation + * that does not include all of `BinaryExpr.getAnOperand`. + */ +private Expr getALogicalOrOperand(LogOrExpr e) { + result = e.getAnOperand() +} + +/** + * A `BarrierGuardNode` that controls which data flow + * configurations it is used in. + * + * Note: For performance reasons, all subclasses of this class should be part + * of the standard library. Override `Configuration::isBarrierGuard` + * for analysis-specific barrier guards. + */ +abstract class AdditionalBarrierGuardNode extends BarrierGuardNode { + abstract predicate appliesTo(Configuration cfg); +} + +/** + * A function that returns the result of a barrier guard. + */ +private class BarrierGuardFunction extends Function { + DataFlow::ParameterNode sanitizedParameter; + BarrierGuardNode guard; + boolean guardOutcome; + string label; + + BarrierGuardFunction() { + exists(Expr e | + exists(Expr returnExpr | + returnExpr = guard.asExpr() + or + // ad hoc support for conjunctions: + getALogicalAndOperand+(returnExpr) = guard.asExpr() and guardOutcome = true + or + // ad hoc support for disjunctions: + getALogicalOrOperand+(returnExpr) = guard.asExpr() and guardOutcome = false + | + exists(SsaExplicitDefinition ssa | + ssa.getDef().getSource() = returnExpr and + ssa.getVariable().getAUse() = getAReturnedExpr() + ) + or + returnExpr = getAReturnedExpr() + ) and + sanitizedParameter.flowsToExpr(e) and + barrierGuardBlocksExpr(guard, guardOutcome, e, label) + ) and + getNumParameter() = 1 and + sanitizedParameter.getParameter() = getParameter(0) + } + + /** + * Holds if this function sanitizes argument `e` of call `call`, provided the call evaluates to `outcome`. + */ + predicate isBarrierCall(DataFlow::CallNode call, Expr e, boolean outcome, string lbl) { + exists(DataFlow::Node arg | + arg.asExpr() = e and + arg = call.getArgument(0) and + call.getNumArgument() = 1 and + argumentPassing(call, arg, this, sanitizedParameter) and + outcome = guardOutcome and + lbl = label + ) + } + + /** + * Holds if this function applies to the flow in `cfg`. + */ + predicate appliesTo(Configuration cfg) { cfg.isBarrierGuard(guard) } +} + +/** + * A call that sanitizes an argument. + */ +private class AdditionalBarrierGuardCall extends AdditionalBarrierGuardNode, DataFlow::CallNode { + BarrierGuardFunction f; + + AdditionalBarrierGuardCall() { f.isBarrierCall(this, _, _, _) } + + override predicate blocks(boolean outcome, Expr e) { + f.isBarrierCall(this, e, outcome, "") + } + + predicate internalBlocksLabel(boolean outcome, Expr e, DataFlow::FlowLabel label) { + f.isBarrierCall(this, e, outcome, label) + } + + override predicate appliesTo(Configuration cfg) { f.appliesTo(cfg) } +} diff --git a/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll b/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll index 41afdb653f75..43a04c553632 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll @@ -133,8 +133,8 @@ module TaintTracking { * configurations it is used in. * * Note: For performance reasons, all subclasses of this class should be part - * of the standard library. Override `Configuration::isSanitizer` - * for analysis-specific taint steps. + * of the standard library. Override `Configuration::isSanitizerGuard` + * for analysis-specific taint sanitizer guards. */ abstract class AdditionalSanitizerGuardNode extends SanitizerGuardNode { /** @@ -846,91 +846,6 @@ module TaintTracking { override predicate appliesTo(Configuration cfg) { any() } } - /** - * Gets an operand of the given `&&` operator. - * - * We use this to construct the transitive closure over a relation - * that does not include all of `BinaryExpr.getAnOperand`. - */ - private Expr getALogicalAndOperand(LogAndExpr e) { - result = e.getAnOperand() - } - - /** - * Gets an operand of the given `||` operator. - * - * We use this to construct the transitive closure over a relation - * that does not include all of `BinaryExpr.getAnOperand`. - */ - private Expr getALogicalOrOperand(LogOrExpr e) { - result = e.getAnOperand() - } - - /** - * A function that returns the result of a sanitizer check. - */ - private class SanitizingFunction extends Function { - DataFlow::ParameterNode sanitizedParameter; - SanitizerGuardNode sanitizer; - boolean sanitizerOutcome; - - SanitizingFunction() { - exists(Expr e | - exists(Expr returnExpr | - returnExpr = sanitizer.asExpr() - or - // ad hoc support for conjunctions: - getALogicalAndOperand+(returnExpr) = sanitizer.asExpr() and sanitizerOutcome = true - or - // ad hoc support for disjunctions: - getALogicalOrOperand+(returnExpr) = sanitizer.asExpr() and sanitizerOutcome = false - | - exists(SsaExplicitDefinition ssa | - ssa.getDef().getSource() = returnExpr and - ssa.getVariable().getAUse() = getAReturnedExpr() - ) - or - returnExpr = getAReturnedExpr() - ) and - sanitizedParameter.flowsToExpr(e) and - sanitizer.sanitizes(sanitizerOutcome, e) - ) and - getNumParameter() = 1 and - sanitizedParameter.getParameter() = getParameter(0) - } - - /** - * Holds if this function sanitizes argument `e` of call `call`, provided the call evaluates to `outcome`. - */ - predicate isSanitizingCall(DataFlow::CallNode call, Expr e, boolean outcome) { - exists(DataFlow::Node arg | - arg.asExpr() = e and - arg = call.getArgument(0) and - call.getNumArgument() = 1 and - FlowSteps::argumentPassing(call, arg, this, sanitizedParameter) and - outcome = sanitizerOutcome - ) - } - - /** - * Holds if this function applies to the flow in `cfg`. - */ - predicate appliesTo(Configuration cfg) { cfg.isBarrierGuard(sanitizer) } - } - - /** - * A call that sanitizes an argument. - */ - private class AdditionalSanitizingCall extends AdditionalSanitizerGuardNode, DataFlow::CallNode { - SanitizingFunction f; - - AdditionalSanitizingCall() { f.isSanitizingCall(this, _, _) } - - override predicate sanitizes(boolean outcome, Expr e) { f.isSanitizingCall(this, e, outcome) } - - override predicate appliesTo(Configuration cfg) { f.appliesTo(cfg) } - } - /** * An equality test on `e.origin` or `e.source` where `e` is a `postMessage` event object, * considered as a sanitizer for `e`. From f48d16fcb7450579e4e36e6eaa1a1d8547eed4ef Mon Sep 17 00:00:00 2001 From: Asger F Date: Fri, 1 Nov 2019 15:22:03 +0000 Subject: [PATCH 3/4] JS: Support barrier guards that are reflective calls --- .../semmle/javascript/dataflow/Configuration.qll | 11 +++++++++-- .../ql/src/semmle/javascript/dataflow/DataFlow.qll | 9 +++++++++ .../TaintTracking/BasicTaintTracking.expected | 2 ++ .../TaintTracking/BasicTaintTracking.ql | 6 +++++- .../TaintTracking/DataFlowTracking.expected | 3 +++ .../TaintTracking/sanitizer-guards.js | 14 ++++++++++++++ 6 files changed, 42 insertions(+), 3 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll b/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll index 68771406006e..11cfb24376e6 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll @@ -308,7 +308,7 @@ abstract class BarrierGuardNode extends DataFlow::Node { exists(SsaRefinementNode ref, boolean outcome | nd = DataFlow::ssaDefinitionNode(ref) and forex(SsaVariable input | input = ref.getAnInput() | - asExpr() = ref.getGuard().getTest() and + getExpr() = ref.getGuard().getTest() and outcome = ref.getGuard().(ConditionGuardNode).getOutcome() and barrierGuardBlocksExpr(this, outcome, input.getAUse(), label) ) @@ -317,13 +317,20 @@ abstract class BarrierGuardNode extends DataFlow::Node { // 2) `nd` is an instance of an access path `p`, and dominated by a barrier for `p` exists(AccessPath p, BasicBlock bb, ConditionGuardNode cond, boolean outcome | nd = DataFlow::valueNode(p.getAnInstanceIn(bb)) and - asExpr() = cond.getTest() and + getExpr() = cond.getTest() and outcome = cond.getOutcome() and barrierGuardBlocksAccessPath(this, outcome, p, label) and cond.dominates(bb) ) } + /** Gets the corresponding expression, including that of reflective calls. */ + private Expr getExpr() { + result = asExpr() + or + this = DataFlow::reflectiveCallNode(result) + } + /** * Holds if this node blocks expression `e` provided it evaluates to `outcome`. * diff --git a/javascript/ql/src/semmle/javascript/dataflow/DataFlow.qll b/javascript/ql/src/semmle/javascript/dataflow/DataFlow.qll index 8798df6cc37c..0faecc011095 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/DataFlow.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/DataFlow.qll @@ -932,6 +932,15 @@ module DataFlow { */ DataFlow::Node globalAccessPathRootPseudoNode() { result instanceof TGlobalAccessPathRoot } + /** + * Gets a data flow node representing the underlying call performed by the given + * call to `Function.prototype.call` or `Function.prototype.apply`. + * + * For example, for an expression `fn.call(x, y)`, this gets a call node with `fn` as the + * callee, `x` as the receiver, and `y` as the first argument. + */ + DataFlow::InvokeNode reflectiveCallNode(InvokeExpr expr) { result = TReflectiveCallNode(expr, _) } + /** * Provides classes representing various kinds of calls. * diff --git a/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected b/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected index df1d6306feb8..93dc5e6e200c 100644 --- a/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected +++ b/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected @@ -75,6 +75,8 @@ typeInferenceMismatch | 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 | +| sanitizer-guards.js:43:11:43:18 | source() | sanitizer-guards.js:45:8:45:8 | x | +| sanitizer-guards.js:43:11:43:18 | source() | sanitizer-guards.js:48:10:48:10 | x | | spread.js:2:15:2:22 | source() | spread.js:4:8:4:19 | { ...taint } | | spread.js:2:15:2:22 | source() | spread.js:5:8:5:43 | { f: 'h ... orld' } | | spread.js:2:15:2:22 | source() | spread.js:7:8:7:19 | [ ...taint ] | diff --git a/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.ql b/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.ql index 0b1d5d9da84b..26c66a1b4fe0 100644 --- a/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.ql +++ b/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.ql @@ -1,7 +1,11 @@ import javascript import semmle.javascript.dataflow.InferredTypes -DataFlow::CallNode getACall(string name) { result.getCalleeName() = name } +DataFlow::CallNode getACall(string name) { + result.getCalleeName() = name + or + result.getCalleeNode().getALocalSource() = DataFlow::globalVarRef(name) +} class Sink extends DataFlow::Node { Sink() { this = getACall("sink").getAnArgument() } diff --git a/javascript/ql/test/library-tests/TaintTracking/DataFlowTracking.expected b/javascript/ql/test/library-tests/TaintTracking/DataFlowTracking.expected index f3023ad44daa..f2799d763fdc 100644 --- a/javascript/ql/test/library-tests/TaintTracking/DataFlowTracking.expected +++ b/javascript/ql/test/library-tests/TaintTracking/DataFlowTracking.expected @@ -50,6 +50,9 @@ | 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 | +| sanitizer-guards.js:43:11:43:18 | source() | sanitizer-guards.js:45:8:45:8 | x | +| sanitizer-guards.js:43:11:43:18 | source() | sanitizer-guards.js:48:10:48:10 | x | +| sanitizer-guards.js:43:11:43:18 | source() | sanitizer-guards.js:52:10:52:10 | 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 494e828460d3..03d6a9aedd74 100644 --- a/javascript/ql/test/library-tests/TaintTracking/sanitizer-guards.js +++ b/javascript/ql/test/library-tests/TaintTracking/sanitizer-guards.js @@ -38,3 +38,17 @@ class C { }); } } + +function reflective() { + let x = source(); + + sink(x); // NOT OK + + if (isSafe.call(x)) { + sink(x); // NOT OK - `isSafe` does not sanitize the receiver + } + + if (isSafe.call(null, x)) { + sink(x); // OK + } +} From c373be0dee9b4c8b4edbfb5980b1b0b3f5586bde Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 5 Nov 2019 10:26:04 +0000 Subject: [PATCH 4/4] JS: Update TaintBarriers test --- .../library-tests/TaintBarriers/SanitizingGuard.expected | 5 ----- .../ql/test/library-tests/TaintBarriers/TaintedSink.expected | 1 - .../ql/test/library-tests/TaintBarriers/isBarrier.expected | 1 + javascript/ql/test/library-tests/TaintBarriers/tst.js | 2 +- 4 files changed, 2 insertions(+), 7 deletions(-) diff --git a/javascript/ql/test/library-tests/TaintBarriers/SanitizingGuard.expected b/javascript/ql/test/library-tests/TaintBarriers/SanitizingGuard.expected index c58a7c511d11..010375e1ab81 100644 --- a/javascript/ql/test/library-tests/TaintBarriers/SanitizingGuard.expected +++ b/javascript/ql/test/library-tests/TaintBarriers/SanitizingGuard.expected @@ -44,14 +44,11 @@ | tst.js:236:9:236:24 | isWhitelisted(v) | ExampleConfiguration | true | tst.js:236:23:236:23 | v | | tst.js:240:9:240:28 | config.allowValue(v) | ExampleConfiguration | true | tst.js:240:27:240:27 | v | | tst.js:252:16:252:36 | whiteli ... ains(x) | ExampleConfiguration | true | tst.js:252:35:252:35 | x | -| tst.js:254:9:254:12 | f(v) | ExampleConfiguration | true | tst.js:254:11:254:11 | v | | tst.js:261:25:261:45 | whiteli ... ains(y) | ExampleConfiguration | true | tst.js:261:44:261:44 | y | -| tst.js:264:9:264:12 | g(v) | ExampleConfiguration | true | tst.js:264:11:264:11 | v | | tst.js:271:25:271:45 | whiteli ... ains(z) | ExampleConfiguration | true | tst.js:271:44:271:44 | z | | tst.js:281:16:281:25 | x2 != null | ExampleConfiguration | false | tst.js:281:16:281:17 | x2 | | tst.js:281:16:281:25 | x2 != null | ExampleConfiguration | false | tst.js:281:22:281:25 | null | | tst.js:281:30:281:51 | whiteli ... ins(x2) | ExampleConfiguration | true | tst.js:281:49:281:50 | x2 | -| tst.js:283:9:283:13 | f2(v) | ExampleConfiguration | true | tst.js:283:12:283:12 | v | | tst.js:290:16:290:25 | x3 == null | ExampleConfiguration | true | tst.js:290:16:290:17 | x3 | | tst.js:290:16:290:25 | x3 == null | ExampleConfiguration | true | tst.js:290:22:290:25 | null | | tst.js:290:30:290:51 | whiteli ... ins(x3) | ExampleConfiguration | true | tst.js:290:49:290:50 | x3 | @@ -61,7 +58,6 @@ | tst.js:327:25:327:34 | x7 != null | ExampleConfiguration | false | tst.js:327:25:327:26 | x7 | | tst.js:327:25:327:34 | x7 != null | ExampleConfiguration | false | tst.js:327:31:327:34 | null | | tst.js:327:39:327:60 | whiteli ... ins(x7) | ExampleConfiguration | true | tst.js:327:58:327:59 | x7 | -| tst.js:330:9:330:13 | f7(v) | ExampleConfiguration | true | tst.js:330:12:330:12 | v | | tst.js:337:25:337:46 | whiteli ... ins(x8) | ExampleConfiguration | true | tst.js:337:44:337:45 | x8 | | tst.js:338:16:338:25 | x8 != null | ExampleConfiguration | false | tst.js:338:16:338:17 | x8 | | tst.js:338:16:338:25 | x8 != null | ExampleConfiguration | false | tst.js:338:22:338:25 | null | @@ -70,6 +66,5 @@ | tst.js:356:16:356:27 | x10 !== null | ExampleConfiguration | false | tst.js:356:24:356:27 | null | | tst.js:356:32:356:48 | x10 !== undefined | ExampleConfiguration | false | tst.js:356:32:356:34 | x10 | | tst.js:356:32:356:48 | x10 !== undefined | ExampleConfiguration | false | tst.js:356:40:356:48 | undefined | -| tst.js:358:9:358:14 | f10(v) | ExampleConfiguration | false | tst.js:358:13:358:13 | v | | tst.js:370:9:370:29 | o.p == ... listed" | ExampleConfiguration | true | tst.js:370:9:370:11 | o.p | | tst.js:377:11:377:32 | o[p] == ... listed" | ExampleConfiguration | true | tst.js:377:11:377:14 | o[p] | diff --git a/javascript/ql/test/library-tests/TaintBarriers/TaintedSink.expected b/javascript/ql/test/library-tests/TaintBarriers/TaintedSink.expected index 050f80bc0719..54b35166b15a 100644 --- a/javascript/ql/test/library-tests/TaintBarriers/TaintedSink.expected +++ b/javascript/ql/test/library-tests/TaintBarriers/TaintedSink.expected @@ -53,7 +53,6 @@ | tst.js:333:14:333:14 | v | tst.js:248:13:248:20 | SOURCE() | | tst.js:341:14:341:14 | v | tst.js:248:13:248:20 | SOURCE() | | tst.js:343:14:343:14 | v | tst.js:248:13:248:20 | SOURCE() | -| tst.js:350:14:350:14 | v | tst.js:248:13:248:20 | SOURCE() | | tst.js:352:14:352:14 | v | tst.js:248:13:248:20 | SOURCE() | | tst.js:359:14:359:14 | v | tst.js:248:13:248:20 | SOURCE() | | tst.js:368:10:368:12 | o.p | tst.js:367:13:367:20 | SOURCE() | diff --git a/javascript/ql/test/library-tests/TaintBarriers/isBarrier.expected b/javascript/ql/test/library-tests/TaintBarriers/isBarrier.expected index 50804043f95b..f650c93185c6 100644 --- a/javascript/ql/test/library-tests/TaintBarriers/isBarrier.expected +++ b/javascript/ql/test/library-tests/TaintBarriers/isBarrier.expected @@ -37,6 +37,7 @@ | tst.js:265:14:265:14 | v | ExampleConfiguration | | tst.js:284:14:284:14 | v | ExampleConfiguration | | tst.js:331:14:331:14 | v | ExampleConfiguration | +| tst.js:350:14:350:14 | v | ExampleConfiguration | | tst.js:356:16:356:27 | x10 | ExampleConfiguration | | tst.js:356:32:356:34 | x10 | ExampleConfiguration | | tst.js:361:14:361:14 | v | ExampleConfiguration | diff --git a/javascript/ql/test/library-tests/TaintBarriers/tst.js b/javascript/ql/test/library-tests/TaintBarriers/tst.js index 78986d6af376..a7a05590540c 100644 --- a/javascript/ql/test/library-tests/TaintBarriers/tst.js +++ b/javascript/ql/test/library-tests/TaintBarriers/tst.js @@ -347,7 +347,7 @@ function IndirectSanitizer () { return unknown() && whitelist.contains(x9) && unknown(); } if (f9(v)) { - SINK(v); // SANITIZATION OF THIS IS NOT YET SUPPORTED + SINK(v); } else { SINK(v); }