Skip to content

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)