implement a isNaN guard for unsafe-shell-command-construction

Author: erik-krogh

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

@@ -286,6 +286,29 @@ module UnsafeShellCommandConstruction {
     }
   }
 
+  /**
+   * A guard of the form `isNaN(x)`, which sanitizes `x` in its "else" branch.
+   */
+  class NaNGuard extends TaintTracking::SanitizerGuardNode instanceof DataFlow::CallNode {
+    Expr x;
+
+    NaNGuard() {
+      this = DataFlow::globalVarRef("isNaN").getACall() and
+      (
+        x = this.getArgument(0).asExpr()
+        or
+        exists(DataFlow::CallNode parse |
+          parse = DataFlow::globalVarRef(["parseInt", "parseFloat"]).getACall()
+        |
+          parse = this.getArgument(0) and
+          x = parse.getArgument(0).asExpr()
+        )
+      )
+    }
+
+    override predicate sanitizes(boolean outcome, Expr e) { e = x and outcome = false }
+  }
+
   private import semmle.javascript.dataflow.internal.AccessPaths
   private import semmle.javascript.dataflow.InferredTypes
 

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

@@ -24,7 +24,9 @@ class Configuration extends TaintTracking::Configuration {
 
   override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode guard) {
     guard instanceof PathExistsSanitizerGuard or
-    guard instanceof TaintTracking::AdHocWhitelistCheckSanitizer
+    guard instanceof TaintTracking::AdHocWhitelistCheckSanitizer or
+    guard instanceof NaNGuard or
+    guard instanceof TypeOfSanitizer
   }
 
   // override to require that there is a path without unmatched return steps

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

@@ -254,6 +254,14 @@ nodes
 | lib/lib.js:498:45:498:48 | name |
 | lib/lib.js:499:31:499:34 | name |
 | lib/lib.js:499:31:499:34 | name |
+| lib/lib.js:509:39:509:42 | name |
+| lib/lib.js:509:39:509:42 | name |
+| lib/lib.js:510:22:510:25 | name |
+| lib/lib.js:510:22:510:25 | name |
+| lib/lib.js:513:23:513:26 | name |
+| lib/lib.js:513:23:513:26 | name |
+| lib/lib.js:519:23:519:26 | name |
+| lib/lib.js:519:23:519:26 | name |
 | lib/subLib2/compiled-file.ts:3:26:3:29 | name |
 | lib/subLib2/compiled-file.ts:3:26:3:29 | name |
 | lib/subLib2/compiled-file.ts:4:25:4:28 | name |
@@ -574,6 +582,18 @@ edges
 | lib/lib.js:498:45:498:48 | name | lib/lib.js:499:31:499:34 | name |
 | lib/lib.js:498:45:498:48 | name | lib/lib.js:499:31:499:34 | name |
 | lib/lib.js:498:45:498:48 | name | lib/lib.js:499:31:499:34 | name |
+| lib/lib.js:509:39:509:42 | name | lib/lib.js:510:22:510:25 | name |
+| lib/lib.js:509:39:509:42 | name | lib/lib.js:510:22:510:25 | name |
+| lib/lib.js:509:39:509:42 | name | lib/lib.js:510:22:510:25 | name |
+| lib/lib.js:509:39:509:42 | name | lib/lib.js:510:22:510:25 | name |
+| lib/lib.js:509:39:509:42 | name | lib/lib.js:513:23:513:26 | name |
+| lib/lib.js:509:39:509:42 | name | lib/lib.js:513:23:513:26 | name |
+| lib/lib.js:509:39:509:42 | name | lib/lib.js:513:23:513:26 | name |
+| lib/lib.js:509:39:509:42 | name | lib/lib.js:513:23:513:26 | name |
+| lib/lib.js:509:39:509:42 | name | lib/lib.js:519:23:519:26 | name |
+| lib/lib.js:509:39:509:42 | name | lib/lib.js:519:23:519:26 | name |
+| lib/lib.js:509:39:509:42 | name | lib/lib.js:519:23:519:26 | name |
+| lib/lib.js:509:39:509:42 | name | lib/lib.js:519:23:519:26 | name |
 | lib/subLib2/compiled-file.ts:3:26:3:29 | name | lib/subLib2/compiled-file.ts:4:25:4:28 | name |
 | lib/subLib2/compiled-file.ts:3:26:3:29 | name | lib/subLib2/compiled-file.ts:4:25:4:28 | name |
 | lib/subLib2/compiled-file.ts:3:26:3:29 | name | lib/subLib2/compiled-file.ts:4:25:4:28 | name |
@@ -666,6 +686,9 @@ edges
 | 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 |
 | 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 |
 | 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 |
+| 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 |
+| 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 |
+| 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 |
 | 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 |
 | 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 |
 | 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

@@ -504,4 +504,20 @@ module.exports.myCommand = function (myCommand) {
 var imp = require('./isImported');
 for (var name in imp){
   module.exports[name] = imp[name];
-}
+}
+
+module.exports.sanitizer4 = function (name) {
+	cp.exec("rm -rf " + name); // NOT OK
+
+	if (isNaN(name)) {
+		cp.exec("rm -rf " + name); // NOT OK
+	} else {
+		cp.exec("rm -rf " + name); // OK
+	}
+
+	if (isNaN(parseInt(name))) {
+		cp.exec("rm -rf " + name); // NOT OK
+	} else {
+		cp.exec("rm -rf " + name); // OK
+	}
+}