Skip to content

Commit

Permalink
Security: Fix revision parsing (#5772)
Browse files Browse the repository at this point in the history
A carefully crated URL can cause Etherpad to hang.
  • Loading branch information
JohnMcLear authored Jun 26, 2023
1 parent 1d28952 commit 1e98033
Show file tree
Hide file tree
Showing 9 changed files with 325 additions and 29 deletions.
11 changes: 11 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -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
Expand Down
36 changes: 8 additions & 28 deletions src/node/db/API.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 ****
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
3 changes: 3 additions & 0 deletions src/node/db/Pad.js
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
7 changes: 7 additions & 0 deletions src/node/handler/ExportHandler.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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') {
Expand Down
5 changes: 5 additions & 0 deletions src/node/handler/PadMessageHandler.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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});
Expand Down
6 changes: 5 additions & 1 deletion src/node/utils/ExportHelper.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
34 changes: 34 additions & 0 deletions src/node/utils/checkValidRev.js
Original file line number Diff line number Diff line change
@@ -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;
169 changes: 169 additions & 0 deletions src/tests/backend/specs/api/importexportGetPost.js
Original file line number Diff line number Diff line change
Expand Up @@ -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<br>/));
});

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<br>/));
});

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<br>/));
});

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<br>/));
});
});

describe('Import authorization checks', function () {
let authorize;

Expand Down
Loading

0 comments on commit 1e98033

Please sign in to comment.