security(auth): regenerate session id on authentication (session fixation)

src/node/hooks/express/webaccess.ts

@@ -22,6 +22,22 @@ const aCallFirst0 =
     // @ts-ignore
     async (hookName: string, context:any, pred = null) => (await aCallFirst(hookName, context, pred))[0];
 
+// Rotate the express-session id while preserving the session's data. Used at the
+// authentication boundary to prevent session fixation (GHSA-73h9-c5xp-gfg4).
+// The freshly minted cookie for the new id is kept; all other session data
+// (notably req.session.user) is carried across onto the new session.
+const regenerateSessionPreservingData = (req: any) => new Promise<void>((resolve, reject) => {
+  // Session prototype methods (regenerate/save/...) are non-enumerable, so the
+  // spread captures only data properties. Drop `cookie` so the new session keeps
+  // the fresh cookie regenerate() creates.
+  const {cookie, ...data} = req.session;
+  req.session.regenerate((err: any) => {
+    if (err) return reject(err);
+    Object.assign(req.session, data);
+    req.session.save((saveErr: any) => saveErr != null ? reject(saveErr) : resolve());
+  });
+});
+
 exports.normalizeAuthzLevel = (level: string|boolean) => {
   if (!level) return false;
   switch (level) {
@@ -158,6 +174,11 @@ const checkAccess = async (req:any, res:any, next: Function) => {
 
   if (settings.users == null) settings.users = {};
   const ctx:WebAccessTypes = {req, res, users: settings.users, next};
+  // Identity carried by the session BEFORE the authenticate step runs. Used
+  // below to decide whether authentication changed the principal (anonymous ->
+  // user, or a privilege/identity change such as non-admin -> admin), which is
+  // the point at which the session id must be rotated (see below).
+  const prevUser = req.session != null ? req.session.user : null;
   // If the HTTP basic auth header is present, extract the username and password so it can be given
   // to authn plugins.
   const httpBasicAuth = req.headers.authorization && req.headers.authorization.startsWith('Basic ');
@@ -206,6 +227,26 @@ const checkAccess = async (req:any, res:any, next: Function) => {
     httpLogger.error('authenticate hook failed to add user settings to session');
     return res.status(500).send('Internal Server Error');
   }
+  // Session fixation defense (GHSA-73h9-c5xp-gfg4): rotate the session id
+  // whenever authentication changed the principal — an anonymous session
+  // becoming authenticated, OR an authenticated session changing identity or
+  // privilege level (e.g. non-admin -> admin re-authentication). This prevents a
+  // pre-auth / lower-privilege id (which an attacker may have planted or
+  // captured — e.g. one an SSO plugin persisted before redirecting to the IdP)
+  // from owning the resulting session. A no-op re-authentication of the same
+  // principal is left alone (no churn), and the rotation is skipped when the
+  // session store doesn't expose regenerate().
+  const identityChanged = prevUser == null ||
+      prevUser.username !== req.session.user.username ||
+      !!prevUser.is_admin !== !!req.session.user.is_admin;
+  if (identityChanged && typeof req.session.regenerate === 'function') {
+    try {
+      await regenerateSessionPreservingData(req);
+    } catch (err) {
+      httpLogger.error(`failed to regenerate session on authentication: ${err}`);
+      return res.status(500).send('Internal Server Error');
+    }
+  }
   const {username = '<no username>'} = req.session.user;
   httpLogger.info(
       `Successful authentication from IP ${anonymizeIp(req.ip, settings.ipLogging)} ` +

src/tests/backend/specs/sessionFixation.ts

@@ -0,0 +1,133 @@
+'use strict';
+
+/**
+ * Regression for GHSA-73h9-c5xp-gfg4 (SSO session fixation). Etherpad
+ * establishes a pre-authentication express-session (e.g. an SSO plugin persists
+ * OAuth state before redirecting to the IdP). If the session id is NOT rotated
+ * when the session becomes authenticated, an attacker who planted / captured the
+ * pre-auth cookie ends up owning the victim's authenticated session.
+ *
+ * webaccess must call req.session.regenerate() at the authentication boundary so
+ * the id issued after login differs from the one held before it.
+ */
+
+const assert = require('assert').strict;
+const common = require('../common');
+const plugins = require('../../../static/js/pluginfw/plugin_defs');
+import settings from '../../../node/utils/Settings';
+
+const makeHook = (hookName: string, hookFn: Function) => ({
+  hook_fn: hookFn,
+  hook_fn_name: `sessfix/${hookName}`,
+  hook_name: hookName,
+  part: {plugin: 'sessfix'},
+});
+
+// Etherpad names the session cookie `${cookie.prefix}express_sid`.
+const sessionCookieName = () => `${settings.cookie.prefix || ''}express_sid`;
+
+// Pull a raw Set-Cookie value (e.g. the signed `s%3A<id>.<sig>` blob) by name.
+const getSetCookie = (res: any, name: string): string | null => {
+  const arr: string[] = res.headers['set-cookie'] || [];
+  for (const c of arr) {
+    if (c.startsWith(`${name}=`)) return c.slice(name.length + 1).split(';')[0];
+  }
+  return null;
+};
+
+describe(__filename, function () {
+  this.timeout(30000);
+  let agent: any;
+  const backup: any = {};
+
+  before(async function () { agent = await common.init(); });
+
+  beforeEach(function () {
+    backup.authenticate = plugins.hooks.authenticate;
+    backup.requireAuthentication = settings.requireAuthentication;
+    backup.requireAuthorization = settings.requireAuthorization;
+    settings.requireAuthentication = true;
+    settings.requireAuthorization = false;
+    // An authn plugin that: (a) always touches the session so express-session
+    // persists a cookie even pre-auth, and (b) only establishes req.session.user
+    // when the login header is present (simulating a completed SSO callback).
+    plugins.hooks.authenticate = [makeHook('authenticate', (hookName: string, ctx: any, cb: Function) => {
+      ctx.req.session.sessfixTouched = true;
+      if (ctx.req.headers['x-do-login'] === '1') {
+        ctx.req.session.user = {username: 'victim'};
+        return cb([true]);
+      }
+      return cb([false]);
+    })];
+  });
+
+  afterEach(function () {
+    plugins.hooks.authenticate = backup.authenticate;
+    settings.requireAuthentication = backup.requireAuthentication;
+    settings.requireAuthorization = backup.requireAuthorization;
+  });
+
+  it('rotates the session id when a pre-auth session authenticates', async function () {
+    // Step 1 — anonymous request establishes a server-issued pre-auth session.
+    const r1 = await agent.get('/').expect(401);
+    const preSid = getSetCookie(r1, sessionCookieName());
+    assert.ok(preSid, 'server issued a pre-auth session cookie');
+
+    // Step 2 — same session cookie, now authenticate.
+    const r2 = await agent.get('/')
+        .set('Cookie', `${sessionCookieName()}=${preSid}`)
+        .set('x-do-login', '1')
+        .expect(200);
+    const postSid = getSetCookie(r2, sessionCookieName());
+    assert.ok(postSid,
+        'the authentication response must rotate the session cookie (Set-Cookie present)');
+    assert.notEqual(postSid, preSid,
+        'session id must change at the authentication boundary (session fixation)');
+  });
+
+  it('rotates the session id on a privilege upgrade (non-admin -> admin)', async function () {
+    // Reach Step 3 (authenticate) for an ALREADY-authenticated session by making
+    // authorization deny unless an `x-authz` header is present.
+    settings.requireAuthorization = true;
+    const authzBackup = plugins.hooks.authorize;
+    plugins.hooks.authorize = [makeHook('authorize', (hookName: string, ctx: any, cb: Function) =>
+      cb([ctx.req.headers['x-authz'] === '1']))];
+    plugins.hooks.authenticate = [makeHook('authenticate', (hookName: string, ctx: any, cb: Function) => {
+      const login = ctx.req.headers['x-login'];
+      if (login === 'user') { ctx.req.session.user = {username: 'u', is_admin: false}; return cb([true]); }
+      if (login === 'admin') { ctx.req.session.user = {username: 'u', is_admin: true}; return cb([true]); }
+      return cb([false]);
+    })];
+    try {
+      // Log in as a non-admin (authorization granted via x-authz).
+      const r1 = await agent.get('/').set('x-login', 'user').set('x-authz', '1').expect(200);
+      const sid1 = getSetCookie(r1, sessionCookieName());
+      assert.ok(sid1, 'non-admin login issued a session cookie');
+
+      // Re-authenticate as admin carrying sid1 and NO x-authz, so Step 2
+      // authorization denies and Step 3 runs the privilege upgrade.
+      const r2 = await agent.get('/')
+          .set('Cookie', `${sessionCookieName()}=${sid1}`)
+          .set('x-login', 'admin')
+          .expect(200);
+      const sid2 = getSetCookie(r2, sessionCookieName());
+      assert.ok(sid2, 'the privilege upgrade rotated the session cookie');
+      assert.notEqual(sid2, sid1, 'session id must change on a privilege upgrade');
+    } finally {
+      plugins.hooks.authorize = authzBackup;
+    }
+  });
+
+  it('preserves the authenticated user across the rotation', async function () {
+    const r1 = await agent.get('/').expect(401);
+    const preSid = getSetCookie(r1, sessionCookieName());
+    const r2 = await agent.get('/')
+        .set('Cookie', `${sessionCookieName()}=${preSid}`)
+        .set('x-do-login', '1')
+        .expect(200);
+    const postSid = getSetCookie(r2, sessionCookieName());
+    // The rotated session must still be authenticated: a follow-up request with
+    // the NEW cookie (and no login header) is authorized, not bounced to 401.
+    await agent.get('/').set('Cookie', `${sessionCookieName()}=${postSid}`).expect(200);
+  });
+});