From 2df43c07779137d1bddf7f3b282a1287a8634acd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Go=C5=82e=CC=A8biowski-Owczarek?= Date: Tue, 19 May 2020 14:25:02 +0200 Subject: [PATCH] fix(jqLite): prevent possible XSS due to regex-based HTML replacement This also splits the wrapping logic to one for modern browsers & one for IE 9 as IE 9 has restrictions that make it impossible to make it as secure. --- src/.eslintrc.json | 1 + src/Angular.js | 21 ++++++++++ src/AngularPublic.js | 1 + src/jqLite.js | 73 ++++++++++++++++++++++++++------- test/jqLiteSpec.js | 96 ++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 177 insertions(+), 15 deletions(-) diff --git a/src/.eslintrc.json b/src/.eslintrc.json index dca5e4b006ef..36b653a76649 100644 --- a/src/.eslintrc.json +++ b/src/.eslintrc.json @@ -100,6 +100,7 @@ "VALIDITY_STATE_PROPERTY": false, "reloadWithDebugInfo": false, "stringify": false, + "UNSAFE_restoreLegacyJqLiteXHTMLReplacement": false, "NODE_TYPE_ELEMENT": false, "NODE_TYPE_ATTRIBUTE": false, diff --git a/src/Angular.js b/src/Angular.js index 8ce4f46347d3..5df6836d9083 100644 --- a/src/Angular.js +++ b/src/Angular.js @@ -93,6 +93,7 @@ hasOwnProperty, createMap, stringify, + UNSAFE_restoreLegacyJqLiteXHTMLReplacement, NODE_TYPE_ELEMENT, NODE_TYPE_ATTRIBUTE, @@ -1949,6 +1950,26 @@ function bindJQuery() { bindJQueryFired = true; } +/** + * @ngdoc function + * @name angular.UNSAFE_restoreLegacyJqLiteXHTMLReplacement + * @module ng + * @kind function + * + * @description + * Restores the pre-1.8 behavior of jqLite that turns XHTML-like strings like + * `
` to `
` instead of `
`. + * The new behavior is a security fix so if you use this method, please try to adjust + * to the change & remove the call as soon as possible. + + * Note that this only patches jqLite. If you use jQuery 3.5.0 or newer, please read + * [jQuery 3.5 upgrade guide](https://jquery.com/upgrade-guide/3.5/) for more details + * about the workarounds. + */ +function UNSAFE_restoreLegacyJqLiteXHTMLReplacement() { + JQLite.legacyXHTMLReplacement = true; +} + /** * throw error if the argument is falsy. */ diff --git a/src/AngularPublic.js b/src/AngularPublic.js index 760fa835bd48..7f89362fed9b 100644 --- a/src/AngularPublic.js +++ b/src/AngularPublic.js @@ -156,6 +156,7 @@ function publishExternalAPI(angular) { 'callbacks': {$$counter: 0}, 'getTestability': getTestability, 'reloadWithDebugInfo': reloadWithDebugInfo, + 'UNSAFE_restoreLegacyJqLiteXHTMLReplacement': UNSAFE_restoreLegacyJqLiteXHTMLReplacement, '$$minErr': minErr, '$$csp': csp, '$$encodeUriSegment': encodeUriSegment, diff --git a/src/jqLite.js b/src/jqLite.js index 9884730d5e13..805fd23c9b5c 100644 --- a/src/jqLite.js +++ b/src/jqLite.js @@ -90,6 +90,16 @@ * - [`val()`](http://api.jquery.com/val/) * - [`wrap()`](http://api.jquery.com/wrap/) * + * jqLite also provides a method restoring pre-1.8 insecure treatment of XHTML-like tags + * that makes input like `
` turned to `
` instead of + * `
` like version 1.8 & newer do: + * ```js + * angular.UNSAFE_restoreLegacyJqLiteXHTMLReplacement(); + * ``` + * Note that this only patches jqLite. If you use jQuery 3.5.0 or newer, please read + * [jQuery 3.5 upgrade guide](https://jquery.com/upgrade-guide/3.5/) for more details + * about the workarounds. + * * ## jQuery/jqLite Extras * AngularJS also provides the following additional methods and events to both jQuery and jqLite: * @@ -169,20 +179,36 @@ var HTML_REGEXP = /<|&#?\w+;/; var TAG_NAME_REGEXP = /<([\w:-]+)/; var XHTML_TAG_REGEXP = /<(?!area|br|col|embed|hr|img|input|link|meta|param)(([\w:-]+)[^>]*)\/>/gi; +// Table parts need to be wrapped with `` or they're +// stripped to their contents when put in a div. +// XHTML parsers do not magically insert elements in the +// same way that tag soup parsers do, so we cannot shorten +// this by omitting or other required elements. var wrapMap = { - 'option': [1, ''], - - 'thead': [1, '
', '
'], - 'col': [2, '', '
'], - 'tr': [2, '', '
'], - 'td': [3, '', '
'], - '_default': [0, '', ''] + thead: ['table'], + col: ['colgroup', 'table'], + tr: ['tbody', 'table'], + td: ['tr', 'tbody', 'table'] }; -wrapMap.optgroup = wrapMap.option; wrapMap.tbody = wrapMap.tfoot = wrapMap.colgroup = wrapMap.caption = wrapMap.thead; wrapMap.th = wrapMap.td; +// Support: IE <10 only +// IE 9 requires an option wrapper & it needs to have the whole table structure +// set up up front; assigning `""` to `tr.innerHTML` doesn't work, etc. +var wrapMapIE9 = { + option: [1, ''], + _default: [0, '', ''] +}; + +for (var key in wrapMap) { + var wrapMapValueClosing = wrapMap[key]; + var wrapMapValue = wrapMapValueClosing.slice().reverse(); + wrapMapIE9[key] = [wrapMapValue.length, '<' + wrapMapValue.join('><') + '>', '']; +} + +wrapMapIE9.optgroup = wrapMapIE9.option; function jqLiteIsTextNode(html) { return !HTML_REGEXP.test(html); @@ -203,7 +229,7 @@ function jqLiteHasData(node) { } function jqLiteBuildFragment(html, context) { - var tmp, tag, wrap, + var tmp, tag, wrap, finalHtml, fragment = context.createDocumentFragment(), nodes = [], i; @@ -214,13 +240,30 @@ function jqLiteBuildFragment(html, context) { // Convert html into DOM nodes tmp = fragment.appendChild(context.createElement('div')); tag = (TAG_NAME_REGEXP.exec(html) || ['', ''])[1].toLowerCase(); - wrap = wrapMap[tag] || wrapMap._default; - tmp.innerHTML = wrap[1] + html.replace(XHTML_TAG_REGEXP, '<$1>') + wrap[2]; + finalHtml = JQLite.legacyXHTMLReplacement ? + html.replace(XHTML_TAG_REGEXP, '<$1>') : + html; + + if (msie < 10) { + wrap = wrapMapIE9[tag] || wrapMapIE9._default; + tmp.innerHTML = wrap[1] + finalHtml + wrap[2]; + + // Descend through wrappers to the right content + i = wrap[0]; + while (i--) { + tmp = tmp.firstChild; + } + } else { + wrap = wrapMap[tag] || []; - // Descend through wrappers to the right content - i = wrap[0]; - while (i--) { - tmp = tmp.lastChild; + // Create wrappers & descend into them + i = wrap.length; + while (--i > -1) { + tmp.appendChild(window.document.createElement(wrap[i])); + tmp = tmp.firstChild; + } + + tmp.innerHTML = finalHtml; } nodes = concat(nodes, tmp.childNodes); diff --git a/test/jqLiteSpec.js b/test/jqLiteSpec.js index 14d60900611a..fbf6949cf991 100644 --- a/test/jqLiteSpec.js +++ b/test/jqLiteSpec.js @@ -147,6 +147,13 @@ describe('jqLite', function() { expect(nodes[0].nodeName.toLowerCase()).toBe('option'); }); + it('should allow construction of multiple '); + expect(nodes.length).toBe(2); + expect(nodes[0].nodeName.toLowerCase()).toBe('option'); + expect(nodes[1].nodeName.toLowerCase()).toBe('option'); + }); + // Special tests for the construction of elements which are restricted (in the HTML5 spec) to // being children of specific nodes. @@ -169,6 +176,95 @@ describe('jqLite', function() { expect(nodes[0].nodeName.toLowerCase()).toBe(name); }); }); + + describe('security', function() { + it('shouldn\'t crash at attempts to close the table wrapper', function() { + // jQuery doesn't pass this test yet. + if (!_jqLiteMode) return; + + // Support: IE <10 + // In IE 9 we still need to use the old-style innerHTML assignment + // as that's the only one that works. + if (msie < 10) return; + + expect(function() { + // This test case attempts to close the tags which wrap input + // based on matching done in wrapMap, escaping the wrapper & thus + // triggering an error when descending. + var el = jqLite(''); + expect(el.length).toBe(2); + expect(el[0].nodeName.toLowerCase()).toBe('td'); + expect(el[1].nodeName.toLowerCase()).toBe('td'); + }).not.toThrow(); + }); + + it('shouldn\'t unsanitize sanitized code', function(done) { + // jQuery <3.5.0 fail those tests. + if (isJQuery2x()) { + done(); + return; + } + + var counter = 0, + assertCount = 13, + container = jqLite('
'); + + function donePartial() { + counter++; + if (counter === assertCount) { + container.remove(); + delete window.xss; + done(); + } + } + + jqLite(document.body).append(container); + window.xss = jasmine.createSpy('xss'); + + // Thanks to Masato Kinugawa from Cure53 for providing the following test cases. + // Note: below test cases need to invoke the xss function with consecutive + // decimal parameters for the assertions to be correct. + forEach([ + '<x', + '\n<x', + '' + ], function(htmlString, index) { + var element = jqLite('
'); + + container.append(element); + element.append(jqLite(htmlString)); + + window.setTimeout(function() { + expect(window.xss).not.toHaveBeenCalledWith(index); + donePartial(); + }, 1000); + }); + }); + + it('should allow to restore legacy insecure behavior', function() { + // jQuery doesn't have this API. + if (!_jqLiteMode) return; + + // eslint-disable-next-line new-cap + angular.UNSAFE_restoreLegacyJqLiteXHTMLReplacement(); + + var elem = jqLite('
'); + expect(elem.length).toBe(2); + expect(elem[0].nodeName.toLowerCase()).toBe('div'); + expect(elem[1].nodeName.toLowerCase()).toBe('span'); + }); + }); }); describe('_data', function() {