Merge pull request #4359 from erik-krogh/cookieWrites

Approved by esbena

Author: codeql-ci

change-notes/1.26/analysis-javascript.md

@@ -40,6 +40,7 @@
 | Unsafe shell command constructed from library input (`js/shell-command-constructed-from-input`) | More results | This query now recognizes more commands where colon, dash, and underscore are used. |
 | 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. |
 
 
 ## Changes to libraries

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

@@ -18,8 +18,8 @@ string cookieProperty() { result = "session" or result = "cookies" or result = "
 /** 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
+  exists(DataFlow::PropRef value |
+    value = result.getAPropertyRead(cookieProperty()).getAPropertyReference() 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()`

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

@@ -1,6 +1,7 @@
 | 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 |
 | MissingCsrfMiddlewareBad.js:17:13:17:26 | cookieParser() | This cookie middleware is serving a request handler $@ without CSRF protection. | MissingCsrfMiddlewareBad.js:25:30:27:6 | errorCa ... \\n    }) | here |
 | MissingCsrfMiddlewareBad.js:33:13:33:26 | cookieParser() | This cookie middleware is serving a request handler $@ without CSRF protection. | MissingCsrfMiddlewareBad.js:41:30:43:6 | errorCa ... \\n    }) | here |
+| MissingCsrfMiddlewareBad.js:33:13:33:26 | cookieParser() | This cookie middleware is serving a request handler $@ without CSRF protection. | MissingCsrfMiddlewareBad.js:45:31:47:6 | errorCa ... \\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 |

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

@@ -41,4 +41,8 @@ app.post('/changeEmail', function (req, res) {
     app.post('/changeEmail', errorCatch(async function (req, res) {
         let newEmail = req.cookies["newEmail"];
     }));
+
+    app.post('/doLoginStuff', errorCatch(async function (req, res) {
+        req.session.user = loginStuff(req);
+    }));
 })