Security: Fix revision parsing (#5772)

JohnMcLear · web-flow · commit 1e9803363232 · 2023-06-26T18:17:06.000+01:00
A carefully crated URL can cause Etherpad to hang.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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;
 
diff --git a/src/tests/backend/specs/messages.js b/src/tests/backend/specs/messages.js
