diff --git a/src/node/handler/PadMessageHandler.ts b/src/node/handler/PadMessageHandler.ts index 7ede7d391cd..764ec2e6e68 100644 --- a/src/node/handler/PadMessageHandler.ts +++ b/src/node/handler/PadMessageHandler.ts @@ -204,7 +204,14 @@ class Channels { /** * A changeset queue per pad that is processed by handleUserChanges() */ -const padChannels = new Channels((ch, {socket, message}) => handleUserChanges(socket, message)); +// The channel key `ch` is the pad id captured at enqueue time (see the enqueue +// call). Pass it through to handleUserChanges so the write targets the pad that +// was authorized when the message arrived — NOT whatever pad the mutable +// sessioninfos[socket.id].padId happens to point at by apply time. A concurrent +// same-socket CLIENT_READY can swap that padId between enqueue and apply, which +// otherwise redirects the queued write onto a read-only / unauthorized pad +// (GHSA-6mcx-x5h6-rpw2). +const padChannels = new Channels((ch, {socket, message}) => handleUserChanges(socket, message, ch)); /** * This Method is called by server.ts to tell the message handler on which socket it should send @@ -505,6 +512,17 @@ exports.handleMessage = async (socket:any, message: ClientVarMessage) => { throw new Error(`pre-CLIENT_READY message from IP ${ip}: ${msg}`); } + // Pin the pad this message is authorized against — together with `auth` and + // the read-only flag — BEFORE any of the awaits below (checkAccess and the + // handleMessageSecurity/handleMessage hooks). A concurrent same-socket + // CLIENT_READY can mutate sessioninfos[socket.id] (padId/readonly/auth) IN + // PLACE during those awaits (the object identity check below does not catch an + // in-place mutation). Using these pinned values for the read-only gate, the + // queue key and the write keeps them all referring to the SAME pad, closing + // the cross-pad write TOCTOU (GHSA-6mcx-x5h6-rpw2). + const messagePadId = thisSession.padId; + const messageReadonly = thisSession.readonly; + const {session: {user} = {}} = socket.client.request as SocketClientRequest; const {accessStatus, authorID} = await securityManager.checkAccess(auth.padID, auth.sessionID, auth.token, user); @@ -526,14 +544,15 @@ exports.handleMessage = async (socket:any, message: ClientVarMessage) => { } thisSession.author = authorID; - // Allow plugins to bypass the readonly message blocker - let readOnly = thisSession.readonly; + // Allow plugins to bypass the readonly message blocker. Base the decision on + // the value pinned above so a concurrent CLIENT_READY can't flip it mid-message. + let readOnly = messageReadonly; const context = { message, sessionInfo: { authorId: thisSession.author, - padId: thisSession.padId, - readOnly: thisSession.readonly, + padId: messagePadId, + readOnly: messageReadonly, }, socket, get client() { @@ -580,7 +599,10 @@ exports.handleMessage = async (socket:any, message: ClientVarMessage) => { switch (type) { case 'USER_CHANGES': stats.counter('pendingEdits').inc(); - await padChannels.enqueue(thisSession.padId, {socket, message}); + // Queue key = the pinned pad, NOT the (possibly-swapped) live + // session padId. This value is forwarded to handleUserChanges as + // the write target (see the padChannels executor). + await padChannels.enqueue(messagePadId, {socket, message}); break; case 'PAD_DELETE': await handlePadDelete(socket, message.data as unknown as PadDeleteMessage); break; case 'USERINFO_UPDATE': await handleUserInfoUpdate(socket, message as unknown as UserNewInfoMessage); break; @@ -823,7 +845,7 @@ const handleUserInfoUpdate = async (socket:any, {data: {userInfo: {name, colorId */ const handleUserChanges = async (socket:any, message: { data: ClientUserChangesMessage -}) => { +}, authorizedPadId: string) => { // This one's no longer pending, as we're gonna process it now stats.counter('pendingEdits').dec(); @@ -850,7 +872,9 @@ const handleUserChanges = async (socket:any, message: { if (apool == null) throw new Error('missing apool'); if (changeset == null) throw new Error('missing changeset'); const wireApool = (new AttributePool()).fromJsonable(apool); - const pad = await padManager.getPad(thisSession.padId, null, thisSession.author); + // Use the pad id captured at enqueue time, not the (mutable) session padId, + // which a concurrent CLIENT_READY may have swapped (GHSA-6mcx-x5h6-rpw2). + const pad = await padManager.getPad(authorizedPadId, null, thisSession.author); // Verify that the changeset has valid syntax and is in canonical form checkRep(changeset); @@ -1004,7 +1028,7 @@ const handleUserChanges = async (socket:any, message: { socket.emit('message', {disconnect: 'badChangeset'}); stats.meter('failedChangesets').mark(); messageLogger.warn(`Failed to apply USER_CHANGES from author ${thisSession.author} ` + - `(socket ${socket.id}) on pad ${thisSession.padId}: ${err.stack || err}`); + `(socket ${socket.id}) on pad ${authorizedPadId}: ${err.stack || err}`); } finally { stopWatch.end(); } diff --git a/src/tests/backend/specs/crossPadWriteToctou.ts b/src/tests/backend/specs/crossPadWriteToctou.ts new file mode 100644 index 00000000000..ebac77feaac --- /dev/null +++ b/src/tests/backend/specs/crossPadWriteToctou.ts @@ -0,0 +1,92 @@ +'use strict'; + +/** + * Regression for GHSA-6mcx-x5h6-rpw2 (PadMessageHandler cross-pad write TOCTOU). + * + * A USER_CHANGES message is authorized (access + read-only gate) against the pad + * the session points at when the message arrives, but the write ran later off + * mutable session state. A concurrent same-socket CLIENT_READY could swap + * sessioninfos[socket.id].padId during the awaits in handleMessage(), redirecting + * the queued write onto a read-only / unauthorized pad. + * + * This test drives a real USER_CHANGES over a socket and, via a + * handleMessageSecurity hook (which runs during those awaits), swaps the + * session's padId to a victim pad — exactly the concurrent-CLIENT_READY race. + * The change must still land on the pad the message was authorized against, and + * never on the victim. + */ + +import {MapArrayType} from '../../../node/types/MapType'; + +const assert = require('assert').strict; +const common = require('../common'); +const padManager = require('../../../node/db/PadManager'); +const plugins = require('../../../static/js/pluginfw/plugin_defs'); +const padMessageHandler = require('../../../node/handler/PadMessageHandler'); + +describe(__filename, function () { + this.timeout(30000); + let agent: any; + const backups: MapArrayType = {}; + + before(async function () { agent = await common.init(); }); + + beforeEach(function () { + backups.hooks = {handleMessageSecurity: plugins.hooks.handleMessageSecurity}; + plugins.hooks.handleMessageSecurity = []; + }); + afterEach(function () { + Object.assign(plugins.hooks, backups.hooks); + }); + + it('a mid-message pad swap cannot redirect the write onto another pad', async function () { + // Authorized (writable) pad the client is editing. + const authorizedId = `toctou_ok_${common.randomString()}`; + const authorizedPad = await padManager.getPad(authorizedId, 'seed\n'); + await authorizedPad.setText('seed\n'); + // Victim pad with identical text, so the changeset is valid against either + // pad — the test distinguishes purely by which pad receives the write. + const victimId = `toctou_victim_${common.randomString()}`; + const victimPad = await padManager.getPad(victimId, 'seed\n'); + await victimPad.setText('seed\n'); + + const res = await agent.get(`/p/${authorizedId}`).expect(200); + const socket = await common.connect(res); + try { + const {data: clientVars} = await common.handshake(socket, authorizedId); + const rev = clientVars.collab_client_vars.rev; + const authorId = clientVars.userId; + + // Simulate the concurrent same-socket CLIENT_READY: while THIS USER_CHANGES + // is mid-flight (handleMessageSecurity runs during handleMessage's awaits), + // swap the session's padId to the victim pad in place. + plugins.hooks.handleMessageSecurity = [{ + hook_fn: (hookName: string, ctx: any) => { + if (ctx.message?.data?.type === 'USER_CHANGES') { + padMessageHandler.sessioninfos[ctx.socket.id].padId = victimId; + } + }, + hook_fn_name: 'toctou/handleMessageSecurity', + hook_name: 'handleMessageSecurity', + part: {plugin: 'toctou'}, + }]; + + const accept = common.waitForSocketEvent(socket, 'message'); + await common.sendUserChanges(socket, { + baseRev: rev, + changeset: 'Z:5>2*0+2$ZZ', + apool: {numToAttrib: {0: ['author', authorId]}, nextNum: 1}, + }); + await accept; // wait for the server to finish applying + + assert.equal(victimPad.text(), 'seed\n', + 'the victim pad (swapped session padId) must NOT be modified'); + assert.equal(authorizedPad.text(), 'ZZseed\n', + 'the change must land on the pad the message was authorized against'); + } finally { + socket.close(); + await authorizedPad.remove(); + await victimPad.remove(); + } + }); +});