From 47d9b6d72dab9d60c96bc1c3604219f6385649ea Mon Sep 17 00:00:00 2001 From: Misko Hevery Date: Tue, 15 Dec 2020 13:00:29 -0800 Subject: [PATCH] fix(core): fix possible XSS attack in development through SSR. (#40136) 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 --- .../core/src/render3/instructions/shared.ts | 4 +- .../core/src/render3/node_manipulation.ts | 3 +- packages/core/src/util/dom.ts | 37 +++++++++++++++++++ packages/core/src/view/services.ts | 6 ++- .../core/test/acceptance/security_spec.ts | 34 +++++++++++++++++ packages/core/test/util/dom_spec.ts | 26 +++++++++++++ 6 files changed, 106 insertions(+), 4 deletions(-) create mode 100644 packages/core/src/util/dom.ts create mode 100644 packages/core/test/acceptance/security_spec.ts create mode 100644 packages/core/test/util/dom_spec.ts diff --git a/packages/core/src/render3/instructions/shared.ts b/packages/core/src/render3/instructions/shared.ts index c0b11b180a..f1a8c83608 100644 --- 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 index 8a8cdbe1ba..4b694a598a 100644 --- 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 new file mode 100644 index 0000000000..805daa5a92 --- /dev/null +++ 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. + * `". -->`. 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); +} \ No newline at end of file diff --git a/packages/core/src/view/services.ts b/packages/core/src/view/services.ts index d11566ca5a..c15f0a4aa1 100644 --- 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 new file mode 100644 index 0000000000..0376dcc1cf --- /dev/null +++ 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: `
`}) + class XSSComp { + xssValue: string = '--> -->'; + } + + 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('-->` + const script = div.querySelector('script'); + expect(script).toBeFalsy(); + }); +}); \ No newline at end of file diff --git a/packages/core/test/util/dom_spec.ts b/packages/core/test/util/dom_spec.ts new file mode 100644 index 0000000000..8d552ccf2e --- /dev/null +++ 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'); + }); + }); +});