1813463 - (CVE-2024-0749) Phishing site opens popup which falsely reports "This page is stored on your computer"

Status: VERIFIED FIXED

(Firefox :: Site Identity, defect, P2)

Description

[Kestrel]

Attachment: Popup window claiming site is local file

STR:

1. Set browser.safebrowsing.phishing.enabled = false to bypass the deceptive site warning.
2. Visit https://twitch.gift/csgo.
3. Click "Get free item".
4. Input code: "32TZA6NJQ".
5. Click "Enter".
6. Click "Take my prize".

Popup window opens login page for steampowered.com, address bar displays https://twitch.gift/csgo but the site icon shows a local file and reports:

Site information for about:blank
This page is stored on your computer.

Before the regression:

Site information for about:blank
Connection not secure

Regression window:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f3a829091fef65cf49209ed374688cdd6d5753ed&tochange=e33aea19d0c50b6b5eec8a45a8b21abcfdc79c85

Most likely regressed by Bug 1570678.

Comment 1

[Paul Zühlcke [:pbz]]

Unfortunately (or fortunately, it's a scam site!) the site from the STR is no longer available. Do you have a different site that shows this bug?

Comment 2

[Kestrel]

A currently active URL is https://twltchs-camp.com/csgo with code 57YFl1.

Comment 3

[Paul Zühlcke [:pbz]]

Thanks! I can reproduce. Looking into it. What makes this annoying to debug is that they obfuscate their JS code and inject debugger statements.
I've marked this as a sec bug because it may trick users into thinking the site is safe.

Comment 4

[Paul Zühlcke [:pbz]]

The issue seems to be that this is treated as an about:blank page. Chromium also shows it as such. Firefox shows the page URI, but still shows all the security UI as if it was about:blank, which is bad.

Comment 5

[Paul Zühlcke [:pbz]]

What I think is happening is that the top BrowsingContext of the popup dispatches an onSecurityChange message via nsIWebProgressListener. However, at the time of dispatch the URI is still about:blank. You can see that in the log here:
https://gist.github.com/Trikolon/27ee3d52de198a4a7cf6e034fefcd7c1#file-firefox-bc-log-L3-L6
The site identity code uses this URI to update the security UI, such as the identity icon, to "local page", because it assumes about:blank.

Shortly after an OnLocationChange is triggered as shown in the log, updating to the final URI that is shown in the urlbar:

[Parent 137456: Main Thread]: I/BCWebProgress OnLocationChange({isTopLevel:1, isLoadingDocument:0}, <null>, https://twltchs-camp.com/csgo, LOCATION_CHANGE_SAME_DOCUMENT) on {top:1, id:100000001, url:https://twltchs-camp.com/csgo}

This however does not cause an identity block update, because gIdentityHandler.updateIdentity is only ever called as the result of an onSecurityChange. There are no further calls to onSecurityChange.

Nika, do you know if this is expected behavior from the platform / BrowsingContext side? I'm trying to figure out where exactly we need to fix this bug. I unfortunately don't have a minimal PoC because the site obfuscates its JS code quite well.

Comment 6

[Paul Zühlcke [:pbz]]

Attachment: 2023-02-09_19-54-popup-in-chromium.png

Here is what the popup looks like in Chromium. This looks to be an about:blank page controlled by the parent with a borderless iframe injected.

Comment 7

[Nika Layzell [:nika] (ni? for response)]

I think the URL bar updating is a similar situation to bug 1814019, which I was looking at last week. The site probably opens an about:blank pop-up window, and then loads content into the page. The difference in behaviour between Chrome and Firefox is potentially because the page fills the about:blank document with a command like document.write, which as noted in bug 1814019 will cause window.location to be updated based on the calling principal (in this case the phishing site). For Firefox, we then update the URL bar to reflect the new location, whereas in chrome, the URL bar continues to say "about:blank". I don't think that updating the URL bar to more accurately reflect the actual origin of the content being shown is a problem though, and it's perhaps fine that we don't do a security update in that situation as well, as nothing has actually changed with regards to the security situation.

Given that it would be possible for the website to have updated the content either way without causing the location bar to be updated, so that it would stay on about:blank, we should perhaps make a point of updating the security dialog for content-controlled about:blank documents to no longer state that "This page is stored on your computer" in general? We should perhaps be showing the security principal of the controlling document instead in the security dialog in this case.

We could also try to do a security update when we do this location change, but overall I think that a large issue here is that we claim content in an about:blank document is stored locally. The actual document being rendered in that situation didn't end up changing, just its window.location.

Comment 8

[Nika Layzell [:nika] (ni? for response)]

Attachment: potential poc

While I haven't confirmed that the actual phishing website uses this strategy (as noted, the code is annoying to look at due to frequent debugger; statements, and general obfuscation strategy), I'm guessing that it uses an approach similar to the document.write one I've quickly implemented in this file. I've also included an example which doesn't update the URL bar (leaving it at about:blank), but where the content is still controlled by the website.

We probably want to make sure that the security info for these documents reflects that the document is under the control of web content.

EDIT: I should note that in the second example I showed, the URL bar doesn't even show security info, just showing the magnifying glass, as it is considered to be a blank document.

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1570678

Comment 10

[Paul Zühlcke [:pbz]]

Sorry for the late response, just getting back to this now.

I don't think that updating the URL bar to more accurately reflect the actual origin of the content being shown is a problem though, and it's perhaps fine that we don't do a security update in that situation as well, as nothing has actually changed with regards to the security situation.

Agreed, I think updating the url bar is good. I understand that conceptually the security situation has not changed. But could we still fire a security change when we update the location (because of document.write) so that the frontend shows the correct state? I would rather keep the authority for the security state in the backend.

Comment 11

[Nika Layzell [:nika] (ni? for response)]

I'm not sure I understand why we'd want to fire a security change notification in this situation, as there's been no change to the security state of the document - the exact same document is still loaded, and it has the exact same principal and other security information as before, the only difference is the URL bar being updated. It's sort-of like how the URL changes when you call pushState() or popState().

I suppose I just am not convinced that there's anything which should be updated in the security state backend in this case, and that if it seems to be wrong right now, it was also wrong before the URL changed, and we should be updating what the security dialog looks like in that case.

Comment 12

[Paul Zühlcke [:pbz]]

Attachment: WIP: Bug 1813463

Comment 13

[Paul Zühlcke [:pbz]]

We have two issues here:

1. Showing about:blank in URLBar and identity section when the current window / tab is actually site-controlled. Ideally the user should be able to see what controls the tab looking at any of the two UI surfaces.
2. For this specific edge case where the URI is updated via JS on an about:blank page which leads to a disconnect between the URI in the URLBar and the info in the identity panel. I've attached WIP a patch that targets this specific case.

Fixing (1) isn't trivial (and not as problematic) so I think we should focus on (2) for now.

Comment 14

[Paul Zühlcke [:pbz]]

After some discussion with Nika we agreed that the main security issue is that we show about:blank even though the page is controlled by the site that opened it. This combined with our message "This page is stored on your computer" may give the user a false sense of security. They may assume that it is a trusted page.
We can address this by getting the URI for the identity panel from gBrowser.contentPrincipal and gBrowser.contentPrincipal.precursorPrincipal rather via gBrowser.currentURI. The content principal is a more accurate representation of the entity controlling the given tab, particularly for the about:blank case discussed in this bug.

A potential downside of this approach is that we create a disconnect between the URI shown in the UrlBar and the URI / domain shown in the identity panel. This may be OK for the edge case in this bug, e.g. showing about:blank in the UrlBar while showing example.com in the identity panel, but there may be other edge cases I'm missing here.

Comment 15

[Paul Zühlcke [:pbz]]

Attachment: WIP: Bug 1813463 - Update the identity section from principal.URI rather than gBrowser.currentURI

Comment 16

[Paul Zühlcke [:pbz]]

Gijs, what do you think about the approach I outlined in comment 14? Are you aware of any edge cases we should be mindful of when using the content principal URI rather than the browser URI?

I've attached a draft for the patch in comment 15.

Comment 17

[Paul Zühlcke [:pbz]]

Unfortunately this approach breaks internal pages like about:certerror, their principal has an about URI, e.g.: about:certerror?e=nssBadCert&u=https%3A//expired.badssl.com/&c=UTF-8&d=%20
Curious we could get away with only using the principal URI for the about:blank case.

Comment 18

[:Gijs (he/him)]

(In reply to Paul Zühlcke [:pbz] from comment #16)

Gijs, what do you think about the approach I outlined in comment 14? Are you aware of any edge cases we should be mindful of when using the content principal URI rather than the browser URI?

I've attached a draft for the patch in comment 15.

load errors is the main one which you've already discovered. Otherwise I think it should be OK. You're basically fixing bug 1525943.

I could have sworn there was a bug on file to show the controlling URI in the address bar even for non-document.write'd about:blank, but can't find it at the moment.

The change to onLocationChange has performance implications. I don't know if we can avoid that.

(In reply to Paul Zühlcke [:pbz] from comment #17)

Unfortunately this approach breaks internal pages like about:certerror, their principal has an about URI, e.g.: about:certerror?e=nssBadCert&u=https%3A//expired.badssl.com/&c=UTF-8&d=%20
Curious we could get away with only using the principal URI for the about:blank case.

I'd sooner use the principal URI everywhere except about:neterror/about:certerror (and maybe about:blocked, if that has the same problem? If so, how does that relate to comment 0 and the fact this was originally reported on a safebrowsing-hit site?)

Comment 19

[Paul Zühlcke [:pbz]]

Nice, thanks for linking the bug! Looks like we can land a fix there instead.

and maybe about:blocked, if that has the same problem? If so, how does that relate to comment 0 and the fact this was originally reported on a safebrowsing-hit site?

Yes, about:blocked as the same issue. I don't think this bug is specifically related to Safe Browsing errors. The reporter explicitly disables the warning in the STR.

I need to check where we do the URI display overrides for these pages. It would be good if we could have a single point of truth for them, rather than having another list for URIs where we shouldn't use the principal URI. Or perhaps we just skip any case where the principal URI is an about: URI itself?

Comment 20

[:Gijs (he/him)]

(In reply to Paul Zühlcke [:pbz] from comment #19)

Nice, thanks for linking the bug! Looks like we can land a fix there instead.

and maybe about:blocked, if that has the same problem? If so, how does that relate to comment 0 and the fact this was originally reported on a safebrowsing-hit site?

Yes, about:blocked as the same issue. I don't think this bug is specifically related to Safe Browsing errors. The reporter explicitly disables the warning in the STR.

I need to check where we do the URI display overrides for these pages. It would be good if we could have a single point of truth for them, rather than having another list for URIs where we shouldn't use the principal URI. Or perhaps we just skip any case where the principal URI is an about: URI itself?

In the content process there's failedChannel and stuff. I don't know if we have something in the parent process to distinguish error page loads, though this has some interesting overlap with an otherwise unrelated issue I'm running into atm around what to do for fixupAndLoadURI string input that we can't process into a URL (ie for which we show an error page straight away, without even trying to connect). I expect Nika knows how best to recognize an error page load in the parent.

Comment 21

[Nika Layzell [:nika] (ni? for response)]

Internal error pages like about:neterror are interesting, as technically the page being shown is in fact stored on your computer, but yes you probably want to be showing security information about the page which was originally going to be loaded.

The page will be loaded with LOAD_ERROR_PAGE, and in the content process we definitely track that it's an error page, but I don't know off the top of my head of e.g. a flag on WindowContext tracking this, though it could probably be added.

Comment 22

[Paul Zühlcke [:pbz]]

I unfortunately don't have enough time to further work on this bug. I'll try to find a new assignee.

Comment 23

[Paul Zühlcke [:pbz]]

Sorry, didn't mean to clear the NI.

Comment 24

[Benjamin VanderSloot [:bvandersloot]]

I'm stepping into this bug and playing around with some of Paul's old patches from Bug 1525943.

One problem I'm seeing is that in the onSecurityChange notification to the popup sets the security state to STATE_IS_INSECURE via the aState argument. This state helps define what icon to show- lock, paper, etc. Should we keep the security state on the predecessorPrincipal to allow that to propogate to the about:blank page as well? Or should there be some reaching onto the opener directly when we realize the we inherit the security context? Or something more general and a bigger change?

Comment 25

[Nika Layzell [:nika] (ni? for response)]

We shouldn't be adding any extra flags into the predecessor principal - information like STATE_IS_INSECURE isn't part of the document's principal. I also don't think we should be reaching through opener (the target there is dynamic and could e.g. be navigated or cross-origin).

I think perhaps I don't understand the existing setup and how it's supposedly working right now very well. My understanding was roughly that we would determine the security status of the about:blank pop-up or similar based on the principal which it has, and would communicate that specific information. WE can't be showing things like SSL certificates and whether the document was loaded with one or anything like that because the about:blank document wasn't loaded with one, so we are just showing information based on the principal.

Perhaps there's a different icon we should be showing in this sort of locally-controlled about:blank document case? I imagine that historically we've just shown an icon indicating that it's a local document (a page or similar perhaps?), and that seems like it's probably fine to show still? IIRC the bug here was that we don't communicate that the document is controlled by a webpage in the info dialog text.

Comment 26

[Benjamin VanderSloot [:bvandersloot]]

We shouldn't be adding any extra flags into the predecessor principal - information like STATE_IS_INSECURE isn't part of the document's principal. I also don't think we should be reaching through opener (the target there is dynamic and could e.g. be navigated or cross-origin).

:+1: those approaches were giving me doubt.

Admittedly I don't understand the existing setup and how it's working right now that well either- this is my first time around this code.

I think I misunderstood the desired outcome. I thought you wanted the entire identity popup (including the icon and certificate information) to be that of the predecessor. That obviously isn't true given your last comment. And adding SSL information would probably be misleading, so I agree here. Also, in that case I think the "document" icon is fine. I think I was basing my misunderstanding of what you wanted off of Paul's comment:

After some discussion with Nika we agreed that the main security issue is that we show about:blank even though the page is controlled by the site that opened it. This combined with our message "This page is stored on your computer" may give the user a false sense of security. They may assume that it is a trusted page.

So, to clarify, the ideal scenario here would be a document icon in the address bar that when clicked provides an identity panel that says "Site information for predecessor.tld" and not show a "This page is stored on your computer" message?

I have a follow up question: what should we put in place of the "This page is stored on your computer" message? The current set of alternatives in the panel don't have me jumping at a better idea. Perhaps we could drop the message entirely, add a new alternative, or re-design a more general string that doesn't imply a file://? This would be the only instance of an identity panel not having one of those messages, so that idea doesn't have me jumping either. I lean toward a string re-design, e.g. "No connection"?

Comment 27

[Nika Layzell [:nika] (ni? for response)]

I'm probably not the person to be asking for what to write in that location. I don't know what the aims of the dialog are intended to be, so it's hard to say how it should act in this case. We could perhaps do something where we look at whether the precursor is for a secure scheme (like https), and then say something like "This page is controlled by another webpage"? (I worry that would be confusing, though)

EDIT: We might also want to make it so that the controlled about:blank pop-up shows something like this in the normal case, rather than the search icon?

Comment 28

[:Gijs (he/him)]

So... my loose understanding of this stuff is that the security state notifications were originally set up so we could also communicate things like "this is using TLS but some bits of the page are mixed content" or "this is using TLS but it's using a cert for which an exception was added" or "this is using old encryption that we will stop trusting in the future" (RC4 / SHA1 and whatnot, back in the day). Matt Woodrow last substantially touched the setup of the security change stuff in bug 1631405. I think Dana might know more about it, too?

The STATE_IS_INSECURE bit is based on the URL not using https - https://searchfox.org/mozilla-central/rev/6f0792f9f3f1a6175ce1f8e318a8a0f51ba76dee/dom/ipc/WindowGlobalChild.cpp#232-237 and https://searchfox.org/mozilla-central/rev/6f0792f9f3f1a6175ce1f8e318a8a0f51ba76dee/security/manager/ssl/nsSecureBrowserUI.cpp#59,65 .

AIUI, about:blank that is window.open'd that then loads http: content would count as "mixed content" so I'm surprised that for an about:blank doc we wouldn't treat the doc as "secure" based on the principal. AIUI that would potentially (counterintuitively...) improve things here? But I don't know what other impact that might have...

Comment 29

[Benjamin VanderSloot [:bvandersloot]]

An about:blank that has been window.open'd seems like it should at most inherit the security state of the document that opened it. Just treating it as secure would provide a way for an insecure document to create a window that looks secure to the user. That would require putting the security state information along with the inherited principal. I think Nika didn't want that and I tend to agree.

An alternative approach to solving this would be to not update the identity block in the document.write case. This makes it match the innerHTML PoC where the urlbar shows about:blank and has a search icon. FWIW Chrome has similar behavior, but with a different icon. We can then add a UI change to show a different icon in this case because the search icon doesn't make a ton of sense here, but isn't actively harmful.

That would let us resolve the sec-medium aspect more directly (since the search icon doesn't tell you a page is a file on your local computer), maybe updating the search icon to the file for this case, and we then deal with "what is the best UI for this corner case" independently. The downside to this approach is a reduced likelihood of actually getting to that last step :).

Comment 30

[Benjamin VanderSloot [:bvandersloot]]

The task of identifying precisely "the document.write case," left as an exercise for the reader, was of course, non-trivial.

I think I have a good fix based upon the abandoned patches and the discussions here and on Phabricator. I'm going to sleep on it and submit it for review.

Comment 31

[Benjamin VanderSloot [:bvandersloot]]

Attachment: Bug 1813463 - Treat web-controlled about:blank differently than local files in the site identity UI - r=pbz!,Gijs!,nika!

Depends on D193804

Comment 32

[Benjamin VanderSloot [:bvandersloot]]

Attachment: Bug 1813463 - Add tests to validate the icon, message, and origin displayed for associated popup windows - r=gijs!

This adds testing for this bug and the previous ones in the stack

Depends on D193806

Comment 33

[Phabricator Automation]

Comment on attachment 9363934 [details]
Bug 1813463 - Treat web-controlled about:blank differently than local files in the site identity UI - r=pbz!,Gijs!,nika!

Revision D193806 was moved to bug 1525943. Setting attachment 9363934 [details] to obsolete.

Comment 34

[Benjamin VanderSloot [:bvandersloot]]

Fixed in patches for Bug 1525943. Marking as in-testsuite=? to remember to land tests in https://phabricator.services.mozilla.com/D194901.

Comment 35

[Daniel Bodea [:danibodea]]

This issue could not be reproduced because the test page (https://twitch.gift/csgo) cannot be found. Is there another that we could use to reproduce the issue and verify fix? Is there anything that we might be missing? Would this website be locked in a specific region?

Comment 36

[Benjamin VanderSloot [:bvandersloot]]

The test page was taken down as it was a scam site. You can verify using Nika's PoC (clicking the links to open the popups): https://bugzilla.mozilla.org/attachment.cgi?id=9317389

Comment 37

[Daniel Bodea [:danibodea]]

I have reproduced the issue in Release v121.0.1: When loading https://bugzilla.mozilla.org/attachment.cgi?id=9317389 and clicking "document.write example", the newly opened window's information would show "This page was stored on your computer."

In Beta v122.0b8 and Nightly v123.0a1, performing the same steps would show in the pop-up's page info the text: "This page is loaded from another page.". Reproduction and verfication was performed in Windows 10, Mac OS 11 and Ubuntu 22.

Comment 38

[Jesse Schwartzentruber (:truber)]

Attachment: advisory.txt

Comment 39

[BugBot [:suhaib / :marco/ :calixte]]

2 months ago, bvandersloot placed a reminder on the bug using the whiteboard tag [reminder-test 2024-02-20] .

bvandersloot, please refer to the original comment to better understand the reason for the reminder.

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a2e8a3a99f07
Add tests to validate the icon, message, and origin displayed for associated popup windows - r=Gijs

Comment 41

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

Backed out for causing bc failures in browser_identity_web_controlled_blank.js:
https://hg.mozilla.org/integration/autoland/rev/682b6de9834db4460b2522bb74c9393f55bb0190

Push with failures
Failure log

TEST-UNEXPECTED-FAIL | browser/base/content/test/siteIdentity/browser_identity_web_controlled_blank.js | Uncaught exception in test bound test_document_write - at chrome://mochitests/content/browser/browser/base/content/test/siteIdentity/browser_identity_web_controlled_blank.js:43 - TypeError: can't access property "querySelector", popupWindow.ownerDocument is undefined

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5b23520f478a
Add tests to validate the icon, message, and origin displayed for associated popup windows - r=Gijs

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5b23520f478a

Comment 44

[Daniel Veditz [:dveditz]]

Making Firefox 122 security bugs public. [bugspam filter string: Pilgarlic-Towers]

Comment 45

[Daniel Veditz [:dveditz]]

Sorry for the burst of bugspam: filter on tinkling-glitter-filtrate
Adding reporter-external keyword to security bugs found by non-employees for accounting reasons