Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 0b8e3d5

Browse filesBrowse files
mheveryalxhub
mhevery
authored and
alxhub
committed
fix(core): fix possible XSS attack in development through SSR. (#40152)
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. PR Close #40152
1 parent 1806d6e commit 0b8e3d5
Copy full SHA for 0b8e3d5

File tree

Expand file treeCollapse file tree

5 files changed

+105
-4
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

5 files changed

+105
-4
lines changed
Open diff view settings
Collapse file

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

Copy file name to clipboardExpand all lines: packages/core/src/render3/instructions/shared.ts
+4-2Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import {ViewEncapsulation} from '../../metadata/view';
1313
import {validateAgainstEventAttributes, validateAgainstEventProperties} from '../../sanitization/sanitization';
1414
import {Sanitizer} from '../../sanitization/sanitizer';
1515
import {assertDefined, assertDomNode, assertEqual, assertGreaterThan, assertIndexInRange, assertLessThan, assertNotEqual, assertNotSame, assertSame} from '../../util/assert';
16+
import {escapeCommentText} from '../../util/dom';
1617
import {createNamedArrayType} from '../../util/named_array_type';
1718
import {initNgDevMode} from '../../util/ng_dev_mode';
1819
import {normalizeDebugBindingName, normalizeDebugBindingValue} from '../../util/ng_reflect';
@@ -29,7 +30,7 @@ import {NodeInjectorFactory, NodeInjectorOffset} from '../interfaces/injector';
2930
import {AttributeMarker, InitialInputData, InitialInputs, LocalRefExtractor, PropertyAliases, PropertyAliasValue, TAttributes, TConstantsOrFactory, TContainerNode, TDirectiveHostNode, TElementContainerNode, TElementNode, TIcuContainerNode, TNode, TNodeFlags, TNodeProviderIndexes, TNodeType, TProjectionNode} from '../interfaces/node';
3031
import {isProceduralRenderer, RComment, RElement, Renderer3, RendererFactory3, RNode, RText} from '../interfaces/renderer';
3132
import {SanitizerFn} from '../interfaces/sanitization';
32-
import {isComponentDef, isComponentHost, isContentQueryHost, isLContainer, isRootView} from '../interfaces/type_checks';
33+
import {isComponentDef, isComponentHost, isContentQueryHost, isRootView} from '../interfaces/type_checks';
3334
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';
3435
import {assertNodeNotOfTypes, assertNodeOfPossibleTypes} from '../node_assert';
3536
import {isInlineTemplate, isNodeMatchingSelectorList} from '../node_selector_matcher';
@@ -1050,7 +1051,8 @@ function setNgReflectProperty(
10501051
(element as RElement).setAttribute(attrName, debugValue);
10511052
}
10521053
} else {
1053-
const textContent = `bindings=${JSON.stringify({[attrName]: debugValue}, null, 2)}`;
1054+
const textContent =
1055+
escapeCommentText(`bindings=${JSON.stringify({[attrName]: debugValue}, null, 2)}`);
10541056
if (isProceduralRenderer(renderer)) {
10551057
renderer.setValue((element as RComment), textContent);
10561058
} else {
Collapse file

‎packages/core/src/util/dom.ts‎

Copy file name to clipboard
+37Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
/**
2+
* @license
3+
* Copyright Google LLC All Rights Reserved.
4+
*
5+
* Use of this source code is governed by an MIT-style license that can be
6+
* found in the LICENSE file at https://angular.io/license
7+
*/
8+
9+
const END_COMMENT = /-->/g;
10+
const END_COMMENT_ESCAPED = '-\u200B-\u200B>';
11+
12+
/**
13+
* Escape the content of the strings so that it can be safely inserted into a comment node.
14+
*
15+
* The issue is that HTML does not specify any way to escape comment end text inside the comment.
16+
* `<!-- The way you close a comment is with "-->". -->`. Above the `"-->"` is meant to be text not
17+
* an end to the comment. This can be created programmatically through DOM APIs.
18+
*
19+
* ```
20+
* div.innerHTML = div.innerHTML
21+
* ```
22+
*
23+
* One would expect that the above code would be safe to do, but it turns out that because comment
24+
* text is not escaped, the comment may contain text which will prematurely close the comment
25+
* opening up the application for XSS attack. (In SSR we programmatically create comment nodes which
26+
* may contain such text and expect them to be safe.)
27+
*
28+
* This function escapes the comment text by looking for the closing char sequence `-->` and replace
29+
* it with `-_-_>` where the `_` is a zero width space `\u200B`. The result is that if a comment
30+
* contains `-->` text it will render normally but it will not cause the HTML parser to close the
31+
* comment.
32+
*
33+
* @param value text to make safe for comment node by escaping the comment close character sequence
34+
*/
35+
export function escapeCommentText(value: string): string {
36+
return value.replace(END_COMMENT, END_COMMENT_ESCAPED);
37+
}
Collapse file

‎packages/core/src/view/services.ts‎

Copy file name to clipboardExpand all lines: packages/core/src/view/services.ts
+4-2Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import {ComponentFactory} from '../linker/component_factory';
1515
import {NgModuleRef} from '../linker/ng_module_factory';
1616
import {Renderer2, RendererFactory2, RendererStyleFlags2, RendererType2} from '../render/api';
1717
import {Sanitizer} from '../sanitization/sanitizer';
18+
import {escapeCommentText} from '../util/dom';
1819
import {isDevMode} from '../util/is_dev_mode';
1920
import {normalizeDebugBindingName, normalizeDebugBindingValue} from '../util/ng_reflect';
2021

@@ -447,7 +448,8 @@ function debugCheckAndUpdateNode(
447448
const el = asElementData(view, elDef.nodeIndex).renderElement;
448449
if (!elDef.element!.name) {
449450
// a comment.
450-
view.renderer.setValue(el, `bindings=${JSON.stringify(bindingValues, null, 2)}`);
451+
view.renderer.setValue(
452+
el, escapeCommentText(`bindings=${JSON.stringify(bindingValues, null, 2)}`));
451453
} else {
452454
// a regular element.
453455
for (let attr in bindingValues) {
@@ -726,7 +728,7 @@ export class DebugRenderer2 implements Renderer2 {
726728
}
727729

728730
createComment(value: string): any {
729-
const comment = this.delegate.createComment(value);
731+
const comment = this.delegate.createComment(escapeCommentText(value));
730732
const debugCtx = this.createDebugContext(comment);
731733
if (debugCtx) {
732734
indexDebugNode(new DebugNode__PRE_R3__(comment, null, debugCtx));
Collapse file

‎packages/core/test/acceptance/security_spec.ts‎

Copy file name to clipboard
+34Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
/**
2+
* @license
3+
* Copyright Google LLC All Rights Reserved.
4+
*
5+
* Use of this source code is governed by an MIT-style license that can be
6+
* found in the LICENSE file at https://angular.io/license
7+
*/
8+
9+
import {Component} from '@angular/core';
10+
import {TestBed} from '@angular/core/testing';
11+
12+
13+
describe('comment node text escaping', () => {
14+
it('should not be possible to do XSS through comment reflect data', () => {
15+
@Component({template: `<div><span *ngIf="xssValue"></span><div>`})
16+
class XSSComp {
17+
xssValue: string = '--> --><script>"evil"</script>';
18+
}
19+
20+
TestBed.configureTestingModule({declarations: [XSSComp]});
21+
const fixture = TestBed.createComponent(XSSComp);
22+
fixture.detectChanges();
23+
const div = fixture.nativeElement.querySelector('div') as HTMLElement;
24+
// Serialize into a string to mimic SSR serialization.
25+
const html = div.innerHTML;
26+
// This must be escaped or we have XSS.
27+
expect(html).not.toContain('--><script');
28+
// Now parse it back into DOM (from string)
29+
div.innerHTML = html;
30+
// Verify that we did not accidentally deserialize the `<script>`
31+
const script = div.querySelector('script');
32+
expect(script).toBeFalsy();
33+
});
34+
});
Collapse file

‎packages/core/test/util/dom_spec.ts‎

Copy file name to clipboard
+26Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
/**
2+
* @license
3+
* Copyright Google LLC All Rights Reserved.
4+
*
5+
* Use of this source code is governed by an MIT-style license that can be
6+
* found in the LICENSE file at https://angular.io/license
7+
*/
8+
9+
import {escapeCommentText} from '@angular/core/src/util/dom';
10+
11+
describe('comment node text escaping', () => {
12+
describe('escapeCommentText', () => {
13+
it('should not change anything on basic text', () => {
14+
expect(escapeCommentText('text')).toEqual('text');
15+
});
16+
17+
it('should escape end marker', () => {
18+
expect(escapeCommentText('before-->after')).toEqual('before-\u200b-\u200b>after');
19+
});
20+
21+
it('should escape multiple markers', () => {
22+
expect(escapeCommentText('before-->inline-->after'))
23+
.toEqual('before-\u200b-\u200b>inline-\u200b-\u200b>after');
24+
});
25+
});
26+
});

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.