Skip to content

Commit

Permalink
TINY-6518: Fixed a security issue where urls in attributes weren't co…
Browse files Browse the repository at this point in the history 
…rrectly sanitized (#6179)
  • Loading branch information
@lnewson
lnewson committed Nov 5, 2020
1 parent 9a494e3 commit 4d43978
Show file tree
Hide file tree
Showing 6 changed files with 112 additions and 13 deletions.
1 change: 1 addition & 0 deletions modules/tinymce/changelog.txt
View file
Expand Up @@ -5,6 +5,7 @@ Version 5.6.0 (TBD)
Added new `closest` formatter API to get the closest matching selection format from a set of formats. #TINY-6479
Added new `name` field to the `style_formats` setting object to enable specifying a name for the format. #TINY-4239
Changed `readonly` mode to allow hyperlinks to be clickable #TINY-6248
Fixed a security issue where URLs in attributes weren't correctly sanitized #TINY-6518
Fixed `DOMUtils.getParents` incorrectly including the shadow root in the array of elements returned #TINY-6540
Fixed an issue where the root document could be scrolled while an editor dialog was open inside a shadow root #TINY-6363
Fixed `getContent` with text format returning a new line when the editor is empty #TINY-6281
Expand Down
1 change: 1 addition & 0 deletions modules/tinymce/src/core/main/ts/api/SettingsTypes.ts
View file
Expand Up @@ -45,6 +45,7 @@ interface BaseEditorSettings {
allow_html_data_urls?: boolean;
allow_html_in_named_anchor?: boolean;
allow_script_urls?: boolean;
allow_svg_data_urls?: boolean;
allow_unsafe_link_target?: boolean;
anchor_bottom?: false | string;
anchor_top?: false | string;
Expand Down
2 changes: 2 additions & 0 deletions modules/tinymce/src/core/main/ts/api/html/DomParser.ts
View file
Expand Up @@ -49,6 +49,7 @@ export interface ParserFilter {

export interface DomParserSettings {
allow_html_data_urls?: boolean;
allow_svg_data_urls?: boolean;
allow_conditional_comments?: boolean;
allow_html_in_named_anchor?: boolean;
allow_script_urls?: boolean;
Expand Down Expand Up @@ -477,6 +478,7 @@ const DomParser = function (settings?: DomParserSettings, schema = Schema()): Do
const parser = SaxParser({
validate,
allow_html_data_urls: settings.allow_html_data_urls,
allow_svg_data_urls: settings.allow_svg_data_urls,
allow_script_urls: settings.allow_script_urls,
allow_conditional_comments: settings.allow_conditional_comments,
preserve_cdata: settings.preserve_cdata,
Expand Down
27 changes: 19 additions & 8 deletions modules/tinymce/src/core/main/ts/api/html/SaxParser.ts
View file
Expand Up @@ -5,7 +5,7 @@
* For commercial licenses see https://www.tiny.cloud/
*/

import { Obj, Strings } from '@ephox/katamari';
import { Arr, Obj, Strings, Type } from '@ephox/katamari';
import { Base64Extract, extractBase64DataUris, restoreDataUris } from '../../html/Base64Uris';
import Tools from '../util/Tools';
import Entities from './Entities';
Expand Down Expand Up @@ -97,13 +97,21 @@ const enum MatchType {
Attribute = 9
}

const safeSvgDataUrlElements = [ 'img', 'video' ];

const isValidPrefixAttrName = (name: string): boolean => name.indexOf('data-') === 0 || name.indexOf('aria-') === 0;

const isInvalidUri = (settings: SaxParserSettings, uri: string) => {
const blockSvgDataUris = (allowSvgDataUrls: boolean | undefined, tagName: string) => {
// Only allow SVGs by default on images/videos since the browser won't execute scripts on those elements
const allowed = Type.isNullable(allowSvgDataUrls) ? Arr.contains(safeSvgDataUrlElements, tagName) : allowSvgDataUrls;
return !allowed;
};

const isInvalidUri = (settings: SaxParserSettings, uri: string, tagName: string) => {
if (settings.allow_html_data_urls) {
return false;
} else if (/^data:image\//i.test(uri)) {
return settings.allow_svg_data_urls === false && /^data:image\/svg\+xml/i.test(uri);
return blockSvgDataUris(settings.allow_svg_data_urls, tagName) && /^data:image\/svg\+xml/i.test(uri);
} else {
return /^data:/i.test(uri);
}
Expand Down Expand Up @@ -208,11 +216,11 @@ function SaxParser(settings?: SaxParserSettings, schema = Schema()): SaxParser {

const parseInternal = (base64Extract: Base64Extract, format: ParserFormat = 'html') => {
const html = base64Extract.html;
let matches, index = 0, value, endRegExp;
let matches: RegExpExecArray, index = 0, value, endRegExp;
const stack = [];
let attrList, i, textData, name;
let isInternalElement, isShortEnded;
let elementRule, isValidElement, attr, attribsValue, validAttributesMap, validAttributePatterns;
let elementRule, isValidElement, attr, attribsValue: string, validAttributesMap, validAttributePatterns;
let attributesRequired, attributesDefault, attributesForced;
let anyAttributesRequired, attrValue, idCount = 0;
const decode = Entities.decode;
Expand Down Expand Up @@ -282,7 +290,7 @@ function SaxParser(settings?: SaxParserSettings, schema = Schema()): SaxParser {
return endIndex + 1;
};

const parseAttribute = (match: string, name: string, value?: string, val2?: string, val3?: string) => {
const parseAttribute = (tagName: string, name: string, value?: string, val2?: string, val3?: string) => {
let attrRule, i;
const trimRegExp = /[\s\u0000-\u001F]+/g;

Expand Down Expand Up @@ -336,7 +344,7 @@ function SaxParser(settings?: SaxParserSettings, schema = Schema()): SaxParser {
return;
}

if (isInvalidUri(settings, uri)) {
if (isInvalidUri(settings, uri, tagName)) {
return;
}
}
Expand Down Expand Up @@ -451,7 +459,10 @@ function SaxParser(settings?: SaxParserSettings, schema = Schema()): SaxParser {
attrList = [];
attrList.map = {};

attribsValue.replace(attrRegExp, parseAttribute);
attribsValue.replace(attrRegExp, (match, name, val, val2, val3) => {
parseAttribute(value, name, val, val2, val3);
return '';
});
} else {
attrList = [];
attrList.map = {};
Expand Down
1 change: 1 addition & 0 deletions modules/tinymce/src/core/main/ts/init/InitContentBody.ts
View file
Expand Up @@ -70,6 +70,7 @@ const mkParserSettings = (editor: Editor): DomParserSettings => {
return removeUndefined<DomParserSettings>({
allow_conditional_comments: settings.allow_conditional_comments,
allow_html_data_urls: settings.allow_html_data_urls,
allow_svg_data_urls: settings.allow_svg_data_urls,
allow_html_in_named_anchor: settings.allow_html_in_named_anchor,
allow_script_urls: settings.allow_script_urls,
allow_unsafe_link_target: settings.allow_unsafe_link_target,
Expand Down
93 changes: 88 additions & 5 deletions modules/tinymce/src/core/test/ts/browser/html/SaxParserTest.ts
View file
Expand Up @@ -719,7 +719,8 @@ UnitTest.asynctest('browser.tinymce.core.html.SaxParserTest', function (success,
);
LegacyUnit.equal(
writer.getContent(),
'<a href="javascript:alert(1)">1</a><a href=" 2 ">2</a>' +
'<a href="javascript:alert(1)">1</a>' +
'<a href=" 2 ">2</a>' +
'<a href="data:text/html;base64,PHN2Zy9vbmxvYWQ9YWxlcnQoMik+">3</a>'
);
});
Expand Down Expand Up @@ -751,11 +752,13 @@ UnitTest.asynctest('browser.tinymce.core.html.SaxParserTest', function (success,
const parser = SaxParser(counter, schema);
writer.reset();
parser.parse(
'<a href="data:image/svg+xml;base64,x">1</a>'
'<a href="data:image/svg+xml;base64,x">1</a>' +
'<img src="data:image/svg+xml;base64,x">'
);
LegacyUnit.equal(
writer.getContent(),
'<a>1</a>'
'<a>1</a>' +
'<img />'
);
});

Expand Down Expand Up @@ -786,13 +789,93 @@ UnitTest.asynctest('browser.tinymce.core.html.SaxParserTest', function (success,

LegacyUnit.equal(
writer.getContent(),
'<a>1</a><a>2</a><a>3</a><a>4</a><a>5</a><a>6</a><a>7</a><a>8</a><a>9</a>' +
'<object>10</object><button>11</button><table><tr></tr><tr>12</tr></table><a>13</a><a>14</a>' +
'<a>1</a>' +
'<a>2</a>' +
'<a>3</a>' +
'<a>4</a>' +
'<a>5</a>' +
'<a>6</a>' +
'<a>7</a>' +
'<a>8</a>' +
'<a>9</a>' +
'<object>10</object>' +
'<button>11</button>' +
'<table><tr></tr><tr>12</tr></table>' +
'<a>13</a>' +
'<a>14</a>' +
'<img src="data:image/gif;base64,R0lGODlhAQABAIAAAAAAAP///yH5BAEAAAAALAAAAAABAAEAAAIBRAA7" />' +
'<a href="%E3%82%AA%E3%83%BC%E3%83">Invalid url</a>'
);
});

suite.test('Parse svg urls (default)', function () {
const counter = createCounter(writer);
counter.validate = false;
const parser = SaxParser(counter, schema);
writer.reset();
parser.parse(
'<iframe src="data:image/svg+xml;base64,x"></iframe>' +
'<a href="data:image/svg+xml;base64,x">1</a>' +
'<object data="data:image/svg+xml;base64,x"></object>' +
'<img src="data:image/svg+xml;base64,x">' +
'<video poster="data:image/svg+xml;base64,x"></video>'
);
LegacyUnit.equal(
writer.getContent(),
'<iframe></iframe>' +
'<a>1</a>' +
'<object></object>' +
'<img src="data:image/svg+xml;base64,x" />' +
'<video poster="data:image/svg+xml;base64,x"></video>'
);
});

suite.test('Parse svg urls (allowed)', function () {
const counter = createCounter(writer);
counter.validate = false;
counter.allow_svg_data_urls = true;
const parser = SaxParser(counter, schema);
writer.reset();
parser.parse(
'<iframe src="data:image/svg+xml;base64,x"></iframe>' +
'<a href="data:image/svg+xml;base64,x">1</a>' +
'<object data="data:image/svg+xml;base64,x"></object>' +
'<img src="data:image/svg+xml;base64,x">' +
'<video poster="data:image/svg+xml;base64,x"></video>'
);
LegacyUnit.equal(
writer.getContent(),
'<iframe src="data:image/svg+xml;base64,x"></iframe>' +
'<a href="data:image/svg+xml;base64,x">1</a>' +
'<object data="data:image/svg+xml;base64,x"></object>' +
'<img src="data:image/svg+xml;base64,x" />' +
'<video poster="data:image/svg+xml;base64,x"></video>'
);
});

suite.test('Parse svg urls (denied)', function () {
const counter = createCounter(writer);
counter.validate = false;
counter.allow_svg_data_urls = false;
const parser = SaxParser(counter, schema);
writer.reset();
parser.parse(
'<iframe src="data:image/svg+xml;base64,x"></iframe>' +
'<a href="data:image/svg+xml;base64,x">1</a>' +
'<object data="data:image/svg+xml;base64,x"></object>' +
'<img src="data:image/svg+xml;base64,x">' +
'<video poster="data:image/svg+xml;base64,x"></video>'
);
LegacyUnit.equal(
writer.getContent(),
'<iframe></iframe>' +
'<a>1</a>' +
'<object></object>' +
'<img />' +
'<video></video>'
);
});

suite.test('Parse away bogus elements', function () {
const testBogusSaxParse = function (inputHtml, outputHtml, counters) {
const counter = createCounter(writer);
Expand Down

0 comments on commit 4d43978

Please sign in to comment.