JS: Improve precision of missing CSRF middleware

Author: asgerf

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

@@ -72,19 +72,37 @@ DataFlow::SourceNode csrfMiddlewareCreation() {
   result = Fastify::server().getAPropertyRead("csrfProtection")
 }
 
+/** Holds if the given property has a name indicating that it refers to a CSRF token. */
+pragma[nomagic]
+private predicate isCsrfProperty(DataFlow::PropRef ref) {
+  ref.getPropertyName().regexpMatch("(?i).*(csrf|xsrf).*")
+}
+
 /**
  * Gets a data flow node that flows to the base of a reference to `cookies`, `session`, or `user`,
- * where the referenced property has `csrf` or `xsrf` in its name,
- * and a property is either written or part of a comparison.
+ * of which a property appears to be used in a CSRF token check.
  */
 private DataFlow::SourceNode nodeLeadingToCsrfWriteOrCheck(DataFlow::TypeBackTracker t) {
   t.start() and
   exists(DataFlow::PropRef ref |
-    ref = result.getAPropertyRead(cookieProperty()).getAPropertyReference() and
-    ref.getPropertyName().regexpMatch("(?i).*(csrf|xsrf).*")
+    ref = result.getAPropertyRead(cookieProperty()).getAPropertyReference()
   |
-    ref instanceof DataFlow::PropWrite or
-    ref.(DataFlow::PropRead).asExpr() = any(EqualityTest c).getAnOperand()
+    // Assignment to property with csrf/xsrf in the name
+    ref instanceof DataFlow::PropWrite and
+    isCsrfProperty(ref)
+    or
+    // Comparison where one of the properties has csrf/xsrf in the name
+    exists(EqualityTest test |
+      test.getAnOperand().flow().getALocalSource() = ref and
+      isCsrfProperty(test.getAnOperand().flow().getALocalSource())
+    )
+    or
+    // Comparison via a call, where one of the properties has csrf/xsrf in the name
+    exists(DataFlow::CallNode call |
+      call.getCalleeName().regexpMatch("(?i).*(check|verify|valid|equal).*") and
+      call.getAnArgument().getALocalSource() = ref and
+      isCsrfProperty(call.getAnArgument().getALocalSource())
+    )
   )
   or
   exists(DataFlow::TypeBackTracker t2 | result = nodeLeadingToCsrfWriteOrCheck(t2).backtrack(t2, t))
@@ -128,21 +146,26 @@ predicate hasCsrfMiddleware(Routing::RouteHandler handler) {
   handler.isGuardedByNode(getACsrfMiddleware())
 }
 
-from Routing::RouteSetup setup, Routing::RouteHandler handler, HTTP::CookieMiddlewareInstance cookie
+from
+  Routing::RouteSetup setup, Routing::Node setupArg, Routing::RouteHandler handler,
+  HTTP::CookieMiddlewareInstance cookie
 where
   // 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.
-  setup.getAChild*() = handler and
+  setup.getAChild() = setupArg and
+  setupArg.getAChild*() = handler and
   isRouteHandlerUsingCookies(handler) and
   hasCookieMiddleware(handler, cookie) and
   // Only flag the cookie parser registered first.
   not hasCookieMiddleware(Routing::getNode(cookie), _) and
   not hasCsrfMiddleware(handler) and
+  // Sometimes the CSRF protection comes later in the same route setup.
+  not setup.getAChild*() = getACsrfMiddleware() and
   // Only warn for dangerous handlers, such as for POST and PUT.
   setup.getOwnHttpMethod().isUnsafe()
 select cookie, "This cookie middleware is serving a request handler $@ without CSRF protection.",
-  setup, "here"
+  setupArg, "here"

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

@@ -1,13 +1,13 @@
-| 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 |
+| 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 |
+| fastify.js:5:14:5:38 | require ... ookie') | This cookie middleware is serving a request handler $@ without CSRF protection. | fastify.js:20:12:23:3 | async ( ... ody\\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 |
+| tst.js:6:9:6:22 | cookieParser() | This cookie middleware is serving a request handler $@ without CSRF protection. | tst.js:8:21:10:1 | (req, r ... es.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: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 |