From 4d1a9740f0a38b2e40771f40ece11e1ac1d500b1 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Thu, 15 Oct 2020 00:14:16 +0200 Subject: [PATCH 1/6] add support for home made CSRF protection middlewares in js/missing-token-validation --- .../Security/CWE-352/MissingCsrfMiddleware.ql | 65 ++++++++++++++++++- .../semmle/javascript/frameworks/Express.qll | 2 +- 2 files changed, 64 insertions(+), 3 deletions(-) diff --git a/javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql b/javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql index d63786ba08d..baef11e5471 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 | @@ -97,11 +97,72 @@ DataFlow::CallNode csrfMiddlewareCreation() { ) } +/** + * 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 + exists(DataFlow::PropRef value | + value = result.getAPropertyRead(cookieProperty()).getAPropertyWrite() and + value.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 isACsrfProtectionRouteHandler(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 + isACsrfProtectionRouteHandler(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 CSFR 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") } From 8206933e850173aabc9addbb5934c82b4ee82fa8 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Thu, 15 Oct 2020 13:59:17 +0200 Subject: [PATCH 2/6] add test for home grown CSRF protection --- .../CWE-352/MissingCsrfMiddlewareGood2.js | 75 +++++++++++++++++++ 1 file changed, 75 insertions(+) create mode 100644 javascript/ql/test/query-tests/Security/CWE-352/MissingCsrfMiddlewareGood2.js 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..749e29a8840 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-352/MissingCsrfMiddlewareGood2.js @@ -0,0 +1,75 @@ +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"]; + }) +}); From ff054b985b0a07d03a3df9f7af8f9a77f11eab76 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Thu, 15 Oct 2020 14:01:32 +0200 Subject: [PATCH 3/6] add change note --- change-notes/1.26/analysis-javascript.md | 1 + 1 file changed, 1 insertion(+) diff --git a/change-notes/1.26/analysis-javascript.md b/change-notes/1.26/analysis-javascript.md index 01d35a12c2d..debe63bd1cf 100644 --- a/change-notes/1.26/analysis-javascript.md +++ b/change-notes/1.26/analysis-javascript.md @@ -43,6 +43,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`) | Less results | This query now recognizes more ways of protecting against CSRF attacks. | ## Changes to libraries From 017c73dce31ce81002c96c0ded5f4cd499164589 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Fri, 16 Oct 2020 14:19:21 +0200 Subject: [PATCH 4/6] Apply suggestions from code review Co-authored-by: Asger F --- change-notes/1.26/analysis-javascript.md | 2 +- javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/change-notes/1.26/analysis-javascript.md b/change-notes/1.26/analysis-javascript.md index debe63bd1cf..e88c84b7115 100644 --- a/change-notes/1.26/analysis-javascript.md +++ b/change-notes/1.26/analysis-javascript.md @@ -43,7 +43,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`) | Less results | This query now recognizes more ways of protecting against 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 baef11e5471..2f896418706 100644 --- a/javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql +++ b/javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql @@ -126,7 +126,7 @@ 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 isACsrfProtectionRouteHandler(Express::RouteHandler handler) { +predicate isCsrfProtectionRouteHandler(Express::RouteHandler handler) { DataFlow::parameterNode(handler.getRequestParameter()) = nodeLeadingToCsrfWrite(DataFlow::TypeBackTracker::end()) or @@ -136,7 +136,7 @@ predicate isACsrfProtectionRouteHandler(Express::RouteHandler handler) { /** 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 - isACsrfProtectionRouteHandler(result) + isCsrfProtectionRouteHandler(result) or exists(DataFlow::TypeTracker t2, DataFlow::SourceNode pred | pred = getACsrfProtectionRouteHandler(t2) @@ -150,7 +150,7 @@ private DataFlow::SourceNode getACsrfProtectionRouteHandler(DataFlow::TypeTracke /** * Gets an express route handler expression that is either a custom CSRF protection middleware, - * or a CSFR protecting library. + * or a CSRF protecting library. */ Express::RouteHandlerExpr getACsrfMiddleware() { csrfMiddlewareCreation().flowsToExpr(result) From 27a2cd310d2ded0c995f9749197ff1b7f6f657ab Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Fri, 16 Oct 2020 14:21:49 +0200 Subject: [PATCH 5/6] inline `value` in `nodeLeadingToCsrfWrite` --- .../ql/src/Security/CWE-352/MissingCsrfMiddleware.ql | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql b/javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql index 2f896418706..ceec62f9368 100644 --- a/javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql +++ b/javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql @@ -103,10 +103,11 @@ DataFlow::CallNode csrfMiddlewareCreation() { */ private DataFlow::SourceNode nodeLeadingToCsrfWrite(DataFlow::TypeBackTracker t) { t.start() and - exists(DataFlow::PropRef value | - value = result.getAPropertyRead(cookieProperty()).getAPropertyWrite() and - value.getPropertyName().regexpMatch("(?i).*(csrf|xsrf).*") - ) + result + .getAPropertyRead(cookieProperty()) + .getAPropertyWrite() + .getPropertyName() + .regexpMatch("(?i).*(csrf|xsrf).*") or exists(DataFlow::TypeBackTracker t2 | result = nodeLeadingToCsrfWrite(t2).backtrack(t2, t)) } From ce956761307d92fe2a328661e6d4031b3bc16547 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Mon, 19 Oct 2020 15:39:02 +0200 Subject: [PATCH 6/6] add express.csrf as an CSRF protecting middleware --- .../Security/CWE-352/MissingCsrfMiddleware.ql | 2 ++ .../CWE-352/MissingCsrfMiddlewareGood2.js | 17 +++++++++++++++++ 2 files changed, 19 insertions(+) diff --git a/javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql b/javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql index ceec62f9368..ee931339b76 100644 --- a/javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql +++ b/javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql @@ -94,6 +94,8 @@ DataFlow::CallNode csrfMiddlewareCreation() { exists(result.getOptionArgument(0, "csrf")) or callee = DataFlow::moduleMember("lusca", "csrf") + or + callee = DataFlow::moduleMember("express", "csrf") ) } diff --git a/javascript/ql/test/query-tests/Security/CWE-352/MissingCsrfMiddlewareGood2.js b/javascript/ql/test/query-tests/Security/CWE-352/MissingCsrfMiddlewareGood2.js index 749e29a8840..73154102da9 100644 --- a/javascript/ql/test/query-tests/Security/CWE-352/MissingCsrfMiddlewareGood2.js +++ b/javascript/ql/test/query-tests/Security/CWE-352/MissingCsrfMiddlewareGood2.js @@ -73,3 +73,20 @@ var passport = require('passport'); 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