From c59cbb537abc0bc8b5cee47c964f3f0d5d3b630d Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Wed, 23 Feb 2022 16:10:16 -0500 Subject: [PATCH 1/4] Bump version --- CHANGELOG.md | 4 ++++ src/package-lock.json | 2 +- src/package.json | 2 +- 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 450f8e616..fd9618e28 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,7 @@ +# 1.8.17 + +(not yet released) + # 1.8.16 ### Security fixes diff --git a/src/package-lock.json b/src/package-lock.json index 79539bf9c..dab5b4946 100644 --- a/src/package-lock.json +++ b/src/package-lock.json @@ -1,6 +1,6 @@ { "name": "ep_etherpad-lite", - "version": "1.8.16", + "version": "1.8.17", "lockfileVersion": 1, "requires": true, "dependencies": { diff --git a/src/package.json b/src/package.json index 349810915..a509c6f1e 100644 --- a/src/package.json +++ b/src/package.json @@ -246,6 +246,6 @@ "test": "mocha --timeout 120000 --recursive tests/backend/specs ../node_modules/ep_*/static/tests/backend/specs", "test-container": "mocha --timeout 5000 tests/container/specs/api" }, - "version": "1.8.16", + "version": "1.8.17", "license": "Apache-2.0" } From bdbde88fed0e54acdeba665e51f2fd03a7452130 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Wed, 23 Feb 2022 01:36:48 -0500 Subject: [PATCH 2/4] PadMessageHandler: Fix `USER_CHANGES` queue identifier `message.padId` is normally undefined for `USER_CHANGES` messages. --- CHANGELOG.md | 7 +++++++ src/node/handler/PadMessageHandler.js | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fd9618e28..547b57bc3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,13 @@ (not yet released) +### 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 0735ce97f..542a63711 100644 --- a/src/node/handler/PadMessageHandler.js +++ b/src/node/handler/PadMessageHandler.js @@ -296,7 +296,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') { From ba370b0e05ec4184e5dcf9acd23ca8875a1647ec Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Wed, 23 Feb 2022 01:03:46 -0500 Subject: [PATCH 3/4] PadMessageHandler: Don't trust user-provided `padId` --- CHANGELOG.md | 5 ++ src/node/handler/PadMessageHandler.js | 19 ++++---- src/tests/backend/common.js | 31 ++++++++++++ src/tests/backend/specs/messages.js | 68 +++++++++++++++++++++++++++ 4 files changed, 115 insertions(+), 8 deletions(-) create mode 100644 src/tests/backend/specs/messages.js diff --git a/CHANGELOG.md b/CHANGELOG.md index 547b57bc3..eed89c0cc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,11 @@ (not yet released) +### 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 diff --git a/src/node/handler/PadMessageHandler.js b/src/node/handler/PadMessageHandler.js index 542a63711..c048c2fd2 100644 --- a/src/node/handler/PadMessageHandler.js +++ b/src/node/handler/PadMessageHandler.js @@ -235,6 +235,14 @@ exports.handleMessage = async (socket, message) => { token: message.token, }; } + // 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) { @@ -1115,11 +1123,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; @@ -1145,16 +1148,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/common.js b/src/tests/backend/common.js index 793828ac7..ec5df1404 100644 --- a/src/tests/backend/common.js +++ b/src/tests/backend/common.js @@ -184,3 +184,34 @@ exports.handshake = async (socket, padId) => { logger.debug('received CLIENT_VARS message'); return msg; }; + +/** + * Convenience wrapper around `socket.send()` that waits for acknowledgement. + */ +exports.sendMessage = async (socket, message) => await new Promise((resolve, reject) => { + socket.send(message, (errInfo) => { + if (errInfo != null) { + const {name, message} = errInfo; + const err = new Error(message); + err.name = name; + reject(err); + return; + } + resolve(); + }); +}); + +const alphabet = 'abcdefghijklmnopqrstuvwxyz'; + +/** + * Generates a random string. + * + * @param {number} [len] - The desired length of the generated string. + * @param {string} [charset] - Characters to pick from. + * @returns {string} + */ +exports.randomString = (len = 10, charset = `${alphabet}${alphabet.toUpperCase()}0123456789`) => { + let ret = ''; + while (ret.length < len) ret += charset[Math.floor(Math.random() * charset.length)]; + return ret; +}; diff --git a/src/tests/backend/specs/messages.js b/src/tests/backend/specs/messages.js new file mode 100644 index 000000000..b3da0e96d --- /dev/null +++ b/src/tests/backend/specs/messages.js @@ -0,0 +1,68 @@ +'use strict'; + +const assert = require('assert').strict; +const common = require('../common'); +const padManager = require('../../../node/db/PadManager'); +const plugins = require('../../../static/js/pluginfw/plugin_defs'); +const readOnlyManager = require('../../../node/db/ReadOnlyManager'); + +describe(__filename, function () { + let agent; + let pad; + let padId; + let roPadId; + let roSocket; + + before(async function () { + agent = await common.init(); + }); + + beforeEach(async function () { + padId = common.randomString(); + assert(!await padManager.doesPadExist(padId)); + pad = await padManager.getPad(padId, 'dummy text\n'); + await pad.setText('\n'); // Make sure the pad is created. + assert.equal(pad.text(), '\n'); + + roPadId = await readOnlyManager.getReadOnlyId(padId); + const res = await agent.get(`/p/${roPadId}`).expect(200); + roSocket = await common.connect(res); + await common.handshake(roSocket, roPadId, `t.${common.randomString(8)}`); + }); + + afterEach(async function () { + if (roSocket != null) roSocket.close(); + roSocket = null; + if (pad != null) await pad.remove(); + 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(); + } + }); + }); +}); From d97537d18bab1f5c0c0bba972ec387a8dca7730f Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Wed, 23 Feb 2022 16:12:16 -0500 Subject: [PATCH 4/4] Release v1.8.17 --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index eed89c0cc..c96c4265f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,6 @@ # 1.8.17 -(not yet released) +Released: 2022-02-23 ### Security fixes