CVE-2021-4231: Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting') (CWE-79)
value: A vulnerability was found in Angular up to 11.0.4/11.1.0-next.2. It has been classified as problematic. Affected is the handling of comments. The manipulation leads to cross site scripting. It is possible to launch the attack remotely but it might require an authentication first. Upgrading to version 11.0.5 and 11.1.0-next.3 is able to address this issue. The name of the patch is ba8da742e3b243e8f43d4c63aa842b44e14f2b09. It is recommended to upgrade the affected component.
CWE description: The product does not neutralize or incorrectly neutralizes user-controllable input before it is placed in output that is used as a web page that is served to other users.
CVSS v3 base score: 5.4
Published: 2022-05-26T14:15Z
Repository: https://github.com/angular/angular
Language: TypeScript
Files changed: packages/core/src/util/dom.ts, packages/core/test/acceptance/security_spec.ts, packages/core/test/util/dom_spec.ts
Commit date: 2021-03-18 10:06:02 -0700
Commit message: 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

Diff: 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
@@ -4,34 +4,48 @@
  *
  * Use of this source code is governed by an MIT-style license that can be
  * 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
  * ```
  *
  * 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.
+ * 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
@@ -9,26 +9,35 @@
 import {Component} from '@angular/core';
 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();
+       });
   });
 });
\ 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
@@ -12,15 +12,37 @@ describe('comment node text escaping', () => {
   describe('escapeCommentText', () => {
     it('should not change anything on basic text', () => {
       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('<!-.');
     });
   });
 });

Vulnerable code: const END_COMMENT = /-->/g;
const END_COMMENT_ESCAPED = '-\u200B-\u200B>';
 * Escape the content of the strings so that it can be safely inserted into a comment node.
 * `<!-- 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.
 * 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.
 * @param value text to make safe for comment node by escaping the comment close character sequence
  return value.replace(END_COMMENT, END_COMMENT_ESCAPED);
  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>';
    }
    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();
      expect(escapeCommentText('before-->after')).toEqual('before-\u200b-\u200b>after');
          .toEqual('before-\u200b-\u200b>inline-\u200b-\u200b>after');
Fixed code: /**
 * 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 comment strings so that it can be safely inserted into a comment node.
 * 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
 * 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 open/close character
 *     sequence.
  return value.replace(
      COMMENT_DISALLOWED, (text) => text.replace(COMMENT_DELIMITER, COMMENT_DELIMITER_ESCAPED));
  // 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();
       });
    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');
    });

      expect(escapeCommentText('before-->after')).toEqual('before--\u200b>\u200bafter');
          .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('<!-.');