big refactor of UselessUseOfCal

javascript/ql/src/Security/CWE-078/UselessUseOfCat.ql

@@ -15,5 +15,9 @@ import semmle.javascript.security.UselessUseOfCat
 import semmle.javascript.RestrictedLocations
 
 
-from UselessCat cat
-select cat.asExpr().(FirstLineOf), "Useless use of `cat`. Can be replaced with: " + createReadFileCall(cat)
+from UselessCat cat, string message
+where 
+    message = " Can be replaced with: " + PrettyPrintCatCall::createReadFileCall(cat)
+    or
+    not exists(PrettyPrintCatCall::createReadFileCall(cat)) and message = ""
+select cat.asExpr().(FirstLineOf), "Useless use of `cat`." + message

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

@@ -1,160 +1,233 @@
 /**
- * Provides predicates and classes for working with useless uses of `cat`.
+ * Provides predicates and classes for working with useless uses of the unix command `cat`.
  */
 
 import javascript
+import Expressions.ExprHasNoEffect
+import Declarations.UnusedVariable
 
 /**
- * Gets a string representation of an equivalent call to `fs.readFile` for a given command execution `cat`.
+ * A call that executes a system command.
+ * This class provide utility predicates for reasoning about command execution calls.
  */
-string createReadFileCall(UselsesCatCandidates::UselessCatCandicate cat) {
-  exists(string sync, string extraArg, string callback |
-    (if cat.isSync() then sync = "Sync" else sync = "") and
-    (
-      extraArg = ", " + printOptionsArg(cat.getOptionsArg()) + ")"
-      or
-      extraArg = "" and not exists(cat.getOptionsArg())
-    ) and
-    (
-      callback = constructCallbackString(cat.getCallback())
-      or
-      callback = "" and not exists(cat.getCallback())
-    )
-  |
-    result = "fs.readFile" + sync + "(" + cat.getFileArgument().trim() + extraArg + callback + ")"
-  )
-}
+private class CommandCall extends DataFlow::InvokeNode {
+  SystemCommandExecution command;
 
-/**
- * Constructs a string representing the callback `func`.
- */
-string constructCallbackString(DataFlow::FunctionNode func) {
-  exists(string args | args = getCallbackArgs(func) |
-    if func.getFunction() instanceof ArrowFunctionExpr
-    then
-      if func.getFunction().getBody() instanceof Expr
-      then result = ", (" + args + ") => ..."
-      else result = ", (" + args + ") => {...}"
-    else result = ", function(" + args + ") {...}"
-  )
-}
+  CommandCall() { this = command }
 
-/**
- * Gets a string concatenation of the parameter names in a function `func`.
- */
-private string getCallbackArgs(DataFlow::FunctionNode func) {
-  result =
-    concat(int i |
-      i = [0 .. func.getNumParameter()]
+  /**
+   * Holds if the call is synchronous (e.g. `execFileSync`).
+   */
+  predicate isSync() { command.isSync() }
+
+  /**
+   * Gets an argument to this command execution that specifies the argument list to the command.
+   */
+  DataFlow::Node getArgumentList() { result = command.getArgumentList() }
+
+  /**
+   * Gets the callback (if it exists) for an async `exec`-like call.
+   */
+  DataFlow::FunctionNode getCallback() {
+    not this.isSync() and result = getLastArgument().getALocalSource()
+  }
+
+  /**
+   * Holds if the executed command execution has an argument list as a separate argument.
+   */
+  predicate hasArgumentList() { exists(command.getArgumentList()) }
+
+  /**
+   * Gets the data-flow node (if it exists) for a 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
     |
-      func.getParameter(i).getName(), ", " order by i
+      result = getArgument(n)
     )
+    or
+    // Fallback in case normal API conventions are broken.
+    result = getAnArgument() and
+    result.getALocalSource() instanceof DataFlow::ObjectLiteralNode
+  }
+
+  /**
+   * Gets the constant-string parts that are not part of the command itself. 
+   * E.g. for a command execution `exec("/bin/cat foo bar")` this predicate will have result `"foo bar"`. 
+   */ 
+  string getNonCommandConstantString() {
+    if this.hasArgumentList()
+    then
+      result =
+        getConstantStringParts(getArgumentList()
+              .getALocalSource()
+              .(DataFlow::ArrayCreationNode)
+              .getElement(_))
+    else
+      exists(string commandString | commandString = getConstantStringParts(getArgument(0)) |
+        result = commandString.suffix(1 + commandString.indexOf(" ", 0, 0))
+      )
+  }
+
+  /**
+   * Holds if this command execution invokes the executeable `name`.
+   */
+  bindingset[name]
+  predicate isACallTo(string name) {
+    if this.hasArgumentList()
+    then getArgument(0).mayHaveStringValue(name)
+    else
+      exists(string arg | arg = getConstantStringParts(getArgument(0)) |
+        arg.prefix(name.length()) = name
+      )
+  }
 }
 
 /**
- * Gets a string representation of the options argument `arg` from an exec-like call.
+ * Gets the constant string parts from a data-flow node.
+ * Either the string is some constant
  */
-private string printOptionsArg(DataFlow::Node arg) {
-  result = arg.asExpr().(VarAccess).getVariable().getName()
+private string getConstantStringParts(DataFlow::Node node) {
+  node.mayHaveStringValue(result)
   or
-  // fall back to toString(), but ensure that we don't have dots in the middle.
-  result = arg.(DataFlow::ObjectLiteralNode).toString() and not result.regexpMatch(".*\\.\\..*")
+  result = node.(StringOps::ConcatenationRoot).getConstantStringParts()
 }
 
 /**
  * A call to a useless use of `cat`.
  */
-class UselessCat extends DataFlow::CallNode {
-  UselsesCatCandidates::UselessCatCandicate candidate;
-
+class UselessCat extends CommandCall {
   UselessCat() {
-    this = candidate and
-    // We can create an equivalent `fs.readFile` call.
-    exists(createReadFileCall(this)) and
+    this = command and
+    isACallTo(getACatExecuteable()) and
     // There is a file to read, and not just a pair of quotes.
-    candidate.getFileArgument().length() >= 3 and
-    // wildcards, pipes, redirections, and multiple files are OK.
-    // (The multiple files detection relies on the fileArgument not containing spaces anywhere)
-    not candidate.getFileArgument().regexpMatch(".*(\\*|\\||>|<| ).*") and
+    (
+      not exists(PrettyPrintCatCall::createFileArgument(this))
+      or
+      exists(string fileArg | fileArg = PrettyPrintCatCall::createFileArgument(this) |
+        fileArg.length() >= 3
+      )
+    ) and
+    // wildcards, pipes, redirections, other bash features, and multiple files (spaces) are OK.
+    not getNonCommandConstantString().regexpMatch(".*(\\*|\\||>|<| |\\$|&|,|\\`).*") and
     // Only acceptable option is "encoding", everything else is non-trivial to emulate with fs.readFile.
     (
-      not exists(candidate.getOptionsArg())
+      not exists(getOptionsArg())
       or
-      forex(string prop |
-        exists(candidate.getOptionsArg().getALocalSource().getAPropertyWrite(prop))
-      |
+      forex(string prop | exists(getOptionsArg().getALocalSource().getAPropertyWrite(prop)) |
         prop = "encoding"
       )
     ) and
     // If there is a callback, then it must either have one or two arguments, or if there is a third argument it must be unused.
     (
-      not exists(candidate.getCallback())
+      not exists(getCallback())
       or
-      exists(DataFlow::FunctionNode func | func = candidate.getCallback() |
+      exists(DataFlow::FunctionNode func | func = getCallback() |
         func.getNumParameter() = 1
         or
         func.getNumParameter() = 2
         or
         // `exec` can use 3 parameters, `readFile` can only use two, so it is OK to have a third parameter if it is unused,
-        func.getNumParameter() = 3 and not exists(SSA::definition(func.getParameter(2).getParameter()))
+        func.getNumParameter() = 3 and
+        not exists(SSA::definition(func.getParameter(2).getParameter()))
       )
+    ) and
+    // The process returned by an async call is unused.
+    (
+      isSync()
+      or
+      inVoidContext(this.getEnclosingExpr())
+      or
+      this.getEnclosingExpr() = any(UnusedLocal v).getAnAssignedExpr()
     )
   }
 }
 
-module UselsesCatCandidates {
+/**
+ * Gets a string used to call `cat`.
+ */
+string getACatExecuteable() {
+  result = "cat" or result = "/bin/cat" or result = "sudo cat" or result = "sudo /bin/cat"
+}
+
+/**
+ * Predicates for creating an equivalent call to `fs.readFile` from a command execution of `cat`.
+ */
+module PrettyPrintCatCall {
   /**
-   * A candidate for a useless use of `cat`.
-   * This class describes the structure of a call to cat
-   * This class does not determine whether it is a useless call, or even if it is a call to `cat`.
+   * Create a string representation of an equivalent call to `fs.readFile` for a given command execution `cat`.
    */
-  class UselessCatCandicate extends DataFlow::CallNode {
-    SystemCommandExecution command;
-
-    UselessCatCandicate() { this = command }
-
-    /**
-     * Holds if the call is synchronous (e.g. `execFileSync`).
-     */
-    predicate isSync() { command.isSync() }
-
-    /**
-     * Holds if the executed command execution has an argument list as a separate argument.
-     */
-    predicate hasArgumentList() { exists(command.getArgumentList()) }
-
-    /**
-     * Gets a string representation of the expression that determines what file is read by `cat`.
-     */
-    string getFileArgument() {
-      if hasArgumentList()
-      then
-        getArgument(0).mayHaveStringValue(cat()) and
-        result = getFileThatIsReadFromCommandList(this)
-      else result = getFileArgumentWithoutCat(getArgument(0))
-    }
-
-    /**
-     * Gets the data-flow node (if it exists) for the options argument to the `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 calls have a callback as their last call.
-        if this.isSync() then n < getNumArgument() else n < getNumArgument() - 1
-      |
-        result = getArgument(n)
+  string createReadFileCall(UselessCat cat) {
+    exists(string sync, string extraArg, string callback |
+      (if cat.isSync() then sync = "Sync" else sync = "") and
+      (
+        extraArg = ", " + createOptionsArg(cat.getOptionsArg()) + ")"
+        or
+        extraArg = "" and not exists(cat.getOptionsArg())
+      ) and
+      (
+        callback = createCallbackString(cat.getCallback())
+        or
+        callback = "" and not exists(cat.getCallback())
       )
-    }
+    |
+      result =
+        "fs.readFile" + sync + "(" + createFileArgument(cat).trim() + extraArg + callback + ")"
+    )
+  }
 
-    /**
-     * Gets the callback (if it exists) for an async `exec` like call.
-     */
-    DataFlow::FunctionNode getCallback() {
-      not this.isSync() and result = getLastArgument().getALocalSource()
-    }
+  /**
+   * Create a string representation of the expression that determines what file is read by `cat`.
+   */
+  string createFileArgument(CommandCall cat) {
+    if cat.hasArgumentList()
+    then
+      cat.getArgument(0).mayHaveStringValue(getACatExecuteable()) and
+      result = createFileThatIsReadFromCommandList(cat)
+    else result = createFileArgumentWithoutCat(cat.getArgument(0))
+  }
+
+  /**
+   * Create a string representing the callback `func`.
+   */
+  string createCallbackString(DataFlow::FunctionNode func) {
+    exists(string args | args = createCallbackArgs(func) |
+      if func.getFunction() instanceof ArrowFunctionExpr
+      then
+        if func.getFunction().getBody() instanceof Expr
+        then result = ", (" + args + ") => ..."
+        else result = ", (" + args + ") => {...}"
+      else result = ", function(" + args + ") {...}"
+    )
+  }
+
+  /**
+   * Create a string concatenation of the parameter names in a function `func`.
+   */
+  private string createCallbackArgs(DataFlow::FunctionNode func) {
+    result =
+      concat(int i |
+        i = [0 .. func.getNumParameter()]
+      |
+        func.getParameter(i).getName(), ", " order by i
+      )
+  }
+
+  /**
+   * Create a string representation of the options argument `arg` from an exec-like call.
+   */
+  private string createOptionsArg(DataFlow::Node arg) {
+    result = arg.asExpr().(VarAccess).getVariable().getName()
+    or
+    // fall back to toString(), but ensure that we don't have dots in the middle.
+    result = arg.(DataFlow::ObjectLiteralNode).toString() and not result.regexpMatch(".*\\.\\..*")
   }
 
   /**
@@ -169,7 +242,7 @@ module UselsesCatCandidates {
       concat(Expr leaf |
         leaf = root.getALeaf().asExpr()
       |
-        createLeafRepresentation(leaf), "+" order by leaf.getFirstToken().getIndex()
+        createLeafRepresentation(leaf), " + " order by leaf.getFirstToken().getIndex()
       )
     or
     // Template string
@@ -186,7 +259,7 @@ module UselsesCatCandidates {
   }
 
   /**
-   * Gets a string representing the expression needed to re-create the value for a leaf in a string-concatenation.
+   * Create a string representing the expression needed to re-create the value for a leaf in a string-concatenation.
    */
   private string createLeafRepresentation(Expr e) {
     result = "\"" + e.getStringValue() + "\"" or
@@ -194,7 +267,7 @@ module UselsesCatCandidates {
   }
 
   /**
-   * Gets a string representing the expression needed to re-create the value for an element of a template string.
+   * Create a string representing the expression needed to re-create the value for an element of a template string.
    */
   private string createTemplateElementRepresentation(Expr e) {
     result = "${" + e.(VarAccess).getVariable().getName() + "}"
@@ -203,18 +276,11 @@ module UselsesCatCandidates {
   }
 
   /**
-   * Gets a string used to call `cat`.
-   */
-  private string cat() {
-    result = "cat" or result = "/bin/cat" or result = "sudo cat" or result = "sudo /bin/cat"
-  }
-
-  /**
-   * Gets a string representing an expression that gets the file read by a call to `cat`.
+   * Create a string representing an expression that gets the file read by a call to `cat`.
    * The input `arg` is the node that determines the commandline where `cat` is invoked.
    */
-  private string getFileArgumentWithoutCat(DataFlow::Node arg) {
-    exists(string cat | cat = cat() |
+  private string createFileArgumentWithoutCat(DataFlow::Node arg) {
+    exists(string cat | cat = getACatExecuteable() |
       exists(string command | arg.mayHaveStringValue(command) |
         command.prefix(cat.length()) = cat and
         result = "\"" + command.suffix(cat.length()).trim() + "\""
@@ -226,20 +292,20 @@ module UselsesCatCandidates {
         (if root.asExpr() instanceof TemplateLiteral then quote = "`" else quote = "\"") and
         root.getFirstLeaf().getStringValue().prefix(cat.length()) = cat and
         exists(string rawConcat | rawConcat = quote + printed.suffix(cat.length()).trim() |
-          result = getSimplifiedStringConcat(rawConcat)
+          result = createSimplifiedStringConcat(rawConcat)
         )
       )
     )
   }
 
   /**
-   * Gets a simplified and equivalent string concatenation for a given string concatenation `str`
+   * Create a simplified and equivalent string concatenation for a given string concatenation `str`
    */
   bindingset[str]
-  private string getSimplifiedStringConcat(string str) {
+  private string createSimplifiedStringConcat(string str) {
     // Remove an initial ""+ (e.g. in `""+file`)
-    if str.prefix(3) = "\"\"+"
-    then result = str.suffix(3)
+    if str.prefix(5) = "\"\" + "
+    then result = str.suffix(5)
     else
       // prettify `${newpath}` to just newpath
       if
@@ -251,11 +317,11 @@ module UselsesCatCandidates {
   }
 
   /**
-   * Gets the file that is read for a call with an explicit command list (e.g. `child_process.execFile/execFileSync`).
+   * Create the file that is read for a call with an explicit command list (e.g. `child_process.execFile/execFileSync`).
    */
-  string getFileThatIsReadFromCommandList(DataFlow::CallNode call) {
+  string createFileThatIsReadFromCommandList(CommandCall call) {
     exists(DataFlow::ArrayCreationNode array, DataFlow::Node element |
-      array = call.getArgument(1).(DataFlow::ArrayCreationNode) and
+      array = call.getArgumentList().(DataFlow::ArrayCreationNode) and
       array.getSize() = 1 and
       element = array.getElement(0)
     |

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

@@ -1,6 +1,6 @@
 readFile
 | uselesscat.js:10:1:10:43 | exec("c ... ut) {}) | fs.readFile("foo/bar", function(err, out) {...}) |
-| uselesscat.js:12:1:14:2 | exec("c ... ut);\\n}) | fs.readFile("/proc/"+id+"/status", function(err, out) {...}) |
+| uselesscat.js:12:1:14:2 | exec("c ... ut);\\n}) | fs.readFile("/proc/" + id + "/status", function(err, out) {...}) |
 | uselesscat.js:16:1:16:29 | execSyn ... uinfo') | fs.readFileSync("/proc/cpuinfo") |
 | uselesscat.js:18:1:18:26 | execSyn ... path}`) | fs.readFileSync(newpath) |
 | uselesscat.js:32:1:32:34 | execSyn ... path}`) | fs.readFileSync(`foo/bar/${newpath}`) |
@@ -12,12 +12,14 @@ readFile
 | uselesscat.js:76:1:76:39 | execFil ... xml' ]) | fs.readFileSync("pom.xml") |
 | uselesscat.js:79:1:79:46 | execFil ...   opts) | fs.readFileSync("pom.xml", opts)) |
 | uselesscat.js:82:1:82:90 | execFil ... String) | fs.readFileSync("pom.xml", anOptsFileNameThatIsTooLongToBePrintedByToString)) |
-| uselesscat.js:86:1:86:75 | execFil ... utf8'}) | fs.readFileSync("foo/"+newPath+"bar", {encoding: 'utf8'})) |
-| uselesscat.js:88:1:88:35 | execSyn ...  + foo) | fs.readFileSync("/proc/cpuinfo"+foo) |
+| uselesscat.js:86:1:86:75 | execFil ... utf8'}) | fs.readFileSync("foo/" + newPath + "bar", {encoding: 'utf8'})) |
+| uselesscat.js:88:1:88:35 | execSyn ...  + foo) | fs.readFileSync("/proc/cpuinfo" + foo) |
 | uselesscat.js:90:1:90:50 | execFil ... th}` ]) | fs.readFileSync(`foo/bar/${newpath}`) |
 | uselesscat.js:94:1:94:43 | exec("c ... ut) {}) | fs.readFile("foo/bar", function(err, out) {...}) |
 | uselesscat.js:96:1:96:53 | exec("c ... (out)}) | fs.readFile("foo/bar", (err, out) => {...}) |
 | uselesscat.js:98:1:98:55 | exec("c ... h(out)) | fs.readFile("foo/bar", (err, out) => ...) |
+| 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) => {...}) |
 syncCommand
 | child_process-test.js:9:5:9:22 | cp.execSync("foo") |
 | child_process-test.js:11:5:11:26 | cp.exec ... ("foo") |
@@ -65,6 +67,7 @@ syncCommand
 | uselesscat.js:90:1:90:50 | execFil ... th}` ]) |
 | uselesscat.js:92:1:92:46 | execFil ... th}` ]) |
 | uselesscat.js:100:1:100:56 | execFil ... ptions) |
+| uselesscat.js:104:1:104:31 | execFil ... cat` ]) |
 #select
 | False negative | uselesscat.js:54:42:54:69 | // NOT  ... lagged] |
-| False negative | uselesscat.js:84:118:84:144 | // NOT  ... lagged] |
+| False positive | uselesscat.js:44:37:44:85 | // OK [ ... le read |

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

@@ -17,7 +17,7 @@ where
   )
 select msg, comment
 
-query string readFile(UselessCat cat) { result = createReadFileCall(cat) }
+query string readFile(UselessCat cat) { result = PrettyPrintCatCall::createReadFileCall(cat) }
 
 query SystemCommandExecution syncCommand() {
   result.isSync()	

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

@@ -41,7 +41,7 @@ execSync(`cat ${newpath} > ${destpath}`).toString(); // OK.
 
 execSync(`cat ${files.join(' ')} > ${outFile}`); // OK
 
-execSync(`cat ${files.join(' ')}`); // OK - not just a simple file read
+execSync(`cat ${files.join(' ')}`); // OK [but flagged] - not just a simple file read
 
 exec("cat /proc/cpuinfo | grep name"); // OK - pipes
 
@@ -81,7 +81,7 @@ execFileSync('/bin/cat', [ 'pom.xml' ],  opts); // NOT OK
 var anOptsFileNameThatIsTooLongToBePrintedByToString = {encoding: 'utf8'};
 execFileSync('/bin/cat', [ 'pom.xml' ],  anOptsFileNameThatIsTooLongToBePrintedByToString); // NOT OK
 
-execFileSync('/bin/cat', [ 'pom.xml' ],  {encoding: 'someEncodingValueThatIsCompletelyBogusAndTooLongForToString'}); // NOT OK [but not flagged]
+execFileSync('/bin/cat', [ 'pom.xml' ],  {encoding: 'someEncodingValueThatIsCompletelyBogusAndTooLongForToString'}); // NOT OK
 
 execFileSync('/bin/cat', [ "foo/" + newPath + "bar" ],  {encoding: 'utf8'}); // NOT OK
 
@@ -97,4 +97,38 @@ exec("cat foo/bar", (err, out) => {console.log(out)}); // NOT OK
 
 exec("cat foo/bar", (err, out) => doSomethingWith(out)); // NOT OK
 
-execFileSync('/bin/cat', [ 'pom.xml' ],  unknownOptions); // OK - unknown options.
+execFileSync('/bin/cat', [ 'pom.xml' ],  unknownOptions); // OK - unknown options.
+
+exec("node foo/bar", (err, out) => doSomethingWith(out)); // OK - Not a call to cat
+
+execFileSync('node', [ `cat` ]); // OK - not a call to cat
+
+exec("cat foo/bar&", function (err, out) {}); // OK - contains &
+exec("cat foo/bar,", function (err, out) {}); // OK - contains ,
+exec("cat foo/bar$", function (err, out) {}); // OK - contains $
+exec("cat foo/bar`", function (err, out) {}); // OK - contains `
+
+spawn('cat', { stdio: ['pipe', stdin, 'inherit'] }); // OK - Non trivial use. (But weird API use.)
+
+(function () {
+  const cat = spawn('cat', [filename]); // OK - non trivial use.
+  cat.stdout.on('data', (data) => {
+    res.write(data);
+  });
+  cat.stdout.on('end', () => res.end());
+})();
+
+var dead = exec("cat foo/bar", (err, out) => {console.log(out)}); // NOT OK
+
+var notDead = exec("cat foo/bar", (err, out) => {console.log(out)}); // OK
+console.log(notDead);
+
+(function () {
+  var dead = exec("cat foo/bar", (err, out) => {console.log(out)}); // NOT OK
+
+  someCall(
+	exec("cat foo/bar", (err, out) => {console.log(out)}) // OK - non-trivial use of returned proccess.
+  );
+
+  return exec("cat foo/bar", (err, out) => {console.log(out)}); // OK - non-trivial use of returned proccess.
+})();