fix(core): fix possible XSS attack in development through SSR. (#40136)

mhevery · alxhub · commit 0aa220bc0000 · 2020-12-16T09:38:08.000-08:00
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. `". -->`. 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. PR Close #40136
diff --git a/packages/core/src/render3/instructions/shared.ts b/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 {
diff --git a/packages/core/src/render3/node_manipulation.ts b/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));
 }
 
 /**
diff --git a/packages/core/src/util/dom.ts b/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);
+}
diff --git a/packages/core/src/view/services.ts b/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));
diff --git a/packages/core/test/acceptance/security_spec.ts b/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();
+  });
+});
diff --git a/packages/core/test/util/dom_spec.ts b/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');
+    });
+  });
+});
