From a4076c70ccb85cdbb3b38ca33d366746120178e5 Mon Sep 17 00:00:00 2001 From: Victor Berchet Date: Tue, 14 Mar 2017 17:15:46 -0700 Subject: [PATCH] fix(platform-browser): prevent clobbered elements from freezing the browser see https://github.com/angular/angular.js/commit/4f69d38f097fab76e683105d1c758706e6cbe1a9 --- .../src/browser/browser_adapter.ts | 9 ++++++++ .../platform-browser/src/dom/dom_adapter.ts | 1 + .../src/security/html_sanitizer.ts | 21 +++++++++++++----- .../test/security/html_sanitizer_spec.ts | 22 +++++++++++++++++++ .../platform-server/src/parse5_adapter.ts | 9 ++++++++ .../src/web_workers/worker/worker_adapter.ts | 1 + 6 files changed, 58 insertions(+), 5 deletions(-) diff --git a/packages/platform-browser/src/browser/browser_adapter.ts b/packages/platform-browser/src/browser/browser_adapter.ts index f1c8f3e27b..4cfc739934 100644 --- a/packages/platform-browser/src/browser/browser_adapter.ts +++ b/packages/platform-browser/src/browser/browser_adapter.ts @@ -61,6 +61,14 @@ const _chromeNumKeyPadMap = { '\x90': 'NumLock' }; +let nodeContains: (a: any, b: any) => boolean; + +if (global['Node']) { + nodeContains = global['Node'].prototype.contains || function(node) { + return !!(this.compareDocumentPosition(node) & 16); + }; +} + /** * A `DomAdapter` powered by full browser DOM APIs. * @@ -107,6 +115,7 @@ export class BrowserDomAdapter extends GenericBrowserDomAdapter { get attrToPropMap(): any { return _attrToPropMap; } + contains(nodeA: any, nodeB: any): boolean { return nodeContains.call(nodeA, nodeB); } querySelector(el: Element, selector: string): any { return el.querySelector(selector); } querySelectorAll(el: any, selector: string): any[] { return el.querySelectorAll(selector); } on(el: Node, evt: any, listener: any) { el.addEventListener(evt, listener, false); } diff --git a/packages/platform-browser/src/dom/dom_adapter.ts b/packages/platform-browser/src/dom/dom_adapter.ts index 136b079b44..22e4334640 100644 --- a/packages/platform-browser/src/dom/dom_adapter.ts +++ b/packages/platform-browser/src/dom/dom_adapter.ts @@ -52,6 +52,7 @@ export abstract class DomAdapter { /** @internal */ _attrToPropMap: {[key: string]: string}; + abstract contains(nodeA: any, nodeB: any): boolean; abstract parse(templateHtml: string): any; abstract querySelector(el: any, selector: string): any; abstract querySelectorAll(el: any, selector: string): any[]; diff --git a/packages/platform-browser/src/security/html_sanitizer.ts b/packages/platform-browser/src/security/html_sanitizer.ts index 1e4db7db28..338c801768 100644 --- a/packages/platform-browser/src/security/html_sanitizer.ts +++ b/packages/platform-browser/src/security/html_sanitizer.ts @@ -9,7 +9,6 @@ import {isDevMode} from '@angular/core'; import {DomAdapter, getDOM} from '../dom/dom_adapter'; -import {DOCUMENT} from '../dom/dom_tokens'; import {sanitizeSrcset, sanitizeUrl} from './url_sanitizer'; @@ -146,11 +145,15 @@ class SanitizingHtmlSerializer { if (DOM.isElementNode(current)) { this.endElement(current as Element); } - if (DOM.nextSibling(current)) { - current = DOM.nextSibling(current); + + let next = checkClobberedElement(current, DOM.nextSibling(current)); + + if (next) { + current = next; break; } - current = DOM.parentElement(current); + + current = checkClobberedElement(current, DOM.parentElement(current)); } } return this.buf.join(''); @@ -191,7 +194,15 @@ class SanitizingHtmlSerializer { } } - private chars(chars: any /** TODO #9100 */) { this.buf.push(encodeEntities(chars)); } + private chars(chars: string) { this.buf.push(encodeEntities(chars)); } +} + +function checkClobberedElement(node: Node, nextNode: Node): Node { + if (nextNode && DOM.contains(node, nextNode)) { + throw new Error( + `Failed to sanitize html because the element is clobbered: ${DOM.getOuterHTML(node)}`); + } + return nextNode; } // Regular Expressions for parsing tags and attributes diff --git a/packages/platform-browser/test/security/html_sanitizer_spec.ts b/packages/platform-browser/test/security/html_sanitizer_spec.ts index c5bf35987c..bf8a0b16f9 100644 --- a/packages/platform-browser/test/security/html_sanitizer_spec.ts +++ b/packages/platform-browser/test/security/html_sanitizer_spec.ts @@ -112,6 +112,28 @@ export function main() { } }); + it('should not enter an infinite loop on clobbered elements', () => { + // Some browsers are vulnerable to clobbered elements and will throw an expected exception + // IE and EDGE does not seems to be affected by those cases + // Anyway what we want to test is that browsers do not enter an infinite loop which would + // result in a timeout error for the test. + try { + sanitizeHtml(defaultDoc, '
'); + } catch (e) { + // depending on the browser, we might ge an exception + } + try { + sanitizeHtml(defaultDoc, '
') + } catch (e) { + // depending on the browser, we might ge an exception + } + try { + sanitizeHtml(defaultDoc, '
'); + } catch (e) { + // depending on the browser, we might ge an exception + } + }); + if (browserDetection.isWebkit) { it('should prevent mXSS attacks', function() { expect(sanitizeHtml(defaultDoc, 'CLICKME')) diff --git a/packages/platform-server/src/parse5_adapter.ts b/packages/platform-server/src/parse5_adapter.ts index db4f7d65fd..9e76abd5ae 100644 --- a/packages/platform-server/src/parse5_adapter.ts +++ b/packages/platform-server/src/parse5_adapter.ts @@ -63,6 +63,15 @@ export class Parse5DomAdapter extends DomAdapter { setRootDomAdapter(new Parse5DomAdapter()); } + contains(nodeA: any, nodeB: any): boolean { + let inner = nodeB; + while (inner) { + if (inner === nodeA) return true; + inner = inner.parent; + } + return false; + } + hasProperty(element: any, name: string): boolean { return _HTMLElementPropertyList.indexOf(name) > -1; } diff --git a/packages/platform-webworker/src/web_workers/worker/worker_adapter.ts b/packages/platform-webworker/src/web_workers/worker/worker_adapter.ts index 6cfc957ec2..c8509b6e3d 100644 --- a/packages/platform-webworker/src/web_workers/worker/worker_adapter.ts +++ b/packages/platform-webworker/src/web_workers/worker/worker_adapter.ts @@ -44,6 +44,7 @@ export class WorkerDomAdapter extends DomAdapter { } } + contains(nodeA: any, nodeB: any): boolean { throw 'not implemented'; } hasProperty(element: any, name: string): boolean { throw 'not implemented'; } setProperty(el: Element, name: string, value: any) { throw 'not implemented'; } getProperty(el: Element, name: string): any { throw 'not implemented'; }