Xss fix

packages/core/src/render3/instructions/shared.ts

@@ -13,6 +13,7 @@ import {ViewEncapsulation} from '../../metadata/view';
 import {validateAgainstEventAttributes, validateAgainstEventProperties} from '../../sanitization/sanitization';
 import {Sanitizer} from '../../sanitization/sanitizer';
 import {assertDefined, assertDomNode, assertEqual, assertGreaterThanOrEqual, assertIndexInRange, assertNotEqual, assertNotSame, assertSame, assertString} from '../../util/assert';
+import {escapeCommentText} from '../../util/dom';
 import {createNamedArrayType} from '../../util/named_array_type';
 import {initNgDevMode} from '../../util/ng_dev_mode';
 import {normalizeDebugBindingName, normalizeDebugBindingValue} from '../../util/ng_reflect';
@@ -1043,7 +1044,8 @@ function setNgReflectProperty(
           (element as RElement).setAttribute(attrName, debugValue);
     }
   } else {
-    const textContent = `bindings=${JSON.stringify({[attrName]: debugValue}, null, 2)}`;
+    const textContent =
+        escapeCommentText(`bindings=${JSON.stringify({[attrName]: debugValue}, null, 2)}`);
     if (isProceduralRenderer(renderer)) {
       renderer.setValue((element as RComment), textContent);
     } else {

packages/core/src/render3/node_manipulation.ts

@@ -11,6 +11,7 @@ import {Renderer2} from '../render/api';
 import {RendererStyleFlags2} from '../render/api_flags';
 import {addToArray, removeFromArray} from '../util/array_utils';
 import {assertDefined, assertDomNode, assertEqual, assertFunction, assertString} from '../util/assert';
+import {escapeCommentText} from '../util/dom';
 import {assertLContainer, assertLView, assertTNodeForLView} from './assert';
 import {attachPatchData} from './context_discovery';
 import {icuContainerIterate} from './i18n/i18n_tree_shaking';
@@ -113,7 +114,7 @@ export function createCommentNode(renderer: Renderer3, value: string): RComment
   ngDevMode && ngDevMode.rendererCreateComment++;
   // isProceduralRenderer check is not needed because both `Renderer2` and `Renderer3` have the same
   // method name.
-  return renderer.createComment(value);
+  return renderer.createComment(escapeCommentText(value));
 }
 
 /**

packages/core/src/util/dom.ts

@@ -0,0 +1,37 @@
+/**
+ * @license
+ * Copyright Google LLC All Rights Reserved.
+ *
+ * Use of this source code is governed by an MIT-style license that can be
+ * found in the LICENSE file at https://angular.io/license
+ */
+
+const END_COMMENT = /-->/g;
+const END_COMMENT_ESCAPED = '-\u200B-\u200B>';
+
+/**
+ * Escape the content of the strings so that it can be safely inserted into a comment node.
+ *
+ * The issue is that HTML does not specify any way to escape comment end text inside the comment.
+ * `<!-- The way you close a comment is with "-->". -->`. Above the `"-->"` is meant to be text not
+ * an end to the comment. This can be created programmatically through DOM APIs.
+ *
+ * ```
+ * div.innerHTML = div.innerHTML
+ * ```
+ *
+ * One would expect that the above code would be safe to do, but it turns out that because comment
+ * text is not escaped, the comment may contain text which will prematurely close the comment
+ * opening up the application for XSS attack. (In SSR we programmatically create comment nodes which
+ * may contain such text and expect them to be safe.)
+ *
+ * This function escapes the comment text by looking for the closing char sequence `-->` and replace
+ * it with `-_-_>` where the `_` is a zero width space `\u200B`. The result is that if a comment
+ * contains `-->` text it will render normally but it will not cause the HTML parser to close the
+ * comment.
+ *
+ * @param value text to make safe for comment node by escaping the comment close character sequence
+ */
+export function escapeCommentText(value: string): string {
+  return value.replace(END_COMMENT, END_COMMENT_ESCAPED);
+}

packages/core/src/view/services.ts

@@ -16,6 +16,7 @@ import {NgModuleRef} from '../linker/ng_module_factory';
 import {Renderer2, RendererFactory2} from '../render/api';
 import {RendererStyleFlags2, RendererType2} from '../render/api_flags';
 import {Sanitizer} from '../sanitization/sanitizer';
+import {escapeCommentText} from '../util/dom';
 import {isDevMode} from '../util/is_dev_mode';
 import {normalizeDebugBindingName, normalizeDebugBindingValue} from '../util/ng_reflect';
 
@@ -448,7 +449,8 @@ function debugCheckAndUpdateNode(
       const el = asElementData(view, elDef.nodeIndex).renderElement;
       if (!elDef.element!.name) {
         // a comment.
-        view.renderer.setValue(el, `bindings=${JSON.stringify(bindingValues, null, 2)}`);
+        view.renderer.setValue(
+            el, escapeCommentText(`bindings=${JSON.stringify(bindingValues, null, 2)}`));
       } else {
         // a regular element.
         for (let attr in bindingValues) {
@@ -727,7 +729,7 @@ export class DebugRenderer2 implements Renderer2 {
   }
 
   createComment(value: string): any {
-    const comment = this.delegate.createComment(value);
+    const comment = this.delegate.createComment(escapeCommentText(value));
     const debugCtx = this.createDebugContext(comment);
     if (debugCtx) {
       indexDebugNode(new DebugNode__PRE_R3__(comment, null, debugCtx));

packages/core/test/acceptance/security_spec.ts

@@ -0,0 +1,34 @@
+/**
+ * @license
+ * Copyright Google LLC All Rights Reserved.
+ *
+ * Use of this source code is governed by an MIT-style license that can be
+ * found in the LICENSE file at https://angular.io/license
+ */
+
+import {Component} from '@angular/core';
+import {TestBed} from '@angular/core/testing';
+
+
+describe('comment node text escaping', () => {
+  it('should not be possible to do XSS through comment reflect data', () => {
+    @Component({template: `<div><span *ngIf="xssValue"></span><div>`})
+    class XSSComp {
+      xssValue: string = '--> --><script>"evil"</script>';
+    }
+
+    TestBed.configureTestingModule({declarations: [XSSComp]});
+    const fixture = TestBed.createComponent(XSSComp);
+    fixture.detectChanges();
+    const div = fixture.nativeElement.querySelector('div') as HTMLElement;
+    // Serialize into a string to mimic SSR serialization.
+    const html = div.innerHTML;
+    // This must be escaped or we have XSS.
+    expect(html).not.toContain('--><script');
+    // Now parse it back into DOM (from string)
+    div.innerHTML = html;
+    // Verify that we did not accidentally deserialize the `<script>`
+    const script = div.querySelector('script');
+    expect(script).toBeFalsy();
+  });
+});

packages/core/test/util/dom_spec.ts

@@ -0,0 +1,26 @@
+/**
+ * @license
+ * Copyright Google LLC All Rights Reserved.
+ *
+ * Use of this source code is governed by an MIT-style license that can be
+ * found in the LICENSE file at https://angular.io/license
+ */
+
+import {escapeCommentText} from '@angular/core/src/util/dom';
+
+describe('comment node text escaping', () => {
+  describe('escapeCommentText', () => {
+    it('should not change anything on basic text', () => {
+      expect(escapeCommentText('text')).toEqual('text');
+    });
+
+    it('should escape end marker', () => {
+      expect(escapeCommentText('before-->after')).toEqual('before-\u200b-\u200b>after');
+    });
+
+    it('should escape multiple markers', () => {
+      expect(escapeCommentText('before-->inline-->after'))
+          .toEqual('before-\u200b-\u200b>inline-\u200b-\u200b>after');
+    });
+  });
+});