From 264f4ab5ab8892f8d38930d430ed91920064693d Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik-krogh@github.com>
Date: Tue, 2 Nov 2021 14:35:02 +0100
Subject: [PATCH] add js/session-fixation query

---
 .../2021-11-02-session-fixation.md            |  2 +
 .../Security/CWE-384/SessionFixation.qhelp    | 48 +++++++++++++++
 .../src/Security/CWE-384/SessionFixation.ql   | 58 +++++++++++++++++++
 .../CWE-384/examples/SessionFixation.js       | 18 ++++++
 .../CWE-384/examples/SessionFixationFixed.js  | 24 ++++++++
 .../Security/CWE-384/SessionFixation.expected |  2 +
 .../Security/CWE-384/SessionFixation.qlref    |  1 +
 .../test/query-tests/Security/CWE-384/tst.js  | 40 +++++++++++++
 8 files changed, 193 insertions(+)
 create mode 100644 javascript/change-notes/2021-11-02-session-fixation.md
 create mode 100644 javascript/ql/src/Security/CWE-384/SessionFixation.qhelp
 create mode 100644 javascript/ql/src/Security/CWE-384/SessionFixation.ql
 create mode 100644 javascript/ql/src/Security/CWE-384/examples/SessionFixation.js
 create mode 100644 javascript/ql/src/Security/CWE-384/examples/SessionFixationFixed.js
 create mode 100644 javascript/ql/test/query-tests/Security/CWE-384/SessionFixation.expected
 create mode 100644 javascript/ql/test/query-tests/Security/CWE-384/SessionFixation.qlref
 create mode 100644 javascript/ql/test/query-tests/Security/CWE-384/tst.js

