diff --git a/src/node/hooks/express/webaccess.ts b/src/node/hooks/express/webaccess.ts index 890a6f37a3d..440cb352553 100644 --- a/src/node/hooks/express/webaccess.ts +++ b/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((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 = ''} = req.session.user; httpLogger.info( `Successful authentication from IP ${anonymizeIp(req.ip, settings.ipLogging)} ` + diff --git a/src/tests/backend/specs/sessionFixation.ts b/src/tests/backend/specs/sessionFixation.ts new file mode 100644 index 00000000000..26a0c88266d --- /dev/null +++ b/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.` 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); + }); +});