1719026 - Mozilla Firefox Focus for Android - UXSS

Status: RESOLVED FIXED

(Focus :: General, defect)

Description

[James Lee]

The problem is window.open() runs on top page as a parent site from the inside of nested x-origin iframe(s). Surprisingly, running javascript: uri with window.open() was enough to access to the top parent site.

An alert msgbox will show top document (parent site) URL when you open the PoC. This all work without any user interaction, no matter how many other x-origin iframes are nested. (No limitations from determined attacker for widespread attacks and further)

Top page PoC: <iframe width=1000 height=800 src="https://x-origin-ifrsite.com/test.html"></iframe>

(https://x-origin-ifrsite.com/test.html) x-origin iframe PoC: <script>window.open('javascript:alert(parent.document.domain)');</script>

Comment 1

[Daniel Veditz [:dveditz]]

It's a semi-Universal XSS since the victim site would have to frame the malicious site. That means that a least attackers can't go crazy and hit random bank sites as with an unrestricted UXSS, but if you imagine malicious ads for example this is still super bad.

Comment 2

[Frederik Braun [:freddy]]

Oh, this is bad. I think this could be worse and there might be a way to remove the caveat that the victim websites needs to frame the attacker by using popups? Either way, we should fix it quickly.

Comment 3

[Frederik Braun [:freddy]]

st3fan: This is a candidate for "fix as soon as possible". Can you help us getting the right people involved?

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

My test page:
http://pocmo.de/bmn/parent.html

The implementation in Fenix / Android-Components does not seem to be affected. Focus is going to switch to this in 92.0 too. The problem seems to be in the original, legacy integration of GeckoView in Focus. I'll dig into Focus code to figure out what's happening.

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

This is caused by the following snippet in Focus (release branch, main branch is not affected):
https://github.com/mozilla-mobile/focus-android/blob/release/app/src/main/java/org/mozilla/focus/web/GeckoWebViewProvider.kt#L507-L510

Whenever we see a load request with a "new window" target, we deny the load and instead load the URL in the current gecko session (tab). And it looks like this has been the case in Focus since forever.

I'm not sure yet what the best fix for the release branch is. We are going to ship Focus 91 and 92 from the release branch and with 93 we will cut release branches from the main branch again, which is build on top of the same Android Components as Fenix (and has a proper "window request" implementation). So we will need a fix for 91 and 92 on the release branch. Adding proper "window request" to the legacy code base of Focus is properly out of scope. Alternatively we could consider ignoring window requests (without loading the URL in the current tab as we do now) in 91/92. Not great either.

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Alternatively we could consider ignoring window requests (without loading the URL in the current tab as we do now) in 91/92. Not great either.

Something like:

                     val complete = when {
                         request.target == GeckoSession.NavigationDelegate.TARGET_WINDOW_NEW -> {
-                            geckoSession.loadUri(request.uri)
                             AllowOrDeny.DENY
                         }

Alternatively, maybe we could only load the URL in the parent if it's http/https (avoiding to break a bunch of use cases). But I am afraid this will also have security implications?

                     val complete = when {
                         request.target == GeckoSession.NavigationDelegate.TARGET_WINDOW_NEW -> {
-                            geckoSession.loadUri(request.uri)
+                            if (UrlUtils.isHttpOrHttps(request.uri)) {
+                                geckoSession.loadUri(request.uri)
+                            }
                             AllowOrDeny.DENY
                         }

Comment 7

[Daniel Veditz [:dveditz]]

I don't have the time to digest that code to see what context we know at that point. Is this code only fired for window.open()? Options:

1. You might be able to simply deny opening javascript: urls in in new windows since it will almost never make much sense, especially in a single-tab browser where there's no opener to talk back to.
2. Alternatively you could clear the window's context first before running the javascript: url. Still might make no sense, but anything self-contained (if that ever happens) would at least still work. how? not sure from Java. Maybe load data:text/plain, into it first to force a null-principal context?

Can't break sites navigating themselves to a javascript: url--too many sites do that as button/link actions--but it's dangerous when targeted at another window/frame. parent.location = "javascript:something" is dangerous too, but I think that might be handled inside GeckoView for you (it should inherit the context of the running script, not the container it's going into).

Comment 8

[Daniel Veditz [:dveditz]]

The sec team decided this meets the criteria for sec-critical because it's a reliable and easy attack vector that could spread quickly once known, and will be easy to deduce from most imaginable ways of patching this.

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 0001-Update-window-handling.patch

Comment 10

[Christian Sadilek [:csadilek]]

Comment on attachment 9230441 [details] [diff] [review]
0001-Update-window-handling.patch

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

Looks good to me as a solution until 92. Let's improve the `isJavaScriptUrl` though to make it case insensitive.

::: app/src/main/java/org/mozilla/focus/utils/UrlUtils.java
@@ +59,5 @@
>  
>      }
>  
> +    public static boolean isJavaScriptUrl(String url) {
> +        return url.startsWith("javascript:");

Let's call `url.toLowerCase()` first or use `Uri.parse(url).normalizeScheme().scheme` instead for the comparison.

Comment 11

[Frederik Braun [:freddy]]

How confident are we that a block-list approach won't be bypassed?

In particular (but certainly not exhaustive!)

• How does geckoSession.loadUri deal with whitespaces at the beginning of the URL?
• What about line-breaks and url-encoding?

Comment 12

[Sebastian Kaspari (:sebastian; :pocmo)]

The request.uri that is passed to us in NavigationDelegate.onLoadRequest() is coming from Gecko(View) and is already cleaned up - and I assume we can trust Gecko? I can add an additional clean up step in Kotlin on top (I'll attach another patch for that), but I do wonder whether this doesn't actually broaden the theoretical attack vector (tricking the clean up step / parser).

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 0002-Update-window-handling.patch

Comment 14

[Sebastian Kaspari (:sebastian; :pocmo)]

I am unable to request sec-approval here. So I'll use NI and the form from the docs instead.

[Security approval request comment]

How easily can the security issue be deduced from the patch?

Likely very easily - especially with the explicit check for javascript: uris in this context.

Do comments in the patch, the check-in comment, or tests included in
the patch paint a bulls-eye on the security problem?

Not directly. We only need to apply this patch to the release branch (for the 90.0 based release next week) and maybe can fold it into the version bump commit.

Which older supported branches are affected by this flaw?

The release branch in the Focus repository is affected and what we are shipping 90 from. Starting with 91 we will continue shipping from our main branch again. Most code on main (and the one affected here) was replaced with the Android Components implementation that Fenix uses too.

If not all supported branches, which bug introduced the flaw?

It looks like this has been in Focus for a very long time almost since the time we started using GeckoView.

Do you have backports for the affected branches? If not, how
different, hard to create, and risky will they be?

N/A

How likely is this patch to cause regressions; how much testing does
it need?

I think the patch is unlikely to cause regressions since it only changes the behavior for "javascript:" uris that target a new window.

Comment 15

[James Lee]

Thanks all for swiftly fixing this!

(In reply to Daniel Veditz [:dveditz] from comment #1)

It's a semi-Universal XSS since the victim site would have to frame the malicious site. That means that a least attackers can't go crazy and hit random bank sites as with an unrestricted UXSS, but if you imagine malicious ads for example this is still super bad.

Note that we can also do unrestricted UXSS using the behavior from https://bugzilla.mozilla.org/show_bug.cgi?id=1719814

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 9230493 [details] [diff] [review]
0002-Update-window-handling.patch

sec-approval=dveditz for landing this in the GV-90 based Focus release. (and everywhere else we need it -- but that branch for sure also)

Comment 17

[Christian Sadilek [:csadilek]]

Patch landed and is about to ship in Focus 8.17.1: https://github.com/mozilla-mobile/focus-android/commit/f0e44adf88af7ab1cc5b7ef831dfbc7ccff8c900

Comment 18

[Daniel Veditz [:dveditz]]

We have awarded a bounty for this bug, where the severity (and the bounty amount) is increased by the increased attack surface offered in combination with bug 1719814.