diff --git a/packages/core/src/render3/instructions/shared.ts b/packages/core/src/render3/instructions/shared.ts index 284761ef92a3b..be6acaf93bbbf 100644 --- a/packages/core/src/render3/instructions/shared.ts +++ b/packages/core/src/render3/instructions/shared.ts @@ -12,6 +12,7 @@ import {ViewEncapsulation} from '../../metadata/view'; import {validateAgainstEventAttributes, validateAgainstEventProperties} from '../../sanitization/sanitization'; import {Sanitizer} from '../../sanitization/sanitizer'; import {assertDataInRange, assertDefined, assertDomNode, assertEqual, assertGreaterThan, assertNotEqual, assertNotSame, assertSame} 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'; @@ -27,7 +28,7 @@ import {INJECTOR_BLOOM_PARENT_SIZE, NodeInjectorFactory} from '../interfaces/inj import {AttributeMarker, InitialInputData, InitialInputs, LocalRefExtractor, PropertyAliases, PropertyAliasValue, TAttributes, TConstants, TContainerNode, TDirectiveHostNode, TElementContainerNode, TElementNode, TIcuContainerNode, TNode, TNodeFlags, TNodeProviderIndexes, TNodeType, TProjectionNode, TViewNode} from '../interfaces/node'; import {isProceduralRenderer, RComment, RElement, Renderer3, RendererFactory3, RNode, RText} from '../interfaces/renderer'; import {SanitizerFn} from '../interfaces/sanitization'; -import {isComponentDef, isComponentHost, isContentQueryHost, isLContainer, isRootView} from '../interfaces/type_checks'; +import {isComponentDef, isComponentHost, isContentQueryHost, isRootView} from '../interfaces/type_checks'; import {CHILD_HEAD, CHILD_TAIL, CLEANUP, CONTEXT, DECLARATION_COMPONENT_VIEW, DECLARATION_VIEW, FLAGS, HEADER_OFFSET, HOST, InitPhaseState, INJECTOR, LView, LViewFlags, NEXT, PARENT, RENDERER, RENDERER_FACTORY, RootContext, RootContextFlags, SANITIZER, T_HOST, TData, TRANSPLANTED_VIEWS_TO_REFRESH, TVIEW, TView, TViewType} from '../interfaces/view'; import {assertNodeOfPossibleTypes} from '../node_assert'; import {isInlineTemplate, isNodeMatchingSelectorList} from '../node_selector_matcher'; @@ -37,7 +38,6 @@ import {isAnimationProp, mergeHostAttrs} from '../util/attrs_utils'; import {INTERPOLATION_DELIMITER, renderStringify, stringifyForError} from '../util/misc_utils'; import {getFirstLContainer, getLViewParent, getNextLContainer} from '../util/view_traversal_utils'; import {getComponentLViewByIndex, getNativeByIndex, getNativeByTNode, isCreationMode, readPatchedLView, resetPreOrderHookFlags, unwrapLView, updateTransplantedViewCount, viewAttachedToChangeDetector} from '../util/view_utils'; - import {selectIndexInternal} from './advance'; import {attachLContainerDebug, attachLViewDebug, cloneToLViewFromTViewBlueprint, cloneToTViewData, LCleanup, LViewBlueprint, MatchesArray, TCleanup, TNodeDebug, TNodeInitialInputs, TNodeLocalNames, TViewComponents, TViewConstructor} from './lview_debug'; @@ -1044,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/util/dom.ts b/packages/core/src/util/dom.ts new file mode 100644 index 0000000000000..374e522ed7303 --- /dev/null +++ b/packages/core/src/util/dom.ts @@ -0,0 +1,37 @@ +/** + * @license + * Copyright Google Inc. 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 97b9f2bf9208d..70e774dbcadff 100644 --- a/packages/core/src/view/services.ts +++ b/packages/core/src/view/services.ts @@ -15,6 +15,7 @@ import {ComponentFactory} from '../linker/component_factory'; import {NgModuleRef} from '../linker/ng_module_factory'; import {Renderer2, RendererFactory2, RendererStyleFlags2, RendererType2} from '../render/api'; import {Sanitizer} from '../sanitization/sanitizer'; +import {escapeCommentText} from '../util/dom'; import {isDevMode} from '../util/is_dev_mode'; import {normalizeDebugBindingName, normalizeDebugBindingValue} from '../util/ng_reflect'; @@ -447,7 +448,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) { @@ -726,7 +728,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..8a57dc5bd043e --- /dev/null +++ b/packages/core/test/acceptance/security_spec.ts @@ -0,0 +1,34 @@ +/** + * @license + * Copyright Google Inc. 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 0000000000000..88461fc079863 --- /dev/null +++ b/packages/core/test/util/dom_spec.ts @@ -0,0 +1,26 @@ +/** + * @license + * Copyright Google Inc. 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'); + }); + }); +});