security: encode and sanitize values at frontend HTML sinks (24.05)

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 `</script>` 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:

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, '</scr"+"ipt>');
-            obj = obj.replace(/<!--/g, '<\\!--');
+            // Escape "<" so nothing serialized here can open or close a tag in the inline
+            // <script> block this value is written into. A "</script>" end tag terminates the
+            // script in any of its whitespace/slash spellings (</script >, </script/>, ...),
+            // which an exact-match replace of "</script>" misses; escaping every "<" closes all
+            // of those plus "<!--" and "<script". The escape parses back to "<", so runtime
+            // values read from the exposed object are unchanged.
+            obj = obj.replace(/</g, '\\u003c');
             obj = obj.replace(/\u2028/g, '\\u2028'); // Line separator
             obj = obj.replace(/\u2029/g, '\\u2029'); // Paragraph separator
         }
@@ -222,7 +226,8 @@ function escape_js_string(str) {
         .replace(/\0/g, '\\0') // Null character
         .replace(/[\u0000-\u001F\u007F-\u009F]/g, function(ch) {
             return '\\u' + ('0000' + ch.charCodeAt(0).toString(16)).slice(-4);
-        });
+        })
+        .replace(/</g, '\\u003c'); // "<" so a key cannot break out of the <script> block
 }
 
 exports = module.exports = function(app) {

frontend/express/public/javascripts/countly/countly.common.js

@@ -1115,7 +1115,7 @@
                                         var noteTime = moment(notes[0].ts).format("D MMM, HH:mm");
                                         var noteId = notes[0].app_id;
                                         var app = countlyGlobal.apps[noteId] || {};
-                                        titleDom = "<div> <div class='note-header'><div class='note-title'>" + noteTime + "</div><div class='note-app' style='display:flex;line-height: 15px;'> <div class='icon' style='display:inline-block; border-radius:2px; width:15px; height:15px; margin-right: 5px; background: url(appimages/" + noteId + ".png) center center / cover no-repeat;'></div><span>" + app.name + "</span></div></div>" +
+                                        titleDom = "<div> <div class='note-header'><div class='note-title'>" + noteTime + "</div><div class='note-app' style='display:flex;line-height: 15px;'> <div class='icon' style='display:inline-block; border-radius:2px; width:15px; height:15px; margin-right: 5px; background: url(appimages/" + noteId + ".png) center center / cover no-repeat;'></div><span>" + countlyCommon.encodeHtml(app.name) + "</span></div></div>" +
                                         "<div class='note-content'>" + notes[0].note + "</div>" +
                                         "<div class='note-footer'> <span class='note-owner'>" + (notes[0].owner_name) + "</span> | <span class='note-type'>" + (jQuery.i18n.map["notes.note-" + notes[0].noteType] || notes[0].noteType) + "</span> </div>" +
                                             "</div>";

frontend/express/public/javascripts/countly/countly.template.js

@@ -2403,7 +2403,7 @@ var AppRouter = Backbone.Router.extend({
 
                 countlyCommon.setActiveApp(activeApp._id);
                 self.activeAppName = activeApp.name;
-                $('#active-app-name').html(activeApp.name);
+                $('#active-app-name').text(activeApp.name);
                 $('#active-app-name').attr('title', activeApp.name);
                 $("#active-app-icon").css("background-image", "url('" + countlyGlobal.cdn + "appimages/" + countlyCommon.ACTIVE_APP_ID + ".png')");
             }

plugins/compliance-hub/frontend/public/javascripts/countly.models.js

@@ -194,7 +194,13 @@
                 var ret = "<p>" + ((jQuery.i18n.map["systemlogs.action." + row.a]) ? jQuery.i18n.map["systemlogs.action." + row.a] : row.a) + "</p>";
                 if (typeof row.i === "object") {
                     if (typeof row.i.app_id !== "undefined" && countlyGlobal.apps[row.i.app_id]) {
-                        ret += "<p title='" + row.i.app_id + "'>" + jQuery.i18n.map["systemlogs.for-app"] + ": " + countlyGlobal.apps[row.i.app_id].name + "</p>";
+                        //this string is rendered with v-html, so every interpolated value must already be
+                        //HTML-safe. Everything taken from "row" arrives through common.returnOutput, which
+                        //escape_html_entities has already escaped, so it must NOT be escaped again here or
+                        //the entities would show up literally. The app name is the exception: it comes from
+                        //countlyGlobal, which is serialized into the dashboard's script island by
+                        //express-expose and is never HTML-escaped, so it reaches us raw and is escaped here.
+                        ret += "<p title='" + row.i.app_id + "'>" + jQuery.i18n.map["systemlogs.for-app"] + ": " + countlyCommon.encodeHtml(countlyGlobal.apps[row.i.app_id].name) + "</p>";
                     }
                     if (typeof row.i.appuser_id !== "undefined") {
                         ret += "<p title='" + row.i.appuser_id + "'>" + jQuery.i18n.map["systemlogs.for-appuser"] + ": " + row.i.appuser_id + "</p>";

plugins/populator/frontend/public/javascripts/countly.views.js

@@ -552,7 +552,11 @@
                     saveButtonLabel: CV.i18n('common.yes'),
                     cancelButtonLabel: CV.i18n('common.cancel'),
                     title: CV.i18n('populator.environment-delete-warning-title'),
-                    text: CV.i18n('populator.environment-delete-warning-description', this.filterByEnvironmentOptions.filter(x => x.value === this.environmentId)[0].label)
+                    //this sentence is rendered with v-html because the localized string itself carries
+                    //markup, so the environment name substituted into it has to be escaped here. The name
+                    //was html-decoded when the dropdown options were built, which is what the dropdown
+                    //needs, so the escaping the api applied no longer holds by this point.
+                    text: CV.i18n('populator.environment-delete-warning-description', countlyCommon.encodeHtml(this.filterByEnvironmentOptions.filter(x => x.value === this.environmentId)[0].label))
                 };
             },
             calculateWidth: function(percentage) {

plugins/populator/frontend/public/templates/populator.html

@@ -170,7 +170,8 @@ <h3 class="populator-stop-modal-wrapper__finished-confirm-header" data-test-id="
     </cly-tabs>
     <cly-confirm-dialog class="populator-wrapper__start-dialog" :show-close="false" @cancel="closeConfirmDialog" @confirm="submitConfirmDialog" :before-close="closeConfirmDialog" ref="deleteConfirmDialog" :visible.sync="dialog.showDialog" dialogType="success" :saveButtonLabel="dialog.saveButtonLabel" :cancelButtonLabel="dialog.cancelButtonLabel" :saveButtonVisibility="dialog.saveButtonVisibility" :title="dialog.title" >
         <template slot-scope="scope">
-            <div v-html="dialog.text"></div>
+            <!-- dialog.text is a localized sentence with a template name substituted in, never markup -->
+            <div>{{dialog.text}}</div>
         </template>
     </cly-confirm-dialog>
     <cly-populator-template-drawer ref="populatorTemplateDrawer" @refresh-table="refresh" @closeHandler="refreshTable" :titleDescription="titleDescription" :controls="drawers.populatorTemplate"></cly-populator-template-drawer>

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 <span>. 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();
             },

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, "&amp;").replace(/</g, "&lt;").replace(/>/g, "&gt;");
+    };
+    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('<img src=x onerror="alert(1)">');
+        actions.indexOf("<img").should.equal(-1);
+        actions.should.containEql("&lt;img src=x onerror=&quot;alert(1)&quot;&gt;".replace(/&quot;/g, '"'));
+        done();
+    });
+
+    it("escapes an app name that closes the surrounding tag", function(done) {
+        var actions = actionsFor("</p><script>alert(1)</script>");
+        actions.indexOf("<script").should.equal(-1);
+        actions.indexOf("</script>").should.equal(-1);
+        done();
+    });
+
+    it("leaves no raw angle bracket from the app name", function(done) {
+        var actions = actionsFor("<svg onload=alert(1)>");
+        // the only markup left must be the <p> 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 &#39;; escaping again would surface "&amp;#39;" in the UI
+        var actions = actionsFor("My Application", { app_id: APP_ID, appuser_id: "user&#39;s-id" });
+        actions.should.containEql("user&#39;s-id");
+        actions.indexOf("&amp;#39;").should.equal(-1);
+        done();
+    });
+});

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 <span>, 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");
+        });
+    });
+});