Skip to content

Commit 3791b15

Browse files
erik-krogherik-krogh
authored
Merge pull request #7892 from erik-krogh/nanSan
JS: Add a `isNaN` sanitizer, and use it in queries that already had a typeof check
2 parents 2ffd79d + eb56a5a commit 3791b15

12 files changed

+191
-3
lines changed

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

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1166,6 +1166,36 @@ module TaintTracking {
11661166
)
11671167
}
11681168

1169+
/** Holds if `guard` is a test that checks if `operand` is a number. */
1170+
predicate isNumberGuard(DataFlow::Node guard, Expr operand, boolean polarity) {
1171+
exists(DataFlow::CallNode isNaN |
1172+
isNaN = DataFlow::globalVarRef("isNaN").getACall() and guard = isNaN and polarity = false
1173+
|
1174+
operand = isNaN.getArgument(0).asExpr()
1175+
or
1176+
exists(DataFlow::CallNode parse |
1177+
parse = DataFlow::globalVarRef(["parseInt", "parseFloat"]).getACall()
1178+
|
1179+
parse = isNaN.getArgument(0) and
1180+
operand = parse.getArgument(0).asExpr()
1181+
)
1182+
or
1183+
exists(UnaryExpr unary | unary.getOperator() = ["+", "-"] |
1184+
unary = isNaN.getArgument(0).asExpr() and
1185+
operand = unary.getOperand()
1186+
)
1187+
or
1188+
exists(BinaryExpr bin | bin.getOperator() = ["+", "-"] |
1189+
bin = isNaN.getArgument(0).asExpr() and
1190+
operand = bin.getAnOperand() and
1191+
bin.getAnOperand() instanceof NumberLiteral
1192+
)
1193+
)
1194+
or
1195+
isTypeofGuard(guard.asExpr(), operand, "number") and
1196+
polarity = guard.asExpr().(EqualityTest).getPolarity()
1197+
}
1198+
11691199
/** DEPRECATED. This class has been renamed to `MembershipTestSanitizer`. */
11701200
deprecated class StringInclusionSanitizer = MembershipTestSanitizer;
11711201

‎javascript/ql/lib/semmle/javascript/security/TaintedObject.qll

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -110,6 +110,16 @@ module TaintedObject {
110110
}
111111
}
112112

113+
/** A guard that checks whether `x` is a number. */
114+
class NumberGuard extends SanitizerGuard instanceof DataFlow::CallNode {
115+
Expr x;
116+
boolean polarity;
117+
118+
NumberGuard() { TaintTracking::isNumberGuard(this, x, polarity) }
119+
120+
override predicate sanitizes(boolean outcome, Expr e) { e = x and outcome = polarity }
121+
}
122+
113123
/**
114124
* A sanitizer guard that validates an input against a JSON schema.
115125
*/

‎javascript/ql/lib/semmle/javascript/security/dataflow/PrototypePollutingAssignmentQuery.qll

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -122,6 +122,7 @@ class Configuration extends TaintTracking::Configuration {
122122
guard instanceof InstanceofCheck or
123123
guard instanceof IsArrayCheck or
124124
guard instanceof TypeofCheck or
125+
guard instanceof NumberGuard or
125126
guard instanceof EqualityCheck or
126127
guard instanceof IncludesCheck
127128
}
@@ -228,6 +229,16 @@ private class TypeofCheck extends TaintTracking::LabeledSanitizerGuardNode, Data
228229
}
229230
}
230231

232+
/** A guard that checks whether `x` is a number. */
233+
class NumberGuard extends TaintTracking::SanitizerGuardNode instanceof DataFlow::CallNode {
234+
Expr x;
235+
boolean polarity;
236+
237+
NumberGuard() { TaintTracking::isNumberGuard(this, x, polarity) }
238+
239+
override predicate sanitizes(boolean outcome, Expr e) { e = x and outcome = polarity }
240+
}
241+
231242
/** A call to `Array.isArray`, which is false for `Object.prototype`. */
232243
private class IsArrayCheck extends TaintTracking::LabeledSanitizerGuardNode, DataFlow::CallNode {
233244
IsArrayCheck() { this = DataFlow::globalVarRef("Array").getAMemberCall("isArray") }

‎javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeJQueryPluginCustomizations.qll

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -153,6 +153,16 @@ module UnsafeJQueryPlugin {
153153
}
154154
}
155155

