PadMessageHandler: Don't trust user-provided `padId`

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

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}`);
   }
 };
 

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;
+};

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();
+      }
+    });
+  });
+});