diff --git a/CHANGELOG.md b/CHANGELOG.md index 8715d5bc8..97882a0e6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -80,6 +80,22 @@ generator function. * `newOp()`: Deprecated in favor of the new `Op` class. +# 1.8.17 + +Released: 2022-02-23 + +### Security fixes + +* Fixed a vunlerability in the `CHANGESET_REQ` message handler that allowed a + user with any access to read any pad if the pad ID is known. + +### Notable enhancements and fixes + +* Fixed a bug that caused all pad edit messages received at the server to go + through a single queue. Now there is a separate queue per pad as intended, + which should reduce message processing latency when many pads are active at + the same time. + # 1.8.16 ### Security fixes diff --git a/src/node/handler/PadMessageHandler.js b/src/node/handler/PadMessageHandler.js index c4d141293..b0491e6b7 100644 --- a/src/node/handler/PadMessageHandler.js +++ b/src/node/handler/PadMessageHandler.js @@ -241,6 +241,14 @@ exports.handleMessage = async (socket, message) => { thisSession.readonly = padIds.readonly || !webaccess.userCanModify(thisSession.auth.padID, socket.client.request); } + // Outside of the checks done by this function, message.padId must not be accessed because it is + // too easy to introduce a security vulnerability that allows malicious users to read or modify + // pads that they should not be able to access. Code should instead use + // sessioninfos[socket.id].padId if the real pad ID is needed or + // sessioninfos[socket.id].auth.padID if the original user-supplied pad ID is needed. + Object.defineProperty(message, 'padId', {get: () => { + throw new Error('message.padId must not be accessed (for security reasons)'); + }}); const auth = thisSession.auth; if (!auth) { @@ -330,7 +338,7 @@ exports.handleMessage = async (socket, message) => { messageLogger.warn('Dropped message, COLLABROOM for readonly pad'); } else if (message.data.type === 'USER_CHANGES') { stats.counter('pendingEdits').inc(); - await padChannels.enqueue(message.padId, {socket, message}); + await padChannels.enqueue(thisSession.padId, {socket, message}); } else if (message.data.type === 'USERINFO_UPDATE') { await handleUserInfoUpdate(socket, message); } else if (message.data.type === 'CHAT_MESSAGE') { @@ -1121,11 +1129,6 @@ const handleChangesetRequest = async (socket, message) => { return; } - if (message.padId == null) { - messageLogger.warn('Dropped message, changeset request has no padId!'); - return; - } - if (message.data.granularity == null) { messageLogger.warn('Dropped message, changeset request has no granularity!'); return; @@ -1151,16 +1154,16 @@ const handleChangesetRequest = async (socket, message) => { const start = message.data.start; const end = start + (100 * granularity); - const padIds = await readOnlyManager.getIds(message.padId); + const {padId} = sessioninfos[socket.id]; // build the requested rough changesets and send them back try { - const data = await getChangesetInfo(padIds.padId, start, end, granularity); + const data = await getChangesetInfo(padId, start, end, granularity); data.requestID = message.data.requestID; socket.json.send({type: 'CHANGESET_REQ', data}); } catch (err) { messageLogger.error(`Error while handling a changeset request ${message.data} ` + - `for ${padIds.padId}: ${err.stack || err}`); + `for ${padId}: ${err.stack || err}`); } }; diff --git a/src/tests/backend/specs/messages.js b/src/tests/backend/specs/messages.js index e36e8d98b..c436bf601 100644 --- a/src/tests/backend/specs/messages.js +++ b/src/tests/backend/specs/messages.js @@ -25,7 +25,7 @@ describe(__filename, function () { plugins.hooks.handleMessageSecurity = []; padId = common.randomString(); assert(!await padManager.doesPadExist(padId)); - pad = await padManager.getPad(padId, 'dummy text'); + pad = await padManager.getPad(padId, 'dummy text\n'); await pad.setText('\n'); // Make sure the pad is created. assert.equal(pad.text(), '\n'); let res = await agent.get(`/p/${padId}`).expect(200); @@ -50,6 +50,35 @@ describe(__filename, function () { pad = null; }); + describe('CHANGESET_REQ', function () { + it('users are unable to read changesets from other pads', async function () { + const otherPadId = `${padId}other`; + assert(!await padManager.doesPadExist(otherPadId)); + const otherPad = await padManager.getPad(otherPadId, 'other text\n'); + try { + await otherPad.setText('other text\n'); + const resP = common.waitForSocketEvent(roSocket, 'message'); + await common.sendMessage(roSocket, { + component: 'pad', + padId: otherPadId, // The server should ignore this. + type: 'CHANGESET_REQ', + data: { + granularity: 1, + start: 0, + requestID: 'requestId', + }, + }); + const res = await resP; + assert.equal(res.type, 'CHANGESET_REQ'); + assert.equal(res.data.requestID, 'requestId'); + // Should match padId's text, not otherPadId's text. + assert.match(res.data.forwardsChangesets[0], /^[^$]*\$dummy text\n/); + } finally { + await otherPad.remove(); + } + }); + }); + describe('USER_CHANGES', function () { const sendUserChanges = async (socket, cs) => await common.sendUserChanges(socket, {baseRev: rev, changeset: cs});