change useless cat query to only flag instances that can be re-written to

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

@@ -14,7 +14,8 @@
 import javascript
 import semmle.javascript.security.UselessUseOfCat
 
+
 from UselessCat cat
-select cat.getCommand(), "Useless use of `cat` in $@.", cat, "command execution"
+select cat, "Useless use of `cat`. Can be replaced with: " + createReadFileCall(cat)
 
 

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

@@ -6,54 +6,312 @@ import javascript
 import semmle.javascript.security.dataflow.IndirectCommandArgument
 
 /**
- * Gets the first string from a string/string-concatenation.
+ * Gets a string representing an equivalent call to `fs.ReadFile` for a call to `cat`.
  */
-private string getStartingString(DataFlow::Node node) {
-  node.mayHaveStringValue(result) or
-  node.(StringOps::ConcatenationRoot).getFirstLeaf().mayHaveStringValue(result)
+string createReadFileCall(UselsesCatCandidates::UselessCatCandicate cat) {
+  exists(string sync, string extraArg, string callback |
+    (if cat.isSync() then sync = "Sync" else sync = "") and
+    (
+      if exists(cat.getOptionsArg())
+      then extraArg = ", " + printOptionsArg(cat.getOptionsArg()) + ")"
+      else extraArg = ""
+    ) and
+    if exists(cat.getCallback())
+    then callback = ", function(" + getCallbackArgs(cat.getCallback()) + ") {...}"
+    else callback = ""
+  |
+    result = "fs.readFile" + sync + "(" + cat.getFileArgument().trim() + extraArg + callback + ")"
+  )
 }
 
 /**
- * Gets a string from a string/string-concatenation.
+ * Gets a string concatenation of the parameters to a function.
  */
-private string getAString(DataFlow::Node node) {
-  node.mayHaveStringValue(result) or
-  node.(StringOps::ConcatenationRoot).getALeaf().mayHaveStringValue(result)
+private string getCallbackArgs(DataFlow::FunctionNode func) {
+  result = concat(int i | i = [0 .. 2] | func.getParameter(i).getName(), ", ")
 }
 
 /**
- * An command-line execution of `cat` that only reads a file.
+ * Gets a string representation of the options argument from an exec-like call.
  */
-class UselessCat extends DataFlow::Node {
-  DataFlow::Node command;
+private string printOptionsArg(DataFlow::Node node) {
+  result = node.asExpr().(VarAccess).getVariable().getName()
+  or
+  // fall back to toString(), but ensure that we don't have dots in the middle.
+  result = node.(DataFlow::ObjectLiteralNode).toString() and not result.regexpMatch(".*\\.\\..*")
+}
+
+/**
+ * A call to a useless use of `cat`.
+ */
+class UselessCat extends DataFlow::CallNode {
+  UselsesCatCandidates::UselessCatCandicate candidate;
 
   UselessCat() {
-    command = this.(SystemCommandExecution).getACommandArgument() and
-    exists(string cat |
-      cat = "cat" or cat = "/bin/cat" or cat = "sudo cat" or cat = "sudo /bin/cat"
-    |
-      exists(string commandString |
-        commandString = getStartingString(command).trim() and
-        (commandString = cat or commandString.regexpMatch(cat + " .*"))
+    this = candidate 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
+    // Only acceptable option is "encoding", everything else is non-trivial to emulate with fs.readFile.
+    not exists(string prop |
+      not prop = "encoding" and
+      exists(candidate.getOptionsArg().getALocalSource().getAPropertyWrite(prop))
     ) and
-      // `cat` is OK in combination with pipes, wildcards, and redirections.
-      not getAString(command).regexpMatch(".*(\\*|\\||>|<).*") and
-      // It is OK just to spawn "cat" without any arguments.
-      not (
-        command.mayHaveStringValue(cat) and
-        not exists(
-          this
-              .(SystemCommandExecution)
-              .getArgumentList()
-              .(DataFlow::ArrayCreationNode)
-              .getAnElement()
+    exists(createReadFileCall(this)) 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())
+      or
+      exists(DataFlow::FunctionNode func | func = candidate.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(DataFlow::Node node |
+          not node = func.getParameter(2) and func.getParameter(2) = node.getALocalSource()
+        )
+      )
+    )
+  }
+}
+
+module UselsesCatCandidates {
+  /**
+   * A candidate for a useless use of `cat`.
+   * Subclasses of this class specify the structure of the `exec`-like call.
+   */
+  abstract class UselessCatCandicate extends DataFlow::CallNode {
+    /**
+     * Holds if the call is synchronous (e.g. `execFileSync`).
+     */
+    abstract predicate isSync();
+
+    /**
+     * Gets a string representation of the expression that determines what file is read.
+     */
+    abstract string getFileArgument();
+
+    /**
+     * Gets the data-flow node for the options argument to the `exec`-like call.
+     */
+    abstract DataFlow::Node getOptionsArg();
+
+    /**
+     * Gets the callback used for the `exec` like call (if it exists).
+     */
+    abstract DataFlow::FunctionNode getCallback();
+  }
+
+  /**
+   * Create a string representation of a string concatenation.
+   */
+  private string createConcatRepresentation(StringOps::ConcatenationRoot root) {
+    // String concat
+    not exists(root.getStringValue()) and
+    not root.asExpr() instanceof TemplateLiteral and
+    forall(Expr e | e = root.getALeaf().asExpr() | exists(createLeafRepresentation(e))) and
+    result =
+      concat(Expr leaf |
+        leaf = root.getALeaf().asExpr()
+      |
+        createLeafRepresentation(leaf), "+" order by leaf.getFirstToken().getIndex()
+      )
+    or
+    // Template string
+    exists(TemplateLiteral template | template = root.asExpr() |
+      forall(Expr e | e = template.getAChild() | exists(createTemplateElementRepresentation(e))) and
+      result =
+        "`" +
+          concat(int i |
+            i = [0 .. template.getNumChild() - 1]
+          |
+            createTemplateElementRepresentation(template.getChild(i)) order by i
+          ) + "`"
+    )
+  }
+
+  /**
+   * Gets 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
+    result = e.(VarAccess).getVariable().getName()
+  }
+
+  /**
+   * Gets 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() + "}"
+    or
+    result = e.(TemplateElement).getValue()
+  }
+
+  /**
+   * 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`.
+   * 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() |
+      exists(string command | arg.mayHaveStringValue(command) |
+        command.prefix(cat.length()) = cat and
+        result = "\"" + command.suffix(cat.length()).trim() + "\""
+      )
+      or
+      exists(StringOps::ConcatenationRoot root, string printed, string quote |
+        root = arg and printed = createConcatRepresentation(root).suffix(1) // remove initial quote
+      |
+        (if root.asExpr() instanceof TemplateLiteral then quote = "`" else quote = "\"") and
+        root.getFirstLeaf().getStringValue().prefix(cat.length()) = cat and
+        // Remove an initial ""+ (e.g. in `""+file`)
+        exists(string rawConcat | rawConcat = quote + printed.suffix(cat.length()).trim() |
+          if rawConcat.prefix(3) = "\"\"+" then result = rawConcat.suffix(3) else result = rawConcat
         )
       )
     )
   }
 
   /**
-   * Gets the dataflow node determining the command executed.
+   * A call to child_process.exec that might be a useless call to cat.
    */
-  DataFlow::Node getCommand() { result = command }
+  private class ExecCall extends UselessCatCandicate {
+    string fileArgument;
+    boolean hasOptions;
+
+    ExecCall() {
+      this = DataFlow::moduleImport("child_process").getAMemberCall("exec") and
+      (
+        this.getNumArgument() = 2 and hasOptions = false
+        or
+        this.getNumArgument() = 3 and hasOptions = true
+      ) and
+      fileArgument = getFileArgumentWithoutCat(getArgument(0))
+    }
+
+    override predicate isSync() { none() }
+
+    override string getFileArgument() { result = fileArgument }
+
+    override DataFlow::Node getOptionsArg() { hasOptions = true and result = getArgument(1) }
+
+    override DataFlow::FunctionNode getCallback() { result = getLastArgument() }
+  }
+
+  /**
+   * A call to child_process.execSync that might be a useless call to cat.
+   */
+  private class ExecSyncCall extends UselessCatCandicate {
+    string fileArgument;
+    boolean hasOptions;
+
+    ExecSyncCall() {
+      this = DataFlow::moduleImport("child_process").getAMemberCall("execSync") and
+      (
+        this.getNumArgument() = 1 and hasOptions = false
+        or
+        this.getNumArgument() = 2 and hasOptions = true
+      ) and
+      fileArgument = getFileArgumentWithoutCat(getArgument(0))
+    }
+
+    override predicate isSync() { any() }
+
+    override string getFileArgument() { result = fileArgument }
+
+    override DataFlow::Node getOptionsArg() { hasOptions = true and result = getArgument(1) }
+
+    override DataFlow::FunctionNode getCallback() { none() }
+  }
+
+  // TODO: No!
+  bindingset[str, quote]
+  string surroundInQuotes(string str, string quote) {
+    if not str.prefix(1) = quote and not str.suffix(str.length() - 1) = quote
+    then result = quote + str + quote
+    else
+      if not str.prefix(1) = quote
+      then result = str + quote
+      else
+        if not str.suffix(str.length() - 1) = quote
+        then result = quote + str
+        else result = str
+  }
+
+  /**
+   * Gets the file that is read for a call to child_process.execFile/execFileSync.
+   */
+  string getFileThatIsRead(DataFlow::CallNode call) {
+    exists(DataFlow::ArrayCreationNode array, DataFlow::Node element |
+      array = call.getArgument(1).(DataFlow::ArrayCreationNode) and
+      array.getSize() = 1 and
+      element = array.getElement(0)
+    |
+      result = element.asExpr().(VarAccess).getVariable().getName() or
+      result = "\"" + element.getStringValue() + "\"" or
+      result = createConcatRepresentation(element)
+    )
+  }
+
+  /**
+   * A call to child_process.execFile that might be a useless call to cat.
+   */
+  private class ExecFileCall extends UselessCatCandicate {
+    string fileArgument;
+    boolean hasOptions;
+
+    ExecFileCall() {
+      this.getArgument(0).mayHaveStringValue(cat()) and
+      this = DataFlow::moduleImport("child_process").getAMemberCall("execFile") and
+      (
+        this.getNumArgument() = 3 and hasOptions = false
+        or
+        this.getNumArgument() = 4 and hasOptions = true
+      ) and
+      fileArgument = getFileThatIsRead(this)
+    }
+
+    override predicate isSync() { none() }
+
+    override string getFileArgument() { result = fileArgument }
+
+    override DataFlow::Node getOptionsArg() { hasOptions = true and result = getArgument(2) }
+
+    override DataFlow::FunctionNode getCallback() { result = getLastArgument() }
+  }
+
+  /**
+   * A call to child_process.execFileSync that might be a useless call to cat.
+   */
+  private class ExecFileSyncCall extends UselessCatCandicate {
+    string fileArgument;
+    boolean hasOptions;
+
+    ExecFileSyncCall() {
+      this.getArgument(0).mayHaveStringValue(cat()) and
+      this = DataFlow::moduleImport("child_process").getAMemberCall("execFileSync") and
+      (
+        this.getNumArgument() = 2 and hasOptions = false
+        or
+        this.getNumArgument() = 3 and hasOptions = true
+      ) and
+      fileArgument = getFileThatIsRead(this)
+    }
+
+    override predicate isSync() { any() }
+
+    override string getFileArgument() { result = fileArgument }
+
+    override DataFlow::Node getOptionsArg() { hasOptions = true and result = getArgument(2) }
+
+    override DataFlow::FunctionNode getCallback() { none() }
+  }
 }

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

@@ -1,3 +0,0 @@
-| False negative | uselesscat.js:69:42:69:69 | // NOT  ... lagged] |
-| False positive | uselesscat.js:18:70:18:118 | // OK [ ... jection |
-| False positive | uselesscat.js:82:80:82:128 | // OK ( ... / gid)) |

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

@@ -0,0 +1,20 @@
+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: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}`) |
+| uselesscat.js:34:1:34:54 | execSyn ... utf8'}) | fs.readFileSync(`foo/bar/${newpath}`, {encoding: 'utf8'})) |
+| uselesscat.js:51:9:51:31 | execSyn ... + file) | fs.readFileSync(file) |
+| uselesscat.js:59:1:62:2 | execFil ... ut);\\n}) | fs.readFile("pom.xml", function(error, stderr, stdout) {...}) |
+| uselesscat.js:69:1:72:2 | execFil ... ut);\\n}) | fs.readFile("pom.xml", {encoding: 'utf8'}), function(error, stderr, stdout) {...}) |
+| uselesscat.js:74:1:74:60 | execFil ... utf8'}) | fs.readFileSync("pom.xml", {encoding: 'utf8'})) |
+| 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:90:1:90:50 | execFil ... th}` ]) | fs.readFileSync(`foo/bar/${newpath}`) |
+#select
+| False negative | uselesscat.js:54:42:54:69 | // NOT  ... lagged] |
+| False negative | uselesscat.js:84:118:84:144 | // NOT  ... lagged] |

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

@@ -16,3 +16,5 @@ where
     msg = "False positive"
   )
 select msg, comment
+
+query string readFile(UselessCat cat) { result = createReadFileCall(cat) }

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

@@ -7,76 +7,86 @@ var spawnSync = child_process.spawnSync;
 var fs = require('fs');
 var app = express();
 
-function readStatus(id) {
-	exec("cat /proc/" + id + "/status", function (err, out) { // NOT OK
+exec("cat foo/bar", function (err, out) {}); // NOT OK
+
+exec("cat /proc/" + id + "/status", function (err, out) { // NOT OK
 	console.log(out);
-	});
-};
-
-var basePath = '/foo/bar';
-app.get('/:data', function (req, res) {
-	res.send(execSync('cat ' + basePath + req.params.data).toString()); // OK [but flagged] - caught by command-injection
 });
 
-spawn('cat', ['package.json']); // NOT OK
-
-spawn('/bin/cat', ['package.json']); // NOT 
-
-spawn('/bin/cat', [someValue]); // NOT OK
-spawn('sudo /bin/cat', [someValue]); // NOT OK
-spawn('sudo cat', [someValue]); // NOT OK  
-
-child_process.spawnSync('cat', [someValue]); // NOT OK
-
-child_process.execSync('cat package.json | wc -l'); // OK - pipes!
-
-const cat = child_process.spawn('cat') // OK - benign use (usually pipe to and from).
-
-spawn('cat', ['package.json']); // NOT OK.
-
-exec('cat *.js') // OK - wildcard use.
-exec('cat *.js | wc -l') // OK - wildcard use and pipes!
-exec('cat *.js | wc -l', { cwd: './' }, function () { }); // OK - wildcard and pipes
-
-spawnSync('cat', ['/proc/cpuinfo']) // NOT OK.
-
-exec(`cat ${path.join(__dirname, 'package.json')} | sort | uniq`, () => {}); // OK: pipes
-
 execSync('cat /proc/cpuinfo').toString(); // NOT OK.
 
-var cmd = "cat /proc/cpuinfo"
-execSync(cmd); // NOT OK
+execSync(`cat ${newpath}`) // NOT OK
 
-execSync("cat /proc/cpuinfo | grep -c '" + someValue + "'"); // OK - pipes
+child_process.execSync('cat package.json | wc -l'); // OK - pipes!
 
-function cat(file) {
-	return execSync('cat ' + file).toString(); // NOT OK
-}
+execSync('cat /proc/cpuinfo /foo/bar').toString(); // OK multiple files.
 
-execSync(`cat ${files.join(' ')} > ${outFile}`); // OK
+execSync(`cat ${newpath} /foo/bar`).toString(); // OK multiple files.
 
-var cmd = 'cat package.json | grep'
-exec(cmd); // OK - pipes!
+exec(`cat ${newpath} | grep foo`, function (err, out) { }) // OK - pipes
 
-execSync("sudo cat " + newpath + "*.js | grep foo").toString(); // OK - wildcard and pipes
+execSync(`cat ${newpath}`, {uid: 1000}) // OK - non trivial options
 
-execSync(`cat ${newpath}`); // NOT OK
+exec('cat *.js | wc -l', { cwd: './' }, function (err, out) { }); // OK - wildcard and pipes
 
-exec("cat /proc/cpuinfo | grep name"); // OK - pipes
+execSync(`cat foo/bar/${newpath}`); // NOT OK ("encoding" is used EXACTLY the same way in fs.readFileSync)
 
-execSync(`cat ${newpath} | ${othertool}`); // OK - pipes
+execSync(`cat foo/bar/${newpath}`, {encoding: 'utf8'}); // NOT OK ("encoding" is used EXACTLY the same way in fs.readFileSync)
 
-execSync("sh -c 'cat " + newpath + "'"); // NOT OK. [but not flagged]
-
-exec(` cat ${newpath}`) // NOT OK
-
-exec(` cat ${newpath} | grep foo`) // OK - pipes
+execSync("/bin/cat /proc/cpuinfo", { uid: 1000, gid: 1000, encoding: 'utf8'}); // OK (fs.readFileSync cannot emulate uid / gid))
 
 execSync('cat /proc/cpuinfo > foo/bar/baz').toString(); // OK.
 
 execSync(`cat ${newpath} > ${destpath}`).toString(); // OK.
 
-const Opts = {encoding: 'utf8'}
-execSync(`cat foo/bar/${newpath}`, Opts).slice(0, 7); // NOT OK ("encoding" is used EXACTLY the same way in fs.readFileSync)
+execSync(`cat ${files.join(' ')} > ${outFile}`); // OK
 
-execSync("/bin/cat /proc/cpuinfo", { uid: 1000, gid: 1000, encoding: 'utf8'}); // OK (fs.readFileSync cannot emulate uid / gid))
+execSync(`cat ${files.join(' ')}`); // OK - not just a simple file read
+
+exec("cat /proc/cpuinfo | grep name"); // OK - pipes
+
+execSync(`cat ${newpath} | ${othertool}`); // OK - pipes
+
+function cat(file) {
+	return execSync('cat ' + file).toString(); // NOT OK
+}
+
+execSync("sh -c 'cat " + newpath + "'"); // NOT OK. [but not flagged]
+
+var execFile = child_process.execFile;
+var execFileSync = child_process.execFileSync;
+
+execFile('/bin/cat', [ 'pom.xml' ], function(error, stdout, stderr ) { // NOT OK
+  // Not using stderr
+  console.log(stdout);
+});
+
+execFile('/bin/cat', [ 'pom.xml' ], function(error, stdout, stderr ) { // OK. - stderr is used.
+  console.log(stderr); 
+});
+
+
+execFile('/bin/cat', [ 'pom.xml' ],  {encoding: 'utf8'}, function(error, stdout, stderr ) { // NOT OK
+  // Not using stderr
+  console.log(stdout);
+});
+
+execFileSync('/bin/cat', [ 'pom.xml' ],  {encoding: 'utf8'}); // NOT OK
+
+execFileSync('/bin/cat', [ 'pom.xml' ]);  // NOT OK
+
+var opts = {encoding: 'utf8'};
+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', [ "foo/" + newPath + "bar" ],  {encoding: 'utf8'}); // NOT OK
+
+execSync('cat /proc/cpuinfo' + foo).toString(); // NOT OK.
+
+execFileSync('/bin/cat', [ `foo/bar/${newpath}` ]); // NOT OK
+
+execFileSync('node', [ `foo/bar/${newpath}` ]); // OK - not a call to cat