Skip to content

Navigation Menu

Sign in

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 0aa220b

Browse filesBrowse files
mheveryalxhub
mhevery
authored and
alxhub
committed
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. `<!-- 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 #40136
1 parent 03e8a92 commit 0aa220b
Copy full SHA for 0aa220b

File tree

6 files changed

+106
-4
lines changed
Filter options

6 files changed

+106
-4
lines changed

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

Copy file name to clipboardExpand all lines: packages/core/src/render3/instructions/shared.ts
+3-1Lines changed: 3 additions & 1 deletion
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, assertGreaterThanOrEqual, assertIndexInRange, assertNotEqual, assertNotSame, assertSame, assertString} 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';
@@ -1043,7 +1044,8 @@ function setNgReflectProperty(
10431044
(element as RElement).setAttribute(attrName, debugValue);
10441045
}
10451046
} else {
1046-
const textContent = `bindings=${JSON.stringify({[attrName]: debugValue}, null, 2)}`;
1047+
const textContent =
1048+
escapeCommentText(`bindings=${JSON.stringify({[attrName]: debugValue}, null, 2)}`);
10471049
if (isProceduralRenderer(renderer)) {
10481050
renderer.setValue((element as RComment), textContent);
10491051
} else {

‎packages/core/src/render3/node_manipulation.ts

Copy file name to clipboardExpand all lines: packages/core/src/render3/node_manipulation.ts
+2-1Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import {Renderer2} from '../render/api';
1111
import {RendererStyleFlags2} from '../render/api_flags';
1212
import {addToArray, removeFromArray} from '../util/array_utils';
1313
import {assertDefined, assertDomNode, assertEqual, assertFunction, assertString} from '../util/assert';
14+
import {escapeCommentText} from '../util/dom';
1415
import {assertLContainer, assertLView, assertTNodeForLView} from './assert';
1516
import {attachPatchData} from './context_discovery';
1617
import {icuContainerIterate} from './i18n/i18n_tree_shaking';
@@ -113,7 +114,7 @@ export function createCommentNode(renderer: Renderer3, value: string): RComment
113114
ngDevMode && ngDevMode.rendererCreateComment++;
114115
// isProceduralRenderer check is not needed because both `Renderer2` and `Renderer3` have the same
115116
// method name.
116-
return renderer.createComment(value);
117+
return renderer.createComment(escapeCommentText(value));
117118
}
118119

119120
/**

‎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+
}

‎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
@@ -16,6 +16,7 @@ import {NgModuleRef} from '../linker/ng_module_factory';
1616
import {Renderer2, RendererFactory2} from '../render/api';
1717
import {RendererStyleFlags2, RendererType2} from '../render/api_flags';
1818
import {Sanitizer} from '../sanitization/sanitizer';
19+
import {escapeCommentText} from '../util/dom';
1920
import {isDevMode} from '../util/is_dev_mode';
2021
import {normalizeDebugBindingName, normalizeDebugBindingValue} from '../util/ng_reflect';
2122

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

729731
createComment(value: string): any {
730-
const comment = this.delegate.createComment(value);
732+
const comment = this.delegate.createComment(escapeCommentText(value));
731733
const debugCtx = this.createDebugContext(comment);
732734
if (debugCtx) {
733735
indexDebugNode(new DebugNode__PRE_R3__(comment, null, debugCtx));

‎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+
});

‎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.