diff --git a/change-notes/1.26/analysis-javascript.md b/change-notes/1.26/analysis-javascript.md index 939c85dbf68..8797c05cd42 100644 --- a/change-notes/1.26/analysis-javascript.md +++ b/change-notes/1.26/analysis-javascript.md @@ -52,6 +52,7 @@ | Unsafe jQuery plugin (`js/unsafe-jquery-plugin`) | More results | This query now detects more unsafe uses of nested option properties. | | Client-side URL redirect (`js/client-side-unvalidated-url-redirection`) | More results | This query now recognizes some unsafe uses of `importScripts()` inside WebWorkers. | | Missing CSRF middleware (`js/missing-token-validation`) | More results | This query now recognizes writes to cookie and session variables as potentially vulnerable to CSRF attacks. | +| Missing CSRF middleware (`js/missing-token-validation`) | Fewer results | This query now recognizes more ways of protecting against CSRF attacks. | ## Changes to libraries diff --git a/javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql b/javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql index d63786ba08d..ee931339b76 100644 --- a/javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql +++ b/javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql @@ -15,7 +15,7 @@ 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 an access to `cookies`, `session`, or `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 exists(DataFlow::PropRef value | @@ -94,14 +94,78 @@ DataFlow::CallNode csrfMiddlewareCreation() { exists(result.getOptionArgument(0, "csrf")) or callee = DataFlow::moduleMember("lusca", "csrf") + or + callee = DataFlow::moduleMember("express", "csrf") ) } +/** + * Gets a data flow node that flows to the base of a write to `cookies`, `session`, or `user`, + * where the written property has `csrf` or `xsrf` in its name. + */ +private DataFlow::SourceNode nodeLeadingToCsrfWrite(DataFlow::TypeBackTracker t) { + t.start() and + result + .getAPropertyRead(cookieProperty()) + .getAPropertyWrite() + .getPropertyName() + .regexpMatch("(?i).*(csrf|xsrf).*") + or + exists(DataFlow::TypeBackTracker t2 | result = nodeLeadingToCsrfWrite(t2).backtrack(t2, t)) +} + +/** + * Gets a route handler that sets an CSRF related cookie. + */ +private Express::RouteHandler getAHandlerSettingCsrfCookie() { + exists(HTTP::CookieDefinition setCookie | + setCookie.getNameArgument().getStringValue().regexpMatch("(?i).*(csrf|xsrf).*") and + result = setCookie.getRouteHandler() + ) +} + +/** + * Holds if `handler` is protecting from CSRF. + * 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()) = + nodeLeadingToCsrfWrite(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) + or + getACsrfProtectionRouteHandler(DataFlow::TypeTracker::end()).flowsToExpr(result) +} + /** * Holds if the given route handler is protected by CSRF middleware. */ predicate hasCsrfMiddleware(Express::RouteHandlerExpr handler) { - csrfMiddlewareCreation().flowsToExpr(handler.getAMatchingAncestor()) + getACsrfMiddleware() = handler.getAMatchingAncestor() } from diff --git a/javascript/ql/src/semmle/javascript/frameworks/Express.qll b/javascript/ql/src/semmle/javascript/frameworks/Express.qll index af6675d188e..828fb4ac115 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/Express.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/Express.qll @@ -718,7 +718,7 @@ module Express { /** * An invocation of the `cookie` method on an HTTP response object. */ - private class SetCookie extends HTTP::CookieDefinition, MethodCallExpr { + class SetCookie extends HTTP::CookieDefinition, MethodCallExpr { RouteHandler rh; SetCookie() { calls(rh.getAResponseExpr(), "cookie") } diff --git a/javascript/ql/test/query-tests/Security/CWE-352/MissingCsrfMiddlewareGood2.js b/javascript/ql/test/query-tests/Security/CWE-352/MissingCsrfMiddlewareGood2.js new file mode 100644 index 00000000000..73154102da9 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-352/MissingCsrfMiddlewareGood2.js @@ -0,0 +1,92 @@ +var express = require('express'); +var cookieParser = require('cookie-parser'); +var passport = require('passport'); + +(function () { + + var app = express() + + app.use(cookieParser()) + app.use(passport.authorize({ session: true })) + + function getCsrfToken(request) { + return request.headers['x-xsrf-token']; + } + + function setCsrfToken(request, response, next) { + response.cookie('XSRF-TOKEN', request.csrfToken()); + next(); + } + + const csrf = { + getCsrfToken: getCsrfToken, + setCsrfToken: setCsrfToken + }; + + app.use(express.csrf({ value: csrf.getCsrfToken })); + app.use(csrf.setCsrfToken); + + app.post('/changeEmail', function (req, res) { + let newEmail = req.cookies["newEmail"]; + }) +}); + + + +(function () { + var app = express() + + app.use(cookieParser()) + app.use(passport.authorize({ session: true })) + + var crypto = require('crypto'); + + var generateToken = function (len) { + return crypto.randomBytes(Math.ceil(len * 3 / 4)) + .toString('base64') + .slice(0, len); + }; + function defaultValue(req) { + return (req.body && req.body._csrf) + || (req.query && req.query._csrf) + || (req.headers['x-csrf-token']); + } + var checkToken = function (req, res, next) { + var token = req.session._csrf || (req.session._csrf = generateToken(24)); + if ('GET' == req.method || 'HEAD' == req.method || 'OPTIONS' == req.method) return next(); + var val = defaultValue(req); + if (val != token) return next(function () { + res.send({ auth: false }); + }); + next(); + } + const csrf = { + check: checkToken + }; + + app.use(express.cookieParser()); + app.use(express.session({ secret: 'thomasdavislovessalmon' })); + app.use(express.bodyParser()); + app.use(csrf.check); + + app.post('/changeEmail', function (req, res) { + let newEmail = req.cookies["newEmail"]; + }) +}); + +(function () { + + var app = express() + + app.use(cookieParser()) + app.use(passport.authorize({ session: true })) + + // Assume token is being set somewhere + app.use(express.csrf({ value: function (request) { + return request.headers['x-xsrf-token']; + }})); + + app.post('/changeEmail', function (req, res) { + let newEmail = req.cookies["newEmail"]; + }) +}); \ No newline at end of file