Merge pull request #7352 from github/esbena/atm-endpoint-polish

ATM Endpoint filtering improvements
2026-05-02 04:05:14 +02:00 · 2021-12-14 08:19:23 +00:00
parent 81dedfe22f 1949a4e59a
commit 1c79d1f985
7 changed files with 149 additions and 4 deletions
--- a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/CoreKnowledge.qll
+++ b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/CoreKnowledge.qll
@@ -157,6 +157,9 @@ predicate isOtherModeledArgument(DataFlow::Node n, FilteringReason reason) {
  any(LodashUnderscore::Member m).getACall().getAnArgument() = n and
  reason instanceof LodashUnderscoreArgumentReason
  or
+  any(JQuery::MethodCall m).getAnArgument() = n and
+  reason instanceof JQueryArgumentReason
+  or
  exists(ClientRequest r |
    r.getAnArgument() = n or n = r.getUrl() or n = r.getHost() or n = r.getADataNode()
  ) and
@@ -197,12 +200,23 @@ predicate isOtherModeledArgument(DataFlow::Node n, FilteringReason reason) {
    or
    call instanceof FileSystemAccess and reason instanceof FileSystemAccessReason
    or
-    call instanceof DatabaseAccess and reason instanceof DatabaseAccessReason
+    // TODO database accesses are less well defined than database query sinks, so this may cover unmodeled sinks on existing database models
+    [
+      call, call.getAMethodCall()
+    /* command pattern where the query is built, and then exec'ed later */ ] instanceof
+      DatabaseAccess and
+    reason instanceof DatabaseAccessReason
    or
    call = DOM::domValueRef() and reason instanceof DOMReason
    or
    call.getCalleeName() = "next" and
    exists(DataFlow::FunctionNode f | call = f.getLastParameter().getACall()) and
    reason instanceof NextFunctionCallReason
+    or
+    call = DataFlow::globalVarRef("dojo").getAPropertyRead("require").getACall() and
+    reason instanceof DojoRequireReason
  )
+  or
+  (exists(Base64::Decode d | n = d.getInput()) or exists(Base64::Encode d | n = d.getInput())) and
+  reason instanceof Base64ManipulationReason
 }
--- a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/FilteringReasons.qll
+++ b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/FilteringReasons.qll
@@ -29,7 +29,10 @@ newtype TFilteringReason =
  TArgumentToArrayReason() or
  TArgumentToBuiltinGlobalVarRefReason() or
  TConstantReceiverReason() or
-  TBuiltinCallNameReason()
+  TBuiltinCallNameReason() or
+  TBase64ManipulationReason() or
+  TJQueryArgumentReason() or
+  TDojoRequireReason()

 /** A reason why a particular endpoint was filtered out by the endpoint filters. */
 abstract class FilteringReason extends TFilteringReason {
@@ -194,3 +197,21 @@ class BuiltinCallNameReason extends NotASinkReason, TBuiltinCallNameReason {

  override int getEncoding() { result = 27 }
 }
+
+class Base64ManipulationReason extends NotASinkReason, TBase64ManipulationReason {
+  override string getDescription() { result = "Base64Manipulation" }
+
+  override int getEncoding() { result = 28 }
+}
+
+class JQueryArgumentReason extends NotASinkReason, TJQueryArgumentReason {
+  override string getDescription() { result = "JQueryArgument" }
+
+  override int getEncoding() { result = 29 }
+}
+
+class DojoRequireReason extends NotASinkReason, TDojoRequireReason {
+  override string getDescription() { result = "DojoRequire" }
+
+  override int getEncoding() { result = 30 }
+}
--- a/javascript/ql/lib/semmle/javascript/frameworks/ExpressModules.qll
+++ b/javascript/ql/lib/semmle/javascript/frameworks/ExpressModules.qll
@@ -226,3 +226,30 @@ module ExpressLibraries {
    predicate producesUserControlledObjects() { isJson() or isExtendedUrlEncoded() }
  }
 }
