From 1e9803363232bb3da1647e00b18a3363170e8b4c Mon Sep 17 00:00:00 2001 From: John McLear Date: Mon, 26 Jun 2023 18:17:06 +0100 Subject: [PATCH] Security: Fix revision parsing (#5772) A carefully crated URL can cause Etherpad to hang. --- CHANGELOG.md | 11 ++ src/node/db/API.js | 36 +--- src/node/db/Pad.js | 3 + src/node/handler/ExportHandler.js | 7 + src/node/handler/PadMessageHandler.js | 5 + src/node/utils/ExportHelper.js | 6 +- src/node/utils/checkValidRev.js | 34 ++++ .../backend/specs/api/importexportGetPost.js | 169 ++++++++++++++++++ src/tests/backend/specs/messages.js | 83 +++++++++ 9 files changed, 325 insertions(+), 29 deletions(-) create mode 100644 src/node/utils/checkValidRev.js diff --git a/CHANGELOG.md b/CHANGELOG.md index 524899dc0..e82d99ed8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,14 @@ +# Next release + +### Notable enhancements and fixes + +* Security + * Limit requested revisions in timeslider and export to head revision. (affects v1.9.0) + +* Bugfixes + * revisions in `CHANGESET_REQ` (timeslider) and export (txt, html, custom) + are now checked to be numbers. + # 1.9.0 ### Notable enhancements and fixes diff --git a/src/node/db/API.js b/src/node/db/API.js index 9b2ecadc7..3f92c47dd 100644 --- a/src/node/db/API.js +++ b/src/node/db/API.js @@ -33,6 +33,7 @@ const exportTxt = require('../utils/ExportTxt'); const importHtml = require('../utils/ImportHtml'); const cleanText = require('./Pad').cleanText; const PadDiff = require('../utils/padDiff'); +const { checkValidRev, isInt } = require('../utils/checkValidRev'); /* ******************** * GROUP FUNCTIONS **** @@ -777,6 +778,13 @@ exports.createDiffHTML = async (padID, startRev, endRev) => { // get the pad const pad = await getPadSafe(padID, true); + const headRev = pad.getHeadRevisionNumber(); + if (startRev > headRev) + startRev = headRev; + + if (endRev > headRev) + endRev = headRev; + let padDiff; try { padDiff = new PadDiff(pad, startRev, endRev); @@ -822,9 +830,6 @@ exports.getStats = async () => { ** INTERNAL HELPER FUNCTIONS * **************************** */ -// checks if a number is an int -const isInt = (value) => (parseFloat(value) === parseInt(value, 10)) && !isNaN(value); - // gets a pad safe const getPadSafe = async (padID, shouldExist, text, authorId = '') => { // check if padID is a string @@ -854,31 +859,6 @@ const getPadSafe = async (padID, shouldExist, text, authorId = '') => { return padManager.getPad(padID, text, authorId); }; -// checks if a rev is a legal number -// pre-condition is that `rev` is not undefined -const checkValidRev = (rev) => { - if (typeof rev !== 'number') { - rev = parseInt(rev, 10); - } - - // check if rev is a number - if (isNaN(rev)) { - throw new CustomError('rev is not a number', 'apierror'); - } - - // ensure this is not a negative number - if (rev < 0) { - throw new CustomError('rev is not a negative number', 'apierror'); - } - - // ensure this is not a float value - if (!isInt(rev)) { - throw new CustomError('rev is a float value', 'apierror'); - } - - return rev; -}; - // checks if a padID is part of a group const checkGroupPad = (padID, field) => { // ensure this is a group pad diff --git a/src/node/db/Pad.js b/src/node/db/Pad.js index b692962f1..a9c87541f 100644 --- a/src/node/db/Pad.js +++ b/src/node/db/Pad.js @@ -172,6 +172,9 @@ class Pad { async getInternalRevisionAText(targetRev) { const keyRev = this.getKeyRevisionNumber(targetRev); + const headRev = this.getHeadRevisionNumber(); + if (targetRev > headRev) + targetRev = headRev; const [keyAText, changesets] = await Promise.all([ this._getKeyRevisionAText(keyRev), Promise.all( diff --git a/src/node/handler/ExportHandler.js b/src/node/handler/ExportHandler.js index f3fde047c..417380866 100644 --- a/src/node/handler/ExportHandler.js +++ b/src/node/handler/ExportHandler.js @@ -29,6 +29,7 @@ const os = require('os'); const hooks = require('../../static/js/pluginfw/hooks'); const TidyHtml = require('../utils/TidyHtml'); const util = require('util'); +const { checkValidRev } = require('../utils/checkValidRev'); const fsp_writeFile = util.promisify(fs.writeFile); const fsp_unlink = util.promisify(fs.unlink); @@ -53,6 +54,12 @@ exports.doExport = async (req, res, padId, readOnlyId, type) => { // tell the browser that this is a downloadable file res.attachment(`${fileName}.${type}`); + if (req.params.rev !== undefined) { + // ensure revision is a number + // modify req, as we use it in a later call to exportConvert + req.params.rev = checkValidRev(req.params.rev); + } + // if this is a plain text export, we can do this directly // We have to over engineer this because tabs are stored as attributes and not plain text if (type === 'etherpad') { diff --git a/src/node/handler/PadMessageHandler.js b/src/node/handler/PadMessageHandler.js index 9a1885b73..35c76b5d9 100644 --- a/src/node/handler/PadMessageHandler.js +++ b/src/node/handler/PadMessageHandler.js @@ -39,6 +39,7 @@ const stats = require('../stats'); const assert = require('assert').strict; const {RateLimiterMemory} = require('rate-limiter-flexible'); const webaccess = require('../hooks/express/webaccess'); +const { checkValidRev } = require('../utils/checkValidRev'); let rateLimiter; let socketio = null; @@ -1076,10 +1077,14 @@ const handleChangesetRequest = async (socket, {data: {granularity, start, reques if (granularity == null) throw new Error('missing granularity'); if (!Number.isInteger(granularity)) throw new Error('granularity is not an integer'); if (start == null) throw new Error('missing start'); + start = checkValidRev(start); if (requestID == null) throw new Error('mising requestID'); const end = start + (100 * granularity); const {padId, author: authorId} = sessioninfos[socket.id]; const pad = await padManager.getPad(padId, null, authorId); + const headRev = pad.getHeadRevisionNumber(); + if (start > headRev) + start = headRev; const data = await getChangesetInfo(pad, start, end, granularity); data.requestID = requestID; socket.json.send({type: 'CHANGESET_REQ', data}); diff --git a/src/node/utils/ExportHelper.js b/src/node/utils/ExportHelper.js index 7962476e8..48054e7f4 100644 --- a/src/node/utils/ExportHelper.js +++ b/src/node/utils/ExportHelper.js @@ -21,10 +21,14 @@ const AttributeMap = require('../../static/js/AttributeMap'); const Changeset = require('../../static/js/Changeset'); +const { checkValidRev } = require('./checkValidRev'); +/* + * This method seems unused in core and no plugins depend on it + */ exports.getPadPlainText = (pad, revNum) => { const _analyzeLine = exports._analyzeLine; - const atext = ((revNum !== undefined) ? pad.getInternalRevisionAText(revNum) : pad.atext); + const atext = ((revNum !== undefined) ? pad.getInternalRevisionAText(checkValidRev(revNum)) : pad.atext); const textLines = atext.text.slice(0, -1).split('\n'); const attribLines = Changeset.splitAttributionLines(atext.attribs, atext.text); const apool = pad.pool; diff --git a/src/node/utils/checkValidRev.js b/src/node/utils/checkValidRev.js new file mode 100644 index 000000000..862c6a2bd --- /dev/null +++ b/src/node/utils/checkValidRev.js @@ -0,0 +1,34 @@ +'use strict'; + +const CustomError = require('../utils/customError'); + +// checks if a rev is a legal number +// pre-condition is that `rev` is not undefined +const checkValidRev = (rev) => { + if (typeof rev !== 'number') { + rev = parseInt(rev, 10); + } + + // check if rev is a number + if (isNaN(rev)) { + throw new CustomError('rev is not a number', 'apierror'); + } + + // ensure this is not a negative number + if (rev < 0) { + throw new CustomError('rev is not a negative number', 'apierror'); + } + + // ensure this is not a float value + if (!isInt(rev)) { + throw new CustomError('rev is a float value', 'apierror'); + } + + return rev; +}; + +// checks if a number is an int +const isInt = (value) => (parseFloat(value) === parseInt(value, 10)) && !isNaN(value); + +exports.isInt = isInt; +exports.checkValidRev = checkValidRev; diff --git a/src/tests/backend/specs/api/importexportGetPost.js b/src/tests/backend/specs/api/importexportGetPost.js index e90f7c71e..e69f0d120 100644 --- a/src/tests/backend/specs/api/importexportGetPost.js +++ b/src/tests/backend/specs/api/importexportGetPost.js @@ -447,6 +447,175 @@ describe(__filename, function () { }); }); + describe('revisions are supported in txt and html export', function () { + const makeGoodExport = () => ({ + 'pad:testing': { + atext: { + text: 'oofoo\n', + attribs: '|1+6', + }, + pool: { + numToAttrib: { + 0: ['author', 'a.foo'], + }, + nextNum: 1, + }, + head: 2, + savedRevisions: [], + }, + 'globalAuthor:a.foo': { + colorId: '#000000', + name: 'author foo', + timestamp: 1598747784631, + padIDs: 'testing', + }, + 'pad:testing:revs:0': { + changeset: 'Z:1>3+3$foo', + meta: { + author: 'a.foo', + timestamp: 1597632398288, + pool: { + nextNum: 1, + numToAttrib: { + 0: ['author', 'a.foo'], + }, + }, + atext: { + text: 'foo\n', + attribs: '|1+4', + }, + }, + }, + 'pad:testing:revs:1': { + changeset: 'Z:4>1+1$o', + meta: { + author: 'a.foo', + timestamp: 1597632398288, + pool: { + nextNum: 1, + numToAttrib: { + 0: ['author', 'a.foo'], + }, + }, + atext: { + text: 'fooo\n', + attribs: '*0|1+5', + }, + }, + }, + 'pad:testing:revs:2': { + changeset: 'Z:5>1+1$o', + meta: { + author: 'a.foo', + timestamp: 1597632398288, + pool: { + numToAttrib: {}, + nextNum: 0, + }, + atext: { + text: 'foooo\n', + attribs: '*0|1+6', + }, + }, + }, + }); + + const importEtherpad = (records) => agent.post(`/p/${testPadId}/import`) + .attach('file', Buffer.from(JSON.stringify(records), 'utf8'), { + filename: '/test.etherpad', + contentType: 'application/etherpad', + }); + + before(async function () { + // makeGoodExport() is assumed to produce good .etherpad records. Verify that assumption so + // that a buggy makeGoodExport() doesn't cause checks to accidentally pass. + const records = makeGoodExport(); + await deleteTestPad(); + await importEtherpad(records) + .expect(200) + .expect('Content-Type', /json/) + .expect((res) => assert.deepEqual(res.body, { + code: 0, + message: 'ok', + data: {directDatabaseAccess: true}, + })); + await agent.get(`/p/${testPadId}/export/txt`) + .expect(200) + .buffer(true).parse(superagent.parse.text) + .expect((res) => assert.equal(res.text, 'oofoo\n')); + }); + + it('txt request rev 1', async function () { + await agent.get(`/p/${testPadId}/1/export/txt`) + .expect(200) + .buffer(true).parse(superagent.parse.text) + .expect((res) => assert.equal(res.text, 'ofoo\n')); + }); + + it('txt request rev 2', async function () { + await agent.get(`/p/${testPadId}/2/export/txt`) + .expect(200) + .buffer(true).parse(superagent.parse.text) + .expect((res) => assert.equal(res.text, 'oofoo\n')); + }); + + it('txt request rev 1test returns rev 1', async function () { + await agent.get(`/p/${testPadId}/1test/export/txt`) + .expect(200) + .buffer(true).parse(superagent.parse.text) + .expect((res) => assert.equal(res.text, 'ofoo\n')); + }); + + it('txt request rev test1 is 403', async function () { + await agent.get(`/p/${testPadId}/test1/export/txt`) + .expect(500) + .buffer(true).parse(superagent.parse.text) + .expect((res) => assert.match(res.text, /rev is not a number/)); + }); + + it('txt request rev 5 returns head rev', async function () { + await agent.get(`/p/${testPadId}/5/export/txt`) + .expect(200) + .buffer(true).parse(superagent.parse.text) + .expect((res) => assert.equal(res.text, 'oofoo\n')); + }); + + it('html request rev 1', async function () { + await agent.get(`/p/${testPadId}/1/export/html`) + .expect(200) + .buffer(true).parse(superagent.parse.text) + .expect((res) => assert.match(res.text, /ofoo
/)); + }); + + it('html request rev 2', async function () { + await agent.get(`/p/${testPadId}/2/export/html`) + .expect(200) + .buffer(true).parse(superagent.parse.text) + .expect((res) => assert.match(res.text, /oofoo
/)); + }); + + it('html request rev 1test returns rev 1', async function () { + await agent.get(`/p/${testPadId}/1test/export/html`) + .expect(200) + .buffer(true).parse(superagent.parse.text) + .expect((res) => assert.match(res.text, /ofoo
/)); + }); + + it('html request rev test1 results in 500 response', async function () { + await agent.get(`/p/${testPadId}/test1/export/html`) + .expect(500) + .buffer(true).parse(superagent.parse.text) + .expect((res) => assert.match(res.text, /rev is not a number/)); + }); + + it('html request rev 5 returns head rev', async function () { + await agent.get(`/p/${testPadId}/5/export/html`) + .expect(200) + .buffer(true).parse(superagent.parse.text) + .expect((res) => assert.match(res.text, /oofoo
/)); + }); + }); + describe('Import authorization checks', function () { let authorize; diff --git a/src/tests/backend/specs/messages.js b/src/tests/backend/specs/messages.js index bccb2584d..643005f12 100644 --- a/src/tests/backend/specs/messages.js +++ b/src/tests/backend/specs/messages.js @@ -77,6 +77,89 @@ describe(__filename, function () { await otherPad.remove(); } }); + + it('CHANGESET_REQ: verify revNum is a number (regression)', async function () { + const otherPadId = `${padId}other`; + assert(!await padManager.doesPadExist(otherPadId)); + const otherPad = await padManager.getPad(otherPadId, 'other text\n'); + let errorCatched = 0; + try { + await otherPad.setText('other text\n'); + await common.sendMessage(roSocket, { + component: 'pad', + padId: otherPadId, // The server should ignore this. + type: 'CHANGESET_REQ', + data: { + granularity: 1, + start: 'test123', + requestID: 'requestId', + }, + }); + assert.equal('This code should never run', 1); + } + catch(e) { + assert.match(e.message, /rev is not a number/); + errorCatched = 1; + } + finally { + await otherPad.remove(); + assert.equal(errorCatched, 1); + } + }); + + it('CHANGESET_REQ: revNum is converted to number if possible (regression)', 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: '1test123', + requestID: 'requestId', + }, + }); + const res = await resP; + assert.equal(res.type, 'CHANGESET_REQ'); + assert.equal(res.data.requestID, 'requestId'); + assert.equal(res.data.start, 1); + } + finally { + await otherPad.remove(); + } + }); + + it('CHANGESET_REQ: revNum 2 is converted to head rev 1 (regression)', 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: '2', + requestID: 'requestId', + }, + }); + const res = await resP; + assert.equal(res.type, 'CHANGESET_REQ'); + assert.equal(res.data.requestID, 'requestId'); + assert.equal(res.data.start, 1); + } + finally { + await otherPad.remove(); + } + }); }); describe('USER_CHANGES', function () {