Merge pull request github#2867 from erik-krogh/UselessCat

Approved by esbena

Author: semmle-qlci

change-notes/1.24/analysis-javascript.md

@@ -46,6 +46,8 @@
 | Polynomial regular expression used on uncontrolled data (`js/polynomial-redos`) | security, external/cwe/cwe-730, external/cwe/cwe-400 | Highlights expensive regular expressions that may be used on malicious input. Results are shown on LGTM by default. | 
 | Prototype pollution in utility function (`js/prototype-pollution-utility`) | security, external/cwe/cwe-400, external/cwe/cwe-471 | Highlights recursive copying operations that are susceptible to prototype pollution. Results are shown on LGTM by default. |
 | Unsafe jQuery plugin (`js/unsafe-jquery-plugin`) | Highlights potential XSS vulnerabilities in unsafely designed jQuery plugins. Results are shown on LGTM by default. |
+| Unnecessary use of `cat` process (`js/unnecessary-use-of-cat`) | correctness, security, maintainability | Highlights command executions of `cat` where the fs API should be used instead. Results are shown on LGTM by default. |
+
 
 ## Changes to existing queries
 

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

@@ -0,0 +1,45 @@
+<!DOCTYPE qhelp PUBLIC
+"-//Semmle//qhelp//EN"
+"qhelp.dtd">
+<qhelp>
+<overview>
+<p>Using the unix command <code>cat</code> only to read a file is an 
+unnecessarily complex way to achieve something that can be done in a simpler 
+and safer manner using the Node.js <code>fs.readFile</code> API.
+</p>
+<p>
+The use of <code>cat</code> for simple file reads leads to code that is 
+unportable, inefficient, complex, and can lead to subtle bugs or even 
+security vulnerabilities. 
+</p>
+</overview>
+<recommendation>
+<p>
+Use <code>fs.readFile</code> or <code>fs.readFileSync</code> to read files 
+from the file system. 
+</p>
+</recommendation>
+<example>
+
+<p>The following example shows code that reads a file using <code>cat</code>:</p>
+
+<sample src="examples/useless-cat.js"/>
+
+<p>The code in the example will break if the input <code>name</code> contains 
+special characters (including space). Additionally, it does not work on Windows 
+and if the input is user-controlled, a command injection attack can happen.</p>
+
+<p>The <code>fs.readFile</code> API should be used to avoid these potential issues: </p>
+
+<sample src="examples/useless-cat-fixed.js"/>
+
+</example>
+<references>
+
+<li>OWASP: <a href="https://www.owasp.org/index.php/Command_Injection">Command Injection</a>.</li>
+<li>Node.js: <a href="https://nodejs.org/api/fs.html">File System API</a>.</li>
+<li><a href="http://porkmail.org/era/unix/award.html#cat">The Useless Use of Cat Award</a>.</li>
+
+
+</references>
+</qhelp>

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

@@ -0,0 +1,25 @@
+/**
+ * @name Unnecessary use of `cat` process
+ * @description Using the  `cat` process to read a file is unnecessarily complex, inefficient, unportable, and can lead to subtle bugs, or even security vulnerabilities.
+ * @kind problem
+ * @problem.severity error
+ * @precision high
+ * @id js/unnecessary-use-of-cat
+ * @tags correctness
+ *       security
+ *       maintainability
+ */
+
+import javascript
+import semmle.javascript.security.UselessUseOfCat
+import semmle.javascript.RestrictedLocations
+
+from UselessCat cat, string message
+where
+  message = " Can be replaced with: " + PrettyPrintCatCall::createReadFileCall(cat)
+  or
+  not exists(PrettyPrintCatCall::createReadFileCall(cat)) and
+  if cat.isSync()
+  then message = " Can be replaced with a call to fs.readFileSync(..)."
+  else message = " Can be replaced with a call to fs.readFile(..)."
+select cat.asExpr().(FirstLineOf), "Unnecessary use of `cat` process." + message

javascript/ql/src/Security/CWE-078/examples/useless-cat-fixed.js

@@ -0,0 +1,5 @@
+var fs = require('fs');
+
+module.exports = function (name) {
+    return fs.readFileSync(name).toString();
+};

javascript/ql/src/Security/CWE-078/examples/useless-cat.js

@@ -0,0 +1,5 @@
+var child_process = require('child_process');
+
+module.exports = function (name) {
+    return child_process.execSync("cat " + name).toString();
+};

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

@@ -22,6 +22,14 @@ abstract class SystemCommandExecution extends DataFlow::Node {
    * to the command.
    */
   DataFlow::Node getArgumentList() { none() }
+
+  /** 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

@@ -621,6 +621,22 @@ module NodeJSLib {
       // all of the above methods take the argument list as their second argument
       result = getArgument(1)
     }
+
+    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

@@ -160,6 +160,15 @@ module ShellJS {
     override DataFlow::Node getACommandArgument() { result = getArgument(0) }
 
     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,14 +7,17 @@ import javascript
 
 private class SystemCommandExecutors extends SystemCommandExecution, DataFlow::InvokeNode {
   int cmdArg;
+  int optionsArg; // either a positive number representing the n'th argument, or a negative number representing the n'th last argument (e.g. -2 is the second last argument).
   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
+        optionsArg = -1 and
         (
           shell = false and
           (
@@ -30,27 +33,30 @@ private class SystemCommandExecutors extends SystemCommandExecution, DataFlow::I
         ) and
         cmdArg = 0
       |
-        callee = DataFlow::moduleMember(mod, method)
+        callee = DataFlow::moduleMember(mod, method) and
+        sync = getSync(method)
       )
       or
+      sync = false and
       (
         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
+          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)
@@ -64,4 +70,30 @@ private class SystemCommandExecutors extends SystemCommandExecution, DataFlow::I
   override predicate isShellInterpreted(DataFlow::Node arg) {
     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) and
+        getNumArgument() + optionsArg > cmdArg
+      else result = getArgument(optionsArg)
+    ) and
+    not result.getALocalSource() instanceof DataFlow::FunctionNode and // looks like callback
+    not result.getALocalSource() instanceof DataFlow::ArrayCreationNode // looks like argumentlist
+  }
+}
+
+/**
+ * Gets a boolean reflecting if the name ends with "sync" or "Sync".
+ */
+bindingset[name]
+private boolean getSync(string name) {
+  if name.suffix(name.length() - 4) = "Sync" or name.suffix(name.length() - 4) = "sync"
+  then result = true
+  else result = false
 }