Merge pull request #4478 from erik-krogh/homegrownCsrf

Approved by asgerf

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

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

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") }

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"];
+    })
+});