+
+/**
+ * Provides classes for working with the `express-fileupload` package (https://github.com/richardgirges/express-fileupload);
+ */
+module FileUpload {
+  /** Gets a data flow node referring to `req.files`. */
+  private DataFlow::SourceNode filesRef(Express::RequestSource req, DataFlow::TypeTracker t) {
+    t.start() and
+    result = req.ref().getAPropertyRead("files")
+    or
+    exists(DataFlow::TypeTracker t2 | result = filesRef(req, t2).track(t2, t))
+  }
+
+  /**
+   * A call to `req.files.<name>.mv`
+   */
+  class Move extends FileSystemWriteAccess, DataFlow::MethodCallNode {
+    Move() {
+      exists(DataFlow::moduleImport("express-fileupload")) and
+      this = filesRef(_, DataFlow::TypeTracker::end()).getAPropertyRead().getAMethodCall("mv")
+    }
+
+    override DataFlow::Node getAPathArgument() { result = getArgument(0) }
+
+    override DataFlow::Node getADataNode() { none() }
+  }
+}
--- a/javascript/ql/lib/semmle/javascript/frameworks/NoSQL.qll
+++ b/javascript/ql/lib/semmle/javascript/frameworks/NoSQL.qll
@@ -618,14 +618,30 @@ private module Minimongo {
 * Provides classes modeling the MarsDB library.
 */
 private module MarsDB {
+  private class MarsDBAccess extends DatabaseAccess {
+    string method;
+
+    MarsDBAccess() {
+      this =
+        API::moduleImport("marsdb")
+            .getMember("Collection")
+            .getInstance()
+            .getMember(method)
+            .getACall()
+    }
+
+    string getMethod() { result = method }
+
+    override DataFlow::Node getAQueryArgument() { none() }
+  }
+
  /** A call to a MarsDB query method. */
  private class QueryCall extends DatabaseAccess, API::CallNode {
    int queryArgIdx;

    QueryCall() {
      exists(string m |
-        this =
-          API::moduleImport("marsdb").getMember("Collection").getInstance().getMember(m).getACall() and
+        this.(MarsDBAccess).getMethod() = m and
        // implements parts of the Minimongo interface
        Minimongo::CollectionMethodSignatures::interpretsArgumentAsQuery(m, queryArgIdx)
      )
@@ -733,4 +749,37 @@ private module Redis {
      )
    }
  }
+
+  /**
+   * An access to a database through redis
+   */
+  class RedisDatabaseAccess extends DatabaseAccess {
+    RedisDatabaseAccess() { this = redis().getMember(_).getACall() }
+
+    override DataFlow::Node getAQueryArgument() { none() }
+  }
+}
+
+/**
+ * Provides classes modeling the `ioredis` library.
+ *
+ * ```
+ * import Redis from 'ioredis'
+ * let client = new Redis(...)
+ * ```
+ */
+private module IoRedis {
+  /**
+   * Gets an `ioredis` client.
+   */
+  API::Node ioredis() { result = API::moduleImport("ioredis").getInstance() }
+
+  /**
+   * An access to a database through ioredis
+   */
+  class IoRedisDatabaseAccess extends DatabaseAccess {
+    IoRedisDatabaseAccess() { this = ioredis().getMember(_).getACall() }
+
+    override DataFlow::Node getAQueryArgument() { none() }
+  }
 }
--- a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected
@@ -1535,6 +1535,12 @@ nodes
 | TaintedPath.js:214:35:214:38 | path |
 | TaintedPath.js:214:35:214:38 | path |
 | TaintedPath.js:214:35:214:38 | path |
+| express.js:8:20:8:32 | req.query.bar |
+| express.js:8:20:8:32 | req.query.bar |
+| express.js:8:20:8:32 | req.query.bar |
+| express.js:8:20:8:32 | req.query.bar |
+| express.js:8:20:8:32 | req.query.bar |
+| express.js:8:20:8:32 | req.query.bar |
 | normalizedPaths.js:11:7:11:27 | path |
 | normalizedPaths.js:11:7:11:27 | path |
 | normalizedPaths.js:11:7:11:27 | path |
@@ -6321,6 +6327,7 @@ edges
 | TaintedPath.js:211:24:211:30 | req.url | TaintedPath.js:211:14:211:37 | url.par ... , true) |
 | TaintedPath.js:211:24:211:30 | req.url | TaintedPath.js:211:14:211:37 | url.par ... , true) |
 | TaintedPath.js:211:24:211:30 | req.url | TaintedPath.js:211:14:211:37 | url.par ... , true) |
