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
angular · Mar 18, 2021 · ba8da74 · ba8da74
1 parent 4a11226
commit ba8da74
Show file tree

Hide file tree

Showing 3 changed files with 76 additions and 31 deletions.
diff --git 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 = /^>|^->|<!--|-->|--!>|<!-$/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));
 }
diff --git 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: `<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();
+       });
   });
 });
diff --git 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!--');
+      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('<!-.');
     });
   });
 });