Skip to content

Commit e65271d

Browse files
semmle-qlcisemmle-qlci
authored
Merge pull request #2251 from asger-semmle/barrier-guard-improvements
Approved by esbena
2 parents f79c2a7 + c373be0 commit e65271d

12 files changed

Lines changed: 228 additions & 100 deletions

File tree

‎javascript/ql/src/semmle/javascript/dataflow/Configuration.qll‎

Lines changed: 149 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -147,7 +147,7 @@ abstract class Configuration extends string {
147147
*/
148148
predicate isBarrier(DataFlow::Node node) {
149149
exists(BarrierGuardNode guard |
150-
isBarrierGuard(guard) and
150+
isBarrierGuardInternal(guard) and
151151
guard.internalBlocks(node, "")
152152
)
153153
}
@@ -181,7 +181,7 @@ abstract class Configuration extends string {
181181
*/
182182
predicate isLabeledBarrier(DataFlow::Node node, FlowLabel lbl) {
183183
exists(BarrierGuardNode guard |
184-
isBarrierGuard(guard) and
184+
isBarrierGuardInternal(guard) and
185185
guard.internalBlocks(node, lbl)
186186
)
187187
or
@@ -198,6 +198,12 @@ abstract class Configuration extends string {
198198
*/
199199
predicate isBarrierGuard(BarrierGuardNode guard) { none() }
200200

201+
private predicate isBarrierGuardInternal(BarrierGuardNode guard) {
202+
isBarrierGuard(guard)
203+
or
204+
guard.(AdditionalBarrierGuardNode).appliesTo(this)
205+
}
206+
201207
/**
202208
* Holds if data may flow from `source` to `sink` for this configuration.
203209
*/
@@ -302,42 +308,27 @@ abstract class BarrierGuardNode extends DataFlow::Node {
302308
exists(SsaRefinementNode ref, boolean outcome |
303309
nd = DataFlow::ssaDefinitionNode(ref) and
304310
forex(SsaVariable input | input = ref.getAnInput() |
305-
asExpr() = ref.getGuard().getTest() and
311+
getExpr() = ref.getGuard().getTest() and
306312
outcome = ref.getGuard().(ConditionGuardNode).getOutcome() and
307-
internalBlocksExpr(outcome, input.getAUse(), label)
313+
barrierGuardBlocksExpr(this, outcome, input.getAUse(), label)
308314
)
309315
)
310316
or
311317
// 2) `nd` is an instance of an access path `p`, and dominated by a barrier for `p`
312318
exists(AccessPath p, BasicBlock bb, ConditionGuardNode cond, boolean outcome |
313319
nd = DataFlow::valueNode(p.getAnInstanceIn(bb)) and
314-
asExpr() = cond.getTest() and
320+
getExpr() = cond.getTest() and
315321
outcome = cond.getOutcome() and
316-
internalBlocksAccessPath(outcome, p, label) and
322+
barrierGuardBlocksAccessPath(this, outcome, p, label) and
317323
cond.dominates(bb)
318324
)
319325
}
320326

321-
/**
322-
* Holds if data flow node `nd` acts as a barrier for data flow.
323-
*
324-
* `label` is bound to the blocked label, or the empty string if all labels should be blocked.
325-
*/
326-
private predicate internalBlocksExpr(boolean outcome, Expr test, string label) {
327-
blocks(outcome, test) and label = ""
327+
/** Gets the corresponding expression, including that of reflective calls. */
328+
private Expr getExpr() {
329+
result = asExpr()
328330
or
329-
blocks(outcome, test, label)
330-
}
331-
332-
/**
333-
* Holds if data flow node `nd` acts as a barrier for data flow due to aliasing through
334-
* an access path.
335-
*
336-
* `label` is bound to the blocked label, or the empty string if all labels should be blocked.
337-
*/
338-
pragma[noinline]
339-
private predicate internalBlocksAccessPath(boolean outcome, AccessPath ap, string label) {
340-
internalBlocksExpr(outcome, ap.getAnInstance(), label)
331+
this = DataFlow::reflectiveCallNode(result)
341332
}
342333

343334
/**
@@ -353,6 +344,32 @@ abstract class BarrierGuardNode extends DataFlow::Node {
353344
predicate blocks(boolean outcome, Expr e, FlowLabel label) { none() }
354345
}
355346

347+
/**
348+
* Holds if data flow node `nd` acts as a barrier for data flow.
349+
*
350+
* `label` is bound to the blocked label, or the empty string if all labels should be blocked.
351+
*/
352+
private predicate barrierGuardBlocksExpr(BarrierGuardNode guard, boolean outcome, Expr test, string label) {
353+
guard.blocks(outcome, test) and label = ""
354+
or
355+
guard.blocks(outcome, test, label)
356+
or
357+
// Handle labelled barrier guard functions specially, to avoid negative recursion
358+
// through the non-abstract 3-argument version of blocks().
359+
guard.(AdditionalBarrierGuardCall).internalBlocksLabel(outcome, test, label)
360+
}
361+
362+
/**
363+
* Holds if data flow node `nd` acts as a barrier for data flow due to aliasing through
364+
* an access path.
365+
*
366+
* `label` is bound to the blocked label, or the empty string if all labels should be blocked.
367+
*/
368+
pragma[noinline]
369+
private predicate barrierGuardBlocksAccessPath(BarrierGuardNode guard, boolean outcome, AccessPath ap, string label) {
370+
barrierGuardBlocksExpr(guard, outcome, ap.getAnInstance(), label)
371+
}
372+
356373
/**
357374
* A guard node that only blocks specific labels.
358375
*/
@@ -1186,3 +1203,110 @@ module PathGraph {
11861203
not pred = finalMidNode(succ)
11871204
}
11881205
}
1206+
1207+
1208+
1209+
/**
1210+
* Gets an operand of the given `&&` operator.
1211+
*
1212+
* We use this to construct the transitive closure over a relation
1213+
* that does not include all of `BinaryExpr.getAnOperand`.
1214+
*/
1215+
private Expr getALogicalAndOperand(LogAndExpr e) {
1216+
result = e.getAnOperand()
1217+
}
1218+
1219+
/**
1220+
* Gets an operand of the given `||` operator.
1221+
*
1222+
* We use this to construct the transitive closure over a relation
1223+
* that does not include all of `BinaryExpr.getAnOperand`.
1224+
*/
1225+
private Expr getALogicalOrOperand(LogOrExpr e) {
1226+
result = e.getAnOperand()
1227+
}
1228+
1229+
/**
1230+
* A `BarrierGuardNode` that controls which data flow
1231+
* configurations it is used in.
1232+
*
1233+
* Note: For performance reasons, all subclasses of this class should be part
1234+
* of the standard library. Override `Configuration::isBarrierGuard`
1235+
* for analysis-specific barrier guards.
1236+
*/
1237+
abstract class AdditionalBarrierGuardNode extends BarrierGuardNode {
1238+
abstract predicate appliesTo(Configuration cfg);
1239+
}
1240+
1241+
/**
1242+
* A function that returns the result of a barrier guard.
1243+
*/
1244+
private class BarrierGuardFunction extends Function {
1245+
DataFlow::ParameterNode sanitizedParameter;
1246+
BarrierGuardNode guard;
1247+
boolean guardOutcome;
1248+
string label;
1249+
1250+
BarrierGuardFunction() {
1251+
exists(Expr e |
1252+
exists(Expr returnExpr |
1253+
returnExpr = guard.asExpr()
1254+
or
1255+
// ad hoc support for conjunctions:
1256+
getALogicalAndOperand+(returnExpr) = guard.asExpr() and guardOutcome = true
1257+
or
1258+
// ad hoc support for disjunctions:
1259+
getALogicalOrOperand+(returnExpr) = guard.asExpr() and guardOutcome = false
1260+
|
1261+
exists(SsaExplicitDefinition ssa |
1262+
ssa.getDef().getSource() = returnExpr and
1263+
ssa.getVariable().getAUse() = getAReturnedExpr()
1264+
)
1265+
or
1266+
returnExpr = getAReturnedExpr()
1267+
) and
1268+
sanitizedParameter.flowsToExpr(e) and
1269+
barrierGuardBlocksExpr(guard, guardOutcome, e, label)
1270+
) and
1271+
getNumParameter() = 1 and
1272+
sanitizedParameter.getParameter() = getParameter(0)
1273+
}
1274+
1275+
/**
1276+
* Holds if this function sanitizes argument `e` of call `call`, provided the call evaluates to `outcome`.
1277+
*/
1278+
predicate isBarrierCall(DataFlow::CallNode call, Expr e, boolean outcome, string lbl) {
1279+
exists(DataFlow::Node arg |
1280+
arg.asExpr() = e and
1281+
arg = call.getArgument(0) and
1282+
call.getNumArgument() = 1 and
1283+
argumentPassing(call, arg, this, sanitizedParameter) and
1284+
outcome = guardOutcome and
1285+
lbl = label
1286+
)
1287+
}
1288+
1289+
/**
1290+
* Holds if this function applies to the flow in `cfg`.
1291+
*/
1292+
predicate appliesTo(Configuration cfg) { cfg.isBarrierGuard(guard) }
1293+
}
1294+
1295+
/**
1296+
* A call that sanitizes an argument.
1297+
*/
1298+
private class AdditionalBarrierGuardCall extends AdditionalBarrierGuardNode, DataFlow::CallNode {
1299+
BarrierGuardFunction f;
1300+
1301+
AdditionalBarrierGuardCall() { f.isBarrierCall(this, _, _, _) }
1302+
1303+
override predicate blocks(boolean outcome, Expr e) {
1304+
f.isBarrierCall(this, e, outcome, "")
1305+
}
1306+
1307+
predicate internalBlocksLabel(boolean outcome, Expr e, DataFlow::FlowLabel label) {
1308+
f.isBarrierCall(this, e, outcome, label)
1309+
}
1310+
1311+
override predicate appliesTo(Configuration cfg) { f.appliesTo(cfg) }
1312+
}

‎javascript/ql/src/semmle/javascript/dataflow/DataFlow.qll‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -947,6 +947,15 @@ module DataFlow {
947947
*/
948948
DataFlow::Node globalAccessPathRootPseudoNode() { result instanceof TGlobalAccessPathRoot }
949949

950+
/**
951+
* Gets a data flow node representing the underlying call performed by the given
952+
* call to `Function.prototype.call` or `Function.prototype.apply`.
953+
*
954+
* For example, for an expression `fn.call(x, y)`, this gets a call node with `fn` as the
955+
* callee, `x` as the receiver, and `y` as the first argument.
956+
*/
957+
DataFlow::InvokeNode reflectiveCallNode(InvokeExpr expr) { result = TReflectiveCallNode(expr, _) }
958+
950959
/**
951960
* Provides classes representing various kinds of calls.
952961
*

‎javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll‎

Lines changed: 2 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -133,8 +133,8 @@ module TaintTracking {
133133
* configurations it is used in.
134134
*
135135
* Note: For performance reasons, all subclasses of this class should be part
136-
* of the standard library. Override `Configuration::isSanitizer`
137-
* for analysis-specific taint steps.
136+
* of the standard library. Override `Configuration::isSanitizerGuard`
137+
* for analysis-specific taint sanitizer guards.
138138
*/
139139
abstract class AdditionalSanitizerGuardNode extends SanitizerGuardNode {
140140
/**
@@ -846,71 +846,6 @@ module TaintTracking {
846846
override predicate appliesTo(Configuration cfg) { any() }
847847
}
848848

849-
/**
850-
* A function that returns the result of a sanitizer check.
851-
*/
852-
private class SanitizingFunction extends Function {
853-
DataFlow::ParameterNode sanitizedParameter;
854-
SanitizerGuardNode sanitizer;
855-
boolean sanitizerOutcome;
856-
857-
SanitizingFunction() {
858-
exists(Expr e |
859-
exists(Expr returnExpr |
860-
returnExpr = sanitizer.asExpr()
861-
or
862-
// ad hoc support for conjunctions:
863-
returnExpr.(LogAndExpr).getAnOperand() = sanitizer.asExpr() and sanitizerOutcome = true
864-
or
865-
// ad hoc support for disjunctions:
866-
returnExpr.(LogOrExpr).getAnOperand() = sanitizer.asExpr() and sanitizerOutcome = false
867-
|
868-
exists(SsaExplicitDefinition ssa |
869-
ssa.getDef().getSource() = returnExpr and
870-
ssa.getVariable().getAUse() = getAReturnedExpr()
871-
)
872-
or
873-
returnExpr = getAReturnedExpr()
874-
) and
875-
sanitizedParameter.flowsToExpr(e) and
876-
sanitizer.sanitizes(sanitizerOutcome, e)
877-
) and
878-
getNumParameter() = 1 and
879-
sanitizedParameter.getParameter() = getParameter(0)
880-
}
881-
882-
/**
883-
* Holds if this function sanitizes argument `e` of call `call`, provided the call evaluates to `outcome`.
884-
*/
885-
predicate isSanitizingCall(DataFlow::CallNode call, Expr e, boolean outcome) {
886-
exists(DataFlow::Node arg |
887-
arg.asExpr() = e and
888-
arg = call.getArgument(0) and
889-
call.getNumArgument() = 1 and
890-
FlowSteps::argumentPassing(call, arg, this, sanitizedParameter) and
891-
outcome = sanitizerOutcome
892-
)
893-
}
894-
895-
/**
896-
* Holds if this function applies to the flow in `cfg`.
897-
*/
898-
predicate appliesTo(Configuration cfg) { cfg.isBarrierGuard(sanitizer) }
899-
}
900-
901-
/**
902-
* A call that sanitizes an argument.
903-
*/
904-
private class AdditionalSanitizingCall extends AdditionalSanitizerGuardNode, DataFlow::CallNode {
905-
SanitizingFunction f;
906-
907-
AdditionalSanitizingCall() { f.isSanitizingCall(this, _, _) }
908-
909-
override predicate sanitizes(boolean outcome, Expr e) { f.isSanitizingCall(this, e, outcome) }
910-
911-
override predicate appliesTo(Configuration cfg) { f.appliesTo(cfg) }
912-
}
913-
914849
/**
915850
* An equality test on `e.origin` or `e.source` where `e` is a `postMessage` event object,
916851
* considered as a sanitizer for `e`.

‎javascript/ql/test/library-tests/TaintBarriers/SanitizingGuard.expected‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -44,14 +44,11 @@
4444
| tst.js:236:9:236:24 | isWhitelisted(v) | ExampleConfiguration | true | tst.js:236:23:236:23 | v |
4545
| tst.js:240:9:240:28 | config.allowValue(v) | ExampleConfiguration | true | tst.js:240:27:240:27 | v |
4646
| tst.js:252:16:252:36 | whiteli ... ains(x) | ExampleConfiguration | true | tst.js:252:35:252:35 | x |
47-
| tst.js:254:9:254:12 | f(v) | ExampleConfiguration | true | tst.js:254:11:254:11 | v |
4847
| tst.js:261:25:261:45 | whiteli ... ains(y) | ExampleConfiguration | true | tst.js:261:44:261:44 | y |
49-
| tst.js:264:9:264:12 | g(v) | ExampleConfiguration | true | tst.js:264:11:264:11 | v |
5048
| tst.js:271:25:271:45 | whiteli ... ains(z) | ExampleConfiguration | true | tst.js:271:44:271:44 | z |
5149
| tst.js:281:16:281:25 | x2 != null | ExampleConfiguration | false | tst.js:281:16:281:17 | x2 |
5250
| tst.js:281:16:281:25 | x2 != null | ExampleConfiguration | false | tst.js:281:22:281:25 | null |
5351
| tst.js:281:30:281:51 | whiteli ... ins(x2) | ExampleConfiguration | true | tst.js:281:49:281:50 | x2 |
54-
| tst.js:283:9:283:13 | f2(v) | ExampleConfiguration | true | tst.js:283:12:283:12 | v |
5552
| tst.js:290:16:290:25 | x3 == null | ExampleConfiguration | true | tst.js:290:16:290:17 | x3 |
5653
| tst.js:290:16:290:25 | x3 == null | ExampleConfiguration | true | tst.js:290:22:290:25 | null |
5754
| tst.js:290:30:290:51 | whiteli ... ins(x3) | ExampleConfiguration | true | tst.js:290:49:290:50 | x3 |
@@ -61,7 +58,6 @@
6158
| tst.js:327:25:327:34 | x7 != null | ExampleConfiguration | false | tst.js:327:25:327:26 | x7 |
6259
| tst.js:327:25:327:34 | x7 != null | ExampleConfiguration | false | tst.js:327:31:327:34 | null |
6360
| tst.js:327:39:327:60 | whiteli ... ins(x7) | ExampleConfiguration | true | tst.js:327:58:327:59 | x7 |
64-
| tst.js:330:9:330:13 | f7(v) | ExampleConfiguration | true | tst.js:330:12:330:12 | v |
6561
| tst.js:337:25:337:46 | whiteli ... ins(x8) | ExampleConfiguration | true | tst.js:337:44:337:45 | x8 |
6662
| tst.js:338:16:338:25 | x8 != null | ExampleConfiguration | false | tst.js:338:16:338:17 | x8 |
6763
| tst.js:338:16:338:25 | x8 != null | ExampleConfiguration | false | tst.js:338:22:338:25 | null |
@@ -70,6 +66,5 @@
7066
| tst.js:356:16:356:27 | x10 !== null | ExampleConfiguration | false | tst.js:356:24:356:27 | null |
7167
| tst.js:356:32:356:48 | x10 !== undefined | ExampleConfiguration | false | tst.js:356:32:356:34 | x10 |
7268
| tst.js:356:32:356:48 | x10 !== undefined | ExampleConfiguration | false | tst.js:356:40:356:48 | undefined |
73-
| tst.js:358:9:358:14 | f10(v) | ExampleConfiguration | false | tst.js:358:13:358:13 | v |
7469
| tst.js:370:9:370:29 | o.p == ... listed" | ExampleConfiguration | true | tst.js:370:9:370:11 | o.p |
7570
| tst.js:377:11:377:32 | o[p] == ... listed" | ExampleConfiguration | true | tst.js:377:11:377:14 | o[p] |

‎javascript/ql/test/library-tests/TaintBarriers/TaintedSink.expected‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,6 @@
5353
| tst.js:333:14:333:14 | v | tst.js:248:13:248:20 | SOURCE() |
5454
| tst.js:341:14:341:14 | v | tst.js:248:13:248:20 | SOURCE() |
5555
| tst.js:343:14:343:14 | v | tst.js:248:13:248:20 | SOURCE() |
56-
| tst.js:350:14:350:14 | v | tst.js:248:13:248:20 | SOURCE() |
5756
| tst.js:352:14:352:14 | v | tst.js:248:13:248:20 | SOURCE() |
5857
| tst.js:359:14:359:14 | v | tst.js:248:13:248:20 | SOURCE() |
5958
| tst.js:368:10:368:12 | o.p | tst.js:367:13:367:20 | SOURCE() |

‎javascript/ql/test/library-tests/TaintBarriers/isBarrier.expected‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@
3737
| tst.js:265:14:265:14 | v | ExampleConfiguration |
3838
| tst.js:284:14:284:14 | v | ExampleConfiguration |
3939
| tst.js:331:14:331:14 | v | ExampleConfiguration |
40+
| tst.js:350:14:350:14 | v | ExampleConfiguration |
4041
| tst.js:356:16:356:27 | x10 | ExampleConfiguration |
4142
| tst.js:356:32:356:34 | x10 | ExampleConfiguration |
4243
| tst.js:361:14:361:14 | v | ExampleConfiguration |

‎javascript/ql/test/library-tests/TaintBarriers/tst.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -347,7 +347,7 @@ function IndirectSanitizer () {
347347
return unknown() && whitelist.contains(x9) && unknown();
348348
}
349349
if (f9(v)) {
350-
SINK(v); // SANITIZATION OF THIS IS NOT YET SUPPORTED
350+
SINK(v);
351351
} else {
352352
SINK(v);
353353
}

‎javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,10 +69,14 @@ typeInferenceMismatch
6969
| promise.js:5:25:5:32 | source() | promise.js:5:8:5:33 | bluebir ... urce()) |
7070
| promise.js:10:24:10:31 | source() | promise.js:10:8:10:32 | Promise ... urce()) |
7171
| promise.js:12:20:12:27 | source() | promise.js:13:8:13:23 | resolver.promise |
72+
| sanitizer-function.js:12:17:12:24 | source() | sanitizer-function.js:14:10:14:14 | taint |
73+
| sanitizer-function.js:12:17:12:24 | source() | sanitizer-function.js:33:14:33:18 | taint |
7274
| sanitizer-guards.js:2:11:2:18 | source() | sanitizer-guards.js:4:8:4:8 | x |
7375
| sanitizer-guards.js:13:14:13:21 | source() | sanitizer-guards.js:15:10:15:15 | this.x |
7476
| sanitizer-guards.js:13:14:13:21 | source() | sanitizer-guards.js:21:14:21:19 | this.x |
7577
| sanitizer-guards.js:13:14:13:21 | source() | sanitizer-guards.js:26:9:26:14 | this.x |
78+
| sanitizer-guards.js:43:11:43:18 | source() | sanitizer-guards.js:45:8:45:8 | x |
79+
| sanitizer-guards.js:43:11:43:18 | source() | sanitizer-guards.js:48:10:48:10 | x |
7680
| spread.js:2:15:2:22 | source() | spread.js:4:8:4:19 | { ...taint } |
7781
| spread.js:2:15:2:22 | source() | spread.js:5:8:5:43 | { f: 'h ... orld' } |
7882
| spread.js:2:15:2:22 | source() | spread.js:7:8:7:19 | [ ...taint ] |

0 commit comments

Comments
 (0)