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

mhevery · thePunderWoman · commit bb3b315eee8a · 2021-01-22T14:19:52.000-08:00
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
diff --git a/packages/core/src/util/dom.ts b/packages/core/src/util/dom.ts
@@ -6,15 +6,18 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-const END_COMMENT = /-->/g;
-const END_COMMENT_ESCAPED = '-\u200B-\u200B>';
+const END_COMMENT = /(<|>)/g;
+const END_COMMENT_ESCAPED = '\u200B$1\u200B';
 
 /**
  * Escape the content of the 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
@@ -26,7 +29,7 @@ const END_COMMENT_ESCAPED = '-\u200B-\u200B>';
  * 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
+ * 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.
  *
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,20 @@ 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');
     });
   });
 });