+| express.js:8:20:8:32 | req.query.bar | express.js:8:20:8:32 | req.query.bar |
 | normalizedPaths.js:11:7:11:27 | path | normalizedPaths.js:13:19:13:22 | path |
 | normalizedPaths.js:11:7:11:27 | path | normalizedPaths.js:13:19:13:22 | path |
 | normalizedPaths.js:11:7:11:27 | path | normalizedPaths.js:13:19:13:22 | path |
@@ -9638,6 +9645,7 @@ edges
 | TaintedPath.js:212:31:212:34 | path | TaintedPath.js:211:24:211:30 | req.url | TaintedPath.js:212:31:212:34 | path | This path depends on $@. | TaintedPath.js:211:24:211:30 | req.url | a user-provided value |
 | TaintedPath.js:213:45:213:48 | path | TaintedPath.js:211:24:211:30 | req.url | TaintedPath.js:213:45:213:48 | path | This path depends on $@. | TaintedPath.js:211:24:211:30 | req.url | a user-provided value |
 | TaintedPath.js:214:35:214:38 | path | TaintedPath.js:211:24:211:30 | req.url | TaintedPath.js:214:35:214:38 | path | This path depends on $@. | TaintedPath.js:211:24:211:30 | req.url | a user-provided value |
+| express.js:8:20:8:32 | req.query.bar | express.js:8:20:8:32 | req.query.bar | express.js:8:20:8:32 | req.query.bar | This path depends on $@. | express.js:8:20:8:32 | req.query.bar | a user-provided value |
 | normalizedPaths.js:13:19:13:22 | path | normalizedPaths.js:11:14:11:27 | req.query.path | normalizedPaths.js:13:19:13:22 | path | This path depends on $@. | normalizedPaths.js:11:14:11:27 | req.query.path | a user-provided value |
 | normalizedPaths.js:14:19:14:29 | './' + path | normalizedPaths.js:11:14:11:27 | req.query.path | normalizedPaths.js:14:19:14:29 | './' + path | This path depends on $@. | normalizedPaths.js:11:14:11:27 | req.query.path | a user-provided value |
 | normalizedPaths.js:15:19:15:38 | path + '/index.html' | normalizedPaths.js:11:14:11:27 | req.query.path | normalizedPaths.js:15:19:15:38 | path + '/index.html' | This path depends on $@. | normalizedPaths.js:11:14:11:27 | req.query.path | a user-provided value |
--- a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/express.js
+++ b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/express.js
@@ -0,0 +1,9 @@
+var express = require("express"),
+  fileUpload = require("express-fileupload");
+
+let app = express();
+app.use(fileUpload());
+
+app.get("/some/path", function (req, res) {
+  req.files.foo.mv(req.query.bar);
+});
--- a/javascript/ql/test/query-tests/Security/CWE-089/untyped/DatabaseAccesses.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-089/untyped/DatabaseAccesses.expected
@@ -63,6 +63,23 @@
 | pg-promise.js:60:14:60:25 | t.one(query) |
 | pg-promise.js:63:17:63:28 | t.one(query) |
 | pg-promise.js:64:10:64:21 | t.one(query) |
+| redis.js:10:5:10:37 | client. ... value") |
+| redis.js:14:9:14:32 | client. ... value") |
+| redis.js:15:9:15:36 | client. ... alue"]) |
+| redis.js:18:5:18:28 | client. ... value") |
+| redis.js:19:5:19:56 | client. ... alue2") |
+| redis.js:22:5:23:16 | client\\n ... multi() |
+| redis.js:22:5:24:33 | client\\n ... value") |
+| redis.js:22:5:25:26 | client\\n ... value") |
+| redis.js:22:5:26:17 | client\\n ... et(key) |
+| redis.js:22:5:27:42 | client\\n ... s) { }) |
+| redis.js:29:5:31:6 | client. ... \\n    }) |
+| redis.js:30:9:30:35 | newClie ... value") |
+| redis.js:32:5:32:22 | client.duplicate() |
+| redis.js:32:5:32:40 | client. ... value") |
+| redis.js:39:5:39:28 | client. ... value") |
+| redis.js:46:18:46:46 | client. ... value") |
+| redis.js:49:18:49:47 | client. ... value") |
 | socketio.js:11:5:11:54 | db.run( ... ndle}`) |
 | tst2.js:7:3:7:62 | sql.que ... ms.id}` |
 | tst2.js:9:3:9:85 | new sql ...  + "'") |