add query for useless use of cat

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

@@ -0,0 +1,20 @@
+/**
+ * @name Useless use of cat
+ * @description Using cat to simply read a file can lead to unintended bugs, and at worst security issues.
+ * @kind problem
+ * @problem.severity error
+ * @precision high
+ * @id js/useless-use-of-cat
+ * @tags correctness
+ *       security
+ *       external/cwe/cwe-078
+ *       external/cwe/cwe-088
+ */
+
+import javascript
+import semmle.javascript.security.UselessUseOfCat
+
+from UselessCat cat
+select cat.getCommand(), "Useless use of `cat` in $@.", cat, "command execution"
+
+

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

@@ -0,0 +1,59 @@
+/**
+ * Provides predicates and classes for working with useless uses of `cat`.
+ */
+
+import javascript
+import semmle.javascript.security.dataflow.IndirectCommandArgument
+
+/**
+ * Gets the first string from a string/string-concatenation.
+ */
+private string getStartingString(DataFlow::Node node) {
+  node.mayHaveStringValue(result) or
+  node.(StringOps::ConcatenationRoot).getFirstLeaf().mayHaveStringValue(result)
+}
+
+/**
+ * Gets a string from a string/string-concatenation.
+ */
+private string getAString(DataFlow::Node node) {
+  node.mayHaveStringValue(result) or
+  node.(StringOps::ConcatenationRoot).getALeaf().mayHaveStringValue(result)
+}
+
+/**
+ * An command-line execution of `cat` that only reads a file.
+ */
+class UselessCat extends DataFlow::Node {
+  DataFlow::Node command;
+
+  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 + " .*"))
+      ) and
+      // `cat` is OK in combination with pipes and wildcards.
+      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()
+        )
+      )
+    )
+  }
+
+  /**
+   * Gets the dataflow node determining the command executed.
+   */
+  DataFlow::Node getCommand() { result = command }
+}

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

@@ -89,6 +89,10 @@ nodes
 | third-party-command-injection.js:5:20:5:26 | command |
 | third-party-command-injection.js:6:21:6:27 | command |
 | third-party-command-injection.js:6:21:6:27 | command |
+| uselesscat.js:18:20:18:54 | 'cat '  ... ms.data |
+| uselesscat.js:18:20:18:54 | 'cat '  ... ms.data |
+| uselesscat.js:18:40:18:54 | req.params.data |
+| uselesscat.js:18:40:18:54 | req.params.data |
 edges
 | child_process-test.js:6:9:6:49 | cmd | child_process-test.js:17:13:17:15 | cmd |
 | child_process-test.js:6:9:6:49 | cmd | child_process-test.js:17:13:17:15 | cmd |
@@ -178,6 +182,10 @@ edges
 | third-party-command-injection.js:5:20:5:26 | command | third-party-command-injection.js:6:21:6:27 | command |
 | third-party-command-injection.js:5:20:5:26 | command | third-party-command-injection.js:6:21:6:27 | command |
 | third-party-command-injection.js:5:20:5:26 | command | third-party-command-injection.js:6:21:6:27 | command |
+| uselesscat.js:18:40:18:54 | req.params.data | uselesscat.js:18:20:18:54 | 'cat '  ... ms.data |
+| uselesscat.js:18:40:18:54 | req.params.data | uselesscat.js:18:20:18:54 | 'cat '  ... ms.data |
+| uselesscat.js:18:40:18:54 | req.params.data | uselesscat.js:18:20:18:54 | 'cat '  ... ms.data |
+| uselesscat.js:18:40:18:54 | req.params.data | uselesscat.js:18:20:18:54 | 'cat '  ... ms.data |
 #select
 | child_process-test.js:17:13:17:15 | cmd | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:17:13:17:15 | cmd | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
 | child_process-test.js:18:17:18:19 | cmd | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:18:17:18:19 | cmd | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
@@ -211,3 +219,4 @@ edges
 | other.js:18:22:18:24 | cmd | other.js:5:25:5:31 | req.url | other.js:18:22:18:24 | cmd | This command depends on $@. | other.js:5:25:5:31 | req.url | a user-provided value |
 | other.js:19:36:19:38 | cmd | other.js:5:25:5:31 | req.url | other.js:19:36:19:38 | cmd | This command depends on $@. | other.js:5:25:5:31 | req.url | a user-provided value |
 | third-party-command-injection.js:6:21:6:27 | command | third-party-command-injection.js:5:20:5:26 | command | third-party-command-injection.js:6:21:6:27 | command | This command depends on $@. | third-party-command-injection.js:5:20:5:26 | command | a server-provided value |
+| uselesscat.js:18:20:18:54 | 'cat '  ... ms.data | uselesscat.js:18:40:18:54 | req.params.data | uselesscat.js:18:20:18:54 | 'cat '  ... ms.data | This command depends on $@. | uselesscat.js:18:40:18:54 | req.params.data | a user-provided value |

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

@@ -0,0 +1,2 @@
+| False negative | uselesscat.js:69:42:69:69 | // NOT  ... lagged] |
+| False positive | uselesscat.js:18:70:18:118 | // OK [ ... jection |

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

@@ -0,0 +1,18 @@
+import javascript
+import semmle.javascript.security.UselessUseOfCat
+
+from LineComment comment, string msg
+where
+  comment.getFile().getAbsolutePath().regexpMatch(".*/uselesscat.js") and
+  (
+    comment.getText().regexpMatch(".*NOT OK.*") and
+    not any(UselessCat cat).asExpr().getLocation().getStartLine() =
+      comment.getLocation().getStartLine() and
+    msg = "False negative"
+    or
+    comment.getText().regexpMatch(".* OK.*") and
+    not comment.getText().regexpMatch(".*NOT OK.*") and
+    any(UselessCat cat).asExpr().getLocation().getStartLine() = comment.getLocation().getStartLine() and
+    msg = "False positive"
+  )
+select msg, comment

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

@@ -0,0 +1,73 @@
+var express = require('express');
+var child_process = require('child_process');
+var execSync = child_process.execSync;
+var exec = child_process.exec;
+var spawn = child_process.spawn;
+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
+		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 /proc/cpuinfo | grep -c '" + someValue + "'"); // OK - pipes
+
+function cat(file) {
+	return execSync('cat ' + file).toString(); // NOT OK [flagged]
+}
+
+execSync(`cat ${files.join(' ')} > ${outFile}`); // NOT OK [flagged]
+
+var cmd = 'cat package.json | grep'
+exec(cmd); // OK - pipes!
+
+execSync("sudo cat " + newpath + "*.js | grep foo").toString(); // OK - wildcard and pipes
+
+execSync(`cat ${newpath}`); // NOT OK
+
+exec("cat /proc/cpuinfo | grep name"); // OK - pipes
+
+execSync(`cat ${newpath} | ${othertool}`); // OK - pipes
+
+execSync("sh -c 'cat " + newpath + "'"); // NOT OK. [but not flagged]
+
+exec(` cat ${newpath}`) // NOT OK
+
+exec(` cat ${newpath} | grep foo`) // OK - pipes