JS: Port missing rate limiting query

Author: asgerf

javascript/ql/lib/semmle/javascript/security/dataflow/MissingRateLimiting.qll

@@ -30,10 +30,6 @@ private import semmle.javascript.frameworks.ConnectExpressShared::ConnectExpress
  * A route handler that should be rate-limited.
  */
 abstract class ExpensiveRouteHandler extends DataFlow::Node {
-  Express::RouteHandler impl;
-
-  ExpensiveRouteHandler() { this = impl }
-
   /**
    * Holds if `explanation` is a string explaining why this route handler should be rate-limited.
    *
@@ -45,9 +41,15 @@ abstract class ExpensiveRouteHandler extends DataFlow::Node {
 }
 
 /**
- * A route handler expression that is rate limited.
+ * DEPRECATED. Use `RateLimitingMiddleware` instead.
+ *
+ * A route handler expression that is guarded by a rate limiter.
  */
-abstract class RateLimitedRouteHandlerExpr extends Express::RouteHandlerExpr { }
+deprecated class RateLimitedRouteHandlerExpr extends Express::RouteHandlerExpr {
+  RateLimitedRouteHandlerExpr() {
+    Routing::getNode(flow()).isGuardedBy(any(RateLimitingMiddleware m))
+  }
+}
 
 // default implementations
 /**
@@ -106,11 +108,13 @@ class DatabaseAccessAsExpensiveAction extends ExpensiveAction {
 }
 
 /**
+ * DEPRECATED. Use the `Routing::Node` API instead.
+ *
  * A route handler expression that is rate-limited by a rate-limiting middleware.
  */
-class RouteHandlerExpressionWithRateLimiter extends RateLimitedRouteHandlerExpr {
+deprecated class RouteHandlerExpressionWithRateLimiter extends Expr {
   RouteHandlerExpressionWithRateLimiter() {
-    any(RateLimitingMiddleware m).ref().flowsToExpr(this.getAMatchingAncestor())
+    Routing::getNode(this.flow()).isGuardedBy(any(RateLimitingMiddleware m))
   }
 }
 
@@ -139,6 +143,9 @@ abstract class RateLimitingMiddleware extends DataFlow::SourceNode {
 
   /** Gets a data flow node referring to this middleware. */
   DataFlow::SourceNode ref() { result = this.ref(DataFlow::TypeTracker::end()) }
+
+  /** Gets a routing node corresponding to this middleware function. */
+  Routing::Node getRoutingNode() { result = Routing::getNode(this) }
 }
 
 /**
@@ -160,12 +167,21 @@ class BruteForceRateLimit extends RateLimitingMiddleware {
 }
 
 /**
- * A route handler expression that is rate-limited by the `express-limiter` package.
+ * A rate limiter constructed using the `express-limiter` package.
+ *
+ * Note that the `express-limiter` package is unusual in that it may optionally install itself as a middleware.
+ * That aspect is handled by the Express core model.
  */
-class RouteHandlerLimitedByExpressLimiter extends RateLimitedRouteHandlerExpr {
+class RouteHandlerLimitedByExpressLimiter extends RateLimitingMiddleware {
   RouteHandlerLimitedByExpressLimiter() {
-    API::moduleImport("express-limiter").getParameter(0).getARhs().getALocalSource().asExpr() =
-      this.getSetup().getRouter()
+    this = API::moduleImport("express-limiter").getReturn().getReturn().getAnImmediateUse()
+  }
+
+  override Routing::Node getRoutingNode() {
+    result = super.getRoutingNode()
+    or
+    // express-limiter can perform its own route setup
+    result = Routing::getRouteSetupNode(this)
   }
 }
 
@@ -206,3 +222,7 @@ class RateLimiterFlexibleRateLimiter extends DataFlow::FunctionNode {
 class RouteHandlerLimitedByRateLimiterFlexible extends RateLimitingMiddleware {
   RouteHandlerLimitedByRateLimiterFlexible() { this instanceof RateLimiterFlexibleRateLimiter }
 }
+
+private class FastifyRateLimiter extends RateLimitingMiddleware {
+  FastifyRateLimiter() { this = DataFlow::moduleImport("fastify-rate-limit") }
+}

javascript/ql/src/Security/CWE-770/MissingRateLimiting.ql

@@ -4,7 +4,7 @@
  *              restricting the rate at which operations can be carried out is vulnerable
  *              to denial-of-service attacks.
  * @kind problem
- * @problem.severity error
+ * @problem.severity warning
  * @security-severity 7.5
  * @precision high
  * @id js/missing-rate-limiting
@@ -18,12 +18,13 @@ import javascript
 import semmle.javascript.security.dataflow.MissingRateLimiting
 import semmle.javascript.RestrictedLocations
 
+// TODO: Previously we used (FirstLineOf) here, but that doesn't work anymore.
 from
-  ExpensiveRouteHandler r, Express::RouteHandlerExpr rhe, string explanation,
-  DataFlow::Node reference, string referenceLabel
+  Routing::Node useSite, ExpensiveRouteHandler r, string explanation, DataFlow::Node reference,
+  string referenceLabel
 where
-  r = rhe.getBody() and
+  useSite = Routing::getNode(r).getAUseSite() and
   r.explain(explanation, reference, referenceLabel) and
-  not rhe instanceof RateLimitedRouteHandlerExpr
-select rhe.(FirstLineOf), "This route handler " + explanation + ", but is not rate-limited.",
-  reference, referenceLabel
+  not useSite.isGuardedByNode(any(RateLimitingMiddleware m).getRoutingNode())
+select useSite, "This route handler " + explanation + ", but is not rate-limited.", reference,
+  referenceLabel

javascript/ql/test/query-tests/Security/CWE-770/MissingRateLimiting.expected

@@ -1,4 +1,4 @@
-| MissingRateLimiting.js:4:19:4:38 | functio ... ath);\\n} | This route handler performs $@, but is not rate-limited. | MissingRateLimiting.js:7:5:7:22 | res.sendFile(path) | a file system access |
+| MissingRateLimiting.js:4:19:8:1 | functio ... ath);\\n} | This route handler performs $@, but is not rate-limited. | MissingRateLimiting.js:7:5:7:22 | res.sendFile(path) | a file system access |
 | MissingRateLimiting.js:25:19:25:20 | f1 | This route handler performs $@, but is not rate-limited. | MissingRateLimiting.js:13:5:13:22 | res.sendFile(path) | a file system access |
 | MissingRateLimiting.js:25:27:25:28 | f3 | This route handler performs $@, but is not rate-limited. | MissingRateLimiting.js:22:5:22:22 | res.sendFile(path) | a file system access |
 | tst.js:22:24:22:40 | expensiveHandler1 | This route handler performs $@, but is not rate-limited. | tst.js:14:40:14:46 | login() | authorization |

javascript/ql/test/query-tests/Security/CWE-770/tst.js

@@ -58,7 +58,7 @@ app2.get('/:path', bruteforce.prevent, expensiveHandler1); // OK
 
 // rate limiting using express-limiter
 var app3 = express();
-var limiter = require('express-limiter')(app3);
+require('express-limiter')(app3)({ method: 'get', path: '/' });
 app3.get('/:path', expensiveHandler1); // OK
 
 express().get('/:path', function(req, res) { verifyUser(req); });  // NOT OK