fix(core): use appropriate inert document strategy for Firefox & Safari #17019

petebacondarwin · 2017-05-25T13:46:26Z

Both Firefox and Safari are vulnerable to XSS if we use an inert document
created via document.implementation.createHTMLDocument().

Now we check for those vulnerabilities and then use a DOMParser or XHR
strategy if needed.

Thanks to @cure53 for the heads up on this issue.

petebacondarwin · 2017-05-25T13:52:38Z

Locally I am getting this warning:

LOG: 'Attempting to configure '__esModule' with descriptor '{"value":true}' on object 'undefined' and got error, giving up: TypeError: Object.defineProperty called on non-object'

I am not sure what is causing this.

mary-poppins · 2017-05-25T13:56:50Z

The angular.io preview for abc704a is available here.

mary-poppins · 2017-05-25T14:01:17Z

The angular.io preview for 5c0104d is available here.

mary-poppins · 2017-05-25T14:15:38Z

The angular.io preview for bef4d33 is available here.

mhevery · 2017-05-25T19:14:59Z

packages/platform-browser/test/security/html_sanitizer_spec.ts

+    it('should not allow JavaScript execution when creating inert document', () => {
+      sanitizeHtml(defaultDoc, '<svg><g onload="window.xxx = 100"></g></svg>');
+      expect((window as any).xxx).toBe(undefined);
+      delete (window as any).xxx;


Please don't do delete it will make window a slow object for the rest of the tests. Just assign undefined

Although that is quite an ironic statement given how slow the tests are generally :-P

petebacondarwin · 2017-05-25T20:32:14Z

The unit tests are failing on Travis. It seems it is because the inertBodyElement does not have the querySelector method. Is this some different mode than I am running locally?

mary-poppins · 2017-05-26T05:10:19Z

The angular.io preview for 3c1adac is available here.

mprobst · 2017-05-29T08:59:16Z

/CC @rjamet

mprobst · 2017-05-29T09:00:07Z

packages/platform-browser/src/security/html_sanitizer.ts

@@ -121,53 +95,54 @@ class SanitizingHtmlSerializer {
  // because characters were re-encoded.
  public sanitizedSomething = false;
  private buf: string[] = [];
+  private DOM = getDOM();


nit: should be lowerCamelCase, i.e. dom, right? It doesn't have to be a field in the class instance btw, it should be an initialized-once module level field. We just need to take care to initialize after the initial angular load, otherwise we risk getting the wrong DOM.

I was copying this casing from the previous code: https://github.com/angular/angular/pull/17019/files/3c1adac41b8a17262ae4c2dde7b61370869244f4#diff-4fba2f404f1048cbc43f747e3bc2606dL18. Happy to change this if appropriate.

I was not sure about the benefit of using a module level variable. Throughout the rest of the Angular code base, this getDOM() is used in a variety of ways: as it is needed (not cached at all) - e.g. the Title service; cached per instance - e.g. Meta service; and cached per module. But the majority use the call as required approach and looking at the implementation of this method it is super simple method:

export function getDOM() { return _DOM; }

The downside of caching is that we get the wrong version of the DOM so it seems to me that it is actually safer and easier (and no slower) to just call getDOM() as required.

Indeed, we don't really have a system here. I think I'd either access it through the getter (which hopefully your optimizer should inline) or from a lazily initialized module variable. YMMV, and I don't have data pointing either way.

mprobst · 2017-05-29T09:02:09Z

packages/platform-browser/src/security/html_sanitizer.ts


 /**
 * Sanitizes the given unsafe, untrusted HTML fragment, and returns HTML text that is safe to add to
 * the DOM in a browser environment.
 */
 export function sanitizeHtml(defaultDoc: any, unsafeHtmlInput: string): string {
+  const DOM = getDOM();


initialize DOM as a module level field here, as you do with inertBodyHelper below

The only reason I use a module level variable for inertBodyHelper is that it is used in a free standing exported function (rather than a class) and so there was no other container to hold the cached object.

Also, inertBodyHelper creates DOM elements and so it more performance sensitive than the simple implementation of getDOM().

mprobst · 2017-05-29T09:03:29Z