Merge remote-tracking branch 'upstream/master' into codeql-c-analysis/34-Bad-Overlap

Author: Dave Bartolomeo

change-notes/1.24/analysis-javascript.md

@@ -82,6 +82,7 @@
 | Use of password hash with insufficient computational effort (`js/insufficient-password-hash`) | Fewer false positive results | This query now recognizes additional cases that do not require secure hashing. |
 | Useless regular-expression character escape (`js/useless-regexp-character-escape`) | Fewer false positive results | This query now distinguishes escapes in strings and regular expression literals. |
 | Identical operands (`js/redundant-operation`) | Fewer results | This query now recognizes cases where the operands change a value using ++/-- expressions. |
+| Superfluous trailing arguments (`js/superfluous-trailing-arguments`) | Fewer results | This query now recognizes cases where a function uses the `Function.arguments` value to process a variable number of parameters. |
 
 ## Changes to libraries
 

java/ql/src/semmle/code/java/Member.qll

@@ -481,13 +481,13 @@ class GetterMethod extends Method {
 
 /**
  * A finalizer method, with name `finalize`,
- * return type `void` and modifier `protected`.
+ * return type `void` and no parameters.
  */
 class FinalizeMethod extends Method {
   FinalizeMethod() {
     this.hasName("finalize") and
     this.getReturnType().hasName("void") and
-    this.isProtected()
+    this.hasNoParameters()
   }
 }
 

javascript/ql/src/experimental/SockJS/SockJS.qll

@@ -0,0 +1,46 @@
+/**
+ * Provides classes for working with [SockJS](http://sockjs.org).
+ */
+
+import javascript
+
+/**
+ * A model of the `SockJS` websocket data handler (https://sockjs.org).
+ */
+module SockJS {
+  /**
+   * Access to user-controlled data object received from websocket
+   * For example:
+   * ```
+   * server.on('connection', function(conn) {
+   *     conn.on('data', function(message) {
+   *       ...
+   *     });
+   *  });
+   * ```
+   */
+  class SourceFromSocketJS extends RemoteFlowSource {
+    SourceFromSocketJS() {
+      exists(
+        DataFlow::CallNode createServer, DataFlow::CallNode connNode,
+        DataFlow::CallNode dataHandlerNode
+      |
+        createServer = appCreation() and
+        connNode = createServer.getAMethodCall("on") and
+        connNode.getArgument(0).getStringValue() = "connection" and
+        dataHandlerNode = connNode.getCallback(1).getParameter(0).getAMethodCall("on") and
+        dataHandlerNode.getArgument(0).getStringValue() = "data" and
+        this = dataHandlerNode.getCallback(1).getParameter(0)
+      )
+    }
+
+    override string getSourceType() { result = "input from SockJS WebSocket" }
+  }
+
+  /**
+   * Gets a new SockJS server.
+   */
+  private DataFlow::CallNode appCreation() {
+    result = DataFlow::moduleImport("sockjs").getAMemberCall("createServer")
+  }
+}

javascript/ql/src/experimental/SockJS/examples/server.js

@@ -0,0 +1,16 @@
+const express = require('express');
+const http = require('http');
+const sockjs = require('sockjs');
+
+const app = express();
+const server = http.createServer(app);
+const sockjs_echo = sockjs.createServer({});
+sockjs_echo.on('connection', function(conn) {
+    conn.on('data', function(message) {
+        var data = JSON.parse(message);
+        conn.write(JSON.stringify(eval(data.test)));
+    });
+});
+
+sockjs_echo.installHandlers(server, {prefix:'/echo'});
+server.listen(9090, '127.0.0.1');

javascript/ql/src/semmle/javascript/Functions.qll

@@ -117,7 +117,14 @@ class Function extends @function, Parameterized, TypeParameterized, StmtContaine
   ArgumentsVariable getArgumentsVariable() { result.getFunction() = this }
 
   /** Holds if the body of this function refers to the function's `arguments` variable. */
-  predicate usesArgumentsObject() { exists(getArgumentsVariable().getAnAccess()) }
+  predicate usesArgumentsObject() {
+    exists(getArgumentsVariable().getAnAccess())
+    or
+    exists(PropAccess read |
+      read.getBase() = getVariable().getAnAccess() and
+      read.getPropertyName() = "arguments"
+    )
+  }
 
   /**
    * Holds if this function declares a parameter or local variable named `arguments`.

javascript/ql/src/semmle/javascript/Promises.qll

@@ -167,6 +167,7 @@ module PromiseTypeTracking {
    * Gets the result from a single step through a promise, from `pred` to `result` summarized by `summary`.
    * This can be loading a resolved value from a promise, storing a value in a promise, or copying a resolved value from one promise to another.
    */
+  pragma[inline]
   DataFlow::SourceNode promiseStep(DataFlow::SourceNode pred, StepSummary summary) {
     exists(PromiseFlowStep step, string field | field = Promises::valueProp() |
       summary = LoadStep(field) and
@@ -184,6 +185,7 @@ module PromiseTypeTracking {
    * Gets the result from a single step through a promise, from `pred` with tracker `t2` to `result` with tracker `t`.
    * This can be loading a resolved value from a promise, storing a value in a promise, or copying a resolved value from one promise to another.
    */
+  pragma[inline]
   DataFlow::SourceNode promiseStep(
     DataFlow::SourceNode pred, DataFlow::TypeTracker t, DataFlow::TypeTracker t2
   ) {

javascript/ql/src/semmle/javascript/dataflow/Configuration.qll

@@ -958,18 +958,31 @@ private predicate reachableFromStoreBase(
         s2.getEndLabel())
   )
   or
-  exists(DataFlow::Node mid, PathSummary oldSummary, PathSummary newSummary |
-    reachableFromStoreBase(prop, rhs, mid, cfg, oldSummary) and
-    (
-      flowStep(mid, cfg, nd, newSummary)
-      or
-      isAdditionalLoadStoreStep(mid, nd, prop, cfg) and
-      newSummary = PathSummary::level()
-    ) and
+  exists(PathSummary oldSummary, PathSummary newSummary |
+    reachableFromStoreBaseStep(prop, rhs, nd, cfg, oldSummary, newSummary) and
     summary = oldSummary.appendValuePreserving(newSummary)
   )
 }
 
+/**
+ * Holds if `rhs` is the right-hand side of a write to property `prop`, and `nd` is reachable
+ * from the base of that write under configuration `cfg` (possibly through callees) along a
+ * path whose last step is summarized by `newSummary`, and the previous steps are summarized
+ * by `oldSummary`.
+ */
+pragma[noinline]
+private predicate reachableFromStoreBaseStep(
+  string prop, DataFlow::Node rhs, DataFlow::Node nd, DataFlow::Configuration cfg,
+  PathSummary oldSummary, PathSummary newSummary
+) {
+  exists(DataFlow::Node mid | reachableFromStoreBase(prop, rhs, mid, cfg, oldSummary) |
+    flowStep(mid, cfg, nd, newSummary)
+    or
+    isAdditionalLoadStoreStep(mid, nd, prop, cfg) and
+    newSummary = PathSummary::level()
+  )
+}
+
 /**
  * Holds if the value of `pred` is written to a property of some base object, and that base
  * object may flow into the base of property read `succ` under configuration `cfg` along
@@ -981,13 +994,29 @@ pragma[noinline]
 private predicate flowThroughProperty(
   DataFlow::Node pred, DataFlow::Node succ, DataFlow::Configuration cfg, PathSummary summary
 ) {
-  exists(string prop, DataFlow::Node base, PathSummary oldSummary, PathSummary newSummary |
-    reachableFromStoreBase(prop, pred, base, cfg, oldSummary) and
-    loadStep(base, succ, prop, cfg, newSummary) and
+  exists(PathSummary oldSummary, PathSummary newSummary |
+    storeToLoad(pred, succ, cfg, oldSummary, newSummary) and
     summary = oldSummary.append(newSummary)
   )
 }
 
+/**
+ * Holds if the value of `pred` is written to a property of some base object, and that base
+ * object may flow into the base of property read `succ` under configuration `cfg` along
+ * a path whose last step is summarized by `newSummary`, and the previous steps are summarized
+ * by `oldSummary`.
+ */
+pragma[noinline]
+private predicate storeToLoad(
+  DataFlow::Node pred, DataFlow::Node succ, DataFlow::Configuration cfg, PathSummary oldSummary,
+  PathSummary newSummary
+) {
+  exists(string prop, DataFlow::Node base |
+    reachableFromStoreBase(prop, pred, base, cfg, oldSummary) and
+    loadStep(base, succ, prop, cfg, newSummary)
+  )
+}
+
 /**
  * Holds if `arg` and `cb` are passed as arguments to a function which in turn
  * invokes `cb`, passing `arg` as its `i`th argument.

javascript/ql/src/semmle/javascript/dataflow/Nodes.qll

@@ -157,12 +157,12 @@ class InvokeNode extends DataFlow::SourceNode {
    * `name` is set to `result`.
    */
   DataFlow::ValueNode getOptionArgument(int i, string name) {
-    exists(ObjectLiteralNode obj |
-      obj.flowsTo(getArgument(i)) and
-      obj.hasPropertyWrite(name, result)
-    )
+    getOptionsArgument(i).hasPropertyWrite(name, result)
   }
 
+  pragma[noinline]
+  private ObjectLiteralNode getOptionsArgument(int i) { result.flowsTo(getArgument(i)) }
+
   /** Gets an abstract value representing possible callees of this call site. */
   final AbstractValue getACalleeValue() { result = getCalleeNode().analyze().getAValue() }
 

javascript/ql/src/semmle/javascript/frameworks/Files.qll

@@ -201,7 +201,18 @@ private class RecursiveReadDir extends FileSystemAccess, FileNameProducer, DataF
 
   override DataFlow::Node getAPathArgument() { result = getArgument(0) }
 
-  override DataFlow::Node getAFileName() { result = getCallback([1 .. 2]).getParameter(1) }
+  override DataFlow::Node getAFileName() { result = trackFileSource(DataFlow::TypeTracker::end()) }
+
+  private DataFlow::SourceNode trackFileSource(DataFlow::TypeTracker t) {
+    t.start() and result = getCallback([1 .. 2]).getParameter(1)
+    or
+    t.startInPromise() and not exists(getCallback([1 .. 2])) and result = this
+    or
+    // Tracking out of a promise
+    exists(DataFlow::TypeTracker t2 |
+      result = PromiseTypeTracking::promiseStep(trackFileSource(t2), t, t2)
+    )
+  }
 }
 
 /**
@@ -220,10 +231,24 @@ private module JSONFile {
 
     override DataFlow::Node getAPathArgument() { result = getArgument(0) }
 
-    override DataFlow::Node getADataNode() {
-      this.getCalleeName() = "readFile" and result = getCallback([1 .. 2]).getParameter(1)
+    override DataFlow::Node getADataNode() { result = trackRead(DataFlow::TypeTracker::end()) }
+
+    private DataFlow::SourceNode trackRead(DataFlow::TypeTracker t) {
+      this.getCalleeName() = "readFile" and
+      (
+        t.start() and result = getCallback([1 .. 2]).getParameter(1)
+        or
+        t.startInPromise() and not exists(getCallback([1 .. 2])) and result = this
+      )
       or
-      this.getCalleeName() = "readFileSync" and result = this
+      t.start() and
+      this.getCalleeName() = "readFileSync" and
+      result = this
+      or
+      // Tracking out of a promise
+      exists(DataFlow::TypeTracker t2 |
+        result = PromiseTypeTracking::promiseStep(trackRead(t2), t, t2)
+      )
     }
   }
 
@@ -243,6 +268,122 @@ private module JSONFile {
   }
 }
 
+/**
+ * A call to the library `load-json-file`.
+ */
+private class LoadJsonFile extends FileSystemReadAccess, DataFlow::CallNode {
+  LoadJsonFile() {
+    this = DataFlow::moduleImport("load-json-file").getACall()
+    or
+    this = DataFlow::moduleMember("load-json-file", "sync").getACall()
+  }
+
+  override DataFlow::Node getAPathArgument() { result = getArgument(0) }
+
+  override DataFlow::Node getADataNode() { result = trackRead(DataFlow::TypeTracker::end()) }
+
+  private DataFlow::SourceNode trackRead(DataFlow::TypeTracker t) {
+    this.getCalleeName() = "sync" and t.start() and result = this
+    or
+    not this.getCalleeName() = "sync" and t.startInPromise() and result = this
+    or
+    // Tracking out of a promise
+    exists(DataFlow::TypeTracker t2 |
+      result = PromiseTypeTracking::promiseStep(trackRead(t2), t, t2)
+    )
+  }
+}
+
+/**
+ * A call to the library `write-json-file`.
+ */
+private class WriteJsonFile extends FileSystemWriteAccess, DataFlow::CallNode {
+  WriteJsonFile() {
+    this = DataFlow::moduleImport("write-json-file").getACall()
+    or
+    this = DataFlow::moduleMember("write-json-file", "sync").getACall()
+  }
+
+  override DataFlow::Node getAPathArgument() { result = getArgument(0) }
+
+  override DataFlow::Node getADataNode() { result = getArgument(1) }
+}
+
+/**
+ * A call to the library `walkdir`.
+ */
+private class WalkDir extends FileNameProducer, FileSystemAccess, DataFlow::CallNode {
+  WalkDir() {
+    this = DataFlow::moduleImport("walkdir").getACall()
+    or
+    this = DataFlow::moduleMember("walkdir", "sync").getACall()
+    or
+    this = DataFlow::moduleMember("walkdir", "async").getACall()
+  }
+
+  override DataFlow::Node getAPathArgument() { result = getArgument(0) }
+
+  override DataFlow::Node getAFileName() { result = trackFileSource(DataFlow::TypeTracker::end()) }
+
+  private DataFlow::SourceNode trackFileSource(DataFlow::TypeTracker t) {
+    not this.getCalleeName() = any(string s | s = "sync" or s = "async") and
+    t.start() and
+    (
+      result = getCallback(getNumArgument() - 1).getParameter(0)
+      or
+      result = getAMethodCall(EventEmitter::on()).getCallback(1).getParameter(0)
+    )
+    or
+    t.start() and this.getCalleeName() = "sync" and result = this
+    or
+    t.startInPromise() and this.getCalleeName() = "async" and result = this
+    or
+    // Tracking out of a promise
+    exists(DataFlow::TypeTracker t2 |
+      result = PromiseTypeTracking::promiseStep(trackFileSource(t2), t, t2)
+    )
+  }
+}
+
+/**
+ * A call to the library `globule`.
+ */
+private class Globule extends FileNameProducer, FileSystemAccess, DataFlow::CallNode {
+  Globule() {
+    this = DataFlow::moduleMember("globule", "find").getACall()
+    or
+    this = DataFlow::moduleMember("globule", "match").getACall()
+    or
+    this = DataFlow::moduleMember("globule", "isMatch").getACall()
+    or
+    this = DataFlow::moduleMember("globule", "mapping").getACall()
+    or
+    this = DataFlow::moduleMember("globule", "findMapping").getACall()
+  }
+
+  override DataFlow::Node getAPathArgument() {
+    (this.getCalleeName() = "match" or this.getCalleeName() = "isMatch") and
+    result = getArgument(1)
+    or
+    this.getCalleeName() = "mapping" and
+    (
+      result = getAnArgument() and not exists(result.getALocalSource().getAPropertyWrite("src"))
+      or
+      result = getAnArgument().getALocalSource().getAPropertyWrite("src").getRhs()
+    )
+  }
+
+  override DataFlow::Node getAFileName() {
+    result = this and
+    (
+      this.getCalleeName() = "find" or
+      this.getCalleeName() = "match" or
+      this.getCalleeName() = "findMapping" or
+      this.getCalleeName() = "mapping"
+    )
+  }
+}
+
 /**
  * A file system access made by a NodeJS library.
  * This class models multiple NodeJS libraries that access files.
@@ -257,6 +398,10 @@ private class LibraryAccess extends FileSystemAccess, DataFlow::InvokeNode {
       or
       this = DataFlow::moduleImport("rimraf").getACall()
       or
+      this = DataFlow::moduleImport("readdirp").getACall()
+      or
+      this = DataFlow::moduleImport("walker").getACall()
+      or
       this =
         DataFlow::moduleMember("node-dir",
           any(string s |