Skip to content

Commit 9549cac

Browse files
erik-krogherik-krogh
committed
add an additional barrier guard that finds "=== true" versions of previous barrier guards
1 parent c355a26 commit 9549cac

File tree

5 files changed

+93
-23
lines changed

5 files changed

+93
-23
lines changed

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

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -368,6 +368,8 @@ private predicate barrierGuardBlocksExpr(
368368
// Handle labelled barrier guard functions specially, to avoid negative recursion
369369
// through the non-abstract 3-argument version of blocks().
370370
guard.(AdditionalBarrierGuardCall).internalBlocksLabel(outcome, test, label)
371+
or
372+
guard.(CallAgainstEqualityCheck).internalBlocksLabel(outcome, test, label)
371373
}
372374

373375
/**
@@ -1999,6 +2001,42 @@ private class AdditionalBarrierGuardCall extends AdditionalBarrierGuardNode, Dat
19992001
override predicate appliesTo(Configuration cfg) { f.appliesTo(cfg) }
20002002
}
20012003

2004+
/**
2005+
* A sanitizer where an inner sanitizer is compared against a boolean.
2006+
* E.g. (assuming `sanitizes(e)` is an existing sanitizer):
2007+
* ```javascript
2008+
* if (sanitizes(e) === true) {
2009+
* // e is sanitized
2010+
* }
2011+
* ```
2012+
*/
2013+
private class CallAgainstEqualityCheck extends AdditionalBarrierGuardNode {
2014+
DataFlow::BarrierGuardNode prev;
2015+
boolean polarity;
2016+
2017+
CallAgainstEqualityCheck() {
2018+
prev instanceof DataFlow::CallNode and
2019+
exists(EqualityTest test, BooleanLiteral bool |
2020+
this.asExpr() = test and
2021+
test.hasOperands(prev.asExpr(), bool) and
2022+
polarity = test.getPolarity().booleanXor(bool.getBoolValue())
2023+
)
2024+
}
2025+
2026+
override predicate blocks(boolean outcome, Expr e) {
2027+
none() // handled by internalBlocksLabel
2028+
}
2029+
2030+
predicate internalBlocksLabel(boolean outcome, Expr e, DataFlow::FlowLabel lbl) {
2031+
exists(boolean prevOutcome |
2032+
barrierGuardBlocksExpr(prev, prevOutcome, e, lbl) and
2033+
outcome = prevOutcome.booleanXor(polarity)
2034+
)
2035+
}
2036+
2037+
override predicate appliesTo(Configuration cfg) { cfg.isBarrierGuard(prev) }
2038+
}
2039+
20022040
/**
20032041
* A guard node for a variable in a negative condition, such as `x` in `if(!x)`.
20042042
* Can be added to a `isBarrier` in a data-flow configuration to block flow through such checks.

‎javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -301,6 +301,16 @@ nodes
301301
| lib/lib.js:562:26:562:29 | name |
302302
| lib/lib.js:566:26:566:29 | name |
303303
| lib/lib.js:566:26:566:29 | name |
304+
| lib/lib.js:572:41:572:44 | name |
305+
| lib/lib.js:572:41:572:44 | name |
306+
| lib/lib.js:573:22:573:25 | name |
307+
| lib/lib.js:573:22:573:25 | name |
308+
| lib/lib.js:579:25:579:28 | name |
309+
| lib/lib.js:579:25:579:28 | name |
310+
| lib/lib.js:590:29:590:32 | name |
311+
| lib/lib.js:590:29:590:32 | name |
312+
| lib/lib.js:593:25:593:28 | name |
313+
| lib/lib.js:593:25:593:28 | name |
304314
| lib/subLib2/compiled-file.ts:3:26:3:29 | name |
305315
| lib/subLib2/compiled-file.ts:3:26:3:29 | name |
306316
| lib/subLib2/compiled-file.ts:4:25:4:28 | name |
@@ -703,6 +713,22 @@ edges
703713
| lib/lib.js:558:41:558:44 | name | lib/lib.js:566:26:566:29 | name |
704714
| lib/lib.js:558:41:558:44 | name | lib/lib.js:566:26:566:29 | name |
705715
| lib/lib.js:558:41:558:44 | name | lib/lib.js:566:26:566:29 | name |
716+
| lib/lib.js:572:41:572:44 | name | lib/lib.js:573:22:573:25 | name |
717+
| lib/lib.js:572:41:572:44 | name | lib/lib.js:573:22:573:25 | name |
718+
| lib/lib.js:572:41:572:44 | name | lib/lib.js:573:22:573:25 | name |
719+
| lib/lib.js:572:41:572:44 | name | lib/lib.js:573:22:573:25 | name |
720+
| lib/lib.js:572:41:572:44 | name | lib/lib.js:579:25:579:28 | name |
721+
| lib/lib.js:572:41:572:44 | name | lib/lib.js:579:25:579:28 | name |
722+
| lib/lib.js:572:41:572:44 | name | lib/lib.js:579:25:579:28 | name |
723+
| lib/lib.js:572:41:572:44 | name | lib/lib.js:579:25:579:28 | name |
724+
| lib/lib.js:572:41:572:44 | name | lib/lib.js:590:29:590:32 | name |
725+
| lib/lib.js:572:41:572:44 | name | lib/lib.js:590:29:590:32 | name |
726+
| lib/lib.js:572:41:572:44 | name | lib/lib.js:590:29:590:32 | name |
727+
| lib/lib.js:572:41:572:44 | name | lib/lib.js:590:29:590:32 | name |
728+
| lib/lib.js:572:41:572:44 | name | lib/lib.js:593:25:593:28 | name |
729+
| lib/lib.js:572:41:572:44 | name | lib/lib.js:593:25:593:28 | name |
730+
| lib/lib.js:572:41:572:44 | name | lib/lib.js:593:25:593:28 | name |
731+
| lib/lib.js:572:41:572:44 | name | lib/lib.js:593:25:593:28 | name |
706732
| lib/subLib2/compiled-file.ts:3:26:3:29 | name | lib/subLib2/compiled-file.ts:4:25:4:28 | name |
707733
| lib/subLib2/compiled-file.ts:3:26:3:29 | name | lib/subLib2/compiled-file.ts:4:25:4:28 | name |
708734
| lib/subLib2/compiled-file.ts:3:26:3:29 | name | lib/subLib2/compiled-file.ts:4:25:4:28 | name |
@@ -826,6 +852,10 @@ edges
826852
| lib/lib.js:560:14:560:29 | "rm -rf " + name | lib/lib.js:558:41:558:44 | name | lib/lib.js:560:26:560:29 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:558:41:558:44 | name | library input | lib/lib.js:560:9:560:30 | exec("r ... + name) | shell command |
827853
| lib/lib.js:562:14:562:29 | "rm -rf " + name | lib/lib.js:558:41:558:44 | name | lib/lib.js:562:26:562:29 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:558:41:558:44 | name | library input | lib/lib.js:562:9:562:30 | exec("r ... + name) | shell command |
828854
| lib/lib.js:566:14:566:29 | "rm -rf " + name | lib/lib.js:558:41:558:44 | name | lib/lib.js:566:26:566:29 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:558:41:558:44 | name | library input | lib/lib.js:566:9:566:30 | exec("r ... + name) | shell command |
855+
| lib/lib.js:573:10:573:25 | "rm -rf " + name | lib/lib.js:572:41:572:44 | name | lib/lib.js:573:22:573:25 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:572:41:572:44 | name | library input | lib/lib.js:573:2:573:26 | cp.exec ... + name) | shell command |
856+
| lib/lib.js:579:13:579:28 | "rm -rf " + name | lib/lib.js:572:41:572:44 | name | lib/lib.js:579:25:579:28 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:572:41:572:44 | name | library input | lib/lib.js:579:5:579:29 | cp.exec ... + name) | shell command |
857+
| lib/lib.js:590:17:590:32 | "rm -rf " + name | lib/lib.js:572:41:572:44 | name | lib/lib.js:590:29:590:32 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:572:41:572:44 | name | library input | lib/lib.js:590:9:590:33 | cp.exec ... + name) | shell command |
858+
| lib/lib.js:593:13:593:28 | "rm -rf " + name | lib/lib.js:572:41:572:44 | name | lib/lib.js:593:25:593:28 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:572:41:572:44 | name | library input | lib/lib.js:593:5:593:29 | cp.exec ... + name) | shell command |
829859
| lib/subLib2/compiled-file.ts:4:13:4:28 | "rm -rf " + name | lib/subLib2/compiled-file.ts:3:26:3:29 | name | lib/subLib2/compiled-file.ts:4:25:4:28 | name | This string concatenation which depends on $@ is later used in a $@. | lib/subLib2/compiled-file.ts:3:26:3:29 | name | library input | lib/subLib2/compiled-file.ts:4:5:4:29 | cp.exec ... + name) | shell command |
830860
| lib/subLib2/special-file.js:4:10:4:25 | "rm -rf " + name | lib/subLib2/special-file.js:3:28:3:31 | name | lib/subLib2/special-file.js:4:22:4:25 | name | This string concatenation which depends on $@ is later used in a $@. | lib/subLib2/special-file.js:3:28:3:31 | name | library input | lib/subLib2/special-file.js:4:2:4:26 | cp.exec ... + name) | shell command |
831861
| lib/subLib3/my-file.ts:4:10:4:25 | "rm -rf " + name | lib/subLib3/my-file.ts:3:28:3:31 | name | lib/subLib3/my-file.ts:4:22:4:25 | name | This string concatenation which depends on $@ is later used in a $@. | lib/subLib3/my-file.ts:3:28:3:31 | name | library input | lib/subLib3/my-file.ts:4:2:4:26 | cp.exec ... + name) | shell command |

‎javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction/lib/lib.js

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -568,3 +568,27 @@ module.exports.badSanitizer = function (name) {
568568
exec("rm -rf " + name); // OK
569569
}
570570
}
571+
572+
module.exports.safeWithBool = function (name) {
573+
cp.exec("rm -rf " + name); // NOT OK
574+
575+
if (isSafeName(name)) {
576+
cp.exec("rm -rf " + name); // OK
577+
}
578+
579+
cp.exec("rm -rf " + name); // NOT OK
580+
581+
if (isSafeName(name) === true) {
582+
cp.exec("rm -rf " + name); // OK
583+
}
584+
585+
if (isSafeName(name) !== false) {
586+
cp.exec("rm -rf " + name); // OK
587+
}
588+
589+
if (isSafeName(name) == false) {
590+
cp.exec("rm -rf " + name); // NOT OK
591+
}
592+
593+
cp.exec("rm -rf " + name); // NOT OK
594+
}

‎javascript/ql/test/query-tests/Security/CWE-089/untyped/SqlInjection.expected

Lines changed: 0 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -68,17 +68,6 @@ nodes
6868
| json-schema-validator.js:59:22:59:26 | query |
6969
| json-schema-validator.js:61:22:61:26 | query |
7070
| json-schema-validator.js:61:22:61:26 | query |
71-
| koarouter.js:5:11:5:33 | version |
72-
| koarouter.js:5:13:5:19 | version |
73-
| koarouter.js:5:13:5:19 | version |
74-
| koarouter.js:11:11:11:28 | conditions |
75-
| koarouter.js:11:24:11:28 | ['1'] |
76-
| koarouter.js:14:25:14:46 | `versio ... rsion}` |
77-
| koarouter.js:14:38:14:44 | version |
78-
| koarouter.js:17:27:17:77 | `SELECT ... nd ')}` |
79-
| koarouter.js:17:27:17:77 | `SELECT ... nd ')}` |
80-
| koarouter.js:17:52:17:61 | conditions |
81-
| koarouter.js:17:52:17:75 | conditi ... and ') |
8271
| ldap.js:20:7:20:34 | q |
8372
| ldap.js:20:11:20:34 | url.par ... , true) |
8473
| ldap.js:20:21:20:27 | req.url |
@@ -493,16 +482,6 @@ edges
493482
| json-schema-validator.js:50:23:50:48 | JSON.pa ... y.data) | json-schema-validator.js:50:15:50:48 | query |
494483
| json-schema-validator.js:50:34:50:47 | req.query.data | json-schema-validator.js:50:23:50:48 | JSON.pa ... y.data) |
495484
| json-schema-validator.js:50:34:50:47 | req.query.data | json-schema-validator.js:50:23:50:48 | JSON.pa ... y.data) |
496-
| koarouter.js:5:11:5:33 | version | koarouter.js:14:38:14:44 | version |
497-
| koarouter.js:5:13:5:19 | version | koarouter.js:5:11:5:33 | version |
498-
| koarouter.js:5:13:5:19 | version | koarouter.js:5:11:5:33 | version |
499-
| koarouter.js:11:11:11:28 | conditions | koarouter.js:17:52:17:61 | conditions |
500-
| koarouter.js:11:24:11:28 | ['1'] | koarouter.js:11:11:11:28 | conditions |
501-
| koarouter.js:14:25:14:46 | `versio ... rsion}` | koarouter.js:11:24:11:28 | ['1'] |
502-
| koarouter.js:14:38:14:44 | version | koarouter.js:14:25:14:46 | `versio ... rsion}` |
503-
| koarouter.js:17:52:17:61 | conditions | koarouter.js:17:52:17:75 | conditi ... and ') |
504-
| koarouter.js:17:52:17:75 | conditi ... and ') | koarouter.js:17:27:17:77 | `SELECT ... nd ')}` |
505-
| koarouter.js:17:52:17:75 | conditi ... and ') | koarouter.js:17:27:17:77 | `SELECT ... nd ')}` |
506485
| ldap.js:20:7:20:34 | q | ldap.js:22:18:22:18 | q |
507486
| ldap.js:20:11:20:34 | url.par ... , true) | ldap.js:20:7:20:34 | q |
508487
| ldap.js:20:21:20:27 | req.url | ldap.js:20:11:20:34 | url.par ... , true) |
@@ -950,7 +929,6 @@ edges
950929
| json-schema-validator.js:55:22:55:26 | query | json-schema-validator.js:50:34:50:47 | req.query.data | json-schema-validator.js:55:22:55:26 | query | This query depends on a $@. | json-schema-validator.js:50:34:50:47 | req.query.data | user-provided value |
951930
| json-schema-validator.js:59:22:59:26 | query | json-schema-validator.js:50:34:50:47 | req.query.data | json-schema-validator.js:59:22:59:26 | query | This query depends on a $@. | json-schema-validator.js:50:34:50:47 | req.query.data | user-provided value |
952931
| json-schema-validator.js:61:22:61:26 | query | json-schema-validator.js:50:34:50:47 | req.query.data | json-schema-validator.js:61:22:61:26 | query | This query depends on a $@. | json-schema-validator.js:50:34:50:47 | req.query.data | user-provided value |
953-
| koarouter.js:17:27:17:77 | `SELECT ... nd ')}` | koarouter.js:5:13:5:19 | version | koarouter.js:17:27:17:77 | `SELECT ... nd ')}` | This query depends on a $@. | koarouter.js:5:13:5:19 | version | user-provided value |
954932
| ldap.js:28:30:28:34 | opts1 | ldap.js:20:21:20:27 | req.url | ldap.js:28:30:28:34 | opts1 | This query depends on a $@. | ldap.js:20:21:20:27 | req.url | user-provided value |
955933
| ldap.js:32:5:32:61 | { filte ... e}))` } | ldap.js:20:21:20:27 | req.url | ldap.js:32:5:32:61 | { filte ... e}))` } | This query depends on a $@. | ldap.js:20:21:20:27 | req.url | user-provided value |
956934
| ldap.js:66:30:66:53 | { filte ... ilter } | ldap.js:20:21:20:27 | req.url | ldap.js:66:30:66:53 | { filte ... ilter } | This query depends on a $@. | ldap.js:20:21:20:27 | req.url | user-provided value |

‎javascript/ql/test/query-tests/Security/CWE-089/untyped/koarouter.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ new Router().get("/hello", (ctx) => {
1414
conditions.push(`version = ${version}`)
1515
}
1616

17-
new Sequelize().query(`SELECT * FROM t WHERE ${conditions.join(' and ')}`, null); // OK - but still flagged [INCONSISTENCY]
17+
new Sequelize().query(`SELECT * FROM t WHERE ${conditions.join(' and ')}`, null); // OK
1818
});
1919

2020
function validVersion(version) {

0 commit comments

Comments
 (0)