From e60a51a18b008604995f57e9f64c8e2959b38f86 Mon Sep 17 00:00:00 2001 From: John McLear Date: Sat, 25 Jul 2026 18:52:51 +0100 Subject: [PATCH 1/2] security(collab): apply queued USER_CHANGES to the enqueue-time pad (cross-pad write TOCTOU) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GHSA-6mcx-x5h6-rpw2. handleUserChanges re-read the mutable sessioninfos[socket.id].padId at apply time, while the authorization / read-only gate and the channel key were taken at enqueue time. Because per-socket messages are processed concurrently and a thrown handler does not disconnect the socket, a same-socket CLIENT_READY could swap the session's padId between enqueue and apply — redirecting a queued write onto a read-only or otherwise unauthorized pad (runtime-proven cross-pad write; a read-only share holder could overwrite the pad). Thread the enqueue-time pad id (already the padChannels channel key) into handleUserChanges as `authorizedPadId` and use it for the write instead of re-reading the session. The gate and the write now refer to the same pad, closing the TOCTOU window. Normal (non-racing) flows are unaffected because the session padId equals the enqueue-time padId. Adds a deterministic regression test that invokes handleUserChanges with the session padId already swapped to a victim pad and asserts the change lands on the authorized (argument) pad, never the victim. Verified it fails when the vulnerable read is reintroduced. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/node/handler/PadMessageHandler.ts | 22 ++++- .../backend/specs/crossPadWriteToctou.ts | 86 +++++++++++++++++++ 2 files changed, 104 insertions(+), 4 deletions(-) create mode 100644 src/tests/backend/specs/crossPadWriteToctou.ts diff --git a/src/node/handler/PadMessageHandler.ts b/src/node/handler/PadMessageHandler.ts index 7ede7d391cd..5d82e2816da 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 @@ -823,7 +830,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 +857,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,12 +1013,17 @@ 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(); } }; +// Exported for the cross-pad TOCTOU regression test (GHSA-6mcx-x5h6-rpw2), which +// verifies the write targets the enqueue-time `authorizedPadId` argument rather +// than the mutable sessioninfos[socket.id].padId. +exports.handleUserChanges = handleUserChanges; + exports.updatePadClients = async (pad: PadType) => { // skip this if no-one is on this pad const roomSockets = _getRoomSockets(pad.id); diff --git a/src/tests/backend/specs/crossPadWriteToctou.ts b/src/tests/backend/specs/crossPadWriteToctou.ts new file mode 100644 index 00000000000..e05e87c6427 --- /dev/null +++ b/src/tests/backend/specs/crossPadWriteToctou.ts @@ -0,0 +1,86 @@ +'use strict'; + +/** + * Regression for GHSA-6mcx-x5h6-rpw2 (PadMessageHandler TOCTOU cross-pad write). + * + * A USER_CHANGES message is authorized and enqueued against the pad the session + * points at when the message ARRIVES, but the actual write ran later and re-read + * the mutable sessioninfos[socket.id].padId. A concurrent same-socket + * CLIENT_READY could swap that padId in between, redirecting the queued write + * onto a read-only / unauthorized pad. + * + * The fix threads the enqueue-time pad id (the channel key) into + * handleUserChanges as `authorizedPadId`, which is used for the write. This test + * drives that seam directly: it invokes handleUserChanges with the session's + * padId ALREADY swapped to a victim pad, and asserts the change lands on the + * authorized (argument) pad and never on the victim. + */ + +const assert = require('assert').strict; +const common = require('../common'); +const padManager = require('../../../node/db/PadManager'); +const authorManager = require('../../../node/db/AuthorManager'); +const padMessageHandler = require('../../../node/handler/PadMessageHandler'); + +describe(__filename, function () { + this.timeout(30000); + + before(async function () { await common.init(); }); + + it('applies a queued change to the enqueue-time pad, not the swapped session pad', + async function () { + const authorizedId = `toctou_authorized_${common.randomString()}`; + const victimId = `toctou_victim_${common.randomString()}`; + // Identical seed text so the changeset is valid against either pad — the + // point is *which* pad receives it, not changeset validity. + const authorizedPad = await padManager.getPad(authorizedId, 'seed\n'); + await authorizedPad.setText('seed\n'); + const victimPad = await padManager.getPad(victimId, 'seed\n'); + await victimPad.setText('seed\n'); + assert.equal(authorizedPad.text(), 'seed\n'); + const baseRev = authorizedPad.getHeadRevisionNumber(); + + const {authorID} = await authorManager.createAuthor('toctou'); + + // Fake socket that captures server emits instead of using the wire. + const emitted: any[] = []; + const socket = {id: `toctou-socket-${common.randomString()}`, emit: (_e: string, m: any) => emitted.push(m)}; + + // Simulate the attack window: the session padId has ALREADY been swapped + // to the victim by a concurrent CLIENT_READY. The author is kept (the + // TOCTOU keeps the attacker's author, so the author check passes). + const {sessioninfos} = padMessageHandler; + sessioninfos[socket.id] = {padId: victimId, author: authorID, rev: baseRev, readonly: false}; + + // A valid insert of "ZZ" at the start, attributed to `authorID`. + const message = { + data: { + baseRev, + apool: {numToAttrib: {0: ['author', authorID]}, nextNum: 1}, + changeset: 'Z:5>2*0+2$ZZ', + }, + }; + + const realUpdatePadClients = padMessageHandler.updatePadClients; + padMessageHandler.updatePadClients = async () => {}; // neutralise fan-out + try { + // authorizedPadId = the pad authorized at enqueue time (NOT the swapped + // session padId). + await padMessageHandler.handleUserChanges(socket, message, authorizedId); + } finally { + padMessageHandler.updatePadClients = realUpdatePadClients; + delete sessioninfos[socket.id]; + } + + const freshAuthorized = await padManager.getPad(authorizedId); + const freshVictim = await padManager.getPad(victimId); + + assert.equal(freshVictim.text(), 'seed\n', + 'the victim pad (swapped session padId) must NOT be modified'); + assert.equal(freshAuthorized.text(), 'ZZseed\n', + 'the change must land on the enqueue-time authorized pad'); + + await authorizedPad.remove(); + await victimPad.remove(); + }); +}); From 947a6d1a4eb3c8ad365ecd83172d6dde31a74f68 Mon Sep 17 00:00:00 2001 From: John McLear Date: Sun, 26 Jul 2026 08:21:58 +0100 Subject: [PATCH 2/2] security(collab): pin pad snapshot before awaits to fully close cross-pad write TOCTOU MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses Qodo review on #8075: the first cut used the padChannels channel key as the write target, but that key was still read from thisSession.padId at enqueue time — AFTER the awaits in handleMessage() (checkAccess and the handleMessageSecurity/handleMessage hooks). A concurrent same-socket CLIENT_READY can mutate sessioninfos[socket.id] in place during those awaits (the object-identity disconnect check does not catch an in-place mutation), so the queue key — and thus the write — could still be redirected onto a read-only / unauthorized pad after the read-only gate passed. Pin the pad id and read-only flag into a consistent snapshot (messagePadId / messageReadonly) together with `auth`, BEFORE any of those awaits, and use the snapshot for the read-only gate, the hook context, the queue key and the write. The gate, the queue key and the write now all refer to the same pad regardless of concurrent CLIENT_READY swaps. handleUserChanges keeps using its authorizedPadId argument (defence for the enqueue->apply window). The test now drives a real USER_CHANGES over a socket and swaps the session padId mid-message via a handleMessageSecurity hook (the concurrent-CLIENT_READY race), asserting the write lands on the authorized pad and never the victim — verified RED when the pinned key is reverted to thisSession.padId. This also drops the handleUserChanges export and the direct-call test (Qodo maintainability / pendingEdits / cleanup findings). Co-Authored-By: Claude Opus 4.8 (1M context) --- src/node/handler/PadMessageHandler.ts | 30 ++-- .../backend/specs/crossPadWriteToctou.ts | 134 +++++++++--------- 2 files changed, 90 insertions(+), 74 deletions(-) diff --git a/src/node/handler/PadMessageHandler.ts b/src/node/handler/PadMessageHandler.ts index 5d82e2816da..764ec2e6e68 100644 --- a/src/node/handler/PadMessageHandler.ts +++ b/src/node/handler/PadMessageHandler.ts @@ -512,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); @@ -533,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() { @@ -587,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; @@ -1019,11 +1034,6 @@ const handleUserChanges = async (socket:any, message: { } }; -// Exported for the cross-pad TOCTOU regression test (GHSA-6mcx-x5h6-rpw2), which -// verifies the write targets the enqueue-time `authorizedPadId` argument rather -// than the mutable sessioninfos[socket.id].padId. -exports.handleUserChanges = handleUserChanges; - exports.updatePadClients = async (pad: PadType) => { // skip this if no-one is on this pad const roomSockets = _getRoomSockets(pad.id); diff --git a/src/tests/backend/specs/crossPadWriteToctou.ts b/src/tests/backend/specs/crossPadWriteToctou.ts index e05e87c6427..ebac77feaac 100644 --- a/src/tests/backend/specs/crossPadWriteToctou.ts +++ b/src/tests/backend/specs/crossPadWriteToctou.ts @@ -1,86 +1,92 @@ 'use strict'; /** - * Regression for GHSA-6mcx-x5h6-rpw2 (PadMessageHandler TOCTOU cross-pad write). + * Regression for GHSA-6mcx-x5h6-rpw2 (PadMessageHandler cross-pad write TOCTOU). * - * A USER_CHANGES message is authorized and enqueued against the pad the session - * points at when the message ARRIVES, but the actual write ran later and re-read - * the mutable sessioninfos[socket.id].padId. A concurrent same-socket - * CLIENT_READY could swap that padId in between, redirecting the queued write - * onto a read-only / unauthorized pad. + * 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. * - * The fix threads the enqueue-time pad id (the channel key) into - * handleUserChanges as `authorizedPadId`, which is used for the write. This test - * drives that seam directly: it invokes handleUserChanges with the session's - * padId ALREADY swapped to a victim pad, and asserts the change lands on the - * authorized (argument) pad and never on the victim. + * 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 authorManager = require('../../../node/db/AuthorManager'); +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 () { await common.init(); }); - - it('applies a queued change to the enqueue-time pad, not the swapped session pad', - async function () { - const authorizedId = `toctou_authorized_${common.randomString()}`; - const victimId = `toctou_victim_${common.randomString()}`; - // Identical seed text so the changeset is valid against either pad — the - // point is *which* pad receives it, not changeset validity. - const authorizedPad = await padManager.getPad(authorizedId, 'seed\n'); - await authorizedPad.setText('seed\n'); - const victimPad = await padManager.getPad(victimId, 'seed\n'); - await victimPad.setText('seed\n'); - assert.equal(authorizedPad.text(), 'seed\n'); - const baseRev = authorizedPad.getHeadRevisionNumber(); - - const {authorID} = await authorManager.createAuthor('toctou'); + before(async function () { agent = await common.init(); }); - // Fake socket that captures server emits instead of using the wire. - const emitted: any[] = []; - const socket = {id: `toctou-socket-${common.randomString()}`, emit: (_e: string, m: any) => emitted.push(m)}; + beforeEach(function () { + backups.hooks = {handleMessageSecurity: plugins.hooks.handleMessageSecurity}; + plugins.hooks.handleMessageSecurity = []; + }); + afterEach(function () { + Object.assign(plugins.hooks, backups.hooks); + }); - // Simulate the attack window: the session padId has ALREADY been swapped - // to the victim by a concurrent CLIENT_READY. The author is kept (the - // TOCTOU keeps the attacker's author, so the author check passes). - const {sessioninfos} = padMessageHandler; - sessioninfos[socket.id] = {padId: victimId, author: authorID, rev: baseRev, readonly: false}; + 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'); - // A valid insert of "ZZ" at the start, attributed to `authorID`. - const message = { - data: { - baseRev, - apool: {numToAttrib: {0: ['author', authorID]}, nextNum: 1}, - changeset: 'Z:5>2*0+2$ZZ', - }, - }; + 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; - const realUpdatePadClients = padMessageHandler.updatePadClients; - padMessageHandler.updatePadClients = async () => {}; // neutralise fan-out - try { - // authorizedPadId = the pad authorized at enqueue time (NOT the swapped - // session padId). - await padMessageHandler.handleUserChanges(socket, message, authorizedId); - } finally { - padMessageHandler.updatePadClients = realUpdatePadClients; - delete sessioninfos[socket.id]; - } + // 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 freshAuthorized = await padManager.getPad(authorizedId); - const freshVictim = await padManager.getPad(victimId); - - assert.equal(freshVictim.text(), 'seed\n', - 'the victim pad (swapped session padId) must NOT be modified'); - assert.equal(freshAuthorized.text(), 'ZZseed\n', - 'the change must land on the enqueue-time authorized pad'); - - await authorizedPad.remove(); - await victimPad.remove(); + 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(); + } + }); });