Merge branch 'master' of github.com:Semmle/ql into js/more-fs-modules

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

@@ -632,6 +632,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
 }

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

@@ -0,0 +1,331 @@
+/**
+ * Provides predicates and classes for working with useless uses of the unix command `cat`.
+ */
+
+import javascript
+import Expressions.ExprHasNoEffect
+import Declarations.UnusedVariable
+
+/**
+ * A call that executes a system command.
+ * This class provides utility predicates for reasoning about command execution calls.
+ */
+private class CommandCall extends DataFlow::InvokeNode {
+  SystemCommandExecution command;
+
+  CommandCall() { this = command }
+
+  /**
+   * Holds if the call is synchronous (e.g. `execFileSync`).
+   */
+  predicate isSync() { command.isSync() }
+
+  /**
+   * Gets a list that specifies the arguments given to the command.
+   */
+  DataFlow::ArrayCreationNode getArgumentList() { result = command.getArgumentList().getALocalSource() }
+
+  /**
+   * 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(getArgumentList()) }
+
+  /**
+   * Gets the data-flow node (if it exists) for an options argument for an `exec`-like call.
+   */
+  DataFlow::Node getOptionsArg() { result = command.getOptionsArg() }
+
+  /**
+   * 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
+      )
+  }
+}
+
+/**
+ * Holds if the input `str` contains some character that might be interpreted in a non-trivial way by a shell. 
+ */ 
+bindingset[str]
+private predicate containsNonTrivialShellChar(string str) {
+  exists(str.regexpFind("\\*|\\||>|<| |\\$|&|,|\\`| |;", _, _))
+}
+
+/**
+ * Gets the constant string parts from a data-flow node.
+ * Either the result is a constant string value that the node can hold, or the node is a string-concatenation and the result is the string parts from the concatenation.
+ */
+private string getConstantStringParts(DataFlow::Node node) {
+  node.mayHaveStringValue(result)
+  or
+  result = node.(StringOps::ConcatenationRoot).getConstantStringParts()
+}
+
+/**
+ * A call to a useless use of `cat`.
+ */
+class UselessCat extends CommandCall {
+  UselessCat() {
+    this = command and
+    isACallTo(getACatExecuteable()) and
+    // There is a file to read, it's not just spawning `cat`.
+    not (
+      not exists(getArgumentList()) and
+      getArgument(0).mayHaveStringValue(getACatExecuteable())
+    ) and
+    // wildcards, pipes, redirections, other bash features, and multiple files (spaces) are OK.
+    not containsNonTrivialShellChar(getNonCommandConstantString()) and
+    // Only acceptable option is "encoding", everything else is non-trivial to emulate with fs.readFile.
+    (
+      not exists(getOptionsArg())
+      or
+      forex(string prop | exists(getOptionsArg().getALocalSource().getAPropertyWrite(prop)) |
+        prop = "encoding"
+      )
+    ) and
+    // If there is a callback, then it must either have one or two parameters, or if there is a third parameter it must be unused.
+    (
+      not exists(getCallback())
+      or
+      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()))
+      )
+    ) and
+    // The process returned by an async call is unused.
+    (
+      isSync()
+      or
+      inVoidContext(this.getEnclosingExpr())
+      or
+      this.getEnclosingExpr() = any(UnusedLocal v).getAnAssignedExpr()
+    )
+  }
+}
+
+/**
+ * Gets a string used to call `cat`.
+ */
+private string getACatExecuteable() {
+  result = "cat" or result = "/bin/cat"
+}
+
+/**
+ * Predicates for creating an equivalent call to `fs.readFile` from a command execution of `cat`.
+ */
+module PrettyPrintCatCall {
+  /**
+   * Create a string representation of an equivalent call to `fs.readFile` for a given command execution `cat`.
+   */
+  string createReadFileCall(UselessCat cat) {
+    exists(string sync, string extraArg, string callback, string fileArg |
+      (if cat.isSync() then sync = "Sync" else sync = "") and
+      (
+        exists(cat.getOptionsArg()) and
+        (
+          extraArg = ", " + createOptionsArg(cat.getOptionsArg())
+          or
+          not exists(createOptionsArg(cat.getOptionsArg())) and
+          extraArg = ", ..."
+        )
+        or
+        extraArg = "" and not exists(cat.getOptionsArg())
+      ) and
+      (
+        callback = createCallbackString(cat.getCallback())
+        or
+        callback = "" and not exists(cat.getCallback())
+      ) and
+      fileArg = createFileArgument(cat).trim() and
+     // sanity check in case of surprising `toString` results, other uses of `containsNonTrivialBashChar` should ensure that this conjunct will hold most of the time
+      not(containsNonTrivialShellChar(fileArg.regexpReplaceAll("\\$|\\`| ", ""))) // string concat might contain " ", template strings might contain "$" or `, and that is OK. 
+    |
+      result =
+        "fs.readFile" + sync + "(" + fileArg + extraArg + callback + ")"
+    )
+  }
+
+  /**
+   * 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 params | params = createCallbackParams(func) |
+      if func.getFunction() instanceof ArrowFunctionExpr
+      then
+        if func.getFunction().getBody() instanceof Expr
+        then result = ", (" + params + ") => ..."
+        else result = ", (" + params + ") => {...}"
+      else result = ", function(" + params + ") {...}"
+    )
+  }
+
+  /**
+   * Create a string concatenation of the parameter names in a function `func`.
+   */
+  private string createCallbackParams(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(".*\\.\\..*")
+  }
+
+  /**
+   * 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
+          ) + "`"
+    )
+  }
+
+  /**
+   * 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
+    result = e.(VarAccess).getVariable().getName()
+  }
+
+  /**
+   * 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() + "}"
+    or
+    result = e.(TemplateElement).getValue()
+  }
+
+  /**
+   * 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 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() + "\""
+      )
+      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
+        exists(string rawConcat | rawConcat = quote + printed.suffix(cat.length()).trim() |
+          result = createSimplifiedStringConcat(rawConcat)
+        )
+      )
+    )
+  }
+
+  /**
+   * Create a simplified and equivalent string concatenation for a given string concatenation `str`
+   */
+  bindingset[str]
+  private string createSimplifiedStringConcat(string str) {
+    // Remove an initial ""+ (e.g. in `""+file`)
+    if str.prefix(5) = "\"\" + "
+    then result = str.suffix(5)
+    else
+      // prettify `${newpath}` to just newpath
+      if
+        str.prefix(3) = "`${" and
+        str.suffix(str.length() - 2) = "}`" and
+        not str.suffix(3).matches("%{%")
+      then result = str.prefix(str.length() - 2).suffix(3)
+      else result = str
+  }
+
+  /**
+   * Create the file that is read for a call with an explicit command list (e.g. `child_process.execFile/execFileSync`).
+   */
+  string createFileThatIsReadFromCommandList(CommandCall call) {
+    exists(DataFlow::ArrayCreationNode array, DataFlow::Node element |
+      array = call.getArgumentList().(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)
+    )
+  }
+}

