diff --git a/change-notes/1.24/analysis-javascript.md b/change-notes/1.24/analysis-javascript.md index 5e0dea684c9..8f36921b149 100644 --- a/change-notes/1.24/analysis-javascript.md +++ b/change-notes/1.24/analysis-javascript.md @@ -33,6 +33,7 @@ | Unbound event handler receiver (`js/unbound-event-handler-receiver`) | Fewer false positive results | This query now recognizes additional ways event handler receivers can be bound. | | Expression has no effect (`js/useless-expression`) | Fewer false positive results | The query now recognizes block-level flow type annotations and ignores the first statement of a try block. | | Use of call stack introspection in strict mode (`js/strict-mode-call-stack-introspection`) | Fewer false positive results | The query no longer flags expression statements. | +| Missing CSRF middleware (`js/missing-token-validation`) | Fewer false positive results | The query reports fewer duplicates and only flags handlers that explicitly access cookie data. | ## Changes to libraries diff --git a/javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql b/javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql index 192f32dc79b..bf1f6215b53 100644 --- a/javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql +++ b/javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql @@ -12,6 +12,45 @@ import javascript +/** Gets a data flow node that flows to the base of an access to `cookies` or `session`. */ +private DataFlow::SourceNode nodeLeadingToCookieAccess(DataFlow::TypeBackTracker t) { + t.start() and + exists(string name | name = "session" or name = "cookies" | + exists(result.getAPropertyRead(name)) + ) + 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` or `session`. */ +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 | + result = getARouteUsingCookies(t2).track(t2, t) + ) +} + +/** 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`. * @@ -63,11 +102,23 @@ from 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.getPreviousMiddleware*()) and hasCookieMiddleware(handler, cookie) and + + // Only flag the first cookie parser registered first. + not hasCookieMiddleware(cookie, _) and + not hasCsrfMiddleware(handler) and // Only warn for the last handler in a chain. handler.isLastHandler() and - // Only warn for dangerous for handlers, such as for POST and PUT. + // Only warn for dangerous handlers, such as for POST and PUT. not setup.getRequestMethod().isSafe() select cookie, "This cookie middleware is serving a request handler $@ without CSRF protection.", handler, "here"