1704420 - (CVE-2023-37205) Address Bar Spoofing due to Arabic RTL characters (fix domain highlighting)

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P3)

Description

[Rohan Sharma]

Attachment: firefox-desktop-PoC.html

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Firefox/78.0

Steps to reproduce:

Tested on:
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:87.0) Gecko/20100101 Firefox/87.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:87.0) Gecko/20100101 Firefox/87.0

STEP 1: Click on "run" in attached file ("firefox-desktop-PoC.html")
The address bar will point to "www.mozilla.org", however the actual content is being delivered by "اختبار.اختبار"

The URL is not handled properly. Arabic characters in the domain name are being rendered from Right to Left(RTL) direction rather than Left to Right(LTR).

Actual results:

http://www.mozilla.org/hello/1/index.html/رابتخا.رابتخا

Expected results:

http://اختبار.اختبار/www.mozilla.org/hello/1/index.html
The address bar should point to "اختبار.اختبار" instead of "www.mozilla.org"

Comment 1

[Rohan Sharma]

Attachment: firefox-poc.png

Added PoC image

Comment 2

[:Gijs (he/him)]

Weirdly, for me the URL is initially correct, but then is incorrect when switching away from the tab and then back. Marco, do you know what's going on here?

Comment 3

[Marco Bonardo [:mak]]

