Closed Bug 1704420 (CVE-2023-37205) Opened 3 years ago Closed 1 year ago

Address Bar Spoofing due to Arabic RTL characters (fix domain highlighting)

Categories

(Firefox :: Address Bar, defect, P3)

Firefox 87
defect

Tracking

()

VERIFIED FIXED
115 Branch
Tracking Status
firefox-esr102 --- wontfix
firefox113 --- wontfix
firefox114 --- wontfix
firefox115 --- fixed
firefox119 --- fixed
firefox120 --- verified
firefox121 --- verified

People

(Reporter: rohansharma.rohan27, Assigned: mseibert)

References

Details

(Keywords: csectype-spoof, reporter-external, sec-moderate, Whiteboard: [adv-main115+])

Attachments

(6 files, 3 obsolete files)

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"

Attached image firefox-poc.png

Added PoC image

Version: unspecified → Firefox 87

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?

Component: Untriaged → Address Bar
Flags: needinfo?(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.

Flags: needinfo?(mak) → needinfo?(gijskruitbosch+bugs)

(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?

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(mak)

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

(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.

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

Flags: needinfo?(mak)

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

See Also: → CVE-2021-4221

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.

Flags: needinfo?(jfkthame)

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.

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.

Flags: needinfo?(jfkthame)

The reported has submitted this for bounty consideration.

Flags: sec-bounty?

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

Severity: -- → S3
Priority: -- → P3
Status: UNCONFIRMED → RESOLVED
Closed: 2 years ago
Resolution: --- → DUPLICATE
Flags: sec-bounty? → sec-bounty-

(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?

Depends on: CVE-2023-5732
Flags: sec-bounty?
Flags: sec-bounty-
Flags: needinfo?(jfkthame)
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: DUPLICATE → ---

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.

Flags: needinfo?(jfkthame)
Assignee: nobody → 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!

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.

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.

Attached file WIP: Bug 1704420 (obsolete) —

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?

Flags: needinfo?(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.

Flags: needinfo?(dveditz)

(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.

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.

Flags: sec-bounty? → sec-bounty-
Duplicate of this bug: 1825692
Attached file Bug 1704420.r=mak
Assignee: jfkthame → mseibert
Attachment #9330368 - Attachment description: WIP: Bug 1704420 → Bug 1704420 - Address Bar Spoofing due to Arabic RTL characters.r=mak
Attachment #9268715 - Attachment is obsolete: true
Attachment #9330368 - Attachment description: Bug 1704420 - Address Bar Spoofing due to Arabic RTL characters.r=mak → Bug 1704420.r=mak
Attached file Bug 1704420 - Test.r=mak (obsolete) —

Depends on D176529

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

Status: REOPENED → RESOLVED
Closed: 2 years ago1 year ago
Resolution: --- → FIXED
Attachment #9265067 - Attachment is obsolete: true
Group: firefox-core-security → core-security-release
Target Milestone: --- → 115 Branch

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.

Flags: needinfo?(mseibert)

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?

Flags: needinfo?(mseibert) → needinfo?(dveditz)

That seems reasonable

Flags: needinfo?(dveditz)
Flags: qe-verify+

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

Flags: needinfo?(mak)

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

(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.

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

Awesome.

Flags: needinfo?(mak)
Whiteboard: [reminder-land-tests 2023-07-05]
Whiteboard: [reminder-land-tests 2023-07-05] → [reminder-land-tests 2023-07-05][adv-main115+]
Alias: CVE-2023-37205
See Also: → 1844915

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

Whiteboard: [reminder-land-tests 2023-07-05][adv-main115+] → [adv-main115+]
Attachment #9333592 - Attachment is obsolete: true
Summary: Address Bar Spoofing due to Arabic RTL characters → Address Bar Spoofing due to Arabic RTL characters (fix domain highlighting)
Group: core-security-release

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.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
Flags: sec-bounty-hof+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: