1914797 - (CVE-2024-11701) onbeforeunload event causes Fenix to mistakenly show wrong domain (e.g. your bank URL) in address bar while showing attacker-controlled content

Status: VERIFIED FIXED

(Firefox for Android :: Browser Engine, defect, P2)

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

STR:

1. Load attached testcase.
2. Focus the textfield in the testcase. (This sets up a beforeunload event handler
3. Tap the URLbar.
4. Type in any site URL (or even just a search term). E.g. type "https://www.citi.com" and submit.

EXPECTED RESULTS:

• beforeunload handler should cause a dialog to pop up, to check that I want to leave the site ("Data you have entered may not be saved").
• If my entered URL is in the URLbar, it should still look like it's being edited (e.g. no lock icon)

ACTUAL RESULTS:

• The browser's beforeunload dialog does not appear for some reason, though the event does fire and the testcase gets an opportunity to run some JS (adding some sneaky phishing text in this case).
• The location bar contents change to show the site that I entered, despite the fact that Firefox did not actually navigate.
  ** If you tap the lock icon, Firefox seems to confirm that you're actually looking at the site that you tried to navigate to, ssl certificate and all.

(Not sure precisely where this belongs; starting in Fenix:Browser Engine since its description mentions prompts, and the first issue that we stumble on here seems to be a prompt that we fail to pop up for some reason.)

Comment 1

[Daniel Holbert [:dholbert]]

Gaming out how this might be abused by an actual malicious actor:

I imagine this could be deployed in a typo-domain-name-squatting scenario.

e.g. supposing https://example.org is a bank -- maybe an attacker registers a bunch of similar typo sites like https://examplee.org (with an extra e at the end) and then sets up a beforeunload handler on their page there, like in my testcase. Then, if a user makes a typo and notices their mistake, then when the user goes correct their typo, they end up seeing Firefox tell them that they're on the correct site, when in fact they're still viewing attacker-controlled content.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: screencast of bug (notice the location bar and site-info-popup supposedly shows me at https://www.citi.com at the end, though I'm seeing attacker-controlled content from the testcase)

Comment 3

[Daniel Holbert [:dholbert]]

Interestingly, in the site-info-popup at the very end of the testcase, we seem to show the SSL validation info from the bugzilla testcase, mistakenly attributed to citi.com.

In particular, the last frame of the screencast shows:

https://www.citi.com
Connection is secure
Verified By: Let's Encrypt

...which is the cert provider for our bugzilla attachment URLs (the testcase here), not the cert provider for the actual https://www.citi.com website. (Citi seems to use DigiCert, if I go to their site "normally" (not via the STR in this bug) and view their site-info-panel.)

The fact that we can end up showing a mismatched pairing of certificate-issuer vs. site-address seems like something we should separately investigate and address; but it's a second-tier issue compared to the main issue here, about the address bar contents themselves being wrong.

Comment 4

[Daniel Holbert [:dholbert]]

This seems to have regressed in February:
INFO: Newest known good nightly: 2024-02-12
INFO: Oldest known bad nightly: 2024-02-13

Hashes in about-firefox dialog (which I think are git revisions at that point in history):
2024-02-12: 95e9fa455f
2024-02-13: 7658c55755

In the "last-good" build and builds before it: when I reach the end of the STR, the location bar shows the testcase's URL, instead of citi.com (and and so does the site-info popup, if you tap the lock icon). We still fail to show the beforeunload dialog which still seems like a bug, but at least we show the correct URL for the content that we're showing (and for the ssl issuer that we're showing in the site-info dialog, per additional details in comment 3)

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://github.com/mozilla-mobile/firefox-android/compare/95e9fa455f...7658c55755
https://bugzilla.mozilla.org/show_bug.cgi?id=1868469 in that range looks plausible.

Comment 6

[Daniel Holbert [:dholbert]]

Yup, looks quite likely to be a regression from that bug.

jonalmeida, could you take a look?

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #4)

INFO: Newest known good nightly: 2024-02-12
[...]
In the "last-good" build and builds before it: [...] We still fail to show the beforeunload dialog which still seems like a bug,

Minor followup on this^ note: the missing-dialog issue reproduces at least as far as Fenix Nightly 2021-12-21; that's the oldest Fenix Nightly I have locally available via my mozregression cache (and I don't think we have older ones easily available).

So I don't know if the missing-dialog is itself a regression or not; but if it is, it's a pretty old regression.

(Anyway, this is a bit of a tangent, since the missing-dialog isn't the proximal security problem here. I want to focus mostly on the address bar showing the wrong contents, which regressed in February per comment 4.)

Comment 8

[Jonathan Almeida [:jonalmeida]]

(In reply to Daniel Holbert [:dholbert] from comment #0)

ACTUAL RESULTS:

• The browser's beforeunload dialog does not appear for some reason, though the event does fire and the testcase gets an opportunity to run some JS (adding some sneaky phishing text in this case).

So from a read of the thread so far, these are the only expected results for this bug and the missing dialog one is separate one. We do have support for BeforeUnload events, so this regression has happened silently somehow - please file a bug and thank you for discovering!

(In reply to Daniel Holbert [:dholbert] from comment #0)

• The location bar contents change to show the site that I entered, despite the fact that Firefox did not actually navigate.
  ** If you tap the lock icon, Firefox seems to confirm that you're actually looking at the site that you tried to navigate to, ssl certificate and all.

For this case, the patch was meant to be a perceived performance fix, so we could revert it if the android sec triage deems it high for an immediate action.

I need to spend some time to understand how the fix causes this so I'm leaving the NI on for now.

Comment 9

[Daniel Holbert [:dholbert]]

the missing dialog one is separate one

Indeed, that's a separate (and older) issue.

please file a bug and thank you for discovering!

Sure! I'll file that one as security-sensitive, too, since reproducing-that-bug also reveals the existence of this bug.

Comment 10

[Daniel Holbert [:dholbert]]

(--> Filed bug 1915296 on the missing dialog.)

Comment 11

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

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

Comment 12

[Daniel Veditz [:dveditz]]

sec-high-ish, but to attack successfully the attack site has to know what you're going to type which means they have to suggest it and convince you.

Comment 13

[Daniel Holbert [:dholbert]]

(In reply to Daniel Veditz [:dveditz] from comment #12)

sec-high-ish, but to attack successfully the attack site has to know what you're going to type

Yup.

which means they have to suggest it and convince you.

They don't necessarily have to suggest it; in comment 1, I have a somewhat-realistic typo-squatting attack strategy. I'll expand on it here for a bit more detail:

1. Suppose https://yourbank.example is a bank or other high-value website.
2. An attacker registers one or more typo-variants of that domain, e.g. https://yourbankk.example (extra k). The attacker puts up a site saying "This domain is for sale at GoDaddy" or whatever, to make it look like a generic/benign typo-squatter page (but they included a onbeforeunload payload to trigger this bug)
3. Now, suppose a user arrives at this https://yourbankk.example typo-squatter page when trying to type their bank's URL into their address bar.
4. The user can clearly tell from the page-content that it's the wrong site, and they tap their URLbar to correct their typo to load the correct bank URL.
5. Now, the attacker's onbeforeunload fires and takes advantage of this bug to swap out their page-content and replace it with a mockup that looks exactly like the bank website. The attacker's mockup also includes some sort of urgent call-to-action for the user, telling them that their bank account has been compromised and they need to call an attacker-controlled toll-free number to confirm their identity and regain access to their account.
6. The user is initially skeptical but they tap the lock to check the site ID, and Firefox's site-ID-panel tells them that they are indeed viewing their bank's website https://yourbank.example (though Firefox is not telling them the truth; in fact they're viewing attacker-controlled content).

Fortunately, I think the final attacker-controlled rendering is non-interactive (in my testing at least), so the attacker can't directly phish the user's credentials or anything like that. But, they can still present a static message with malicious instructions, like calling an attacker-controlled phone number as described in my scenario here.

Comment 14

[Cathy Lu [:calu]]

Attachment: Bug 1914797 - Part 1 - Revert bug 1868469

Comment 15

[Cathy Lu [:calu]]

Attachment: Bug 1914797 - Part 2 - Add url change during onPageStart for slow loading sites

Depends on D226431

Comment 16

[Jonathan Almeida [:jonalmeida]]

Clearing NI as calu has a patch up to fix this bug which we discussed offline.

Comment 17

[Cathy Lu [:calu]]

Attachment: fixup! Bug 1914797 - Part 1 - Revert bug 1868469

Depends on D226432

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ff18bedf34ff
Part 1 - Revert bug 1868469 r=android-reviewers,jonalmeida
https://hg.mozilla.org/integration/autoland/rev/1666b0f81ad7
Part 2 - Add url change during onPageStart for slow loading sites r=android-reviewers,jonalmeida

Comment 19

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

https://hg.mozilla.org/mozilla-central/rev/ff18bedf34ff
https://hg.mozilla.org/mozilla-central/rev/1666b0f81ad7

Comment 20

[Donal Meehan [:dmeehan]]

:calu please add a beta uplift request on this when you have a moment

Comment 21

[Cathy Lu [:calu]]

Comment on attachment 9432427 [details]
Bug 1914797 - Part 1 - Revert bug 1868469

Beta/Release Uplift Approval Request

• User impact if declined/Reason for urgency: Issue lists the potential security risks.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Using the attached testcase, the url should change only after beforeunload events.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): It's covered by automated tests.
• String changes made/needed: No
• Is Android affected?: Yes

Comment 22

[Cathy Lu [:calu]]

Comment on attachment 9432428 [details]
Bug 1914797 - Part 2 - Add url change during onPageStart for slow loading sites

Beta/Release Uplift Approval Request

• User impact if declined/Reason for urgency: Issue listed in the bug.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Using the attached testcase, the url should change only after beforeunload events.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): It is covered by automated tests.
• String changes made/needed: No
• Is Android affected?: Yes

Comment 23

[Daniel Holbert [:dholbert]]

Verified fixed in Nightly 134.0a1 2024-11-01.

Performing the STR in...
Nightly 134.0a1 2024-10-31: URLbar shows "citi.com" (actual-results)
Nightly 134.0a1 2024-11-01: URLbar shows the URL that I'm coming from (the url that's popping up the spoofed onbeforeunload interception-content)

(The missing dialog mentioned in comment 0 expected-results is tracked in bug 1915296.)

Thanks for the fix!

Comment 24

[Donal Meehan [:dmeehan]]

Comment on attachment 9432427 [details]
Bug 1914797 - Part 1 - Revert bug 1868469

Approved for 133.0b4

Comment 25

[Donal Meehan [:dmeehan]]

Comment on attachment 9432428 [details]
Bug 1914797 - Part 2 - Add url change during onPageStart for slow loading sites

Approved for 133.0b4

Comment 26

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/c9abcf360c9a
https://hg.mozilla.org/releases/mozilla-beta/rev/a79554879d7b

Comment 27

[Cathy Lu [:calu]]

Requested backout of just part 2 as the solution created a regression that displayed non-navigable urls. That should be resolved by making app links dialog blocking in this Bug 1929028.

Part 1 of the patch still resolves the risk the patch of Bug 1868469 initially created.

Comment 28

[Donal Meehan [:dmeehan]]

Part 2 backed out of Beta, see Comment 27
https://hg.mozilla.org/releases/mozilla-beta/rev/02bbfde723e2477ff89c576ff35b047cc00ea484

Comment 29

[Donal Meehan [:dmeehan]]

also backed out part 2 from autoland
https://hg.mozilla.org/integration/autoland/rev/5b3fb98505a924be8d4a856115ee408a9e139b3c

Comment 30

[Donal Meehan [:dmeehan]]

backout landed in central
https://hg.mozilla.org/mozilla-central/rev/5b3fb98505a924be8d4a856115ee408a9e139b3c

Comment 31

[Tom Ritter [:tjr] (OOTO until April)]

Attachment: advisory.txt

Comment 32

[Florin Bilt]

(In reply to Donal Meehan [:dmeehan] from comment #30)

backout landed in central
https://hg.mozilla.org/mozilla-central/rev/5b3fb98505a924be8d4a856115ee408a9e139b3c

Perfherder has detected a browsertime performance change from push 5b3fb98505a924be8d4a856115ee408a9e139b3c.

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
6%	cnn-ampstories loadtime	android-hw-a55-14-0-aarch64-shippable	warm webrender	223.42 -> 210.68
4%	cnn-ampstories ContentfulSpeedIndex	android-hw-a55-14-0-aarch64-shippable	warm webrender	363.28 -> 349.12
4%	cnn-ampstories FirstVisualChange	android-hw-a55-14-0-aarch64-shippable	warm webrender	363.28 -> 349.12
4%	cnn-ampstories PerceptualSpeedIndex	android-hw-a55-14-0-aarch64-shippable	warm webrender	363.28 -> 349.12
4%	cnn-ampstories fcp	android-hw-a55-14-0-aarch64-shippable	warm webrender	336.14 -> 323.11
...	...	...	...	...
3%	cnn-ampstories fcp	android-hw-a55-14-0-aarch64-shippable	warm webrender	335.58 -> 327.10

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests.

If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a sheriff to do that for you.

You can run these tests on try with ./mach try perf --alert 42796

For more information on performance sheriffing please see our FAQ.