156+
/** A guard that checks whether `x` is a number. */
157+
class NumberGuard extends TaintTracking::SanitizerGuardNode instanceof DataFlow::CallNode {
158+
Expr x;
159+
boolean polarity;
160+
161+
NumberGuard() { TaintTracking::isNumberGuard(this, x, polarity) }
162+
163+
override predicate sanitizes(boolean outcome, Expr e) { e = x and outcome = polarity }
164+
}
165+
156166
/**
157167
* The client-provided options object for a jQuery plugin, considered as a source for unsafe jQuery plugins.
158168
*/

‎javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeJQueryPluginQuery.qll

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,8 @@ class Configuration extends TaintTracking::Configuration {
4646
override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode node) {
4747
super.isSanitizerGuard(node) or
4848
node instanceof IsElementSanitizer or
49-
node instanceof PropertyPresenceSanitizer
49+
node instanceof PropertyPresenceSanitizer or
50+
node instanceof NumberGuard
5051
}
5152
}
5253

‎javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -286,6 +286,16 @@ module UnsafeShellCommandConstruction {
286286
}
287287
}
288288

289+
/** A guard that checks whether `x` is a number. */
290+
class NumberGuard extends TaintTracking::SanitizerGuardNode instanceof DataFlow::CallNode {
291+
Expr x;
292+
boolean polarity;
293+
294+
NumberGuard() { TaintTracking::isNumberGuard(this, x, polarity) }
295+
296+
override predicate sanitizes(boolean outcome, Expr e) { e = x and outcome = polarity }
297+
}
298+
289299
private import semmle.javascript.dataflow.internal.AccessPaths
290300
private import semmle.javascript.dataflow.InferredTypes
291301

‎javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeShellCommandConstructionQuery.qll

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,9 @@ class Configuration extends TaintTracking::Configuration {
2424

2525
override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode guard) {
2626
guard instanceof PathExistsSanitizerGuard or
27-
guard instanceof TaintTracking::AdHocWhitelistCheckSanitizer
27+
guard instanceof TaintTracking::AdHocWhitelistCheckSanitizer or
28+
guard instanceof NumberGuard or
29+
guard instanceof TypeOfSanitizer
2830
}
2931

3032
// override to require that there is a path without unmatched return steps

‎javascript/ql/lib/semmle/javascript/security/dataflow/UnvalidatedDynamicMethodCallCustomizations.qll

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,4 +108,14 @@ module UnvalidatedDynamicMethodCall {
108108
label instanceof MaybeNonFunction
109109
}
110110
}
111+
112+
/** A guard that checks whether `x` is a number. */
113+
class NumberGuard extends TaintTracking::SanitizerGuardNode instanceof DataFlow::CallNode {
114+
Expr x;
115+
boolean polarity;
116+
117+
NumberGuard() { TaintTracking::isNumberGuard(this, x, polarity) }
118+
119+
override predicate sanitizes(boolean outcome, Expr e) { e = x and outcome = polarity }
120+
}
111121
}