javascript/ql/src/semmle/javascript/security/dataflow/TaintedPath.qll

@@ -36,7 +36,7 @@ module TaintedPath {
       guard instanceof StartsWithDirSanitizer or
       guard instanceof IsAbsoluteSanitizer or
       guard instanceof ContainsDotDotSanitizer or
-      guard instanceof RelativePathStartsWithDotDotSanitizer or
+      guard instanceof RelativePathStartsWithSanitizer or
       guard instanceof IsInsideCheckSanitizer
     }
 

javascript/ql/src/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll

@@ -368,29 +368,47 @@ module TaintedPath {
    *   // pathname is safe
    * }
    * ```
+   *
+   * or
+   * ```
+   * var relative = path.resolve(pathname); // or path.normalize
+   * if(relative.startsWith(webroot) {
+   *   // pathname is safe
+   * } else {
+   *   // pathname is unsafe
+   * }
+   * ```
    */
-  class RelativePathStartsWithDotDotSanitizer extends DataFlow::BarrierGuardNode {
+  class RelativePathStartsWithSanitizer extends DataFlow::BarrierGuardNode {
     StringOps::StartsWith startsWith;
-    DataFlow::CallNode relativeCall;
+    DataFlow::CallNode pathCall;
+    string member;
 
-    RelativePathStartsWithDotDotSanitizer() {
+    RelativePathStartsWithSanitizer() {
+      (member = "relative" or member = "resolve" or member = "normalize") and
       this = startsWith and
-      relativeCall = NodeJSLib::Path::moduleMember("relative").getACall() and
+      pathCall = NodeJSLib::Path::moduleMember(member).getACall() and
       (
-        startsWith.getBaseString().getALocalSource() = relativeCall
+        startsWith.getBaseString().getALocalSource() = pathCall
         or
         startsWith
             .getBaseString()
             .getALocalSource()
             .(NormalizingPathCall)
             .getInput()
-            .getALocalSource() = relativeCall
+            .getALocalSource() = pathCall
       ) and
-      isDotDotSlashPrefix(startsWith.getSubstring())
+      (not member = "relative" or isDotDotSlashPrefix(startsWith.getSubstring()))
     }
 
     override predicate blocks(boolean outcome, Expr e) {
-      e = relativeCall.getArgument(1).asExpr() and outcome = startsWith.getPolarity().booleanNot()
+      member = "relative" and
+      e = pathCall.getArgument(1).asExpr() and
+      outcome = startsWith.getPolarity().booleanNot()
+      or
+      not member = "relative" and
+      e = pathCall.getArgument(0).asExpr() and
+      outcome = startsWith.getPolarity()
     }
   }