Skip to content

JS: Barrier guard improvements #2251

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

Merged
merged 4 commits into from
Nov 7, 2019
Merged

JS: Barrier guard improvements #2251

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e2b0ec5
JS: Handle multiple and/or operators in SanitizerFunction
asger-semmle Oct 31, 2019
d615842
JS: Generalize SanitizerFunction to data flow configs and flow labels
asger-semmle Oct 31, 2019
f48d16f
JS: Support barrier guards that are reflective calls
asger-semmle Nov 1, 2019
c373be0
JS: Update TaintBarriers test
asger-semmle Nov 5, 2019
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
174 changes: 149 additions & 25 deletions javascript/ql/src/semmle/javascript/dataflow/Configuration.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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, "")
)
}
Expand Down Expand Up @@ -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
Expand All @@ -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.
*/
Expand Down Expand Up @@ -302,42 +308,27 @@ 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
internalBlocksExpr(outcome, input.getAUse(), label)
barrierGuardBlocksExpr(this, outcome, input.getAUse(), label)
)
)
or
// 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
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 = ""
/** Gets the corresponding expression, including that of reflective calls. */
private Expr getExpr() {
result = asExpr()
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)
this = DataFlow::reflectiveCallNode(result)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I've previously had the same issue as the one you are solving here.

Could there be an idea in changing DataFlow::Node::asExpr() to include reflective calls, or lift this method to DataFlow::Node?

Copy link
Contributor

Choose a reason for hiding this comment

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

In any case getExpr might not be the most descriptive name.

asExprInclReflectiveCalls? (But that name feels too long, feel free to ignore).

Copy link
Contributor

Choose a reason for hiding this comment

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

Could there be an idea in changing DataFlow::Node::asExpr()

I think that leads to a recursion mess, and also a mess of contepts. I would prefer the second option:

lift this method to DataFlow::Node

But a good name escapes me. asUnderlyingExpr is my best suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My main gripe with using asExpr() is that two data flow nodes can suddenly map to the same expression, which brings along its own set of new pitfalls. But maybe we should do it anyway? It's essentially the same issue as parameter nodes not having AST nodes.

The general solution is to take the thing you know is an expression (getTest()), map that to a data-flow node, and follow up with getImmediatePredecessor, getAPredecessor, or getALocalSource. The third version I mentioned in the PR description used getImmediatePredecessor but I backed it out again, because it required more work for that generalization to actually work beyond reflective calls.

I think I've previously had the same issue as the one you are solving here.

@erik-krogh Can you find out what it was? It might help us figure out which solution works best.

Copy link
Contributor

@erik-krogh erik-krogh Nov 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@erik-krogh Can you find out what it was? It might help us figure out which solution works best.

From looking around I can't immediately find an instance of me solving the issue, but I think that is because I ended up not solving it.
The issue definitely exists in UseOfReturnlessFunction, which has no explicit handling of reflective calls, and DataFlow::CallNode::asExpr() is therefore none().

As a consequence, UseOfReturnlessFunction has an FP on the last line in this example:

function onlySideEffects() {
  console.log("Boo!")
}
var dead1 = onlySideEffects(); // Correctly not flagged
var dead2 = onlySideEffects.call(null); // FP! flagged by UseOfReturnlessFunction

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about DataFlow::Node::getEnclosingExpr() or getEnclosingAstNode()? This would map reflective calls to the CallExpr, parameter nodes to the Parameter, and ValueNode to the AST node.

(To elaborate on my previous point, extending asExpr() and flow() can lead to unfortunate cases where code like expr.flow().(CallNode).getArgument(0) returns the first two arguments.)

Anyways, I think it's a bit out of scope for this PR, but I'm happy to continue the discussion here. Ping @max-schaefer.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm currently doing a dist-compare on an implementation of DataFlow::Node::getEnclosingExpr().

Expr getEnclosingExpr() {
  result = asExpr() or
  this = DataFlow::reflectiveCallNode(result) or
  result = this.(ParameterNode).getParameter()
}

The evaluation is on UseOfReturnlessFunction, where i changed some asExpr() to getEnclosingExpr(), which definitely removes some false positives.
If the results are good I'll land it in a PR.

}

/**
Expand All @@ -353,6 +344,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.
*/
Expand Down Expand Up @@ -1186,3 +1203,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) }
}
9 changes: 9 additions & 0 deletions javascript/ql/src/semmle/javascript/dataflow/DataFlow.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand Down
69 changes: 2 additions & 67 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 @@ -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 {
/**
Expand Down Expand Up @@ -846,71 +846,6 @@ module TaintTracking {
override predicate appliesTo(Configuration cfg) { any() }
}

/**
* 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:
returnExpr.(LogAndExpr).getAnOperand() = sanitizer.asExpr() and sanitizerOutcome = true
or
// ad hoc support for disjunctions:
returnExpr.(LogOrExpr).getAnOperand() = 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`.
Expand Down
5 changes: 0 additions & 5 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 @@ -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 |
Expand All @@ -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 |
Expand All @@ -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] |
1 change: 0 additions & 1 deletion 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 @@ -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() |
Expand Down
1 change: 1 addition & 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 @@ -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 |
Expand Down
2 changes: 1 addition & 1 deletion 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 @@ -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);
}
Expand Down
4 changes: 4 additions & 0 deletions javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,10 +69,14 @@ 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 |
| 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 ] |
Expand Down
Loading