‎javascript/ql/lib/semmle/javascript/security/dataflow/UnvalidatedDynamicMethodCallQuery.qll

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,11 @@ class Configuration extends TaintTracking::Configuration {
4040

4141
override predicate isSanitizer(DataFlow::Node nd) { super.isSanitizer(nd) }
4242

43+
override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode guard) {
44+
guard instanceof NumberGuard or
45+
guard instanceof FunctionCheck
46+
}
47+
4348
override predicate isAdditionalFlowStep(
4449
DataFlow::Node src, DataFlow::Node dst, DataFlow::FlowLabel srclabel,
4550
DataFlow::FlowLabel dstlabel

‎javascript/ql/lib/semmle/javascript/security/dataflow/XssThroughDomQuery.qll

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ class Configuration extends TaintTracking::Configuration {
2828
override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode guard) {
2929
guard instanceof TypeTestGuard or
3030
guard instanceof UnsafeJQuery::PropertyPresenceSanitizer or
31+
guard instanceof UnsafeJQuery::NumberGuard or
3132
guard instanceof DomBasedXss::SanitizerGuard
3233
}
3334

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

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -254,6 +254,24 @@ nodes
254254
| lib/lib.js:498:45:498:48 | name |
255255
| lib/lib.js:499:31:499:34 | name |
256256
| lib/lib.js:499:31:499:34 | name |
257+
| lib/lib.js:509:39:509:42 | name |
258+
| lib/lib.js:509:39:509:42 | name |
259+
| lib/lib.js:510:22:510:25 | name |
260+
| lib/lib.js:510:22:510:25 | name |
261+
| lib/lib.js:513:23:513:26 | name |
262+
| lib/lib.js:513:23:513:26 | name |
263+
| lib/lib.js:519:23:519:26 | name |
264+
| lib/lib.js:519:23:519:26 | name |
265+
| lib/lib.js:525:23:525:26 | name |
266+
| lib/lib.js:525:23:525:26 | name |
267+
| lib/lib.js:531:23:531:26 | name |
268+
| lib/lib.js:531:23:531:26 | name |
269+
| lib/lib.js:537:23:537:26 | name |
270+
| lib/lib.js:537:23:537:26 | name |
271+
| lib/lib.js:543:23:543:26 | name |
272+
| lib/lib.js:543:23:543:26 | name |
273+
| lib/lib.js:545:23:545:26 | name |
274+
| lib/lib.js:545:23:545:26 | name |
257275
| lib/subLib2/compiled-file.ts:3:26:3:29 | name |
258276
| lib/subLib2/compiled-file.ts:3:26:3:29 | name |
259277
| lib/subLib2/compiled-file.ts:4:25:4:28 | name |
@@ -574,6 +592,38 @@ edges
574592
| lib/lib.js:498:45:498:48 | name | lib/lib.js:499:31:499:34 | name |
575593
| lib/lib.js:498:45:498:48 | name | lib/lib.js:499:31:499:34 | name |
576594
| lib/lib.js:498:45:498:48 | name | lib/lib.js:499:31:499:34 | name |
595+
| lib/lib.js:509:39:509:42 | name | lib/lib.js:510:22:510:25 | name |
596+
| lib/lib.js:509:39:509:42 | name | lib/lib.js:510:22:510:25 | name |
597+
| lib/lib.js:509:39:509:42 | name | lib/lib.js:510:22:510:25 | name |
598+
| lib/lib.js:509:39:509:42 | name | lib/lib.js:510:22:510:25 | name |
599+
| lib/lib.js:509:39:509:42 | name | lib/lib.js:513:23:513:26 | name |
600+
| lib/lib.js:509:39:509:42 | name | lib/lib.js:513:23:513:26 | name |
601+
| lib/lib.js:509:39:509:42 | name | lib/lib.js:513:23:513:26 | name |
602+
| lib/lib.js:509:39:509:42 | name | lib/lib.js:513:23:513:26 | name |
603+
| lib/lib.js:509:39:509:42 | name | lib/lib.js:519:23:519:26 | name |
604+
| lib/lib.js:509:39:509:42 | name | lib/lib.js:519:23:519:26 | name |
605+
| lib/lib.js:509:39:509:42 | name | lib/lib.js:519:23:519:26 | name |
606+
| lib/lib.js:509:39:509:42 | name | lib/lib.js:519:23:519:26 | name |
607+
| lib/lib.js:509:39:509:42 | name | lib/lib.js:525:23:525:26 | name |
608+
| lib/lib.js:509:39:509:42 | name | lib/lib.js:525:23:525:26 | name |
609+
| lib/lib.js:509:39:509:42 | name | lib/lib.js:525:23:525:26 | name |
610+
| lib/lib.js:509:39:509:42 | name | lib/lib.js:525:23:525:26 | name |
611+
| lib/lib.js:509:39:509:42 | name | lib/lib.js:531:23:531:26 | name |
612+
| lib/lib.js:509:39:509:42 | name | lib/lib.js:531:23:531:26 | name |
613+
| lib/lib.js:509:39:509:42 | name | lib/lib.js:531:23:531:26 | name |
614+
| lib/lib.js:509:39:509:42 | name | lib/lib.js:531:23:531:26 | name |
615+
| lib/lib.js:509:39:509:42 | name | lib/lib.js:537:23:537:26 | name |
616+
| lib/lib.js:509:39:509:42 | name | lib/lib.js:537:23:537:26 | name |
617+
| lib/lib.js:509:39:509:42 | name | lib/lib.js:537:23:537:26 | name |
618+
| lib/lib.js:509:39:509:42 | name | lib/lib.js:537:23:537:26 | name |
619+
| lib/lib.js:509:39:509:42 | name | lib/lib.js:543:23:543:26 | name |
620+
| lib/lib.js:509:39:509:42 | name | lib/lib.js:543:23:543:26 | name |
621+
| lib/lib.js:509:39:509:42 | name | lib/lib.js:543:23:543:26 | name |
622+
| lib/lib.js:509:39:509:42 | name | lib/lib.js:543:23:543:26 | name |
623+
| lib/lib.js:509:39:509:42 | name | lib/lib.js:545:23:545:26 | name |
624+
| lib/lib.js:509:39:509:42 | name | lib/lib.js:545:23:545:26 | name |
625+
| lib/lib.js:509:39:509:42 | name | lib/lib.js:545:23:545:26 | name |
626+
| lib/lib.js:509:39:509:42 | name | lib/lib.js:545:23:545:26 | name |
577627
| lib/subLib2/compiled-file.ts:3:26:3:29 | name | lib/subLib2/compiled-file.ts:4:25:4:28 | name |
578628
| lib/subLib2/compiled-file.ts:3:26:3:29 | name | lib/subLib2/compiled-file.ts:4:25:4:28 | name |
579629
| lib/subLib2/compiled-file.ts:3:26:3:29 | name | lib/subLib2/compiled-file.ts:4:25:4:28 | name |
@@ -666,6 +716,14 @@ edges
666716
| lib/lib.js:478:27:478:46 | config.installedPath | lib/lib.js:477:33:477:38 | config | lib/lib.js:478:27:478:46 | config.installedPath | $@ based on $@ is later used in $@. | lib/lib.js:478:27:478:46 | config.installedPath | Path concatenation | lib/lib.js:477:33:477:38 | config | library input | lib/lib.js:479:12:479:20 | exec(cmd) | shell command |
667717
| lib/lib.js:483:13:483:33 | ' my na ... + name | lib/lib.js:482:40:482:43 | name | lib/lib.js:483:30:483:33 | name | $@ based on $@ is later used in $@. | lib/lib.js:483:13:483:33 | ' my na ... + name | String concatenation | lib/lib.js:482:40:482:43 | name | library input | lib/lib.js:485:2:485:20 | cp.exec(cmd + args) | shell command |
668718
| lib/lib.js:499:19:499:34 | "rm -rf " + name | lib/lib.js:498:45:498:48 | name | lib/lib.js:499:31:499:34 | name | $@ based on $@ is later used in $@. | lib/lib.js:499:19:499:34 | "rm -rf " + name | String concatenation | lib/lib.js:498:45:498:48 | name | library input | lib/lib.js:499:3:499:35 | MyThing ... + name) | shell command |
719+
| lib/lib.js:510:10:510:25 | "rm -rf " + name | lib/lib.js:509:39:509:42 | name | lib/lib.js:510:22:510:25 | name | $@ based on $@ is later used in $@. | lib/lib.js:510:10:510:25 | "rm -rf " + name | String concatenation | lib/lib.js:509:39:509:42 | name | library input | lib/lib.js:510:2:510:26 | cp.exec ... + name) | shell command |
720+
| lib/lib.js:513:11:513:26 | "rm -rf " + name | lib/lib.js:509:39:509:42 | name | lib/lib.js:513:23:513:26 | name | $@ based on $@ is later used in $@. | lib/lib.js:513:11:513:26 | "rm -rf " + name | String concatenation | lib/lib.js:509:39:509:42 | name | library input | lib/lib.js:513:3:513:27 | cp.exec ... + name) | shell command |
721+
| lib/lib.js:519:11:519:26 | "rm -rf " + name | lib/lib.js:509:39:509:42 | name | lib/lib.js:519:23:519:26 | name | $@ based on $@ is later used in $@. | lib/lib.js:519:11:519:26 | "rm -rf " + name | String concatenation | lib/lib.js:509:39:509:42 | name | library input | lib/lib.js:519:3:519:27 | cp.exec ... + name) | shell command |
722+
| lib/lib.js:525:11:525:26 | "rm -rf " + name | lib/lib.js:509:39:509:42 | name | lib/lib.js:525:23:525:26 | name | $@ based on $@ is later used in $@. | lib/lib.js:525:11:525:26 | "rm -rf " + name | String concatenation | lib/lib.js:509:39:509:42 | name | library input | lib/lib.js:525:3:525:27 | cp.exec ... + name) | shell command |
723+
| lib/lib.js:531:11:531:26 | "rm -rf " + name | lib/lib.js:509:39:509:42 | name | lib/lib.js:531:23:531:26 | name | $@ based on $@ is later used in $@. | lib/lib.js:531:11:531:26 | "rm -rf " + name | String concatenation | lib/lib.js:509:39:509:42 | name | library input | lib/lib.js:531:3:531:27 | cp.exec ... + name) | shell command |
724+
| lib/lib.js:537:11:537:26 | "rm -rf " + name | lib/lib.js:509:39:509:42 | name | lib/lib.js:537:23:537:26 | name | $@ based on $@ is later used in $@. | lib/lib.js:537:11:537:26 | "rm -rf " + name | String concatenation | lib/lib.js:509:39:509:42 | name | library input | lib/lib.js:537:3:537:27 | cp.exec ... + name) | shell command |
725+
| lib/lib.js:543:11:543:26 | "rm -rf " + name | lib/lib.js:509:39:509:42 | name | lib/lib.js:543:23:543:26 | name | $@ based on $@ is later used in $@. | lib/lib.js:543:11:543:26 | "rm -rf " + name | String concatenation | lib/lib.js:509:39:509:42 | name | library input | lib/lib.js:543:3:543:27 | cp.exec ... + name) | shell command |
726+
| lib/lib.js:545:11:545:26 | "rm -rf " + name | lib/lib.js:509:39:509:42 | name | lib/lib.js:545:23:545:26 | name | $@ based on $@ is later used in $@. | lib/lib.js:545:11:545:26 | "rm -rf " + name | String concatenation | lib/lib.js:509:39:509:42 | name | library input | lib/lib.js:545:3:545:27 | cp.exec ... + name) | shell command |
669727
| 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 | $@ based on $@ is later used in $@. | lib/subLib2/compiled-file.ts:4:13:4:28 | "rm -rf " + name | String concatenation | 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 |
670728
| 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 | $@ based on $@ is later used in $@. | lib/subLib2/special-file.js:4:10:4:25 | "rm -rf " + name | String concatenation | 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 |
671729
| 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 | $@ based on $@ is later used in $@. | lib/subLib3/my-file.ts:4:10:4:25 | "rm -rf " + name | String concatenation | 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/lib/lib.js

