Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Homoglyphs in URL tooltips #15432

Closed
j4james opened this issue May 25, 2023 · 7 comments · Fixed by #15488
Closed

Homoglyphs in URL tooltips #15432

j4james opened this issue May 25, 2023 · 7 comments · Fixed by #15488
Labels
Area-User Interface Issues pertaining to the user interface of the Console or Terminal In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Terminal The new Windows Terminal.

Comments

@j4james
Copy link
Collaborator

j4james commented May 25, 2023

Windows Terminal version

1.18.1421.0

Windows build number

10.0.19045.2913

Other Software

No response

Steps to reproduce

  1. Open a bash shell in Windows Terminal
  2. Execute the following command:
printf "\n\e]8;;https://www.xn--fcbook-3nf5b.com/\e\\TOTALLY REAL FACEBOOK LINK. YOU CAN TRUST ME.\e]8;;\e\\ \n\n"
  1. Hover over the resulting link text to see where the URL is going to take you.

Expected Behavior

There was a new feature advertised in the v1.18.1421.0 release notes that said "when you're hovering over a URL, we now display it in a partially-encoded form to help you avoid homoglyph attacks". So I expected there would be something in the tooltip indicating that this link was not exactly what it seemed.

Actual Behavior

The URL displayed in the tooltip gives the impression that it's linking to https://www.facebook.com/, which is very misleading.

image

Does PR #15095 perhaps rely on functionality that's only available in Windows 11?

@j4james j4james added Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels May 25, 2023
@j4james
Copy link
Collaborator Author

j4james commented May 25, 2023

I should add, I'm quite impressed that it decoded the punycode URL in the first place, and I think it's a good thing that we don't discriminate against non-Latin languages. However, there should at least be some indication when a URL contains characters that aren't ASCII.

@ianjoneill
Copy link
Contributor

ianjoneill commented May 25, 2023

I can reproduce this in Windows 11 (10.0.22621.1702), so it doesn't seem to be Windows 10 specific.

(Edited to include version number)

@DHowett
Copy link
Member

DHowett commented May 25, 2023

Ah, you know what? I was totally wrong about what we prevented. Great catch.

(The change actually prevents the display of URLs with an RTL/LTR override in them, which can be used to mask the destination.)

@lhecker
Copy link
Member

lhecker commented May 25, 2023

For reference: https://chromium.googlesource.com/chromium/src/+/main/docs/idn.md
The relevant code is fairly complex and it doesn't seem like WinRT or Win32 implement a similar API already. As such the best move for us might be to disable IDNs for the tooltip in general.

@j4james
Copy link
Collaborator Author

j4james commented May 26, 2023

As such the best move for us might be to disable IDNs for the tooltip in general.

If you mean we shouldn't convert the punycode version to Unicode, that wouldn't help, because it could just as easily have been Unicode to start with. I only used the punycode encoding in my example to make it obvious it was doing something dodgy.

A simple solution could be to check if the URL has anything other than ASCII, and if so, add a little warning in the tooltip saying exactly that (e.g. "Warning: this URL contains characters that aren't ASCII").

@lhecker
Copy link
Member

lhecker commented May 26, 2023

Oh, I meant that we could actively encode any non-ASCII URL as punycode ourselves. It's what a hypothetical domain would actually be named anyways after all. IdnToAscii will return the input string if it's ASCII already.

Edit: Actually, we should probably show both: https://www.unicode.org/reports/tr36/#Punycode_Spoofs

@j4james
Copy link
Collaborator Author

j4james commented May 26, 2023

OK, that makes sense. I thought you were saying there wasn't an API for that. I realise now you were talking about Chrome's IDN policy algorithm.

Actually, we should probably show both: https://www.unicode.org/reports/tr36/#Punycode_Spoofs

Yep. I was just going to recommend that.

@lhecker lhecker added Area-User Interface Issues pertaining to the user interface of the Console or Terminal Product-Terminal The new Windows Terminal. Priority-1 A description (P1) labels May 31, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot added the In-PR This issue has a related PR label May 31, 2023
@carlos-zamora carlos-zamora removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label May 31, 2023
@carlos-zamora carlos-zamora added this to the Terminal v1.19 milestone May 31, 2023
microsoft-github-policy-service bot pushed a commit that referenced this issue Jun 8, 2023
06174a9 didn't properly fix the issue of us showing homoglyphs in our
URI tooltip. This commit introduces a different approach where we
display both, the Punycode and Unicode encoding, whenever we encounter
an IDN. This isn't perfect but simple to implement.

Closes #15432

## Validation Steps Performed
* `https://www.xn--fcbook-3nf5b.com/` (which contains confusing glyphs)
  is shown both in its Punycode and Unicode form simultaneously. ✅

---------

Co-authored-by: Carlos Zamora <[email protected]>
DHowett pushed a commit that referenced this issue Jul 27, 2023
06174a9 didn't properly fix the issue of us showing homoglyphs in our
URI tooltip. This commit introduces a different approach where we
display both, the Punycode and Unicode encoding, whenever we encounter
an IDN. This isn't perfect but simple to implement.

Closes #15432

## Validation Steps Performed
* `https://www.xn--fcbook-3nf5b.com/` (which contains confusing glyphs)
  is shown both in its Punycode and Unicode form simultaneously. ✅

---------

Co-authored-by: Carlos Zamora <[email protected]>
(cherry picked from commit f1aa699)
Service-Card-Id: 90012448
Service-Version: 1.17
DHowett pushed a commit that referenced this issue Jul 27, 2023
06174a9 didn't properly fix the issue of us showing homoglyphs in our
URI tooltip. This commit introduces a different approach where we
display both, the Punycode and Unicode encoding, whenever we encounter
an IDN. This isn't perfect but simple to implement.

Closes #15432

## Validation Steps Performed
* `https://www.xn--fcbook-3nf5b.com/` (which contains confusing glyphs)
  is shown both in its Punycode and Unicode form simultaneously. ✅

---------

Co-authored-by: Carlos Zamora <[email protected]>
(cherry picked from commit f1aa699)
Service-Card-Id: 90012449
Service-Version: 1.18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-User Interface Issues pertaining to the user interface of the Console or Terminal In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants