JS: Port CSRF query

Author: asgerf

javascript/ql/lib/semmle/javascript/frameworks/HTTP.qll

@@ -96,6 +96,12 @@ module HTTP {
     predicate isSafe() {
       this = ["GET", "HEAD", "OPTIONS", "PRI", "PROPFIND", "REPORT", "SEARCH", "TRACE"]
     }
+
+    /**
+     * Holds if this kind of HTTP request should not generally be considered free of side effects,
+     * such as for `POST` or `PUT` requests.
+     */
+    predicate isUnsafe() { not isSafe() }
   }
 
   /**

javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql

@@ -16,59 +16,27 @@ import javascript
 /** Gets a property name of `req` which refers to data usually derived from cookie data. */
 string cookieProperty() { result = "session" or result = "cookies" or result = "user" }
 
-/** Gets a data flow node that flows to the base of a reference to `cookies`, `session`, or `user`. */
-private DataFlow::SourceNode nodeLeadingToCookieAccess(DataFlow::TypeBackTracker t) {
-  t.start() and
+/**
+ * Holds if `handler` uses cookies.
+ */
+predicate isRouteHandlerUsingCookies(Routing::RouteHandler handler) {
   exists(DataFlow::PropRef value |
-    value = result.getAPropertyRead(cookieProperty()).getAPropertyReference() and
+    value = handler.getAnInput().ref().getAPropertyRead(cookieProperty()).getAPropertyReference() and
     // Ignore accesses to values that are part of a CSRF or captcha check
     not value.getPropertyName().regexpMatch("(?i).*(csrf|xsrf|captcha).*") and
     // Ignore calls like `req.session.save()`
     not value = any(DataFlow::InvokeNode call).getCalleeNode()
   )
-  or
-  exists(DataFlow::TypeBackTracker t2 | result = nodeLeadingToCookieAccess(t2).backtrack(t2, t))
-}
-
-/** Gets a data flow node that flows to the base of an access to `cookies`, `session`, or `user`. */
-DataFlow::SourceNode nodeLeadingToCookieAccess() {
-  result = nodeLeadingToCookieAccess(DataFlow::TypeBackTracker::end())
-}
-
-/**
- * Holds if `handler` uses cookies.
- */
-predicate isRouteHandlerUsingCookies(Express::RouteHandler handler) {
-  DataFlow::parameterNode(handler.getRequestParameter()) = nodeLeadingToCookieAccess()
-}
-
-/** Gets a data flow node referring to a route handler that uses cookies. */
-private DataFlow::SourceNode getARouteUsingCookies(DataFlow::TypeTracker t) {
-  t.start() and
-  isRouteHandlerUsingCookies(result)
-  or
-  exists(DataFlow::TypeTracker t2, DataFlow::SourceNode pred | pred = getARouteUsingCookies(t2) |
-    result = pred.track(t2, t)
-    or
-    t = t2 and
-    HTTP::routeHandlerStep(pred, result)
-  )
-}
-
-/** Gets a data flow node referring to a route handler that uses cookies. */
-DataFlow::SourceNode getARouteUsingCookies() {
-  result = getARouteUsingCookies(DataFlow::TypeTracker::end())
 }
 
 /**
- * Checks if `expr` is preceded by the cookie middleware `cookie`.
+ * Checks if `route` is preceded by the cookie middleware `cookie`.
  *
  * A router handler following after cookie parsing is assumed to depend on
  * cookies, and thus require CSRF protection.
  */
-predicate hasCookieMiddleware(Express::RouteHandlerExpr expr, Express::RouteHandlerExpr cookie) {
-  any(HTTP::CookieMiddlewareInstance i).flowsToExpr(cookie) and
-  expr.getAMatchingAncestor() = cookie
+predicate hasCookieMiddleware(Routing::Node route, HTTP::CookieMiddlewareInstance cookie) {
+  route.isGuardedBy(cookie)
 }
 
 /**
@@ -87,22 +55,26 @@ predicate hasCookieMiddleware(Express::RouteHandlerExpr expr, Express::RouteHand
  * })
  * ```
  */
-DataFlow::CallNode csrfMiddlewareCreation() {
+DataFlow::SourceNode csrfMiddlewareCreation() {
   exists(DataFlow::SourceNode callee | result = callee.getACall() |
     callee = DataFlow::moduleImport("csurf")
     or
     callee = DataFlow::moduleImport("lusca") and
-    exists(result.getOptionArgument(0, "csrf"))
+    exists(result.(DataFlow::CallNode).getOptionArgument(0, "csrf"))
     or
     callee = DataFlow::moduleMember("lusca", "csrf")
     or
     callee = DataFlow::moduleMember("express", "csrf")
   )
+  or
+  // Note: the 'fastify-csrf' plugin enables the 'fastify.csrfProtection' middleware to be installed.
+  // Simply having the plugin registered is not enough, so we look for the 'csrfProtection' middleware.
+  result = Fastify::server().getAPropertyRead("csrfProtection")
 }
 
 /**
  * Gets a data flow node that flows to the base of a reference to `cookies`, `session`, or `user`,
- * where the references property has `csrf` or `xsrf` in its name,
+ * where the referenced property has `csrf` or `xsrf` in its name,
  * and a property is either written or part of a comparison.
  */
 private DataFlow::SourceNode nodeLeadingToCsrfWriteOrCheck(DataFlow::TypeBackTracker t) {
@@ -121,10 +93,10 @@ private DataFlow::SourceNode nodeLeadingToCsrfWriteOrCheck(DataFlow::TypeBackTra
 /**
  * Gets a route handler that sets an CSRF related cookie.
  */
-private Express::RouteHandler getAHandlerSettingCsrfCookie() {
+private Routing::RouteHandler getAHandlerSettingCsrfCookie() {
   exists(HTTP::CookieDefinition setCookie |
     setCookie.getNameArgument().getStringValue().regexpMatch("(?i).*(csrf|xsrf).*") and
-    result = setCookie.getRouteHandler()
+    result = Routing::getRouteHandler(setCookie.getRouteHandler())
   )
 }
 
@@ -133,65 +105,44 @@ private Express::RouteHandler getAHandlerSettingCsrfCookie() {
  * This is indicated either by the request parameter having a CSRF related write to a session variable.
  * Or by the response parameter setting a CSRF related cookie.
  */
-predicate isCsrfProtectionRouteHandler(Express::RouteHandler handler) {
-  DataFlow::parameterNode(handler.getRequestParameter()) =
-    nodeLeadingToCsrfWriteOrCheck(DataFlow::TypeBackTracker::end())
+predicate isCsrfProtectionRouteHandler(Routing::RouteHandler handler) {
+  handler.getAnInput() = nodeLeadingToCsrfWriteOrCheck(DataFlow::TypeBackTracker::end())
   or
   handler = getAHandlerSettingCsrfCookie()
 }
 
-/** Gets a data flow node refering to a route handler that is protecting against CSRF. */
-private DataFlow::SourceNode getACsrfProtectionRouteHandler(DataFlow::TypeTracker t) {
-  t.start() and
-  isCsrfProtectionRouteHandler(result)
-  or
-  exists(DataFlow::TypeTracker t2, DataFlow::SourceNode pred |
-    pred = getACsrfProtectionRouteHandler(t2)
-  |
-    result = pred.track(t2, t)
-    or
-    t = t2 and
-    HTTP::routeHandlerStep(pred, result)
-  )
-}
-
 /**
  * Gets an express route handler expression that is either a custom CSRF protection middleware,
  * or a CSRF protecting library.
  */
-Express::RouteHandlerExpr getACsrfMiddleware() {
-  csrfMiddlewareCreation().flowsToExpr(result)
+Routing::Node getACsrfMiddleware() {
+  result = Routing::getNode(csrfMiddlewareCreation())
   or
-  getACsrfProtectionRouteHandler(DataFlow::TypeTracker::end()).flowsToExpr(result)
+  isCsrfProtectionRouteHandler(result)
 }
 
 /**
  * Holds if the given route handler is protected by CSRF middleware.
  */
-predicate hasCsrfMiddleware(Express::RouteHandlerExpr handler) {
-  getACsrfMiddleware() = handler.getAMatchingAncestor()
+predicate hasCsrfMiddleware(Routing::RouteHandler handler) {
+  handler.isGuardedByNode(getACsrfMiddleware())
 }
 
-from
-  Express::RouterDefinition router, Express::RouteSetup setup, Express::RouteHandlerExpr handler,
-  Express::RouteHandlerExpr cookie
+from Routing::RouteSetup setup, Routing::RouteHandler handler, HTTP::CookieMiddlewareInstance cookie
 where
-  router = setup.getRouter() and
-  handler = setup.getARouteHandlerExpr() and
   // Require that the handler uses cookies and has cookie middleware.
   //
   // In practice, handlers that use cookies always have the cookie middleware or
   // the handler wouldn't work. However, if we can't find the cookie middleware, it
   // indicates that our middleware model is too incomplete, so in that case we
   // don't trust it to detect the presence of CSRF middleware either.
-  getARouteUsingCookies().flowsToExpr(handler) and
+  setup.getAChild*() = handler and
+  isRouteHandlerUsingCookies(handler) and
   hasCookieMiddleware(handler, cookie) and
   // Only flag the cookie parser registered first.
-  not hasCookieMiddleware(cookie, _) and
+  not hasCookieMiddleware(Routing::getNode(cookie), _) and
   not hasCsrfMiddleware(handler) and
-  // Only warn for the last handler in a chain.
-  handler.isLastHandler() and
   // Only warn for dangerous handlers, such as for POST and PUT.
-  not setup.getRequestMethod().isSafe()
+  setup.getOwnHttpMethod().isUnsafe()
 select cookie, "This cookie middleware is serving a request handler $@ without CSRF protection.",
-  handler, "here"
+  setup, "here"

javascript/ql/test/query-tests/Security/CWE-352/MissingCsrfMiddleware.expected

@@ -1,11 +1,13 @@
-| MissingCsrfMiddlewareBad.js:7:9:7:22 | cookieParser() | This cookie middleware is serving a request handler $@ without CSRF protection. | MissingCsrfMiddlewareBad.js:10:26:12:1 | functio ... il"];\\n} | here |
-| MissingCsrfMiddlewareBad.js:17:13:17:26 | cookieParser() | This cookie middleware is serving a request handler $@ without CSRF protection. | MissingCsrfMiddlewareBad.js:25:30:27:6 | errorCa ... \\n    }) | here |
-| MissingCsrfMiddlewareBad.js:33:13:33:26 | cookieParser() | This cookie middleware is serving a request handler $@ without CSRF protection. | MissingCsrfMiddlewareBad.js:41:30:43:6 | errorCa ... \\n    }) | here |
-| MissingCsrfMiddlewareBad.js:33:13:33:26 | cookieParser() | This cookie middleware is serving a request handler $@ without CSRF protection. | MissingCsrfMiddlewareBad.js:45:31:47:6 | errorCa ... \\n    }) | here |
-| csurf_api_example.js:42:37:42:50 | cookieParser() | This cookie middleware is serving a request handler $@ without CSRF protection. | csurf_api_example.js:42:53:45:3 | functio ... e')\\n  } | here |
-| csurf_example.js:18:9:18:22 | cookieParser() | This cookie middleware is serving a request handler $@ without CSRF protection. | csurf_example.js:31:40:34:1 | functio ... sed')\\n} | here |
-| lusca_example.js:9:9:9:22 | cookieParser() | This cookie middleware is serving a request handler $@ without CSRF protection. | lusca_example.js:26:42:29:1 | functio ... sed')\\n} | here |
-| lusca_example.js:9:9:9:22 | cookieParser() | This cookie middleware is serving a request handler $@ without CSRF protection. | lusca_example.js:31:40:34:1 | functio ... sed')\\n} | here |
-| unused_cookies.js:6:9:6:22 | cookieParser() | This cookie middleware is serving a request handler $@ without CSRF protection. | unused_cookies.js:8:34:13:1 | (req, r ... Ok');\\n} | here |
-| unused_cookies.js:6:9:6:22 | cookieParser() | This cookie middleware is serving a request handler $@ without CSRF protection. | unused_cookies.js:29:19:32:1 | (req, r ... Ok');\\n} | here |
-| unused_cookies.js:6:9:6:22 | cookieParser() | This cookie middleware is serving a request handler $@ without CSRF protection. | unused_cookies.js:34:22:37:1 | (req, r ... Ok');\\n} | here |
+| MissingCsrfMiddlewareBad.js:7:9:7:22 | cookieParser() | This cookie middleware is serving a request handler $@ without CSRF protection. | MissingCsrfMiddlewareBad.js:10:1:12:2 | app.pos ... l"];\\n}) | here |
+| MissingCsrfMiddlewareBad.js:17:13:17:26 | cookieParser() | This cookie middleware is serving a request handler $@ without CSRF protection. | MissingCsrfMiddlewareBad.js:25:5:27:7 | app.pos ...     })) | here |
+| MissingCsrfMiddlewareBad.js:33:13:33:26 | cookieParser() | This cookie middleware is serving a request handler $@ without CSRF protection. | MissingCsrfMiddlewareBad.js:41:5:43:7 | app.pos ...     })) | here |
+| MissingCsrfMiddlewareBad.js:33:13:33:26 | cookieParser() | This cookie middleware is serving a request handler $@ without CSRF protection. | MissingCsrfMiddlewareBad.js:45:5:47:7 | app.pos ...     })) | here |
+| csurf_api_example.js:42:37:42:50 | cookieParser() | This cookie middleware is serving a request handler $@ without CSRF protection. | csurf_api_example.js:42:3:45:4 | router. ... ')\\n  }) | here |
+| csurf_example.js:18:9:18:22 | cookieParser() | This cookie middleware is serving a request handler $@ without CSRF protection. | csurf_example.js:31:1:34:2 | app.pos ... ed')\\n}) | here |
+| fastify.js:5:14:5:38 | require ... ookie') | This cookie middleware is serving a request handler $@ without CSRF protection. | fastify.js:17:1:24:2 | app.rou ... \\n  }\\n}) | here |
+| lusca_example.js:9:9:9:22 | cookieParser() | This cookie middleware is serving a request handler $@ without CSRF protection. | lusca_example.js:26:1:29:2 | app.pos ... ed')\\n}) | here |
+| lusca_example.js:9:9:9:22 | cookieParser() | This cookie middleware is serving a request handler $@ without CSRF protection. | lusca_example.js:31:1:34:2 | app.pos ... ed')\\n}) | here |
+| tst.js:6:9:6:22 | cookieParser() | This cookie middleware is serving a request handler $@ without CSRF protection. | tst.js:8:1:10:2 | app.pos ... s.x;\\n}) | here |
+| unused_cookies.js:6:9:6:22 | cookieParser() | This cookie middleware is serving a request handler $@ without CSRF protection. | unused_cookies.js:8:1:13:2 | app.pos ... k');\\n}) | here |
+| unused_cookies.js:6:9:6:22 | cookieParser() | This cookie middleware is serving a request handler $@ without CSRF protection. | unused_cookies.js:29:1:32:2 | app.pos ... k');\\n}) | here |
+| unused_cookies.js:6:9:6:22 | cookieParser() | This cookie middleware is serving a request handler $@ without CSRF protection. | unused_cookies.js:34:1:37:2 | app.pos ... k');\\n}) | here |

javascript/ql/test/query-tests/Security/CWE-352/MissingCsrfMiddlewareGood2.js

@@ -98,10 +98,11 @@ var passport = require('passport');
     app.use(cookieParser())
     app.use(passport.authorize({ session: true }))
 
-    function checkToken(req) {
+    function checkToken(req, res, next) {
         if (req.headers.xsrfToken !== req.session.xsrfToken) {
             throw new Error("Halt and catch fire!")
         }
+        next();
     }
 
     function setCsrfToken(req, response, next) {

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

@@ -0,0 +1,22 @@
+const express = require('express')
+const cookieParser = require('cookie-parser')
+const csrf = require('csurf')
+
+const app = express()
+app.use(cookieParser())
+
+app.post('/unsafe', (req, res) => { // NOT OK
+  req.cookies.x;
+});
+
+function middlewares() {
+  return express.Router()
+    .use(csrf({ cookie: true}))
+    .use('/', express.bodyParser());
+}
+
+app.use(middlewares());
+
+app.post('/safe', (req, res) => { // OK
+  req.cookies.x;
+});