From d5cbcef0ea8e3ca2c53fa7167e03428538b34bc1 Mon Sep 17 00:00:00 2001
From: Shino Kurian <shino.kurian@yahoo.com>
Date: Sat, 8 Sep 2018 23:22:24 -0700
Subject: [PATCH] fix(core): ignore comment nodes under unsafe elements
 (#25879)

Comment nodes that are child nodes of unsafe elements are identified as text nodes. This results in the comment node being returned as an encoded string.
Add a check to ignore such comment nodes.

PR Close #25879
---
 .../core/src/sanitization/html_sanitizer.ts   | 18 ++++++--
 .../test/linker/security_integration_spec.ts  |  2 +-
 .../test/sanitization/html_sanitizer_spec.ts  | 46 +++++++++++++++----
 3 files changed, 53 insertions(+), 13 deletions(-)

diff --git a/packages/core/src/sanitization/html_sanitizer.ts b/packages/core/src/sanitization/html_sanitizer.ts
index 2c45e72693..6dd295f135 100644
--- a/packages/core/src/sanitization/html_sanitizer.ts
+++ b/packages/core/src/sanitization/html_sanitizer.ts
@@ -98,16 +98,17 @@ class SanitizingHtmlSerializer {
     // However this code never accesses properties off of `document` before deleting its contents
     // again, so it shouldn't be vulnerable to DOM clobbering.
     let current: Node = el.firstChild !;
+    let elementValid = true;
     while (current) {
       if (current.nodeType === Node.ELEMENT_NODE) {
-        this.startElement(current as Element);
+        elementValid = this.startElement(current as Element);
       } else if (current.nodeType === Node.TEXT_NODE) {
         this.chars(current.nodeValue !);
       } else {
         // Strip non-element, non-text nodes.
         this.sanitizedSomething = true;
       }
-      if (current.firstChild) {
+      if (elementValid && current.firstChild) {
         current = current.firstChild !;
         continue;
       }
@@ -130,11 +131,19 @@ class SanitizingHtmlSerializer {
     return this.buf.join('');
   }
 
-  private startElement(element: Element) {
+  /**
+   * Outputs only valid Elements.
+   *
+   * Invalid elements are skipped.
+   *
+   * @param element element to sanitize
+   * Returns true if the element is valid.
+   */
+  private startElement(element: Element): boolean {
     const tagName = element.nodeName.toLowerCase();
     if (!VALID_ELEMENTS.hasOwnProperty(tagName)) {
       this.sanitizedSomething = true;
-      return;
+      return false;
     }
     this.buf.push('<');
     this.buf.push(tagName);
@@ -154,6 +163,7 @@ class SanitizingHtmlSerializer {
       this.buf.push(' ', attrName, '="', encodeEntities(value), '"');
     }
     this.buf.push('>');
+    return true;
   }
 
   private endElement(current: Element) {
diff --git a/packages/core/test/linker/security_integration_spec.ts b/packages/core/test/linker/security_integration_spec.ts
index dede02e5a1..7c65aa8840 100644
--- a/packages/core/test/linker/security_integration_spec.ts
+++ b/packages/core/test/linker/security_integration_spec.ts
@@ -239,7 +239,7 @@ function declareTests({useJit}: {useJit: boolean}) {
 
         ci.ctxProp = 'ha <script>evil()</script>';
         fixture.detectChanges();
-        expect(getDOM().getInnerHTML(e)).toEqual('ha evil()');
+        expect(getDOM().getInnerHTML(e)).toEqual('ha ');
 
         ci.ctxProp = 'also <img src="x" onerror="evil()"> evil';
         fixture.detectChanges();
diff --git a/packages/core/test/sanitization/html_sanitizer_spec.ts b/packages/core/test/sanitization/html_sanitizer_spec.ts
index ab9cf05313..5f64b0abee 100644
--- a/packages/core/test/sanitization/html_sanitizer_spec.ts
+++ b/packages/core/test/sanitization/html_sanitizer_spec.ts
@@ -36,8 +36,8 @@ import {_sanitizeHtml} from '../../src/sanitization/html_sanitizer';
           .toEqual('<p>Hello <br> World</p>');
     });
 
-    it('supports namespaced elements',
-       () => { expect(_sanitizeHtml(defaultDoc, 'a<my:hr/><my:div>b</my:div>c')).toEqual('abc'); });
+    it('supports removal of namespaced elements',
+       () => { expect(_sanitizeHtml(defaultDoc, 'a<my:hr/><my:div>b</my:div>c')).toEqual('a'); });
 
     it('supports namespaced attributes', () => {
       expect(_sanitizeHtml(defaultDoc, '<a xlink:href="something">t</a>'))
@@ -85,15 +85,37 @@ import {_sanitizeHtml} from '../../src/sanitization/html_sanitizer';
           .toEqual('<p alt="% &amp; &#34; !">Hello</p>');  // NB: quote encoded as ASCII &#34;.
     });
 
-    describe('should strip dangerous elements', () => {
+    describe('should strip dangerous elements and its content', () => {
       const dangerousTags = [
-        'frameset', 'form', 'param', 'object', 'embed', 'textarea', 'input', 'button', 'option',
-        'select', 'script', 'style', 'link', 'base', 'basefont'
+        'form',
+        'object',
+        'textarea',
+        'button',
+        'option',
+        'select',
+        'script',
+        'style',
       ];
 
       for (const tag of dangerousTags) {
         it(`${tag}`,
-           () => { expect(_sanitizeHtml(defaultDoc, `<${tag}>evil!</${tag}>`)).toEqual('evil!'); });
+           () => { expect(_sanitizeHtml(defaultDoc, `<${tag}>evil!</${tag}>`)).toEqual(''); });
+      }
+      const dangerousSelfClosingTags = [
+        'frameset',
+        'embed',
+        'input',
+        'param',
+        'base',
+        'basefont',
+        'param',
+        'link',
+      ];
+
+      for (const tag of dangerousSelfClosingTags) {
+        it(`${tag}`, () => {
+          expect(_sanitizeHtml(defaultDoc, `before<${tag}>After`)).toEqual('beforeAfter');
+        });
       }
 
       it(`swallows frame entirely`, () => {
@@ -111,6 +133,14 @@ import {_sanitizeHtml} from '../../src/sanitization/html_sanitizer';
       }
     });
 
+    it('ignores content of style elements', () => {
+      expect(_sanitizeHtml(defaultDoc, '<style><!-- foobar --></style><div>hi</div>'))
+          .toEqual('<div>hi</div>');
+      expect(_sanitizeHtml(defaultDoc, '<style><!-- foobar --></style>')).toEqual('');
+      expect(_sanitizeHtml(defaultDoc, '<style>\<\!-- something--\>hi</style>')).toEqual('');
+      expect(logMsgs.join('\n')).toMatch(/sanitizing HTML stripped some content/);
+    });
+
     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
@@ -154,9 +184,9 @@ import {_sanitizeHtml} from '../../src/sanitization/html_sanitizer';
              .toEqual(
                  isDOMParserAvailable() ?
                      // PlatformBrowser output
-                     '<p>&lt;img src=&#34;<img src="x"></p>' :
+                     '<p><img src="x"></p>' :
                      // PlatformServer output
-                     '<p><img src="&lt;/style&gt;&lt;img src=x onerror=alert(1)//"></p>');
+                     '');
        });
 
     if (browserDetection.isWebkit) {