8000 fix(core): fix possible XSS attack in development through SSR (#40525) · angular/angular@ba8da74 · GitHub
[go: up one dir, main page]
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 ba8da74

Browse files
mheverymhevery
committed
fix(core): fix possible XSS attack in development through SSR (#40525)
This is a follow up fix for 894286d. It turns out that comments can be closed in several ways: - `<!-->` - `<!-- -->` - `<!-- --!>` All of the above are valid ways to close comment per: https://html.spec.whatwg.org/multipage/syntax.html#comments The new fix surrounds `<` and `>` with zero width space so that it renders in the same way, but it prevents the comment to be closed eagerly. PR Close #40525
1 parent 4a11226 commit ba8da74

File tree

3 files changed

+76
-31
lines changed

3 files changed

+76
-31
lines changed

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

+25-11
Original file line numberDiff line numberDiff line change
@@ -6,15 +6,27 @@
66
* found in the LICENSE file at https://angular.io/license
77
*/
88

9-
const END_COMMENT = /-->/g;
10-
const END_COMMENT_ESCAPED = '-\u200B-\u200B>';
9+
/**
10+
* Disallowed strings in the comment.
11+
*
12+
* see: https://html.spec.whatwg.org/multipage/syntax.html#comments
13+
*/
14+
const COMMENT_DISALLOWED = /^>|^->|<!--|-->|--!>|<!-$/g;
15+
/**
16+
* Delimiter in the disallowed strings which needs to be wrapped with zero with character.
17+
*/
18+
const COMMENT_DELIMITER = /(<|>)/;
19+
const COMMENT_DELIMITER_ESCAPED = '\u200B$1\u200B';
1120

1221
/**
13-
* Escape the content of the strings so that it can be safely inserted into a comment node.
22+
* Escape the content of comment strings so that it can be safely inserted into a comment node.
1423
*
1524
* 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.
25+
* Consider: `<!-- The way you close a comment is with ">", and "->" at the beginning or by "-->" or
26+
* "--!>" at the end. -->`. Above the `"-->"` is meant to be text not an end to the comment. This
27+
* can be created programmatically through DOM APIs. (`<!--` are also disallowed.)
28+
*
29+
* see: https://html.spec.whatwg.org/multipage/syntax.html#comments
1830
*
1931
* ```
2032
* div.innerHTML = div.innerHTML
@@ -25,13 +37,15 @@ const END_COMMENT_ESCAPED = '-\u200B-\u200B>';
2537
* opening up the application for XSS attack. (In SSR we programmatically create comment nodes which
2638
* may contain such text and expect them to be safe.)
2739
*
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.
40+
* This function escapes the comment text by looking for comment delimiters (`<` and `>`) and
41+
* surrounding them with `_>_` where the `_` is a zero width space `\u200B`. The result is that if a
42+
* comment contains any of the comment start/end delimiters (such as `<!--`, `-->` or `--!>`) the
43+
* text it will render normally but it will not cause the HTML parser to close/open the comment.
3244
*
33-
* @param value text to make safe for comment node by escaping the comment close character sequence
45+
* @param value text to make safe for comment node by escaping the comment open/close character
46+
* sequence.
3447
*/
3548
export function escapeCommentText(value: string): string {
36-
return value.replace(END_COMMENT, END_COMMENT_ESCAPED);
49+
return value.replace(
50+
COMMENT_DISALLOWED, (text) => text.replace(COMMENT_DELIMITER, COMMENT_DELIMITER_ESCAPED));
3751
}

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

+27-18
Original file line numberDiff line numberDiff line change
@@ -11,24 +11,33 @@ import {TestBed} from '@angular/core/testing';
1111

1212

1313
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-
}
14+
// see: https://html.spec.whatwg.org/multipage/syntax.html#comments
15+
['>', // self closing
16+
'-->', // standard closing
17+
'--!>', // alternate closing
18+
'<!-- -->', // embedded comment.
19+
].forEach((xssValue) => {
20+
it('should not be possible to do XSS through comment reflect data when writing: ' + xssValue,
21+
() => {
22+
@Component({template: `<div><span *ngIf="xssValue"></span><div>`})
23+
class XSSComp {
24+
// ngIf serializes the `xssValue` into a comment for debugging purposes.
25+
xssValue: string = xssValue + '<script>"evil"</script>';
26+
}
1927

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();
28+
TestBed.configureTestingModule({declarations: [XSSComp]});
29+
const fixture = TestBed.createComponent(XSSComp);
30+
fixture.detectChanges();
31+
const div = fixture.nativeElement.querySelector('div') as HTMLElement;
32+
// Serialize into a string to mimic SSR serialization.
33+
const html = div.innerHTML;
34+
// This must be escaped or we have XSS.
35+
expect(html).not.toContain('--><script');
36+
// Now parse it back into DOM (from string)
37+
div.innerHTML = html;
38+
// Verify that we did not accidentally deserialize the `<script>`
39+
const script = div.querySelector('script');
40+
expect(script).toBeFalsy();
41+
});
3342
});
3443
});

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

+24-2
Original file line numberDiff line numberDiff line change
@@ -14,13 +14,35 @@ describe('comment node text escaping', () => {
1414
expect(escapeCommentText('text')).toEqual('text');
1515
});
1616

17+
it('should escape "<" or ">"', () => {
18+
expect(escapeCommentText('<!--')).toEqual('\u200b<\u200b!--');
19+
expect(escapeCommentText('<!--<!--')).toEqual('\u200b<\u200b!--\u200b<\u200b!--');
20+
expect(escapeCommentText('>')).toEqual('\u200b>\u200b');
21+
expect(escapeCommentText('>-->')).toEqual('\u200b>\u200b--\u200b>\u200b');
22+
});
23+
1724
it('should escape end marker', () => {
18-
expect(escapeCommentText('before-->after')).toEqual('before-\u200b-\u200b>after');
25+
expect(escapeCommentText('before-->after')).toEqual('before--\u200b>\u200bafter');
1926
});
2027

2128
it('should escape multiple markers', () => {
2229
expect(escapeCommentText('before-->inline-->after'))
23-
.toEqual('before-\u200b-\u200b>inline-\u200b-\u200b>after');
30+
.toEqual('before--\u200b>\u200binline--\u200b>\u200bafter');
31+
});
32+
33+
it('should caver the spec', () => {
34+
// https://html.spec.whatwg.org/multipage/syntax.html#comments
35+
expect(escapeCommentText('>')).toEqual('\u200b>\u200b');
36+
expect(escapeCommentText('->')).toEqual('-\u200b>\u200b');
37+
expect(escapeCommentText('<!--')).toEqual('\u200b<\u200b!--');
38+
expect(escapeCommentText('-->')).toEqual('--\u200b>\u200b');
39+
expect(escapeCommentText('--!>')).toEqual('--!\u200b>\u200b');
40+
expect(escapeCommentText('<!-')).toEqual('\u200b<\u200b!-');
41+
42+
// Things which are OK
43+
expect(escapeCommentText('.>')).toEqual('.>');
44+
expect(escapeCommentText('.->')).toEqual('.->');
45+
expect(escapeCommentText('<!-.')).toEqual('<!-.');
2446
});
2547
});
2648
});

0 commit comments

Comments
 (0)
0