Merge pull request #2681 from asger-semmle/csrf-only-session-cookie-access

Approved by erik-krogh, max-schaefer

Author: semmle-qlci

change-notes/1.24/analysis-javascript.md

@@ -38,6 +38,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
 

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

@@ -12,6 +12,56 @@
 
 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`. */
+private DataFlow::SourceNode nodeLeadingToCookieAccess(DataFlow::TypeBackTracker t) {
+  t.start() and
+  exists(DataFlow::PropRead value |
+    value = result.getAPropertyRead(cookieProperty()).getAPropertyRead() and
+
+    // Ignore accesses to values that are part of a CSRF or captcha check
+    not value.getPropertyName().regexpMatch("(?i).*(csrf|xsrf|captcha).*") and
+
+    // Ignore calls like `req.session.save()`
+    not value = any(DataFlow::InvokeNode call).getCalleeNode()
+  )
+  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`, `session`, or `user`. */
+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 +113,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) and
   hasCookieMiddleware(handler, cookie) and
+
+  // Only flag the 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"

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

@@ -1,5 +1,8 @@
-| MissingCsrfMiddlewareBad.js:7:9:7:22 | cookieParser() | This cookie middleware is serving a request handler $@ without CSRF protection. | MissingCsrfMiddlewareBad.js:10:26:11:1 | functio ... es) {\\n} | here |
-| csurf_api_example.js:39:37:39:50 | cookieParser() | This cookie middleware is serving a request handler $@ without CSRF protection. | csurf_api_example.js:39:53:41: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:29:40:31: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:23:42:25: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:27:40:29:1 | functio ... sed')\\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 |
+| 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 |
+| 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 |
+| 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 |

javascript/ql/test/query-tests/Security/CWE-352/MissingCsrfMiddlewareBad.js

@@ -8,4 +8,5 @@ app.use(cookieParser())
 app.use(passport.authorize({ session: true }))
 
 app.post('/changeEmail', function (req, res) {
+    let newEmail = req.cookies["newEmail"];
 })

javascript/ql/test/query-tests/Security/CWE-352/MissingCsrfMiddlewareGood.js

@@ -10,4 +10,5 @@ app.use(passport.authorize({ session: true }))
 app.use(csrf({ cookie:true }))
 
 app.post('/changeEmail', function (req, res) {
+    let newEmail = req.cookies["newEmail"];
 })

javascript/ql/test/query-tests/Security/CWE-352/csurf_api_example.js

@@ -21,22 +21,26 @@ app.use(cookieParser())
 app.use(csrf({ cookie: true }))
 
 app.get('/form', function (req, res) {
+  let newEmail = req.cookies["newEmail"];
   // pass the csrfToken to the view
   res.render('send', { csrfToken: req.csrfToken() })
 })
 
 app.post('/process', function (req, res) { // OK
+  let newEmail = req.cookies["newEmail"];
   res.send('csrf was required to get here')
 })
 
 function createApiRouter () {
   var router = new express.Router()
 
   router.post('/getProfile', function (req, res) { // OK - cookies are not parsed
+    let newEmail = req.cookies["newEmail"];
     res.send('no csrf to get here')
   })
 
   router.post('/getProfile_unsafe', cookieParser(), function (req, res) { // NOT OK - may use cookies
+    let newEmail = req.cookies["newEmail"];
     res.send('no csrf to get here')
   })
 

javascript/ql/test/query-tests/Security/CWE-352/csurf_example.js

@@ -18,14 +18,17 @@ var app = express()
 app.use(cookieParser())
 
 app.get('/form', csrfProtection, function (req, res) { // OK
+  let newEmail = req.cookies["newEmail"];
   // pass the csrfToken to the view
   res.render('send', { csrfToken: req.csrfToken() })
 })
 
 app.post('/process', parseForm, csrfProtection, function (req, res) { // OK
+  let newEmail = req.cookies["newEmail"];
   res.send('data is being processed')
 })
 
 app.post('/process_unsafe', parseForm, function (req, res) { // NOT OK
+  let newEmail = req.cookies["newEmail"];
   res.send('data is being processed')
 })

javascript/ql/test/query-tests/Security/CWE-352/lusca_example.js

@@ -9,21 +9,26 @@ var app = express()
 app.use(cookieParser())
 
 app.post('/process', parseForm, lusca.csrf(), function (req, res) { // OK
+  let newEmail = req.cookies["newEmail"];
   res.send('data is being processed')
 })
 
 app.post('/process', parseForm, lusca({csrf:true}), function (req, res) { // OK
+  let newEmail = req.cookies["newEmail"];
   res.send('data is being processed')
 })
 
 app.post('/process', parseForm, lusca({csrf:{}}), function (req, res) { // OK
+  let newEmail = req.cookies["newEmail"];
   res.send('data is being processed')
 })
 
 app.post('/process', parseForm, lusca(), function (req, res) { // NOT OK - missing csrf option
+  let newEmail = req.cookies["newEmail"];
   res.send('data is being processed')
 })
 
 app.post('/process_unsafe', parseForm, function (req, res) { // NOT OK
+  let newEmail = req.cookies["newEmail"];
   res.send('data is being processed')
 })

javascript/ql/test/query-tests/Security/CWE-352/unused_cookies.js

@@ -0,0 +1,39 @@
+let express = require('express');
+let cookieParser = require('cookie-parser');
+
+let app = express();
+
+app.use(cookieParser());
+
+app.post('/doSomethingTerrible', (req, res) => { // NOT OK - uses cookies
+    if (req.cookies['secret'] === app.secret) {
+        somethingTerrible();
+    }
+    res.end('Ok');
+});
+
+app.post('/doSomethingElse', (req, res) => { // OK - doesn't actually use cookies
+    somethingElse(req.query['data']);
+    res.end('Ok');
+});
+
+app.post('/doWithCaptcha', (req, res) => { // OK - attacker can't guess the captcha value either
+    if (req.session['captcha'] !== req.query['captcha']) {
+        res.end("You guessed wrong, that 'u' was actually a 'U'. Try again.");
+        return;
+    }
+    somethingElse(req.query['data']);
+    res.end('Ok');
+});
+
+app.post('/user', (req, res) => { // NOT OK - access to req.user is unprotected
+    somethingElse(req.user.name);
+    res.end('Ok');
+});
+
+app.post('/session', (req, res) => { // NOT OK - access to req.session is unprotected
+    somethingElse(req.session.name);
+    res.end('Ok');
+});
+
+app.listen();