1920423 - (CVE-2024-10462) using long url address in permission prompt truncated lead to spoof

Status: VERIFIED FIXED

(Firefox :: Site Permissions, defect, P1)

Description

[Hafiizh]

Attachment: domainlong.png

when using long url address, url in permission prompt is truncated leading to spoof

steps to reproduce

1. open https://xxxxxxxxxxxxxxxxxxx.login.secures.documents.drives.google.com.aogarantiza.com/chromium/pepc-bounds.html
2. open console do :
   navigator.mediaDevices.getUserMedia(
   {audio: true, video: true}).then()
3. the url shown in permission prompt: https://xxxxxxxxxxxxxxxxxxx.login.secures.documents.drives.google.com not https://xxxxxxxxxxxxxxxxxxx.login.secures.documents.drives.google.com.aogarantiza.com

OS: windows 10

Comment 1

[Hafiizh]

Attachment: bandicam 2024-09-23 13-32-56-223.mp4

Comment 2

[Emma Zühlcke [:emz]]

Attachment: popup-notification-with-working-host-display.png This used to work. Here is the PoC with an older version of Firefox

Comment 3

[Emma Zühlcke [:emz]]

The PopupNotification used to resize properly to accommodate the long URL. This is a regression.
I ran mozgression on it:

 8:57.59 INFO: Narrowed nightly regression window from [2022-10-17, 2022-10-19] (2 days) to [2022-10-17, 2022-10-18] (1 days) (~0 steps left)
 8:57.59 INFO: Got as far as we can go bisecting nightlies...
 8:57.59 INFO: Last good revision: ac1330b68d3e7b231a177cfa1ac52e1b2199bb84 (2022-10-17)
 8:57.59 INFO: First bad revision: b6e04e02b4f8532eac41194d203e72d91dbfc2ff (2022-10-18)
 8:57.59 INFO: Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ac1330b68d3e7b231a177cfa1ac52e1b2199bb84&tochange=b6e04e02b4f8532eac41194d203e72d91dbfc2ff

Not sure yet which is the regressing bug.

Comment 4

[Emma Zühlcke [:emz]]

This is a bit of a long-shot: Emilio, could this be related to Bug 1790616?

Comment 5

[Emilio Cobos Álvarez [:emilio]]

That seems likely, before that, rules like this one weren't really honored. If you change that to min-content then it works as you expect.

We could:

• Add a min-width: min-content to that rule.
• Remove that rule.
• Allow the domain to break by using word-break: anywhere or so?
• Something else?

Comment 6

[Emma Zühlcke [:emz]]

Thanks! Let's mark it as a regression for now.
I like the idea of word-break: anywhere but going back to the original behavior by updating the width may be safer here.

Comment 7

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

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

Comment 8

[Emma Zühlcke [:emz]]

The ideal solution here would be to add an ellipsis in the right spot so that the site (eTLD+1) is still shown. We could also detect the "overflow" and switch to only showing site in that case. I'm wondering if we have existing helpers specifically for URIs for this that are not just cropping somewhere in the center.

As a stop-gap fix I suggest to add word-break: break-all;. However, we can't add it to the entire body because that would mean we would break anywhere for the entire description text which doesn't look good and difficult to read.

Targeting just the host portion turned out to be trickier than I thought. The l10n string uses a %S placeholder for the host which then gets replaced by < >. This is handed to PopupNotifications#show which parses it and eventually sets the name attribute on the custom element <popupnotification>. This name attribute is then mapped to the DOM using a quite obscure selector.

I'm working on a patch which makes the selector a bit clearer and targets only the "name" field, making it wrap anywhere. I need to double check that we really only use this field for hosts and not insert other text which should wrap properly.

Setting custom style just for the host portion of the description would be a lot easier if these prompts used Fluent. Fluent allows developers to add HTML tags as part of the localization string. This is where we could add e.g. a class name to target. However, I consider migrating all site permission prompts to Fluent out of scope for this bug.

While looking into this I've noticed that PopupNotifications supports displaying the host via the displayURI option. Permission prompts don't seem to use that (anymore?). Probably because we now show the host inline as part of the description text. The displayURI implementation uses crop=center which will add an ellipsis in the middle. Most of the time that resolves this problem. There may still be cases where the site (eTLD+1) part is so long that it will hide parts of it too.

Comment 9

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

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

Comment 10

[Emma Zühlcke [:emz]]

Attachment: Bug 1920423, r=Gijs!

Comment 11

[Emma Zühlcke [:emz]]

Attachment: Bug 1920423, r=Gijs!

Original Revision: https://phabricator.services.mozilla.com/D224303

Comment 12

[Phabricator Automation]

beta Uplift Approval Request

• User impact if declined: Spoofing of hostname in permission prompts.
• Code covered by automated testing: no
• Fix verified in Nightly: no
• Needs manual QE test: yes
• Steps to reproduce for manual QE testing: See comment 0
• Risk associated with taking this patch: low
• Explanation of risk level: Only a styling change. There is a low risk for a cosmetic issue where we wrap content of PopupNotifications weirdly.
• String changes made/needed: none
• Is Android affected?: no

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6a15156e22b8
r=Gijs,desktop-theme-reviewers,reusable-components-reviewers,hjones

Comment 14

[Emma Zühlcke [:emz]]

Attachment: Bug 1920423, r=Gijs!

Original Revision: https://phabricator.services.mozilla.com/D224303

Comment 15

[Phabricator Automation]

esr128 Uplift Approval Request

• User impact if declined: Spoofing of hostname in permission prompts.
• Code covered by automated testing: no
• Fix verified in Nightly: no
• Needs manual QE test: yes
• Steps to reproduce for manual QE testing: See comment 0
• Risk associated with taking this patch: low
• Explanation of risk level: Only a styling change. There is a low risk for a cosmetic issue where we wrap content of PopupNotifications weirdly.
• String changes made/needed: none
• Is Android affected?: no

Comment 16

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

https://hg.mozilla.org/mozilla-central/rev/6a15156e22b8

Comment 17

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/4168034c2027

Comment 18

[Giorgia Nichita, Release Desktop QA]

Reproduced this issue on an affected nightly build, following the STR from Comment 0.
Verified as fixed on latest Nightly 133.0a1 (2024-10-03) and latest Beta 132.0b3 (treeherder build - 20241003135730) on Win 10, Win 11, macOS 11 and Ubuntu 22.04.

Comment 19

[Hafiizh]

the sec-bounty is set + is this eligible for bounty because the reward amount has not been set?

Comment 20

[Tom Ritter [:tjr]]

Discussing bounty details over email.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

NI for the rebased ESR128 patch.

Comment 22

[Emma Zühlcke [:emz]]

Rebased https://phabricator.services.mozilla.com/D224349
Though I didn't run into any merge conflicts locally. Might have auto merged properly.

Comment 23

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/4485490156bb

Comment 24

[Giorgia Nichita, Release Desktop QA]

Verified as fixed on Firefox 128.4.0esr (20241021193311) on Win 10, Win 11, macOS 11 and Ubuntu 22.04.

Comment 25

[Jesse Schwartzentruber (:truber)]

Attachment: advisory.txt