From 8e3f2eb858b16d81fbe4f69896cf4b831ce3873f Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Wed, 11 Jan 2017 16:35:37 +0000 Subject: [PATCH 1/8] Allow [bf]g colors for style attrib Instead of dropping the style attribute on `` tags entirely, sanitise aggressively and only keep `background-color` and `color` keys, and also sanitise the values to prevent `url(XXXXXX)` and `expression(XXXXXX)` type XSS attacks. --- src/HtmlUtils.js | 32 +++++++++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/src/HtmlUtils.js b/src/HtmlUtils.js index fc1630b6fb..ae594de960 100644 --- a/src/HtmlUtils.js +++ b/src/HtmlUtils.js @@ -28,6 +28,11 @@ emojione.imagePathSVG = 'emojione/svg/'; emojione.imageType = 'svg'; const EMOJI_REGEX = new RegExp(emojione.unicodeRegexp+"+", "gi"); +const COLOR_REGEX = /^[#a-z0-9]+$/; +const ALLOWED_CSS = { + "background-color": COLOR_REGEX, + "color": COLOR_REGEX, +}; /* modified from https://github.com/Ranks/emojione/blob/master/lib/js/emojione.js * because we want to include emoji shortnames in title text @@ -91,7 +96,7 @@ var sanitizeHtmlParams = { ], allowedAttributes: { // custom ones first: - font: [ 'color' ], // custom to matrix + font: [ 'color' , 'style' ], // custom to matrix a: [ 'href', 'name', 'target', 'rel' ], // remote target: custom to matrix // We don't currently allow img itself by default, but this // would make sense if we did @@ -136,6 +141,31 @@ var sanitizeHtmlParams = { attribs.rel = 'noopener'; // https://mathiasbynens.github.io/rel-noopener/ return { tagName: tagName, attribs : attribs }; }, + '*': function(tagName, attribs) { + // Only allow certain CSS attributes to avoid XSS attacks + // Sanitizing values to avoid `url(...)` and `expression(...)` attacks + if (!attribs.style) { + return { tagName: tagName, attribs : attribs }; + } + + const pairs = attribs.style.split(';'); + let sanitisedStyle = ""; + for (let i = 0; i < pairs.length; i++) { + const pair = pairs[i].split(':'); + if (!Object.keys(ALLOWED_CSS).includes(pair[0]) || !ALLOWED_CSS[pair[0]].test(pair[1])) { + continue; + } + sanitisedStyle += pair[0] + ":" + pair[1] + ";"; + } + + if (sanitisedStyle) { + attribs.style = sanitisedStyle; + } else { + delete attribs.style; + } + + return { tagName: tagName, attribs : attribs }; + }, }, }; From 32185befc009a32534e111fd549a50b873ad3ff4 Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Wed, 11 Jan 2017 16:41:05 +0000 Subject: [PATCH 2/8] Only transform --- src/HtmlUtils.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/HtmlUtils.js b/src/HtmlUtils.js index ae594de960..a8fb763a8d 100644 --- a/src/HtmlUtils.js +++ b/src/HtmlUtils.js @@ -141,7 +141,7 @@ var sanitizeHtmlParams = { attribs.rel = 'noopener'; // https://mathiasbynens.github.io/rel-noopener/ return { tagName: tagName, attribs : attribs }; }, - '*': function(tagName, attribs) { + 'font': function(tagName, attribs) { // Only allow certain CSS attributes to avoid XSS attacks // Sanitizing values to avoid `url(...)` and `expression(...)` attacks if (!attribs.style) { From 886b0a3f13f7dbb75daadec69e8b6ef7030efa2c Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Mon, 27 Feb 2017 11:23:37 +0000 Subject: [PATCH 3/8] Sanitise for *, fix style issues --- src/HtmlUtils.js | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/HtmlUtils.js b/src/HtmlUtils.js index b27ed9e159..447de08867 100644 --- a/src/HtmlUtils.js +++ b/src/HtmlUtils.js @@ -141,18 +141,20 @@ var sanitizeHtmlParams = { attribs.rel = 'noopener'; // https://mathiasbynens.github.io/rel-noopener/ return { tagName: tagName, attribs : attribs }; }, - 'font': function(tagName, attribs) { + '*': function(tagName, attribs) { // Only allow certain CSS attributes to avoid XSS attacks // Sanitizing values to avoid `url(...)` and `expression(...)` attacks if (!attribs.style) { - return { tagName: tagName, attribs : attribs }; + return { tagName: tagName, attribs: attribs }; } const pairs = attribs.style.split(';'); let sanitisedStyle = ""; for (let i = 0; i < pairs.length; i++) { const pair = pairs[i].split(':'); - if (!Object.keys(ALLOWED_CSS).includes(pair[0]) || !ALLOWED_CSS[pair[0]].test(pair[1])) { + if (!Object.keys(ALLOWED_CSS).includes(pair[0]) || + !ALLOWED_CSS[pair[0]].test(pair[1]) + ) { continue; } sanitisedStyle += pair[0] + ":" + pair[1] + ";"; @@ -164,7 +166,7 @@ var sanitizeHtmlParams = { delete attribs.style; } - return { tagName: tagName, attribs : attribs }; + return { tagName: tagName, attribs: attribs }; }, }, }; From 5fc828f24c5fa0deda616da332bed7e15d1b853d Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Mon, 27 Feb 2017 11:32:57 +0000 Subject: [PATCH 4/8] Allow span, and only allow style attrib --- src/HtmlUtils.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/HtmlUtils.js b/src/HtmlUtils.js index 447de08867..3cad98198a 100644 --- a/src/HtmlUtils.js +++ b/src/HtmlUtils.js @@ -92,11 +92,12 @@ var sanitizeHtmlParams = { // deliberately no h1/h2 to stop people shouting. 'h3', 'h4', 'h5', 'h6', 'blockquote', 'p', 'a', 'ul', 'ol', 'nl', 'li', 'b', 'i', 'u', 'strong', 'em', 'strike', 'code', 'hr', 'br', 'div', - 'table', 'thead', 'caption', 'tbody', 'tr', 'th', 'td', 'pre' + 'table', 'thead', 'caption', 'tbody', 'tr', 'th', 'td', 'pre', 'span', ], allowedAttributes: { // custom ones first: font: ['color', 'style'], // custom to matrix + span: ['style'], a: ['href', 'name', 'target', 'rel'], // remote target: custom to matrix // We don't currently allow img itself by default, but this // would make sense if we did From 36795fa19297250148b81838ace08bf664b04df9 Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Thu, 2 Mar 2017 11:36:56 +0000 Subject: [PATCH 5/8] Use data-mx[-bg]-color instead of stripping style This has the benefit of not needing a spec for custom CSS. Instead we rigourously sanitise the values for custom data attributes that are transformed to CSS equivalents. `data-mx-color` translates to CSS `color` for example. --- src/HtmlUtils.js | 43 +++++++++++++++++++++++-------------------- 1 file changed, 23 insertions(+), 20 deletions(-) diff --git a/src/HtmlUtils.js b/src/HtmlUtils.js index 3cad98198a..03be6d35a0 100644 --- a/src/HtmlUtils.js +++ b/src/HtmlUtils.js @@ -96,8 +96,8 @@ var sanitizeHtmlParams = { ], allowedAttributes: { // custom ones first: - font: ['color', 'style'], // custom to matrix - span: ['style'], + font: ['color', 'data-mx-bg-color', 'data-mx-color', 'style'], // custom to matrix + span: ['data-mx-bg-color', 'data-mx-color', 'style'], // custom to matrix a: ['href', 'name', 'target', 'rel'], // remote target: custom to matrix // We don't currently allow img itself by default, but this // would make sense if we did @@ -143,28 +143,31 @@ var sanitizeHtmlParams = { return { tagName: tagName, attribs : attribs }; }, '*': function(tagName, attribs) { - // Only allow certain CSS attributes to avoid XSS attacks - // Sanitizing values to avoid `url(...)` and `expression(...)` attacks - if (!attribs.style) { - return { tagName: tagName, attribs: attribs }; - } + // Delete any style previously assigned + delete attribs.style; - const pairs = attribs.style.split(';'); - let sanitisedStyle = ""; - for (let i = 0; i < pairs.length; i++) { - const pair = pairs[i].split(':'); - if (!Object.keys(ALLOWED_CSS).includes(pair[0]) || - !ALLOWED_CSS[pair[0]].test(pair[1]) + // Sanitise and transform data-mx-color and data-mx-bg-color to their CSS + // equivalents + const customCSSMapper = { + 'data-mx-color': 'color', + 'data-mx-bg-color': 'background-color', + // $customAttributeKey: $cssAttributeKey + }; + + let style = ""; + Object.keys(customCSSMapper).forEach((customAttributeKey) => { + const cssAttributeKey = customCSSMapper[customAttributeKey]; + const customAttributeValue = attribs[customAttributeKey]; + if (customAttributeValue && + typeof customAttributeValue === 'string' && + /#[0-9a-fA-F]{6}/.test(customAttributeValue) ) { - continue; + style += cssAttributeKey + ":" + customAttributeValue + ";"; } - sanitisedStyle += pair[0] + ":" + pair[1] + ";"; - } + }); - if (sanitisedStyle) { - attribs.style = sanitisedStyle; - } else { - delete attribs.style; + if (style) { + attribs.style = style; } return { tagName: tagName, attribs: attribs }; From b951713f7f22c25b14470a56faf8e895597aa791 Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Thu, 2 Mar 2017 11:39:40 +0000 Subject: [PATCH 6/8] Remove custom attribs as consumed --- src/HtmlUtils.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/HtmlUtils.js b/src/HtmlUtils.js index 03be6d35a0..dc167ce5c7 100644 --- a/src/HtmlUtils.js +++ b/src/HtmlUtils.js @@ -163,6 +163,7 @@ var sanitizeHtmlParams = { /#[0-9a-fA-F]{6}/.test(customAttributeValue) ) { style += cssAttributeKey + ":" + customAttributeValue + ";"; + delete attribs[customAttributeKey]; } }); From 0f8ab99158a9565c714132c2b39b07deeab26f41 Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Thu, 2 Mar 2017 17:02:00 +0000 Subject: [PATCH 7/8] Have COLOR_REGEX constant --- src/HtmlUtils.js | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/HtmlUtils.js b/src/HtmlUtils.js index dc167ce5c7..d3d70f5cd5 100644 --- a/src/HtmlUtils.js +++ b/src/HtmlUtils.js @@ -28,11 +28,7 @@ emojione.imagePathSVG = 'emojione/svg/'; emojione.imageType = 'svg'; const EMOJI_REGEX = new RegExp(emojione.unicodeRegexp+"+", "gi"); -const COLOR_REGEX = /^[#a-z0-9]+$/; -const ALLOWED_CSS = { - "background-color": COLOR_REGEX, - "color": COLOR_REGEX, -}; +const COLOR_REGEX = /#[0-9a-fA-F]{6}/; /* modified from https://github.com/Ranks/emojione/blob/master/lib/js/emojione.js * because we want to include emoji shortnames in title text @@ -160,7 +156,7 @@ var sanitizeHtmlParams = { const customAttributeValue = attribs[customAttributeKey]; if (customAttributeValue && typeof customAttributeValue === 'string' && - /#[0-9a-fA-F]{6}/.test(customAttributeValue) + COLOR_REGEX.test(customAttributeValue) ) { style += cssAttributeKey + ":" + customAttributeValue + ";"; delete attribs[customAttributeKey]; From f4278b61ea4ea8648116717e6d963f52d055c784 Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Thu, 2 Mar 2017 18:13:01 +0000 Subject: [PATCH 8/8] Update comment --- src/HtmlUtils.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/HtmlUtils.js b/src/HtmlUtils.js index d3d70f5cd5..04be1154f2 100644 --- a/src/HtmlUtils.js +++ b/src/HtmlUtils.js @@ -139,7 +139,8 @@ var sanitizeHtmlParams = { return { tagName: tagName, attribs : attribs }; }, '*': function(tagName, attribs) { - // Delete any style previously assigned + // Delete any style previously assigned, style is an allowedTag for font and span + // because attributes are stripped after transforming delete attribs.style; // Sanitise and transform data-mx-color and data-mx-bg-color to their CSS