1667456 - (CVE-2021-23998) SSL Spoofing due to Attacker page (HTTP: URL) opened on a popup by a Tab who contains a secure Domain (HTTPS: URL)

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P3)

Description

[Jordi Chancel]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:78.0) Gecko/20100101 Firefox/78.0

Steps to reproduce:

STR 1: Go to the Testcase and click to the button "Click Me" (a New Popup will be opened with the Attacker Webpage with the HTTP: URL).
(The Testcase should be opened by the "HTTPS:" URL with a Valid SSL Indicator or should loaded another "HTTPS:" URL Target Domain after the click on button by user to open the Popup)

STR 2: On the new Popup opened Click to the Button "Click Me" on the Attacker webpage (HTTP: URL) .
(The HTTP:// webpage on the Popup will open a new tab LEADING TO SSL Spoofing)

RESULTS:
This Leads to a SSL Spoofing. The HTTP:// webpage opened on a new tab by the Popup Attacker Page (Same HTTP: URL) have now a SSL Indicator leading to a SSL Spoofing.

SSL Spoofing due to Attacker page (HTTP: URL) opened on a popup by a Tab who contains a secure Domain (HTTPS: URL)

Actual results:

This Leads to a SSL Spoofing. The HTTP:// webpage opened on a new tab by the Popup Attacker Page (Same HTTP: URL) have now a SSL Indicator leading to a SSL Spoofing.

Expected results:

This leads to a SSL Spoofing security issue.

Comment 1

[Jordi Chancel]

SSL SPOOFING TESTCASE URL: http://www.alternativ-testing.fr/Research/Mozilla/ed4dcfg68cc4fg18h7f4d5z9m34q/SSL%20Spoofing%20WebPage%20One.html

Comment 2

[Jordi Chancel]

Attachment: Screenshot SSL Spoofing Result (Secure SSL indicator lock icon on HTTP URL).png

Screenshot SSL Spoofing Issue

Comment 3

[:Gijs (he/him)]

Thanks for the report, Jordi.

Is the user interaction in the popup required? I would have expected that the popup could navigate / write to the newly opened window without further interaction...

If you use a different https URI than addons.mozilla.org, like google.com or https://example.com, does the spoof still work?

Matt or Marco, can either of you take a look? If I switch away from the spoofed tab and back again, the lock icon is updated correctly. Feels like there's some kind of race condition, and because the AMO page gets opened in a different process, I wonder if there's some confusion around the process switching

Comment 4

[Jordi Chancel]

(In reply to :Gijs (he/him) from comment #3)

Thanks for the report, Jordi.

Is the user interaction in the popup required?

I am not sure but it is possible that the click user interaction on popup is may-be not necessary and replaced by another method to Open the new tab leading to the SSL Spoofing security-issue (I will try others methods to determine if the click user interaction on popup is required or not).

If you use a different https URI than addons.mozilla.org, like google.com or https://example.com, does the spoof still work?

Yes you can use a different https URI than addons.mozilla.org like https://www.google.com or any other https URI that you want.

Regards,
Jordi Chancel

Comment 5

[Daniel Veditz [:dveditz]]

If we don't trim "http:" from the URL bar then at least that will show up (but hardly anyone knows to turn off that pref). If you click the lock it will say the about:blank page is not secure. But on the surface with default prefs this spoof does work.

What's the point of spoofing the secure state? Well imagine a wifi MITM where you could replace the content for http://accounts.google.com. User's wouldn't know they were on a fake Google, and you didn't need to get a fraudulent cert for Google.

Doesn't seem all that useful if you don't have a MITM though.

Comment 6

[Matt Woodrow (:mattwoodrow)]

This testcase is doing window.open().document.write(...) from the popup to open a new tab in the main browser window.

It looks like the relevant events Gecko sends to Firefox are:

onLocationChange(about:blank)
onSecurityChange(STATE_IS_INSECURE)
onLocationChange(http://www.alternativ-testing.fr/..., LOCATION_CHANGE_SAME_DOCUMENT)
-- no security change, since we don't send those for same-document navigations.

It looks like we don't update the security state for the onSecurityChange, because gIdentityHandler. _hasInvalidPageProxyState() returns true for the current about:blank document. This leaves the security state from the old tab that we switched away from, which is the secure AMO URL.

We then fire the onLocationChange for the new url from the document.write, but don't fire a new onSecurityChange since it's the same Document (this is documented and tested behaviour).

Maybe we only want to hide the security state for about:blank documents that have a system principal?

Comment 7

[Marco Bonardo [:mak]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #6)

It looks like we don't update the security state for the onSecurityChange, because gIdentityHandler. _hasInvalidPageProxyState() returns true for the current about:blank document. This leaves the security state from the old tab that we switched away from, which is the secure AMO URL.

This sounds like an optimization that was done to avoid flickering the security state with intermediate about:blank loads.

We then fire the onLocationChange for the new url from the document.write, but don't fire a new onSecurityChange since it's the same Document (this is documented and tested behaviour).

Maybe onLocationChange should always update the security state when coming from about:blank, or just do it when the previous onSecurityChange was skipped, we could annotate that we skipped it, and then handle it onLocationChange.

Maybe we only want to hide the security state for about:blank documents that have a system principal?

I wonder if this may cause the same flicker that the onSecurityChange is trying to avoid, where the identity shield would disappear and reappear for a short time during a load.

Comment 8

[:Gijs (he/him)]

(In reply to Marco Bonardo [:mak] from comment #7)

Maybe onLocationChange should always update the security state when coming from about:blank, or just do it when the previous onSecurityChange was skipped, we could annotate that we skipped it, and then handle it onLocationChange.

This seems like the straightforward fix here, yeah.

Comment 9

[:Gijs (he/him)]

Attachment: Bug 1667456, fix pageproxystate handling in the url bar, r?johannh

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/e6671fdfaf4920662fe931cb00e20a8731ae50f8
https://hg.mozilla.org/mozilla-central/rev/e6671fdfaf49

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Is this something we should consider for backport?

Comment 12

[:Gijs (he/him)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #11)

Is this something we should consider for backport?

Maybe, but I'd like to see this make its way through at least beta without issue first... Can we track for the next ESR, assuming that's what you mean?

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Works for me.

Comment 14

[Cornel Ionce [:noni] [Hubs QA]]

Reproduced this issue on macOS 11.2.3 Firefox 87.0.

Confirming the fix using the attached testcase on Firefox 88.0b8, buildID 20210406185740.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Is this ready for an ESR uplift?

Comment 16

[:Gijs (he/him)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #15)

Is this ready for an ESR uplift?

I haven't heard of any issues, so I think so, but Johann would have had more visibility into new URL bar / identity icon issues in the intervening time. Johann?

Comment 17

[Johann Hofmann [:johannh]]

I'm a little behind triage and NI but ignoring that I'm not aware of any regressions either.

Comment 18

[:Gijs (he/him)]

Comment on attachment 9201265 [details]
Bug 1667456, fix pageproxystate handling in the url bar, r?johannh

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-moderate
• User impact if declined: Potential for TLS spoofing, see comment 5
• Fix Landed on Version: 88
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Relatively small, straightforward JS change to update the identity block more frequently, with good existing test coverage to catch regressions, and has had a fair nightly + beta cycle to gather reports of regressions with none so far.
• String or UUID changes made by this patch: Nope

Comment 19

[Ryan VanderMeulen [:RyanVM]]

This needs a rebased patch for ESR78.

Comment 20

[:Gijs (he/him)]

As discussed with Ryan, the initial version of the phab revision which was reviewed does apply cleanly on esr; later versions rebased across bug 1596897 do not - just need to be careful not to land with the test that was in that initial revision! 😅

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Got it sorted with Gijs over Element.
https://hg.mozilla.org/releases/mozilla-esr78/rev/0dac34b853c6

Comment 22

[Ryan VanderMeulen [:RyanVM]]

The uplift to ESR caused browser_identityBlock_flicker.js to start permafailing, because we've effectively regressed bug 1570751 there.
https://treeherder.mozilla.org/logviewer?job_id=336227620&repo=mozilla-esr78&lineNumber=6268

This patch didn't cause issues when it landed on m-c due to the presence of bug 1570678 on 79+. After discussing with Gijs, we decided that the small visual glitch from bug 1570751 was less important than fixing this bug and decided to go ahead skip the test on ESR.
https://hg.mozilla.org/releases/mozilla-esr78/rev/ff36b70d85192fc8212fabf6477bfa3970ba714a

Comment 23

[Tom Ritter [:tjr]]

Attachment: advisory.txt

Comment 24

[Cornel Ionce [:noni] [Hubs QA]]

Also confirming the fix for 78.15.0esr (buildID 20210927121355)