Lines changed: 41 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -504,4 +504,44 @@ module.exports.myCommand = function (myCommand) {
504504
var imp = require('./isImported');
505505
for (var name in imp){
506506
module.exports[name] = imp[name];
507-
}
507+
}
508+
509+
module.exports.sanitizer4 = function (name) {
510+
cp.exec("rm -rf " + name); // NOT OK
511+
512+
if (isNaN(name)) {
513+
cp.exec("rm -rf " + name); // NOT OK
514+
} else {
515+
cp.exec("rm -rf " + name); // OK
516+
}
517+
518+
if (isNaN(parseInt(name))) {
519+
cp.exec("rm -rf " + name); // NOT OK
520+
} else {
521+
cp.exec("rm -rf " + name); // OK
522+
}
523+
524+
if (isNaN(+name)) {
525+
cp.exec("rm -rf " + name); // NOT OK
526+
} else {
527+
cp.exec("rm -rf " + name); // OK
528+
}
529+
530+
if (isNaN(parseInt(name, 10))) {
531+
cp.exec("rm -rf " + name); // NOT OK
532+
} else {
533+
cp.exec("rm -rf " + name); // OK
534+
}
535+
536+
if (isNaN(name - 0)) {
537+
cp.exec("rm -rf " + name); // NOT OK
538+
} else {
539+
cp.exec("rm -rf " + name); // OK
540+
}
541+
542+
if (isNaN(name | 0)) { // <- not a sanitizer
543+
cp.exec("rm -rf " + name); // NOT OK
544+
} else {
545+
cp.exec("rm -rf " + name); // NOT OK
546+
}
547+
}

0 commit comments

Comments
 (0)