1676311 - (CVE-2020-26977) Spoof URL by connecting to an invalid port with a setTimeout call in the originating tab

Status: RESOLVED FIXED

(Fenix :: General, defect)

Description

[u635660]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/86.0.4240.193 Safari/537.36

Steps to reproduce:

go to https://servicenger.com/t.html

click me

click ok
and then Facebook is spoofed

Actual results:

facebook.com is spoofed

Expected results:

it should not spoof

Comment 1

[u635660]

chrome is not affected on andriod and ios

Comment 2

[Kevin Brosnan [Ex-Mozilla]]

Amedyne can we find someone who knows the toolbar to look at this?

Comment 3

[Amedyne Moya [:amoya]]

:snorp is checking

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

I can't reproduce this on GVE, and in fact we never get a facebook.com URL at all in onLocationChange(). It's critical that Fenix only displays URLs delivered via onLocationChange() in the toolbar.

Comment 5

[James Willcox (:snorp) ([e-mail]) (he/him)]

Sorry, we do eventually get a https://www.facebook.com:1234/ in onLocationChange() once the connection times out and we get an error. That's expected.

Comment 6

[Emily Kager [:ekager]]

I'm looking into it

Comment 7

[Emily Kager [:ekager]]

First, we are getting an OPEN WindowRequest, which adds a new tab and dispatches an AddTabAction through the SessionManager. The AddTabAction uses toTabSessionState which codifies the initial URL (the Facebook URL) as the tab truth in the ContentState.

It then selects that tab, and since the tab has been selected and state changed, the ToolbarPresenter renders the new URL.

I propose removing the Android-Components Session initialUrl argument altogether, because it sounds like we should never be trusting URLs we pass in from our side. We should trust the onLocationChange callbacks after attempting a load of the initial URL.

Adding Sebastian from AC for feedback.

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

This seems to be related to us hiding the initial load of about:blank, that Gecko does:
https://github.com/mozilla-mobile/android-components/blob/master/components/browser/engine-gecko-nightly/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineSession.kt#L436-L441

We did this to workaround issues with about:blank appearing briefly in the toolbar, looking like a UI glitch.

In this situation here, the page actually injects content into that about:blank page and the load of facebook.com:1234 takes quite a while to timeout. If we'd stop suppressing the about:blank update then Fenix would behave like desktop and we'd display about:blank in the URL bar until the timeout, where we switch to facebook.com:1234 and show the error page. However that would potentially re-introduce the UI glitch. Unfortunately we do not seem to be able to distinguish between the initial about:blank load and the state where there's actually content getting injected. As a potential workaround for this we could try disabling the about:blank check for sessions that have a parent. That may be the smallest possible patch?

(In reply to Emily Kager [:ekager] from comment #7)

I propose removing the Android-Components Session initialUrl argument altogether, because it sounds like we should never be trusting URLs we pass in from our side. We should trust the onLocationChange callbacks after attempting a load of the initial URL.

Separating the "initial url to load" from the "content url" is an option too. But it will have a ripple effect on a lot of things and potentially cause regressions. I would like to look at all the options before this "heart surgery". :)

I started to look into Fennec source.. it doesn't reproduce in Fennec and we always have a URL for the tab - without the UI glitch happening afaik. Curious how this was solved there.

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

If we'd stop suppressing the about:blank update then Fenix would behave like desktop and we'd display about:blank in the URL bar until the timeout, where we switch to facebook.com:1234 and show the error page.

There are some edge cases where this does introduce other undesired behaviors (like showing about:blank when manually entering a URL and staying on about:blank when stopping the load).

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

There might be an easier solution after all. This problem only exists when opening a new window (link with a target), where we set the URL of the new tab to the given URL immediately. There's absolutely no need to do this, since we are not initiating the load - Gecko(View) does it.

Related code:
https://github.com/mozilla-mobile/android-components/blob/master/components/feature/tabs/src/main/java/mozilla/components/feature/tabs/WindowFeature.kt#L46-L62

Christian wrote WindowFeature and we looked at the various scenarios and this seems to be a viable option. I'll attach a patch for discussion.

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

https://github.com/mozilla-mobile/android-components/pull/8975

Comment 12

[Daniel Veditz [:dveditz]]

Attachment: bug1676311.html

Preserving testcase in case the reporter's server goes away.

Comment 14

[Tom Ritter [:tjr]]

Attachment: poc.html

Attaching the poc for preservation.

Comment 15

[Tom Ritter [:tjr]]

Attachment: advisory.txt

Comment 16

[Tom Ritter [:tjr]]

Attachment: advisory.txt