ngSanitize triggers CSP alert/report in Firefox

[thesam]

I'm submitting a ...

• bug report
• feature request
• other

Current behavior:

If ngSanitize is added as a module dependency and a Content-Security-Policy is set that does not allow inline styles then Firefox shows the following message:

Content Security Policy: The page’s settings observed the loading of a resource at self (“default-src”). A CSP report is being sent.

Our CSP looks like this:

Content-Security-Policy-Report-Only: default-src 'self'; report-uri /foo

If ngSanitize is removed from the module dependencies then the CSP message disappears as well.

Expected / new behavior:

ngSanitize should work in Firefox without triggering CSP alerts, at least if the "ng-csp" mode is enabled.

Minimal reproduction of the problem with instructions:

1. Set the Content-Security-Policy to: default-src: 'self'
2. Add 'ngSanitize' as a module dependency.

AngularJS version: 1.6.9

Browser: Firefox 60.0a1 and 59.0b10

Anything else:
I guess the following code triggers the CSP alert, since it adds an inline <style> tag.
// Check for the Firefox bug - which prevents the inner img JS from being sanitized inertBodyElement.innerHTML = '<svg><p><style><img src="</style><img src=x onerror=alert(1)//">';
From: https://github.com/angular/angular.js/blob/master/src/ngSanitize/sanitize.js
Line 443-444

[petebacondarwin]

@thesam do you have any suggestion for how we could test for this FF bug (which is a security vulnerability) without the inline style?

[thesam]

If I understand correctly this is how it works right now:

1. If Safari bug exists: Use getInertBodyElement_XHR
2. Else if Firefox bug exists: Use getInertBodyElement_DOMParser
3. Else: Use getInertBodyElement_InertDocument

Maybe it could be done like this instead?

1. If Safari bug exists: Use getInertBodyElement_XHR
2. Else if DOMParser is available: Use getInertBodyElement_DOMParser
3. Else if Firefox bug exists: fail/throw/abort!
4. Else: Use getInertBodyElement_InertDocument

This way, the Firefox CSP alert will not be triggered if DOMParser is available.

I guess this would mean that most browsers would use DOMParser instead of InertDocument. Are there any negative side effects of using DOMParser instead of InertDocument?

[petebacondarwin]

I think I remember that inert document approach is faster and more secure, so I would be wary of changing it like this. But... perhaps we could change it to this approach if ng-csp was set?

[oliversalzburg]

This now also triggers an error in Chrome.

[peruukki]

@thesam do you have any suggestion for how we could test for this FF bug (which is a security vulnerability) without the inline style?

I went through the details I could find on this:

• the Angular.js commit that introduced this CSP violation: fix($sanitize): use appropriate inert document strategy for Firefox and Safari
• the Angular pull request that introduced the CSP violation in angular/core: fix(core): use appropriate inert document strategy for Firefox & Safari
• related "Details about the Security Issue" section in DOMPurify 0.6.7 release notes
• related Firefox bug report HTML Parser ambiguity with inline svg and nested styles

It doesn't seem that the inline style tag is relevant; if I replace it with span, the bug is still detected in Firefox (and not in Chrome or Safari). I verified it by adding a console.log after if (inertBodyElement.querySelector('svg img')) { (in the Angular.js code) and if (this.inertBodyElement.querySelector && this.inertBodyElement.querySelector('svg img')) { (in the Angular code).

Also the related unit test description is "should not allow JavaScript hidden in badly formed HTML to get through sanitization (Firefox bug)", and none of the related discussions really mention the style tag. The bug is more related to the HTML structure than the actual tags if I understood correctly (though some tags like img may still be relevant).

Now that Angular.js is in Long Term Support, would it still be worthwhile to fix this? I guess this doesn't really count as a "security flaw".

[crhistianramirez]

I would argue that it does count as a security flaw because now my only course of action is to effectively cripple my content security policy by adding 'unsafe-inline'

[peruukki]

Thanks for your input @crhistianramirez, I went ahead and created a pull request, let's see how it goes. 🙂