add tests for third party command execution libraries (and two small fixes)

Author: erik-krogh

javascript/ql/src/semmle/javascript/frameworks/SystemCommandExecutors.qll

@@ -70,14 +70,15 @@ private class SystemCommandExecutors extends SystemCommandExecution, DataFlow::I
     arg = getACommandArgument() and shell = true
   }
 
+  override DataFlow::Node getArgumentList() { shell = false and result = getArgument(1) }
+
   override predicate isSync() { sync = true }
 
   override DataFlow::Node getOptionsArg() {
     (if optionsArg < 0 then
-     result = getArgument(getNumArgument() - optionsArg) 
+     result = getArgument(getNumArgument() + optionsArg) and getNumArgument() + optionsArg > cmdArg
     else 
      result = getArgument(optionsArg)) and
-    not result = getArgument(0) and
     not result.getALocalSource() instanceof DataFlow::FunctionNode and // looks like callback
     not result.getALocalSource() instanceof DataFlow::ArrayCreationNode // looks like argumentlist
   }

javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll

@@ -21,9 +21,9 @@ private class CommandCall extends DataFlow::InvokeNode {
   predicate isSync() { command.isSync() }
 
   /**
-   * Gets an argument to this command execution that specifies the argument list to the command.
+   * Gets a list that specifies the arguments given to the command.
    */
-  DataFlow::Node getArgumentList() { result = command.getArgumentList() }
+  DataFlow::ArrayCreationNode getArgumentList() { result = command.getArgumentList().getALocalSource() }
 
   /**
    * Gets the callback (if it exists) for an async `exec`-like call.
@@ -35,7 +35,7 @@ private class CommandCall extends DataFlow::InvokeNode {
   /**
    * Holds if the executed command execution has an argument list as a separate argument.
    */
-  predicate hasArgumentList() { exists(command.getArgumentList()) }
+  predicate hasArgumentList() { exists(getArgumentList()) }
 
   /**
    * Gets the data-flow node (if it exists) for an options argument for an `exec`-like call.

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

@@ -22,6 +22,18 @@ readFile
 | uselesscat.js:121:12:121:64 | exec("c ... (out)}) | fs.readFile("foo/bar", (err, out) => {...}) |
 | uselesscat.js:127:14:127:66 | exec("c ... (out)}) | fs.readFile("foo/bar", (err, out) => {...}) |
 | uselesscat.js:136:17:138:2 | execSyn ... tf8'\\n}) | fs.readFileSync("/etc/dnsmasq.conf", ...) |
+| uselesscat.js:146:1:146:61 | shelljs ... (out)}) | fs.readFile("foo/bar", (err, out) => {...}) |
+| uselesscat.js:147:1:147:47 | shelljs ... utf8'}) | fs.readFile("foo/bar", {encoding: 'utf8'}) |
+| uselesscat.js:148:1:148:81 | shelljs ... (out)}) | fs.readFile("foo/bar", (err, out) => {...}) |
+| uselesscat.js:151:1:151:48 | cspawn( ... tf8' }) | fs.readFile("foo/bar", { encoding: 'utf8' }) |
+| uselesscat.js:152:1:152:82 | cspawn( ... (out)}) | fs.readFile("foo/bar", (err, out) => {...}) |
+| uselesscat.js:153:1:153:60 | cspawn( ... (out)}) | fs.readFile("foo/bar", (err, out) => {...}) |
+| uselesscat.js:154:1:154:26 | cspawn( ... /bar']) | fs.readFile("foo/bar") |
+| uselesscat.js:158:16:158:46 | cspawn. ... /bar']) | fs.readFileSync("foo/bar") |
+| uselesscat.js:159:16:159:68 | cspawn. ... tf8' }) | fs.readFileSync("foo/bar", { encoding: 'utf8' }) |
+| uselesscat.js:162:1:162:56 | execmod ... (out)}) | fs.readFile("foo/bar", (err, out) => {...}) |
+| uselesscat.js:163:1:163:42 | execmod ... utf8'}) | fs.readFile("foo/bar") |
+| uselesscat.js:164:1:164:76 | execmod ... (out)}) | fs.readFile("foo/bar", {encoding: 'utf8'}, (err, out) => {...}) |
 syncCommand
 | child_process-test.js:9:5:9:22 | cp.execSync("foo") |
 | child_process-test.js:11:5:11:26 | cp.exec ... ("foo") |
@@ -71,6 +83,8 @@ syncCommand
 | uselesscat.js:100:1:100:56 | execFil ... ptions) |
 | uselesscat.js:104:1:104:31 | execFil ... cat` ]) |
 | uselesscat.js:136:17:138:2 | execSyn ... tf8'\\n}) |
+| uselesscat.js:158:16:158:46 | cspawn. ... /bar']) |
+| uselesscat.js:159:16:159:68 | cspawn. ... tf8' }) |
 options
 | child_process-test.js:53:5:53:59 | cp.spaw ...  cmd])) | child_process-test.js:53:25:53:58 | ['/C',  ... , cmd]) |
 | child_process-test.js:54:5:54:50 | cp.spaw ... t(cmd)) | child_process-test.js:54:25:54:49 | ['/C',  ... at(cmd) |
@@ -88,6 +102,11 @@ options
 | uselesscat.js:100:1:100:56 | execFil ... ptions) | uselesscat.js:100:42:100:55 | unknownOptions |
 | uselesscat.js:111:1:111:51 | spawn(' ... it'] }) | uselesscat.js:111:14:111:50 | { stdio ... rit'] } |
 | uselesscat.js:136:17:138:2 | execSyn ... tf8'\\n}) | uselesscat.js:136:51:138:1 | { // NO ... utf8'\\n} |
+| uselesscat.js:147:1:147:47 | shelljs ... utf8'}) | uselesscat.js:147:29:147:46 | {encoding: 'utf8'} |
+| uselesscat.js:151:1:151:48 | cspawn( ... tf8' }) | uselesscat.js:151:28:151:47 | { encoding: 'utf8' } |
+| uselesscat.js:156:1:156:35 | cspawn( ... tf8' }) | uselesscat.js:156:15:156:34 | { encoding: 'utf8' } |
+| uselesscat.js:159:16:159:68 | cspawn. ... tf8' }) | uselesscat.js:159:48:159:67 | { encoding: 'utf8' } |
+| uselesscat.js:164:1:164:76 | execmod ... (out)}) | uselesscat.js:164:24:164:41 | {encoding: 'utf8'} |
 #select
 | False negative | uselesscat.js:54:42:54:69 | // NOT  ... lagged] |
 | False positive | uselesscat.js:44:37:44:85 | // OK [ ... le read |

javascript/ql/test/query-tests/Security/CWE-078/uselesscat.js

@@ -139,4 +139,28 @@ const stdout2 = execSync('cat /etc/dnsmasq.conf', { // NOT OK.
 
 exec('/bin/cat', function (e, s) {});  // OK
 
-spawn("cat") // OK  
+spawn("cat") // OK  
+
+
+var shelljs = require("shelljs");
+shelljs.exec("cat foo/bar", (err, out) => {console.log(out)}); // NOT OK
+shelljs.exec("cat foo/bar", {encoding: 'utf8'}); // NOT OK
+shelljs.exec("cat foo/bar", {encoding: 'utf8'}, (err, out) => {console.log(out)}); // NOT OK
+
+let cspawn = require('cross-spawn');
+cspawn('cat', ['foo/bar'], { encoding: 'utf8' }); // NOT OK
+cspawn('cat', ['foo/bar'], { encoding: 'utf8' }, (err, out) => {console.log(out)}); // NOT OK
+cspawn('cat', ['foo/bar'], (err, out) => {console.log(out)}); // NOT OK
+cspawn('cat', ['foo/bar']); // NOT OK
+cspawn('cat', (err, out) => {console.log(out)}); // OK
+cspawn('cat', { encoding: 'utf8' }); // OK
+ 
+let myResult = cspawn.sync('cat', ['foo/bar']); // NOT OK
+let myResult = cspawn.sync('cat', ['foo/bar'], { encoding: 'utf8' }); // NOT OK
+
+var execmod = require('exec');
+execmod("cat foo/bar", (err, out) => {console.log(out)}); // NOT OK
+execmod("cat foo/bar", {encoding: 'utf8'}); // NOT OK
+execmod("cat foo/bar", {encoding: 'utf8'}, (err, out) => {console.log(out)}); // NOT OK
+
+