refactor the getOptionsArg predicate into the SystemCommandExecution class

Author: erik-krogh

javascript/ql/src/semmle/javascript/Concepts.qll

@@ -25,6 +25,11 @@ abstract class SystemCommandExecution extends DataFlow::Node {
 
   /** Holds if the command execution happens synchronously. */
   abstract predicate isSync();
+
+  /**
+   * Gets the data-flow node (if it exists) for an options argument.
+   */
+  abstract DataFlow::Node getOptionsArg();
 }
 
 /**

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

@@ -627,6 +627,18 @@ module NodeJSLib {
     override predicate isSync() {
       "Sync" = methodName.suffix(methodName.length() - 4)
     }
+
+    override DataFlow::Node getOptionsArg() {
+      not result.getALocalSource() instanceof DataFlow::FunctionNode and // looks like callback
+      not result.getALocalSource() instanceof DataFlow::ArrayCreationNode and // looks like argumentlist
+      not result = getArgument(0) and
+      // fork/spawn and all sync methos always has options as the last argument
+      if methodName.regexpMatch("fork.*") or methodName.regexpMatch("spawn.*") or methodName.regexpMatch(".*Sync") then
+        result = getLastArgument()
+      else 
+        // the rest (exec/execFile) has the options argument as their second last.
+        result = getArgument(this.getNumArgument() - 2)
+    }
   }
 
   /**

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

@@ -162,6 +162,13 @@ module ShellJS {
     override predicate isShellInterpreted(DataFlow::Node arg) { arg = getACommandArgument() }
 
     override predicate isSync() {none ()}
+
+    override DataFlow::Node getOptionsArg() {
+      result = getLastArgument() 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/frameworks/SystemCommandExecutors.qll

@@ -7,16 +7,17 @@ import javascript
 
 private class SystemCommandExecutors extends SystemCommandExecution, DataFlow::InvokeNode {
   int cmdArg;
+  int optionsArg;
 
   boolean shell;
   boolean sync;
 
   SystemCommandExecutors() {
     exists(string mod, DataFlow::SourceNode callee |
       exists(string method |
-        mod = "cross-spawn" and method = "sync" and cmdArg = 0 and shell = false
+        mod = "cross-spawn" and method = "sync" and cmdArg = 0 and shell = false and optionsArg = -1
         or
-        mod = "execa" and
+        mod = "execa" and optionsArg = -1 and
         (
           shell = false and
           (
@@ -40,21 +41,21 @@ private class SystemCommandExecutors extends SystemCommandExecution, DataFlow::I
       (
         shell = false and
         (
-          mod = "cross-spawn" and cmdArg = 0
+          mod = "cross-spawn" and cmdArg = 0 and optionsArg = -1
           or
-          mod = "cross-spawn-async" and cmdArg = 0
+          mod = "cross-spawn-async" and cmdArg = 0 and optionsArg = -1
           or
-          mod = "exec-async" and cmdArg = 0
+          mod = "exec-async" and cmdArg = 0 and optionsArg = -1
           or
-          mod = "execa" and cmdArg = 0
+          mod = "execa" and cmdArg = 0 and optionsArg = -1  
         )
         or
         shell = true and
         (
-          mod = "exec" and
+          mod = "exec" and optionsArg = -2 and
           cmdArg = 0
           or
-          mod = "remote-exec" and cmdArg = 1
+          mod = "remote-exec" and cmdArg = 1 and optionsArg = -1
         )
       ) and
       callee = DataFlow::moduleImport(mod)
@@ -69,8 +70,16 @@ private class SystemCommandExecutors extends SystemCommandExecution, DataFlow::I
     arg = getACommandArgument() and shell = true
   }
 
-  override predicate isSync() {
-    sync = true
+  override predicate isSync() { sync = true }
+
+  override DataFlow::Node getOptionsArg() {
+    (if optionsArg < 0 then
+     result = getArgument(getNumArgument() - optionsArg) 
+    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

@@ -41,21 +41,7 @@ private class CommandCall extends DataFlow::InvokeNode {
    * Gets the data-flow node (if it exists) for an options argument for an `exec`-like call.
    */
   DataFlow::Node getOptionsArg() {
-    exists(int n |
-      n >= 1 and
-      // if there is a command-list, then the options is at least the third argument.
-      (not exists(command.getArgumentList()) or n >= 2) and
-      // async exec calls can have a callback as their last call.
-      if command.isSync() or not exists(getCallback())
-      then n < getNumArgument()
-      else n < getNumArgument() - 1
-    |
-      result = getArgument(n)
-    )
-    or
-    // Fallback in case normal API conventions are broken.
-    result = getAnArgument() and
-    result.getALocalSource() instanceof DataFlow::ObjectLiteralNode
+    result = command.getOptionsArg()
   }
 
   /**

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

@@ -71,6 +71,23 @@ 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}) |
+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) |
+| child_process-test.js:64:3:64:21 | cp.spawn(cmd, args) | child_process-test.js:64:17:64:20 | args |
+| uselesscat.js:28:1:28:39 | execSyn ...  1000}) | uselesscat.js:28:28:28:38 | {uid: 1000} |
+| uselesscat.js:30:1:30:64 | exec('c ... t) { }) | uselesscat.js:30:26:30:38 | { cwd: './' } |
+| uselesscat.js:34:1:34:54 | execSyn ... utf8'}) | uselesscat.js:34:36:34:53 | {encoding: 'utf8'} |
+| uselesscat.js:36:1:36:77 | execSyn ... utf8'}) | uselesscat.js:36:36:36:76 | { uid:  ... 'utf8'} |
+| uselesscat.js:69:1:72:2 | execFil ... ut);\\n}) | uselesscat.js:69:38:69:55 | {encoding: 'utf8'} |
+| uselesscat.js:74:1:74:60 | execFil ... utf8'}) | uselesscat.js:74:42:74:59 | {encoding: 'utf8'} |
+| uselesscat.js:79:1:79:46 | execFil ...   opts) | uselesscat.js:79:42:79:45 | opts |
+| uselesscat.js:82:1:82:90 | execFil ... String) | uselesscat.js:82:42:82:89 | anOptsF ... oString |
+| uselesscat.js:84:1:84:115 | execFil ... ring'}) | uselesscat.js:84:42:84:114 | {encodi ... tring'} |
+| uselesscat.js:86:1:86:75 | execFil ... utf8'}) | uselesscat.js:86:57:86:74 | {encoding: 'utf8'} |
+| 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} |
 #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/UselessUseOfCat.ql

@@ -21,4 +21,8 @@ query string readFile(UselessCat cat) { result = PrettyPrintCatCall::createReadF
 
 query SystemCommandExecution syncCommand() {
   result.isSync()	
+}
+
+query DataFlow::Node options(SystemCommandExecution sys) {
+  result = sys.getOptionsArg()
 }