From 6bf99e0edaedc6b67e765faa996da867420bf3e7 Mon Sep 17 00:00:00 2001 From: Misko Hevery Date: Fri, 22 Jan 2021 10:05:17 -0800 Subject: [PATCH] fix(core): fix possible XSS attack in development through SSR (#40525) This is a follow up fix for https://github.com/angular/angular/pull/40136/commits/894286dd0c92b5af223364237e63798e18b14f58. 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 --- packages/core/src/util/dom.ts | 36 ++++++++++----- .../core/test/acceptance/security_spec.ts | 45 +++++++++++-------- packages/core/test/util/dom_spec.ts | 26 ++++++++++- 3 files changed, 76 insertions(+), 31 deletions(-) diff --git a/packages/core/src/util/dom.ts b/packages/core/src/util/dom.ts index 805daa5a92..f55098583e 100644 --- a/packages/core/src/util/dom.ts +++ b/packages/core/src/util/dom.ts @@ -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 = /^>|^->||--!>|)/; +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. - * `". -->`. Above the `"-->"` is meant to be text not - * an end to the comment. This can be created programmatically through DOM APIs. + * Consider: `" 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. (`` 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)); } \ No newline at end of file diff --git a/packages/core/test/acceptance/security_spec.ts b/packages/core/test/acceptance/security_spec.ts index 0376dcc1cf..afc8b9ccd4 100644 --- a/packages/core/test/acceptance/security_spec.ts +++ b/packages/core/test/acceptance/security_spec.ts @@ -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: `
`}) - class XSSComp { - xssValue: string = '--> -->'; - } + // 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: `
`}) + class XSSComp { + // ngIf serializes the `xssValue` into a comment for debugging purposes. + xssValue: string = xssValue + ''; + } - 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('-->` - 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('-->` + const script = div.querySelector('script'); + expect(script).toBeFalsy(); + }); }); }); \ No newline at end of file diff --git a/packages/core/test/util/dom_spec.ts b/packages/core/test/util/dom_spec.ts index 8d552ccf2e..c62618f017 100644 --- a/packages/core/test/util/dom_spec.ts +++ b/packages/core/test/util/dom_spec.ts @@ -14,13 +14,35 @@ describe('comment node text escaping', () => { expect(escapeCommentText('text')).toEqual('text'); }); + it('should escape "<" or ">"', () => { + 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('.>'); + expect(escapeCommentText('.->')).toEqual('.->'); + expect(escapeCommentText('