diff --git a/CHANGELOG.md b/CHANGELOG.md index 9881b6a9cb7..306f2cd7fde 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,13 @@ Fixes: Enterprise Fixes: - [data-manager] Fixed editing an event whose key contains `&` creating undeletable duplicate rows in the events table +Security Fixes: +- [core] The graph note tooltip now HTML-encodes the application name before rendering, so an application name is shown as text rather than markup +- [push] The message editor now sanitizes message content before rendering it into the editor, allowing only the user-property token element and rendering any other markup as text +- [compliance-hub] The export/purge history table now HTML-encodes the application name before it is placed in the action cell, so an application name is shown as text rather than markup +- [populator] The populator confirmation dialog bodies are now rendered as text instead of HTML +- [core] The dashboard escapes `<` when serializing the exposed `countlyGlobal` object into the inline page script, so an application name (or any exposed value) containing a `` end tag in any spelling can no longer break out of the script block and run in another user's session; the active-app name is now rendered with `.text()` instead of `.html()` + ## Version 24.05.51 Fixes: diff --git a/frontend/express/libs/express-expose.js b/frontend/express/libs/express-expose.js index 34d662e58e3..5a160207e23 100644 --- a/frontend/express/libs/express-expose.js +++ b/frontend/express/libs/express-expose.js @@ -186,9 +186,13 @@ function string(obj) { else { obj = JSON.stringify(obj); if (obj) { - // Only escape things that could break out of script context - obj = obj.replace(/<\/script>/ig, ''); - obj = obj.replace(/ +
{{dialog.text}}
diff --git a/plugins/push/frontend/public/javascripts/countly.views.component.common.js b/plugins/push/frontend/public/javascripts/countly.views.component.common.js index 84082df30e1..e87cab5754e 100644 --- a/plugins/push/frontend/public/javascripts/countly.views.component.common.js +++ b/plugins/push/frontend/public/javascripts/countly.views.component.common.js @@ -1,6 +1,15 @@ /* eslint-disable no-console */ /*global CV,countlyVue,countlyPushNotification,countlyGlobal,countlyCommon,moment*/ (function(countlyPushNotificationComponent) { + // The message editor is a live contenteditable. Its body is user-authored text; the + // only legitimate markup is the user-property token . Allow just that element + // (with the attributes the token relies on) and let everything else be escaped to inert + // text, so a stored message cannot introduce active markup when the editor is populated. + var PUSH_MESSAGE_EDITOR_XSS_OPTIONS = { + whiteList: { + span: ["class", "id", "contenteditable", "data-user-property-label", "data-user-property-value", "data-user-property-type", "data-user-property-fallback"] + } + }; countlyPushNotificationComponent.LargeRadioButtonWithDescription = countlyVue.views.create({ props: { value: { @@ -706,7 +715,7 @@ }, reset: function(htmlContent, ids) { this.disconnectMutationObserver(); - this.$refs.element.innerHTML = htmlContent; + this.$refs.element.innerHTML = countlyCommon.encodeSomeHtml(htmlContent, PUSH_MESSAGE_EDITOR_XSS_OPTIONS); this.addEventListeners(ids); this.startMutationObserver(); }, diff --git a/test/unit-tests/plugins.compliance-hub.actions-escaping.js b/test/unit-tests/plugins.compliance-hub.actions-escaping.js new file mode 100644 index 00000000000..2c512055487 --- /dev/null +++ b/test/unit-tests/plugins.compliance-hub.actions-escaping.js @@ -0,0 +1,132 @@ +var should = require("should"); +var fs = require("fs"); +var path = require("path"); +var vm = require("vm"); + +// The export/purge history datatable builds an HTML string in onReady and the template renders it +// with v-html (plugins/compliance-hub/frontend/public/templates/exportHistory.html). Everything +// interpolated into that string therefore has to be HTML-safe already. +// +// Two opposite mistakes are possible and this file guards both directions: +// +// 1. NOT escaping the app name. It is read from countlyGlobal, which express-expose serializes +// into the dashboard's inline script island. That serializer escapes for the JavaScript string +// context only and is deliberately value-preserving, so the value arrives raw. An app admin can +// set their own app's name, and a global admin's dashboard lists every app, so an unescaped name +// is a stored cross-user XSS. +// 2. Escaping the values that came from the API. Those already went through +// common.escape_html_entities in common.returnOutput, so escaping them again would render the +// entities literally in the UI. +var SRC = path.resolve(__dirname, "../../plugins/compliance-hub/frontend/public/javascripts/countly.models.js"); + +/** + * Load countly.models.js in a sandbox and hand back the onReady callbacks it registers, + * keyed by data-table name. + * @param {object} apps - the countlyGlobal.apps map the module should see + * @returns {object} map of resource name to its onReady function + */ +function loadResources(apps) { + var resources = {}; + var noop = function() {}; + // matches countlyCommon.encodeHtml, which is `div.innerText = x; return div.innerHTML`: + // the text-node serializer escapes &, < and > and leaves quotes alone. + var encodeHtml = function(html) { + return (html + "").replace(/&/g, "&").replace(//g, ">"); + }; + var sandbox = { + window: {}, + countlyCommon: { + encodeHtml: encodeHtml, + formatTimeAgoText: function() { + return { text: "just now" }; + }, + getDescendantProp: noop, + API_PARTS: { data: { r: "/o" } }, + ACTIVE_APP_ID: "5f1a2b3c4d5e6f0011223344", + periodObj: {}, + getPeriodForAjax: function() { + return "30days"; + } + }, + CountlyHelpers: { createMetricModel: noop }, + jQuery: { i18n: { map: { "systemlogs.for-app": "For app", "systemlogs.for-appuser": "For app user", "systemlogs.action.export": "Data exported" } } }, + CV: { + i18n: function(k) { + return k; + } + }, + countlyGlobal: { apps: apps }, + countlyTaskManager: {}, + countlyVue: { + vuex: { + ServerDataTable: function(name, cfg) { + resources[name] = cfg.onReady; + return { name: name }; + }, + Module: function() { + return {}; + }, + MutationsFor: noop, + ActionsFor: noop + } + } + }; + sandbox.global = sandbox; + vm.createContext(sandbox); + vm.runInContext(fs.readFileSync(SRC, "utf8"), sandbox, { filename: SRC }); + return resources; +} + +describe("compliance-hub export history actions escaping", function() { + var APP_ID = "5f1a2b3c4d5e6f0011223344"; + + /** + * Run the export-history onReady over a single row. + * @param {string} appName - the app name countlyGlobal should carry + * @param {object} i - the row's "i" payload + * @returns {string} the built actions HTML + */ + function actionsFor(appName, i) { + var apps = {}; + apps[APP_ID] = { name: appName }; + var onReady = loadResources(apps).exportHistoryDataResource; + should.exist(onReady); + var rows = onReady({}, [{ a: "export", ts: 0, i: i || { app_id: APP_ID } }]); + return rows[0].actions; + } + + it("escapes an app name that carries a tag", function(done) { + var actions = actionsFor(''); + actions.indexOf(""); + actions.indexOf("").should.equal(-1); + done(); + }); + + it("leaves no raw angle bracket from the app name", function(done) { + var actions = actionsFor(""); + // the only markup left must be the

wrappers this builder emits itself + actions.replace(/<\/?p[^>]*>/g, "").indexOf("<").should.equal(-1); + done(); + }); + + it("keeps an ordinary app name readable", function(done) { + var actions = actionsFor("My Application"); + actions.should.containEql("For app: My Application"); + done(); + }); + + it("does not double-escape values the API already escaped", function(done) { + // returnOutput turns ' into '; escaping again would surface "&#39;" in the UI + var actions = actionsFor("My Application", { app_id: APP_ID, appuser_id: "user's-id" }); + actions.should.containEql("user's-id"); + actions.indexOf("&#39;").should.equal(-1); + done(); + }); +}); diff --git a/test/unit-tests/plugins.push.editor-allowlist.js b/test/unit-tests/plugins.push.editor-allowlist.js new file mode 100644 index 00000000000..981e3cc26fe --- /dev/null +++ b/test/unit-tests/plugins.push.editor-allowlist.js @@ -0,0 +1,100 @@ +require("should"); +var fs = require("fs"); +var path = require("path"); + +// The push message editor is a live contenteditable whose stored content is run through +// xss() every time it is populated. The only legitimate markup in it is the user-property +// token , so the allowlist has to name exactly the attributes that token carries - +// no more, or the sanitizer stops being a boundary, and no fewer, or every sanitize pass +// silently rewrites a stored message into a lossier one and the next edit persists that. +// +// That second direction is the one worth a test, because nothing fails loudly when it +// happens. So rather than restating the list, this derives it from the component itself: +// every attribute the component sets on a token span must survive the sanitizer. + +var COMPONENT = path.join( + __dirname, "../../plugins/push/frontend/public/javascripts/countly.views.component.common.js"); +var MODEL = path.join( + __dirname, "../../plugins/push/frontend/public/javascripts/countly.models.js"); + +var componentSrc = fs.readFileSync(COMPONENT, "utf8"); +var modelSrc = fs.readFileSync(MODEL, "utf8"); + +/** + * The span allowlist as the component declares it + * @returns {Array} allowed attribute names + */ +function allowedSpanAttributes() { + var m = componentSrc.match(/span:\s*\[([^\]]*)\]/); + if (!m) { + throw new Error("span allowlist not found in countly.views.component.common.js"); + } + return m[1].split(",").map(function(part) { + return part.trim().replace(/^["']|["']$/g, ""); + }).filter(Boolean); +} + +/** + * Every attribute name the given source sets with setAttribute + * @param {string} src - source to scan + * @returns {Array} attribute names, lower cased as the DOM treats them + */ +function attributesSetIn(src) { + var found = []; + var re = /setAttribute\(\s*["']([^"']+)["']/g; + var m = re.exec(src); + while (m) { + var name = m[1].toLowerCase(); + if (found.indexOf(name) === -1) { + found.push(name); + } + m = re.exec(src); + } + return found; +} + +describe("push message editor sanitizer allowlist", function() { + var allowed = allowedSpanAttributes(); + + it("allows nothing but the span, and no event handlers on it", function() { + var m = componentSrc.match(/whiteList:\s*\{([\s\S]*?)\n\s*\}/); + m[1].should.match(/span:/); + m[1].should.not.match(/\ba\s*:/); + m[1].should.not.match(/img\s*:/); + allowed.forEach(function(name) { + name.indexOf("on").should.not.equal(0, name + " is an event handler attribute"); + ["href", "src", "style", "srcdoc", "formaction"].indexOf(name) + .should.equal(-1, name + " can carry a url or a script"); + }); + }); + + it("keeps every attribute the editor puts on a token", function() { + // data-user-property-type was missing, so each sanitize pass dropped the type off + // stored token markup and the next editor change persisted it without one + var set = attributesSetIn(componentSrc).filter(function(name) { + return name.indexOf("data-user-property-") === 0; + }); + set.length.should.be.above(0); + set.forEach(function(name) { + allowed.indexOf(name).should.not.equal(-1, name + " is set on a token but sanitized away"); + }); + }); + + it("keeps every attribute the stored-message rebuild puts on a token", function() { + // countly.models.js rebuilds tokens when a saved message is opened, so its + // attributes go through the same sanitizer + var set = attributesSetIn(modelSrc).filter(function(name) { + return name.indexOf("data-user-property-") === 0; + }); + set.length.should.be.above(0); + set.forEach(function(name) { + allowed.indexOf(name).should.not.equal(-1, name + " is set on a token but sanitized away"); + }); + }); + + it("keeps the structural attributes a token needs to work", function() { + ["id", "class", "contenteditable"].forEach(function(name) { + allowed.indexOf(name).should.not.equal(-1, name + " is required by the token"); + }); + }); +});