diff --git a/packages/core/src/render3/instructions/shared.ts b/packages/core/src/render3/instructions/shared.ts index c0b11b180a1e5..f1a8c836085d6 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 8a8cdbe1bae7c..4b694a598af4c 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 0000000000000..7dc029f4c8b18 --- /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 d11566ca5a64c..c15f0a4aa155e 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 0000000000000..3c0d54e2cc994 --- /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('security', () => { + 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 + const html = div.innerHTML; + // This must be escaped or we have XSS. + expect(html).not.toContain('-->` + const script = div.querySelector('script'); + expect(script).not.toBeDefined(); + }); +}); \ 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 0000000000000..aa4e8185a0432 --- /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('dom', () => { + 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'); + }); + }); +});