fix(core): fix possible XSS attack in development through SSR (#40525)

This is a follow up fix for
894286dd0c.

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
This commit is contained in:
Misko Hevery 2021-01-22 10:05:17 -08:00 committed by Jessica Janiuk
parent 365ac5e68e
commit 6bf99e0eda
3 changed files with 76 additions and 31 deletions

View File

@ -6,15 +6,27 @@
* found in the LICENSE file at https://angular.io/license
*/
const END_COMMENT = /-->/g;
const END_COMMENT_ESCAPED = '-\u200B-\u200B>';
/**
* Disallowed strings in the comment.
*
* see: https://html.spec.whatwg.org/multipage/syntax.html#comments
*/
const COMMENT_DISALLOWED = /^>|^->|<!--|-->|--!>|<!-$/g;
/**
* Delimiter in the disallowed strings which needs to be wrapped with zero with character.
*/
const COMMENT_DELIMITER = /(<|>)/;
const COMMENT_DELIMITER_ESCAPED = '\u200B$1\u200B';
/**
* Escape the content of the strings so that it can be safely inserted into a comment node.
* Escape the content of comment 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.
* Consider: `<!-- The way you close a comment is with ">", and "->" at the beginning or by "-->" or
* "--!>" at the end. -->`. Above the `"-->"` is meant to be text not an end to the comment. This
* can be created programmatically through DOM APIs. (`<!--` are also disallowed.)
*
* see: https://html.spec.whatwg.org/multipage/syntax.html#comments
*
* ```
* div.innerHTML = div.innerHTML
@ -25,13 +37,15 @@ const END_COMMENT_ESCAPED = '-\u200B-\u200B>';
* 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.
* This function escapes the comment text by looking for comment delimiters (`<` and `>`) and
* surrounding them with `_>_` where the `_` is a zero width space `\u200B`. The result is that if a
* comment contains any of the comment start/end delimiters (such as `<!--`, `-->` or `--!>`) the
* text it will render normally but it will not cause the HTML parser to close/open the comment.
*
* @param value text to make safe for comment node by escaping the comment close character sequence
* @param value text to make safe for comment node by escaping the comment open/close character
* sequence.
*/
export function escapeCommentText(value: string): string {
return value.replace(END_COMMENT, END_COMMENT_ESCAPED);
return value.replace(
COMMENT_DISALLOWED, (text) => text.replace(COMMENT_DELIMITER, COMMENT_DELIMITER_ESCAPED));
}

View File

@ -11,24 +11,33 @@ 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: `<div><span *ngIf="xssValue"></span><div>`})
class XSSComp {
xssValue: string = '--> --><script>"evil"</script>';
}
// see: https://html.spec.whatwg.org/multipage/syntax.html#comments
['>', // self closing
'-->', // standard closing
'--!>', // alternate closing
'<!-- -->', // embedded comment.
].forEach((xssValue) => {
it('should not be possible to do XSS through comment reflect data when writing: ' + xssValue,
() => {
@Component({template: `<div><span *ngIf="xssValue"></span><div>`})
class XSSComp {
// ngIf serializes the `xssValue` into a comment for debugging purposes.
xssValue: string = xssValue + '<script>"evil"</script>';
}
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('--><script');
// Now parse it back into DOM (from string)
div.innerHTML = html;
// Verify that we did not accidentally deserialize the `<script>`
const script = div.querySelector('script');
expect(script).toBeFalsy();
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('--><script');
// Now parse it back into DOM (from string)
div.innerHTML = html;
// Verify that we did not accidentally deserialize the `<script>`
const script = div.querySelector('script');
expect(script).toBeFalsy();
});
});
});

View File

@ -14,13 +14,35 @@ describe('comment node text escaping', () => {
expect(escapeCommentText('text')).toEqual('text');
});
it('should escape "<" or ">"', () => {
expect(escapeCommentText('<!--')).toEqual('\u200b<\u200b!--');
expect(escapeCommentText('<!--<!--')).toEqual('\u200b<\u200b!--\u200b<\u200b!--');
expect(escapeCommentText('>')).toEqual('\u200b>\u200b');
expect(escapeCommentText('>-->')).toEqual('\u200b>\u200b--\u200b>\u200b');
});
it('should escape end marker', () => {
expect(escapeCommentText('before-->after')).toEqual('before-\u200b-\u200b>after');
expect(escapeCommentText('before-->after')).toEqual('before--\u200b>\u200bafter');
});
it('should escape multiple markers', () => {
expect(escapeCommentText('before-->inline-->after'))
.toEqual('before-\u200b-\u200b>inline-\u200b-\u200b>after');
.toEqual('before--\u200b>\u200binline--\u200b>\u200bafter');
});
it('should caver the spec', () => {
// https://html.spec.whatwg.org/multipage/syntax.html#comments
expect(escapeCommentText('>')).toEqual('\u200b>\u200b');
expect(escapeCommentText('->')).toEqual('-\u200b>\u200b');
expect(escapeCommentText('<!--')).toEqual('\u200b<\u200b!--');
expect(escapeCommentText('-->')).toEqual('--\u200b>\u200b');
expect(escapeCommentText('--!>')).toEqual('--!\u200b>\u200b');
expect(escapeCommentText('<!-')).toEqual('\u200b<\u200b!-');
// Things which are OK
expect(escapeCommentText('.>')).toEqual('.>');
expect(escapeCommentText('.->')).toEqual('.->');
expect(escapeCommentText('<!-.')).toEqual('<!-.');
});
});
});