(In reply to :Gijs (he/him) from comment #2)

Weirdly, for me the URL is initially correct, but then is incorrect when switching away from the tab and then back. Marco, do you know what's going on here?

For me the domain is not highlighted when clicking in the poc, what do you mean by initially correct but incorrect later? If this would be a very long url, it could happen that we ensure the domain stays visible only on initial load and not later on.

Comment 4

[:Gijs (he/him)]

(In reply to Marco Bonardo [:mak] from comment #3)

(In reply to :Gijs (he/him) from comment #2)

Weirdly, for me the URL is initially correct, but then is incorrect when switching away from the tab and then back. Marco, do you know what's going on here?

For me the domain is not highlighted when clicking in the poc,

Same here. I think because we're loading an error page?

what do you mean by initially correct but incorrect later?

I mean the order of the text matches "expected results" for me after I click the link, and doesn't match "actual / broken result" until I switch away from the tab and then switch back. Are you seeing something else?

Comment 5

[Rohan Sharma]

You can visit following link for live version.
http://xn--4gbrim.xn--mgberp4a5d4ar/www.mozilla.org/1

Comment 6

[:Gijs (he/him)]

(In reply to Rohan Sharma from comment #5)

You can visit following link for live version.
http://xn--4gbrim.xn--mgberp4a5d4ar/www.mozilla.org/1

This just redirects to https://nic.sa/www.mozilla.org/1.

Comment 7

[Marco Bonardo [:mak]]

I don't see any difference after switching to other tabs, in Nightly.

Comment 8

[Rohan Sharma]

Attachment: firefox-latest-mac-poc.mov

Added a video PoC for better understanding of how it is working on my end.

Comment 9

[Daniel Veditz [:dveditz]]

I think we have a dupe of this, Jonathan do you remember?

If you set browser.urlbar.trimURLs to false (not the default) then this works correctly -- the scheme anchors the text direction correctly.

Comment 10

[Daniel Veditz [:dveditz]]

I do not reproduce the behavior in the .PNG -- I only get the behavior when we trim schemes, which we only do for http: links (by default?), and we wouldn't show the lock icon for that. I don't get any web content, I get a server error, so we're not going to get a lock in any case.

Comment 11

[Jonathan Kew [:jfkthame]]

We've certainly seen similar examples before, and trimming the scheme from the URL is at least part of the issue, because if the scheme is present it provides an unambiguously-LTR beginning for the text.

With Nightly on macOS, I do see a surprising inconsistency in behavior: when I initially clicked the attachment link here, and then clicked "Run", the URL bar displayed the Arabic-script domain first (leftmost), and the "fake" domain mozilla.org to the right; but when I downloaded the testcase and loaded it from a file://, they appeared reversed.

Further testing shows that it's somewhat flaky: with the testcase loaded directly here, repeatedly clicking "Run" and then go-back, I get sometimes the "correct" order and sometimes the "reversed" version. (With the testcase loaded from a local file, it seems consistently reversed.)

So there is definitely something odd going on, though AFAICS only "trimmed" URLs are subject to this, and so it wouldn't be possible to spoof an https:// location in this way.

Comment 12

[Jonathan Kew [:jfkthame]]

Dupe of bug 1690979?

Comment 13

[Daniel Veditz [:dveditz]]

The reported has submitted this for bounty consideration.

Comment 14

[Rohan Sharma]

Hi, we have passed 60 days for this vulnerability report. Any updates?

Comment 16

[Frederik Braun [:freddy]]

(In reply to Rohan Sharma from comment #14)

Hi, we have passed 60 days for this vulnerability report. Any updates?

We just discovered this was indeed a duplicate of a previous report. Sorry about the lack of notice.

Comment 17

[Daniel Veditz [:dveditz]]

(In reply to Jonathan Kew (:jfkthame) from comment #12)

Dupe of bug 1690979?

I'm not sure it is. This one might be fixed by whatever you eventually do for bug 1690979, but this is a much simpler case of not anchoring the hostname to the left when we don't display the scheme. It only works with http:// domains with the default value for browser.urlbar.trimURLs and does not work for https:// domains.

bug 1690979 shows misordering even within https:// urls. It might be simple to fix this bug without fixing all of bug 1690979

I initially duped it, but I'm going to make it a "depends on" in case we can come up with a shorter-term fix for this variant. Seem reasonable?

Comment 19

[Jonathan Kew [:jfkthame]]

Possibly the simplest thing to do here would be when we trim "http://" from the URL, insert a Left-to-Right mark (U+200E) in its place so that we maintain the same ordering for any bidi content.

This looks trivial to do in BrowserUIUtils.trimURL(), by making it return "\u200e" + url.substring(this.trimURLProtocol.length) instead of just the substring; but I suspect some other adjustments will need to be made to ensure this doesn't disrupt other operations such as URLbar formatting.

Comment 20

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1704420 - Improve bidi handling in the url bar. r=mak

Comment 21

[Jonathan Kew [:jfkthame]]

Here's a strawman patch that appears to improve the behavior, based on my local testing; however, please be aware that I don't really know this code at all, and there's a strong chance that it'll have some kind of side-effects and require further work before it's really usable. Feedback welcome, or better still, if someone who actually knows front-end code wants to take this and finish it, that'd be great!

Comment 22

[Marco Bonardo [:mak]]

Fwiw, in the past we addressed these cases through formatting and highlighting the right domain in the url.
In a few cases where the code can't identify the domain string we just give up and don't format, that seems what is happening here, though I'm not sure why yet, it should be debugged.
If the domain highlight UI is still confusing, maybe that is what requires a change, users should not make assumption between order of parts and safety of the URL in general, our UI here is sub-optimal.

Of course currently this only affects http due to trimming, but it's likely at a certain point the behavior will switch to hide https instead.

(In reply to Jonathan Kew (:jfkthame) from comment #19)

Possibly the simplest thing to do here would be when we trim "http://" from the URL, insert a Left-to-Right mark (U+200E) in its place so that we maintain the same ordering for any bidi content.

That's something we evaluated in the past but it is a bit risky, because it's effectively adding artificial chars to the url, and then we must guarantee any code using that value is accounting for that.
For example, I think the patch is not correctly handling this code:
https://searchfox.org/mozilla-central/rev/f8db81665dc2833fff09dc7eef200539ac1fd351/browser/components/urlbar/UrlbarInput.jsm#1184
then there's various calls to _trimValue related to the selection and what is copied to the clipboard
https://searchfox.org/mozilla-central/rev/f8db81665dc2833fff09dc7eef200539ac1fd351/browser/components/urlbar/UrlbarInput.jsm#2199-2202
and then everything using inputField.value would also have to take that into account:
https://searchfox.org/mozilla-central/rev/f8db81665dc2833fff09dc7eef200539ac1fd351/browser/components/urlbar/UrlbarInput.jsm#1967,1972,1981

So, I don't think it's trivial to add special characters to the urlbar value without the risk of subtle breakage here and there.

Comment 23

[Marco Bonardo [:mak]]

fwiw, the link in comment 5 is doing proper highlighting for me where the RTL domain is highlighted, as well as the link in comment 0.
On the other side the poc.html link on load is not formatted, making things worse.

This is the first thing I wanted to understand, and it seems to be because the trimmed string confuses URIFixup, indeed here
https://searchfox.org/mozilla-central/rev/f8db81665dc2833fff09dc7eef200539ac1fd351/browser/components/urlbar/UrlbarValueFormatter.jsm#136,156
URIFixup thinks the string should be searched for and we end up here https://searchfox.org/mozilla-central/rev/f8db81665dc2833fff09dc7eef200539ac1fd351/browser/components/urlbar/UrlbarValueFormatter.jsm#163

If we'd use the .untrimmedValue instead of the inputField.value, everything seems to be working fine.
We manage that later instead:
https://searchfox.org/mozilla-central/rev/f8db81665dc2833fff09dc7eef200539ac1fd351/browser/components/urlbar/UrlbarValueFormatter.jsm#169-178

So apparently there's a bug here that makes us skip formatting for trimmed RTL domains, and we can write a test for it. Of course it will require some additional checks around using untrimmedValue but it should be fine for the most part.

Comment 24

[Marco Bonardo [:mak]]

Attachment: WIP: Bug 1704420

Comment 25

[Marco Bonardo [:mak]]

Basically, the patch I'm attaching should be addressing the lack of formatting for this case (I still have to write tests). Whether that's enough or not to address our sec concerns, is TBD. Fwiw I'm also happy to fix that separately in a public bug (there's nothing particularly risky there, imo) while keeping this open for further investigation and a broader solution. Wdyt Daniel?

Comment 26

[Daniel Veditz [:dveditz]]

What kinds of other concerns do you want to investigate, or what parts of the original complain here would remain unfixed? We also have bug 1690979 for other kinds of mixed RTL text URL spoofs. Would your investigation and broader solution fit with that, or are your concerns different?

From a bug-bounty perspective it would be better to fix the reporter's testcase in this bug and do the broader follow-up investigation in a new bug, but that's mainly an administrative concern. If you feel strongly the other approach is better we can make that work.

Comment 27

[Marco Bonardo [:mak]]

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

What kinds of other concerns do you want to investigate, or what parts of the original complain here would remain unfixed? We also have bug 1690979 for other kinds of mixed RTL text URL spoofs. Would your investigation and broader solution fit with that, or are your concerns different?

Yes, I think bug 1690979 is covering the broader concern with RTL inverting url parts positioning. We don't have a UI able to remove any misleading use of RTL today, and that would require a deeper UI change, like for example only showing the origin like some other browsers do. That's far away afaict.

Let's keep this to fix highlighting, if we think that is sufficient to address concern with the reported bug. It was considered sufficient so far, for the other cases we hit in the past.

As for adding chars to force direction of the shown url, that's technically feasible, but it may become source of various regressions, especially when it will apply to the most common case of stripping https. It may be necessary to hide inputField into the UrlbarInput object and expose/override only parts of it, that way we could abstract away the special character that would not exist for external consumers and be handled internally. Since this is a larger effort, I'd probably leave it to a separate bug.

Comment 28

[Daniel Veditz [:dveditz]]

The bug bounty committee has decided that this is a sub-set of the earlier reported bug 1690979 and won't be awarded a bounty. Your bug 1704422 was awarded because although visually the same, the fix was in a different codebase.

Comment 30

[Marc Seibert (:mseibert)]

Attachment: Bug 1704420.r=mak

Comment 31

[Marc Seibert (:mseibert)]

Attachment: Bug 1704420 - Test.r=mak

Depends on D176529

Comment 32

[Marco Bonardo [:mak]]

The landed patch makes the origin emphasized (or better, it deemphasize all the rest), making the url less likely to be abused.
In Bug 1833091 we are investigating the approach suggested by Jonathan, of adding a force LTR char when we strip the protocol.
Long term we should experiment with showing only the origin by default (allowing advanced users to revert to the full url).

https://hg.mozilla.org/integration/autoland/rev/ce16eb5623883665dc8db25169efe5b61647f58c
https://hg.mozilla.org/mozilla-central/rev/ce16eb5623883665dc8db25169efe5b61647f58c

Comment 33

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

The patch landed in nightly and beta is affected.
:mseibert, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox114 to wontfix.

For more information, please visit BugBot documentation.

Comment 34

[Marco Bonardo [:mak]]

I'd just let this follow normal train to release, considered it's touching urlbar highlighting anyway, and 115 will become an ESR.
Daniel, wdyt?

Comment 35

[Daniel Veditz [:dveditz]]

That seems reasonable

Comment 36

[Marco Bonardo [:mak]]

Setting a needinfo to remember landing the test in the future.

Comment 37

[Frederik Braun [:freddy]]

FWIW; You can also set a whiteboard tag [reminder-land-tests YYY-mm-dd] and the bot will needinfo you on that day.

Comment 38

[Marco Bonardo [:mak]]

(In reply to Frederik Braun [:freddy] from comment #37)

FWIW; You can also set a whiteboard tag [reminder-land-tests YYY-mm-dd] and the bot will needinfo you on that day.

TIL, thank you!
I assume it will notify the bug assignee though.

Comment 39

[Frederik Braun [:freddy]]

Nope, that goes to the person who set it. :)

Comment 40

[Marco Bonardo [:mak]]

Awesome.

Comment 41

[Jason Kratzer [:jkratzer]]

Attachment: advisory.txt

Comment 42

[Marco Bonardo [:mak]]

Sounds like I should have used reminder-tests in the whiteboard. Checking the test patch applies to land it.

Comment 43

[Marc Seibert (:mseibert)]

Attachment: Bug 1704420 - Test.r=mak

Comment 44

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/411fcc325af6
Test.r=mak

Comment 45

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

https://hg.mozilla.org/mozilla-central/rev/411fcc325af6

Comment 46

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/b89315fc78f8

Comment 47

[Ardelean Oana, Desktop QA [:oardelean]]

Reproducible on Firefox 114.0 on macOS 12.
Verified as fixed on Firefox 120.0 and Nightly 121.0a1 on macOS 12, Windows 10, Ubuntu 22.