Skip to content

Commit 927c322

Browse files
erik-krogherik-krogh
authored
Merge pull request github#11769 from erik-krogh/moreSan
JS: Sanitizer for `sanitizer(x) === true`
2 parents 332b759 + 943bdec commit 927c322

File tree

5 files changed

+192
-15
lines changed

5 files changed

+192
-15
lines changed

‎javascript/ql/lib/semmle/javascript/Expr.qll

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -379,7 +379,10 @@ class NullLiteral extends @null_literal, Literal { }
379379
* false
380380
* ```
381381
*/
382-
class BooleanLiteral extends @boolean_literal, Literal { }
382+
class BooleanLiteral extends @boolean_literal, Literal {
383+
/** Gets the value of this literal. */
384+
boolean getBoolValue() { if this.getRawValue() = "true" then result = true else result = false }
385+
}
383386

384387
/**
385388
* A numeric literal.

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

Lines changed: 52 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -161,7 +161,7 @@ abstract class Configuration extends string {
161161
*/
162162
predicate isBarrier(DataFlow::Node node) {
163163
exists(BarrierGuardNode guard |
164-
isBarrierGuardInternal(guard) and
164+
isBarrierGuardInternal(this, guard) and
165165
barrierGuardBlocksNode(guard, node, "")
166166
)
167167
}
@@ -181,7 +181,7 @@ abstract class Configuration extends string {
181181
*/
182182
predicate isLabeledBarrier(DataFlow::Node node, FlowLabel lbl) {
183183
exists(BarrierGuardNode guard |
184-
isBarrierGuardInternal(guard) and
184+
isBarrierGuardInternal(this, guard) and
185185
barrierGuardBlocksNode(guard, node, lbl)
186186
)
187187
or
@@ -198,17 +198,6 @@ abstract class Configuration extends string {
198198
*/
199199
predicate isBarrierGuard(BarrierGuardNode guard) { none() }
200200

201-
/**
202-
* Holds if `guard` is a barrier guard for this configuration, added through
203-
* `isBarrierGuard` or `AdditionalBarrierGuardNode`.
204-
*/
205-
pragma[nomagic]
206-
private predicate isBarrierGuardInternal(BarrierGuardNode guard) {
207-
isBarrierGuard(guard)
208-
or
209-
guard.(AdditionalBarrierGuardNode).appliesTo(this)
210-
}
211-
212201
/**
213202
* Holds if data may flow from `source` to `sink` for this configuration.
214203
*/
@@ -267,6 +256,17 @@ abstract class Configuration extends string {
267256
}
268257
}
269258

259+
/**
260+
* Holds if `guard` is a barrier guard for this configuration, added through
261+
* `isBarrierGuard` or `AdditionalBarrierGuardNode`.
262+
*/
263+
pragma[nomagic]
264+
private predicate isBarrierGuardInternal(Configuration cfg, BarrierGuardNode guard) {
265+
cfg.isBarrierGuard(guard)
266+
or
267+
guard.(AdditionalBarrierGuardNode).appliesTo(cfg)
268+
}
269+
270270
/**
271271
* A label describing the kind of information tracked by a flow configuration.
272272
*
@@ -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
/**
@@ -1979,7 +1981,7 @@ private class BarrierGuardFunction extends Function {
19791981
/**
19801982
* Holds if this function applies to the flow in `cfg`.
19811983
*/
1982-
predicate appliesTo(Configuration cfg) { cfg.isBarrierGuard(guard) }
1984+
predicate appliesTo(Configuration cfg) { isBarrierGuardInternal(cfg, guard) }
19831985
}
19841986

19851987
/**
@@ -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) { isBarrierGuardInternal(cfg, 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: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -301,6 +301,24 @@ 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 |
314+
| lib/lib.js:608:42:608:45 | name |
315+
| lib/lib.js:608:42:608:45 | name |
316+
| lib/lib.js:609:22:609:25 | name |
317+
| lib/lib.js:609:22:609:25 | name |
318+
| lib/lib.js:626:29:626:32 | name |
319+
| lib/lib.js:626:29:626:32 | name |
320+
| lib/lib.js:629:25:629:28 | name |
321+
| lib/lib.js:629:25:629:28 | name |
304322
| lib/subLib2/compiled-file.ts:3:26:3:29 | name |
305323
| lib/subLib2/compiled-file.ts:3:26:3:29 | name |
306324
| lib/subLib2/compiled-file.ts:4:25:4:28 | name |
@@ -703,6 +721,34 @@ edges
703721
| lib/lib.js:558:41:558:44 | name | lib/lib.js:566:26:566:29 | name |
704722
| lib/lib.js:558:41:558:44 | name | lib/lib.js:566:26:566:29 | name |
705723
| lib/lib.js:558:41:558:44 | name | lib/lib.js:566:26:566:29 | name |
724+
| lib/lib.js:572:41:572:44 | name | lib/lib.js:573:22:573:25 | name |
725+
| lib/lib.js:572:41:572:44 | name | lib/lib.js:573:22:573:25 | name |
726+
| lib/lib.js:572:41:572:44 | name | lib/lib.js:573:22:573:25 | name |
727+
| lib/lib.js:572:41:572:44 | name | lib/lib.js:573:22:573:25 | name |
728+
| lib/lib.js:572:41:572:44 | name | lib/lib.js:579:25:579:28 | name |
729+
| lib/lib.js:572:41:572:44 | name | lib/lib.js:579:25:579:28 | name |
730+
| lib/lib.js:572:41:572:44 | name | lib/lib.js:579:25:579:28 | name |
731+
| lib/lib.js:572:41:572:44 | name | lib/lib.js:579:25:579:28 | name |
732+
| lib/lib.js:572:41:572:44 | name | lib/lib.js:590:29:590:32 | name |
733+
| lib/lib.js:572:41:572:44 | name | lib/lib.js:590:29:590:32 | name |
734+
| lib/lib.js:572:41:572:44 | name | lib/lib.js:590:29:590:32 | name |
735+
| lib/lib.js:572:41:572:44 | name | lib/lib.js:590:29:590:32 | name |
736+
| lib/lib.js:572:41:572:44 | name | lib/lib.js:593:25:593:28 | name |
737+
| lib/lib.js:572:41:572:44 | name | lib/lib.js:593:25:593:28 | name |
738+
| lib/lib.js:572:41:572:44 | name | lib/lib.js:593:25:593:28 | name |
739+
| lib/lib.js:572:41:572:44 | name | lib/lib.js:593:25:593:28 | name |
740+
| lib/lib.js:608:42:608:45 | name | lib/lib.js:609:22:609:25 | name |
741+
| lib/lib.js:608:42:608:45 | name | lib/lib.js:609:22:609:25 | name |
742+
| lib/lib.js:608:42:608:45 | name | lib/lib.js:609:22:609:25 | name |
743+
| lib/lib.js:608:42:608:45 | name | lib/lib.js:609:22:609:25 | name |
744+
| lib/lib.js:608:42:608:45 | name | lib/lib.js:626:29:626:32 | name |
745+
| lib/lib.js:608:42:608:45 | name | lib/lib.js:626:29:626:32 | name |
746+
| lib/lib.js:608:42:608:45 | name | lib/lib.js:626:29:626:32 | name |
747+
| lib/lib.js:608:42:608:45 | name | lib/lib.js:626:29:626:32 | name |
748+
| lib/lib.js:608:42:608:45 | name | lib/lib.js:629:25:629:28 | name |
749+
| lib/lib.js:608:42:608:45 | name | lib/lib.js:629:25:629:28 | name |
750+
| lib/lib.js:608:42:608:45 | name | lib/lib.js:629:25:629:28 | name |
751+
| lib/lib.js:608:42:608:45 | name | lib/lib.js:629:25:629:28 | name |
706752
| lib/subLib2/compiled-file.ts:3:26:3:29 | name | lib/subLib2/compiled-file.ts:4:25:4:28 | name |
707753
| lib/subLib2/compiled-file.ts:3:26:3:29 | name | lib/subLib2/compiled-file.ts:4:25:4:28 | name |
708754
| lib/subLib2/compiled-file.ts:3:26:3:29 | name | lib/subLib2/compiled-file.ts:4:25:4:28 | name |
@@ -826,6 +872,13 @@ edges
826872
| 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 |
827873
| 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 |
828874
| 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 |
875+
| 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 |
876+
| 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 |
877+
| 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 |
878+
| 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 |
879+
| lib/lib.js:609:10:609:25 | "rm -rf " + name | lib/lib.js:608:42:608:45 | name | lib/lib.js:609:22:609:25 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:608:42:608:45 | name | library input | lib/lib.js:609:2:609:26 | cp.exec ... + name) | shell command |
880+
| lib/lib.js:626:17:626:32 | "rm -rf " + name | lib/lib.js:608:42:608:45 | name | lib/lib.js:626:29:626:32 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:608:42:608:45 | name | library input | lib/lib.js:626:9:626:33 | cp.exec ... + name) | shell command |
881+
| lib/lib.js:629:13:629:28 | "rm -rf " + name | lib/lib.js:608:42:608:45 | name | lib/lib.js:629:25:629:28 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:608:42:608:45 | name | library input | lib/lib.js:629:5:629:29 | cp.exec ... + name) | shell command |
829882
| 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 |
830883
| 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 |
831884
| 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: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -568,3 +568,63 @@ 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+
}
595+
596+
function indirectThing(name) {
597+
return isSafeName(name);
598+
}
599+
600+
function indirectThing2(name) {
601+
return isSafeName(name) === true;
602+
}
603+
604+
function moreIndirect(name) {
605+
return indirectThing2(name) !== false;
606+
}
607+
608+
module.exports.veryIndeirect = function (name) {
609+
cp.exec("rm -rf " + name); // NOT OK
610+
611+
if (indirectThing(name)) {
612+
cp.exec("rm -rf " + name); // OK
613+
}
614+
615+
if (indirectThing2(name)) {
616+
cp.exec("rm -rf " + name); // OK
617+
}
618+
619+
if (moreIndirect(name)) {
620+
cp.exec("rm -rf " + name); // OK
621+
}
622+
623+
if (moreIndirect(name) !== false) {
624+
cp.exec("rm -rf " + name); // OK
625+
} else {
626+
cp.exec("rm -rf " + name); // NOT OK
627+
}
628+
629+
cp.exec("rm -rf " + name); // NOT OK
630+
}

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

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
const Router = require('koa-router')
2+
const {Sequelize} = require("sequelize");
3+
4+
new Router().get("/hello", (ctx) => {
5+
const { version } = ctx.query;
6+
7+
if (version && validVersion(version) === false) {
8+
throw new Error(`invalid version ${version}`);
9+
}
10+
11+
const conditions = ['1'];
12+
13+
if (version) {
14+
conditions.push(`version = ${version}`)
15+
}
16+
17+
new Sequelize().query(`SELECT * FROM t WHERE ${conditions.join(' and ')}`, null); // OK
18+
});
19+
20+
function validVersion(version) {
21+
const pattern = /^[a-zA-Z0-9]+$/;
22+
return pattern.test(version);
23+
}

0 commit comments

Comments
 (0)