Closed Bug 1667456 (CVE-2021-23998) Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox :: Address Bar, defect, P3)

80 Branch
defect
Points:
5

Tracking

()

VERIFIED FIXED
88 Branch
Tracking Status
firefox-esr78 88+ verified
firefox86 --- wontfix
firefox87 --- wontfix
firefox88 --- verified

People

(Reporter: jordi.chancel, Assigned: Gijs)

References

()

Details

(Keywords: csectype-spoof, sec-moderate, Whiteboard: [post-critsmash-triage][adv-main88+][adv-esr78.10+])

Attachments

(3 files)

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.

Screenshot SSL Spoofing Issue

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

Status: UNCONFIRMED → NEW
Component: Untriaged → Address Bar
Ever confirmed: true
Flags: needinfo?(matt.woodrow)
Flags: needinfo?(mak)
Flags: needinfo?(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

Flags: needinfo?(jordi.chancel)

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.

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?

Flags: needinfo?(matt.woodrow)

(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.

Flags: needinfo?(mak)
Severity: -- → S3
Priority: -- → P3

(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.

Points: --- → 5
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Attachment #9201265 - Attachment description: Bug 1667456, r?johannh → Bug 1667456, fix pageproxystate handling in the url bar, r?johannh
Blocks: 1695658
Group: firefox-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 88 Branch
Flags: qe-verify+
Whiteboard: [post-critsmash-triage]

Is this something we should consider for backport?

Flags: needinfo?(gijskruitbosch+bugs)

(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?

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(ryanvm)

Works for me.

Flags: needinfo?(ryanvm)

Reproduced this issue on macOS 11.2.3 Firefox 87.0.

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

Status: RESOLVED → VERIFIED
Flags: qe-verify+

Is this ready for an ESR uplift?

Flags: needinfo?(gijskruitbosch+bugs)

(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?

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(jhofmann)

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

Flags: needinfo?(jhofmann)

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
Attachment #9201265 - Flags: approval-mozilla-esr78?

This needs a rebased patch for ESR78.

Flags: needinfo?(gijskruitbosch+bugs)

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! 😅

Flags: needinfo?(gijskruitbosch+bugs)
Attachment #9201265 - Flags: approval-mozilla-esr78? → approval-mozilla-esr78+

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

Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main88+]
Attached file advisory.txt
Whiteboard: [post-critsmash-triage][adv-main88+] → [post-critsmash-triage][adv-main88+][adv-esr78.10+]
Alias: CVE-2021-23998
Group: core-security-release

Also confirming the fix for 78.15.0esr (buildID 20210927121355)

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: