1658276 - Use CSP on Fenix error pages and other resources

Status: RESOLVED FIXED

(Fenix :: General, defect)

Description

[:Gijs (he/him)]

bug 1658144 and bug 1657055 are using XSS in the fenix error page to bypass cert warnings.

Now, of course Fenix should be getting its sanitization right, but I would also expect us to be able to lock this down with CSP. We already have CSP in place (at least on desktop; I assume also for Fenix?), can we make it a lot more strict so as to avoid these issues?

Comment 1

[:Gijs (he/him)]

Huh, so, having looked into this a bit more: as far as I can tell, the Fenix attacks cannot happen on desktop (CSP only allows script from chrome:, so inline script wouldn't run), and the issue is that Fenix's error page has no CSP and extensively uses innerHTML. Going to move this bug as a result.

Comment 2

[Daniel Veditz [:dveditz]]

On desktop the CSP and innerHTML sanitization formed a "chrome sandbox" that has saved our bacon several times. Missing that is a severe lack.

Comment 3

[Petru-Mugurel Lingurar [:petru]]

Filed https://github.com/mozilla-mobile/fenix/issues/16248 for fixing this on Fenix.

Comment 4

[Petru-Mugurel Lingurar [:petru]]

Gijs, can you please add me to the tickets from comment 0?
I expect there to see more details about how Fenix was/is affected to then work on a solution.

Comment 5

[:Gijs (he/him)]

(In reply to Petru-Mugurel Lingurar [:petru] from comment #4)

Gijs, can you please add me to the tickets from comment 0?
I expect there to see more details about how Fenix was/is affected to then work on a solution.

I've added you. I think the CSP here should block all script other than that from chrome: URIs. This will mean any inline script will stop working, so we should also switch from any existing inline JS event handlers to using dynamically added JS event handlers.
It'd probably be a good idea to do something similar for inline styles, though I'm less worried about those.

Comment 6

[:Gijs (he/him)]

Oh, and note that when I looked at this, it was a bit "interesting" because my impression was that some of the markup lives in android components, some in fenix, and ditto for script. I don't know how the interdependencies "should" work and if changing AC would mean updating consumers for the new error page world.

Comment 7

[David Bolter [:davidb] (NeedInfo me for attention)]

Stefan what is left to do here? This bug gets included in a list of high/crit sec bugs with no owner.

Comment 8

[Petru-Mugurel Lingurar [:petru]]

I understand we have something like this on iOS but AC / GV might not yet support this.
Christian, do you know if we have the right CSP APIs to leverage for this in AC or they have to be built now?

Comment 9

[Christian Sadilek [:csadilek]]

Christian, do you know if we have the right CSP APIs to leverage for this in AC or they have to be built now?

We don't currently have any A-C APIs for this, but I am also not sure we should need one. As I understand the ticket, the problem is that CSP rules are not applied for pages served from Android assets (resource:// URIs), right?

We could add a <meta http-equiv="Content-Security-Policy"... tag to our error page to verify and talk to the GV team to understand what we'd need to make this work. Currently, we simply return the resoure:// URI from onLoadError and let GV load the local page.

Comment 10

[Frederik Braun [:freddy]]

(In reply to Christian Sadilek [:csadilek] from comment #9)

We could add a <meta http-equiv="Content-Security-Policy"... tag to our error page to verify and talk to the GV team to understand what we'd need to make this work. Currently, we simply return the resoure:// URI from onLoadError and let GV load the local page.

This is sort-of what we do on Firefox for Desktop.
You might have to rewrite some bits of the page so that JavaScript resources are in their own file (instead of inline-events), but these are easy bugs which you could even hand out to contributors.
Once that's done, you can add a CSP via <meta> like we do on Desktop.
Our CSP is <meta http-equiv="Content-Security-Policy" content="default-src chrome:; object-src 'none'">. I suppose you'll have to add resource:, if I'm understanding comment 9 correctly.

Comment 11

[Petru-Mugurel Lingurar [:petru]]

After reversing Sebastian's patch from https://bugzilla.mozilla.org/show_bug.cgi?id=1657055 I tested with

<meta http-equiv="Content-Security-Policy" content="default-src resource:; object-src 'none'">

everything looks ok, https://self-signed.badssl.com/?%3Cimg%20src%3D%23%20onerror%3DacceptAndContinue(true)%3E doesn't bypass the error page
but our inline js also doesn't work.
To get the scripts working I'd have to add script-src resource: 'unsafe-inline' after which the above url would again bypass the error page.
So it looks like we first need to refactor the pages to not use inline scripts after which a simple CSP header will block XSS.

Comment 12

[Petru-Mugurel Lingurar [:petru]]

Asking csadilek what should be the course of action here:

• Use <meta http-equiv="Content-Security-Policy" content="default-src resource:; script-src resource: 'unsafe-inline'; object-src 'none'"> which must be used in conjuction with the proper sanitization (like in #1657055)
• Use <meta http-equiv="Content-Security-Policy" content="default-src resource:; object-src 'none'"> but having to refactor all the error pages to not use inline scripts.

Comment 13

[Frederik Braun [:freddy]]

The bug is just called "use CSP", but that's not super accurate. A CSP with 'unsafe-inline' is not adding any security.
You will only find a meaningful XSS mitigation if you refactor your internal pages not to use inline scripts.

Comment 14

[Christian Sadilek [:csadilek]]

Thanks :petru for testing and verifying, and :freddy for confirming how this should work with CSP!

So, yes, we would need to add the latter: <meta http-equiv="Content-Security-Policy" content="default-src resource:; object-src 'none'"> and refactor our error pages (error_page_js.html in A-C, as well as high_risk_error_pages.html and low_and_medium_rsik_error_pages.html in Fenix) to not rely on inline scripts.

Comment 15

[Petru-Mugurel Lingurar [:petru]]

The error pages that Fenix uses were refactored to not contain inline scripts anymore and then Content-Security-Policy" content="default-src resource:; object-src 'none' was set.
QA verified that after this there weren't any new issues in https://github.com/mozilla-mobile/fenix/issues/16248.
Closing this ticket as FIXED.