diff --git a/javascript/change-notes/2021-11-02-session-fixation.md b/javascript/change-notes/2021-11-02-session-fixation.md
new file mode 100644
index 00000000000..6c74b6a229a
--- /dev/null
+++ b/javascript/change-notes/2021-11-02-session-fixation.md
@@ -0,0 +1,2 @@
+lgtm,codescanning
+* The `js/session-fixation` query has been added. It highlights servers that reuse a session after a user has logged in.
diff --git a/javascript/ql/src/Security/CWE-384/SessionFixation.qhelp b/javascript/ql/src/Security/CWE-384/SessionFixation.qhelp
new file mode 100644
index 00000000000..257fefe5a52
--- /dev/null
+++ b/javascript/ql/src/Security/CWE-384/SessionFixation.qhelp
@@ -0,0 +1,48 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>
+Reusing a session could allow an attacker to gain unauthorized access to another account. Always
+ensure that, when a user logs in or out, the current session is abandoned so that a new
+session may be started.
+</p>
+</overview>
+
+<recommendation>
+
+<p>
+Always use <code>req.session.regenerate(...);</code> to start a new session when
+a user logs in or out.
+</p>
+
+</recommendation>
+<example>
+
+<p>
+The following example shows the previous session being used after authentication.
+This would allow a previous user to use the new user's account.
+</p>
+
+<sample src="examples/SessionFixation.js" />
+
+<p>
+This code example solves the problem by not reusing the session, and instead calling <code>req.session.regenerate()</code>
+to ensure that the session is not reused.
+</p>
+<sample src="examples/SessionFixationFixed.js" />
+
+</example>
+<references>
+<li>
+  OWASP: <a href="https://www.owasp.org/index.php/Session_fixation">Session fixation</a>
+</li>
+<li>
+  Stackoverflow: <a href="https://stackoverflow.com/questions/22209354/creating-a-new-session-after-authentication-with-passport/30468384#30468384">Creating a new session after authentication with Passport</a>
+</li>
+<li>
+  jscrambler.com: <a href="https://blog.jscrambler.com/best-practices-for-secure-session-management-in-node">Best practices for secure session management in Node</a>
+</li>
+</references>
+</qhelp>
diff --git a/javascript/ql/src/Security/CWE-384/SessionFixation.ql b/javascript/ql/src/Security/CWE-384/SessionFixation.ql
new file mode 100644
index 00000000000..86e62e65175
--- /dev/null
+++ b/javascript/ql/src/Security/CWE-384/SessionFixation.ql
@@ -0,0 +1,58 @@
+/**
+ * @name Failure to abandon session
+ * @description Reusing an existing session as a different user could allow
+ *              an attacker to access someone else's account by using
+ *              their session.
+ * @kind problem
+ * @problem.severity warning
+ * @security-severity 5
+ * @precision medium
+ * @id js/session-fixation
+ * @tags security
+ *       external/cwe/cwe-384
+ */
+
+import javascript
+
+/**
+ * Holds if `setup` uses express-session (or similar) to log in a user.
+ */
+pragma[inline]
+predicate isLoginSetup(Express::RouteSetup setup) {
+  // either some path that contains "login" with a write to `req.session`
+  setup.getPath().matches("%login%") and
+  exists(
+    setup
+        .getARouteHandler()
+        .(Express::RouteHandler)
+        .getARequestSource()
+        .ref()
+        .getAPropertyRead("session")
+        .getAPropertyWrite()
+  )
+  or
+  // or an authentication method is used (e.g. `passport.authenticate`)
+  setup.getARouteHandler().(DataFlow::CallNode).getCalleeName() = "authenticate"
+}
+
+/**
+ * Holds if `handler` is regenerates it's session using `req.session.regenerate`.
+ */
+pragma[inline]
+predicate regeneratesSession(Express::RouteSetup setup) {
+  exists(
+    setup
+        .getARouteHandler()
+        .(Express::RouteHandler)
+        .getARequestSource()
+        .ref()
+        .getAPropertyRead("session")
+        .getAPropertyRead("regenerate")
+  )
+}
+
+from Express::RouteSetup setup
+where
+  isLoginSetup(setup) and
+  not regeneratesSession(setup)
+select setup, "Route handler does not invalidate session following login"
diff --git a/javascript/ql/src/Security/CWE-384/examples/SessionFixation.js b/javascript/ql/src/Security/CWE-384/examples/SessionFixation.js
new file mode 100644
index 00000000000..9b22f255ae4
--- /dev/null
+++ b/javascript/ql/src/Security/CWE-384/examples/SessionFixation.js
@@ -0,0 +1,18 @@
+const express = require('express');
+const session = require('express-session');
+var bodyParser = require('body-parser')
+const app = express();
+app.use(bodyParser.urlencoded({ extended: false }))
+app.use(session({
+    secret: 'keyboard cat'
+}));
+
+app.post('/login', function (req, res) {
+    // Check that username password matches
+    if (req.body.username === 'admin' && req.body.password === 'admin') {
+        req.session.authenticated = true;
+        res.redirect('/');
+    } else {
+        res.redirect('/login');
+    }
+});
\ No newline at end of file
diff --git a/javascript/ql/src/Security/CWE-384/examples/SessionFixationFixed.js b/javascript/ql/src/Security/CWE-384/examples/SessionFixationFixed.js
new file mode 100644
index 00000000000..e17d5eb36bc
--- /dev/null
+++ b/javascript/ql/src/Security/CWE-384/examples/SessionFixationFixed.js
@@ -0,0 +1,24 @@
+const express = require('express');
+const session = require('express-session');
+var bodyParser = require('body-parser')
+const app = express();
+app.use(bodyParser.urlencoded({ extended: false }))
+app.use(session({
+    secret: 'keyboard cat'
+}));
+
+app.post('/login', function (req, res) {
+    // Check that username password matches
+    if (req.body.username === 'admin' && req.body.password === 'admin') {
+        req.session.regenerate(function (err) {
+            if (err) {
+                res.send('Error');
+            } else {
+                req.session.authenticated = true;
+                res.redirect('/');
+            }
+        });
+    } else {
+        res.redirect('/login');
+    }
+});
\ No newline at end of file
diff --git a/javascript/ql/test/query-tests/Security/CWE-384/SessionFixation.expected b/javascript/ql/test/query-tests/Security/CWE-384/SessionFixation.expected
new file mode 100644
index 00000000000..a2e38090145
--- /dev/null
+++ b/javascript/ql/test/query-tests/Security/CWE-384/SessionFixation.expected
@@ -0,0 +1,2 @@
+| tst.js:9:1:14:2 | app.get ... n');\\n}) | Route handler does not invalidate session following login |
+| tst.js:27:1:29:2 | app.get ... n');\\n}) | Route handler does not invalidate session following login |
diff --git a/javascript/ql/test/query-tests/Security/CWE-384/SessionFixation.qlref b/javascript/ql/test/query-tests/Security/CWE-384/SessionFixation.qlref
new file mode 100644
index 00000000000..173c5bed662
--- /dev/null
+++ b/javascript/ql/test/query-tests/Security/CWE-384/SessionFixation.qlref
@@ -0,0 +1 @@
+Security/CWE-384/SessionFixation.ql
diff --git a/javascript/ql/test/query-tests/Security/CWE-384/tst.js b/javascript/ql/test/query-tests/Security/CWE-384/tst.js
new file mode 100644
index 00000000000..752bc5a7c36
--- /dev/null
+++ b/javascript/ql/test/query-tests/Security/CWE-384/tst.js
@@ -0,0 +1,40 @@
+const express = require('express');
+const session = require('express-session');
+const passport = require('passport');
+const app = express();
+app.use(session({
+    secret: 'keyboard cat'
+}));
+// handle login
+app.get('/login', function (req, res) { // NOT OK - no regenerate
+    req.session.user = {
+        userId: something
+    };
+    res.send('logged in');
+});
+
+// with regenerate
+app.get('/login2', function (req, res) { // OK
+    req.session.regenerate(function (err) {
+        req.session.user = {
+            userId: something
+        };
+        res.send('logged in');
+    });
+});
+
+// using passport
+app.get('/passport', passport.authenticate('local'), function (req, res) { // NOT OK - no regenerate
+    res.send('logged in');
+});
+
+// with regenerate, still using passport
+app.get('/passport2', passport.authenticate('local'), function (req, res) { // OK
+    var passport = req._passport.instance;
+    req.session.regenerate(function(err, done, user) {
+        req.session[passport._key] = {};
+        req._passport.instance = passport;
+        req._passport.session = req.session[passport._key];
+        res.send('logged in');
+    });
+});