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)
Tracking
()
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)
71.01 KB,
image/png
|
Details | |
48 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-esr78+
|
Details | Review |
179 bytes,
text/plain
|
Details |
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.
Reporter | ||
Comment 1•4 years ago
|
||
Reporter | ||
Comment 2•4 years ago
|
||
Screenshot SSL Spoofing Issue
Assignee | ||
Comment 3•4 years ago
|
||
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
Reporter | ||
Comment 4•4 years ago
|
||
(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•4 years ago
|
||
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•4 years ago
•
|
||
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•4 years ago
|
||
(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.
Updated•4 years ago
|
Assignee | ||
Comment 8•4 years ago
|
||
(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.
Updated•4 years ago
|
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 9•4 years ago
|
||
Updated•4 years ago
|
Comment 10•4 years ago
|
||
https://hg.mozilla.org/integration/autoland/rev/e6671fdfaf4920662fe931cb00e20a8731ae50f8
https://hg.mozilla.org/mozilla-central/rev/e6671fdfaf49
Updated•4 years ago
|
Comment 11•4 years ago
|
||
Is this something we should consider for backport?
Assignee | ||
Comment 12•4 years ago
|
||
(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•4 years ago
|
||
Works for me.
Comment 14•4 years ago
|
||
Reproduced this issue on macOS 11.2.3 Firefox 87.0.
Confirming the fix using the attached testcase on Firefox 88.0b8, buildID 20210406185740.
Assignee | ||
Comment 16•4 years ago
|
||
(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•4 years ago
|
||
I'm a little behind triage and NI but ignoring that I'm not aware of any regressions either.
Assignee | ||
Comment 18•4 years ago
|
||
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•4 years ago
|
||
This needs a rebased patch for ESR78.
Assignee | ||
Comment 20•4 years ago
|
||
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•4 years ago
|
||
uplift |
Got it sorted with Gijs over Element.
https://hg.mozilla.org/releases/mozilla-esr78/rev/0dac34b853c6
Updated•4 years ago
|
Comment 22•4 years ago
•
|
||
uplift |
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
Updated•4 years ago
|
Comment 23•4 years ago
|
||
Updated•4 years ago
|
Updated•4 years ago
|
Updated•3 years ago
|
Comment 24•3 years ago
|
||
Also confirming the fix for 78.15.0esr (buildID 20210927121355)
Description
•