fix: stored XSS in createDiffHTML author colors (GHSA-6vx2-3gwr-958v)

CHANGELOG.md

@@ -1,5 +1,11 @@
 # 3.3.4
 
+3.3.4 is a security release. It closes a stored XSS in the `createDiffHTML` API output (GHSA-6vx2-3gwr-958v).
+
+### Security
+
+- **Neutralize author IDs and colors in HTML diff export (GHSA-6vx2-3gwr-958v).** `getHTMLFromAtext` placed author colors inside a `<style>` block, and author IDs in both the CSS selector and a `<span class>` attribute, with no escaping. Anyone who can import a `.etherpad` file (anonymous by default) could plant a crafted `colorId` or author ID, so the `createDiffHTML` output carried script into any integration that renders it. Export now only emits `#rgb`/`#rrggbb` colors and limits author class names to `[A-Za-z0-9_-]`. As an extra safeguard, `.etherpad` import replaces a malformed `colorId` with a palette color, matching the live socket validation. Adds backend regression tests. Reported by zx (@manus-pi).
+
 ### Notable fixes
 
 - **API — `movePad` now carries the pad's deletion token to the new id (#7995).** `movePad` is implemented as `copy()` + `remove()`, but `Pad.copy()` only copies the `pad:<id>`, `:revs:N` and `:chat:N` records — never `pad:<id>:deletionToken` — and `remove()` then deleted the source pad's token. The renamed pad therefore had no token at all: the token the creator had been told to save no longer deleted anything, and because the copy keeps the same revision-0 author, their next visit tripped `createDeletionTokenIfAbsent()` and popped a second "save your pad deletion token" modal. The token record is now handed over to the destination as part of the move, so the saved token keeps working and the modal does not reappear. `force`-overwriting an existing destination discards that pad's own token along with its content. `copyPad` is deliberately unchanged — two pads sharing one secret would let a token saved for one delete the other.

src/node/utils/ExportHtml.ts

@@ -81,7 +81,12 @@ const getHTMLFromAtext = async (pad:PadType, atext: AText, authorColors?: string
   const anumMap:MapArrayType<number> = {};
   let css = '';
 
-  const stripDotFromAuthorID = (id: string) => id.replace(/\./g, '_');
+  // Author IDs (from the attribute pool) and author colors (from globalAuthor records) are
+  // attacker-controllable via .etherpad import and end up inside a <style> block and a class
+  // attribute, so constrain both to characters that cannot break out of those contexts.
+  const stripDotFromAuthorID = (id: string) => id.replace(/[^A-Za-z0-9_-]/g, '_');
+  const isSafeCssColor = (color: unknown) =>
+    typeof color === 'string' && /^#(?:[0-9a-f]{3}|[0-9a-f]{6})$/i.test(color);
 
   if (authorColors) {
     css += '<style>\n';
@@ -97,7 +102,9 @@ const getHTMLFromAtext = async (pad:PadType, atext: AText, authorColors?: string
         const newLength = props.push(propName);
         anumMap[a] = newLength - 1;
 
-        css += `.${propName} {background-color: ${authorColors[attr[1]]}}\n`;
+        // @ts-ignore
+        const color = authorColors[attr[1]];
+        if (isSafeCssColor(color)) css += `.${propName} {background-color: ${color}}\n`;
       } else if (attr[0] === 'removed') {
         const propName = 'removed';
         const newLength = props.push(propName);

src/node/utils/ImportEtherpad.ts

@@ -32,6 +32,14 @@ const supportedElems = require('../../static/js/contentcollector').supportedElem
 
 const logger = log4js.getLogger('ImportEtherpad');
 
+// A colorId is either an index into the color palette or a #hex color.
+const isValidColorId = (colorId: unknown) => {
+  if (typeof colorId === 'string' && /^#(?:[0-9a-f]{3}|[0-9a-f]{6})$/i.test(colorId)) return true;
+  const index = typeof colorId === 'string' && /^\d+$/.test(colorId) ? Number(colorId) : colorId;
+  return Number.isInteger(index) &&
+      (index as number) >= 0 && (index as number) < authorManager.getColorPalette().length;
+};
+
 // Not `Pad.SYSTEM_AUTHOR_ID`: that would be a circular import
 // (ImportEtherpad -> Pad -> ImportEtherpad via padManager) at module init
 // time.
@@ -239,6 +247,7 @@ exports.setPadRaw = async (padId: string, r: string, authorId = '') => {
   await padDb.init();
   try {
     const processRecord = async (key:string, value: null|{
+      colorId?: unknown,
       padIDs: string|Record<string, unknown>,
       pool: AttributePool
     }) => {
@@ -258,6 +267,12 @@ exports.setPadRaw = async (padId: string, r: string, authorId = '') => {
           return;
         }
         value.padIDs = {[padId]: 1};
+        // The live socket path only accepts palette indices and #hex colors; hold imported
+        // records to the same rule so a crafted colorId can't reach HTML/CSS sinks.
+        if (!isValidColorId(value.colorId)) {
+          logger.warn(`(pad ${padId}) replacing malformed colorId on imported author ${id}`);
+          value.colorId = Math.floor(Math.random() * authorManager.getColorPalette().length);
+        }
       } else if (padKeyPrefixes.includes(prefix)) {
         checkOriginalPadId(id);
         if (prefix === 'pad' && keyParts.length === 2) {

src/tests/backend/specs/exportHtmlAuthorColorXss.ts

@@ -0,0 +1,109 @@
+'use strict';
+
+// GHSA-6vx2-3gwr-958v: author IDs and colors are attacker-controllable via .etherpad import and
+// must not break out of the <style> block / class attribute emitted for createDiffHTML.
+
+const assert = require('assert').strict;
+const authorManager = require('../../../node/db/AuthorManager');
+const common = require('../common');
+const exportHtml = require('../../../node/utils/ExportHtml');
+const importEtherpad = require('../../../node/utils/ImportEtherpad');
+const padManager = require('../../../node/db/PadManager');
+import {randomString} from '../../../static/js/pad_utils';
+
+describe(__filename, function () {
+  const colorPayload = 'red}</style><script>alert(1)</script><style>{';
+  let padId: string;
+  let authorId: string;
+
+  const makeExport = (colorId: unknown) => ({
+    'pad:src': {
+      atext: {text: 'foo\n', attribs: '*0+3|1+1'},
+      pool: {numToAttrib: {0: ['author', authorId]}, nextNum: 1},
+      head: 0,
+      savedRevisions: [],
+    },
+    [`globalAuthor:${authorId}`]: {
+      colorId,
+      name: 'evil',
+      timestamp: 1598747784631,
+      padIDs: 'src',
+    },
+    'pad:src:revs:0': {
+      changeset: 'Z:1>3*0+3$foo',
+      meta: {
+        author: authorId,
+        timestamp: 1597632398288,
+        pool: {numToAttrib: {0: ['author', authorId]}, nextNum: 1},
+        atext: {text: 'foo\n', attribs: '*0+3|1+1'},
+      },
+    },
+  });
+
+  before(async function () {
+    await common.init();
+  });
+
+  beforeEach(async function () {
+    padId = randomString(10);
+    authorId = `a.${randomString(16)}`;
+  });
+
+  afterEach(async function () {
+    if (await padManager.doesPadExist(padId)) await (await padManager.getPad(padId)).remove();
+  });
+
+  it('import replaces a malformed colorId', async function () {
+    await importEtherpad.setPadRaw(padId, JSON.stringify(makeExport(colorPayload)));
+    const colorId = await authorManager.getAuthorColorId(authorId);
+    assert.notEqual(colorId, colorPayload);
+    assert.equal(typeof colorId, 'number');
+  });
+
+  it('import replaces an out-of-range palette index', async function () {
+    await importEtherpad.setPadRaw(padId, JSON.stringify(makeExport(999)));
+    const colorId = await authorManager.getAuthorColorId(authorId);
+    assert(colorId >= 0 && colorId < authorManager.getColorPalette().length, `${colorId}`);
+  });
+
+  it('import keeps an in-range palette index', async function () {
+    await importEtherpad.setPadRaw(padId, JSON.stringify(makeExport(3)));
+    assert.equal(await authorManager.getAuthorColorId(authorId), 3);
+  });
+
+  it('import keeps valid colorIds', async function () {
+    await importEtherpad.setPadRaw(padId, JSON.stringify(makeExport('#abc123')));
+    assert.equal(await authorManager.getAuthorColorId(authorId), '#abc123');
+  });
+
+  it('export drops a malformed color already in the database', async function () {
+    await importEtherpad.setPadRaw(padId, JSON.stringify(makeExport(3)));
+    // Simulate a record that predates import validation.
+    await authorManager.setAuthorColorId(authorId, colorPayload);
+    const pad = await padManager.getPad(padId);
+    const html = await exportHtml.getHTMLFromAtext(pad, pad.atext, await pad.getAllAuthorColors());
+    assert(!html.includes('<script>'), html);
+    assert(!html.includes(colorPayload), html);
+  });
+
+  it('export emits valid colors unchanged', async function () {
+    await importEtherpad.setPadRaw(padId, JSON.stringify(makeExport('#abc123')));
+    const pad = await padManager.getPad(padId);
+    const html = await exportHtml.getHTMLFromAtext(pad, pad.atext, await pad.getAllAuthorColors());
+    const cls = `author${authorId.replace('.', '_')}`;
+    assert(html.includes(`.${cls} {background-color: #abc123}`), html);
+    assert(html.includes(`<span class="${cls}">foo</span>`), html);
+  });
+
+  it('export neutralizes a malicious author ID in the selector and class attribute', async function () {
+    await importEtherpad.setPadRaw(padId, JSON.stringify(makeExport('#abc123')));
+    const pad = await padManager.getPad(padId);
+    const evilId = 'a.x"><img src=x onerror=alert(1)>{}</style><script>alert(2)</script>';
+    const n = pad.apool().putAttrib(['author', evilId]);
+    const atext = {text: 'foo\n', attribs: `*${n.toString(36)}+3|1+1`};
+    const html = await exportHtml.getHTMLFromAtext(pad, atext, {[evilId]: '#abc123'});
+    assert(!html.includes('<script>'), html);
+    assert(!html.includes('<img'), html);
+    assert(html.includes('<span class="authora_x___img_src_x_onerror_alert_1'), html);
+  });
+});