From ba370b0e05ec4184e5dcf9acd23ca8875a1647ec Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Wed, 23 Feb 2022 01:03:46 -0500 Subject: [PATCH] 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(); + } + }); + }); +});