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

Warn users whenever an ENS name contains a homoglyph vulnerability #9129

Closed
danfinlay opened this issue Aug 3, 2020 · 16 comments
Closed

Warn users whenever an ENS name contains a homoglyph vulnerability #9129

danfinlay opened this issue Aug 3, 2020 · 16 comments
Labels
area-name-systems journey-confirmation Sev1-high High severity; partial loss of service with severe impact upon users, with no workaround. type-bug type-security ux-enhancement

Comments

@danfinlay
Copy link
Contributor

An ENS name is a UTF-8 compatible string of text. That means there are many characters that have the same appearance, or homo-glyph.

This can cause some concerns when using an ENS name:

  • Pasting an ENS name that looks correct could actually be wrong. as tweeted recently
  • Reverse-resolved ENS names could appear familiar but actually be deliberately crafted to be deceptive.

I had mentioned this acceptance criteria in a previous issue, but a basic check with a vulnerable name and I cannot find any warning.

MyCrypto has a tool called ens-validation that can do the hard part of this already, we just need to design & display the appropriate warnings.

Acceptance criteria:

  • Entering a homoglyph-impersonation name on the send screen to field should show a warning. Can verify with vita‍lik.eth, which is not the same as vitalik.eth.
  • A confirmation screen showing an ENS name should also show a warning under any homoglyph-containing name. We could also simply not resolve a homoglyph-containing name.

Possible copy for the warning:

  • This name has some deceptive letters in it that resemble other letters. It is probably trying to impersonate another well-known name. Did you get this name from someone you trust?
@Gudahtt
Copy link
Member

Gudahtt commented Aug 4, 2020

Reverse-resolved ENS names could appear familiar but actually be deliberately crafted to be deceptive.

I believe our reverse-resolved ENS names are always displayed as punycode, so they aren't susceptible to this problem.

@Gudahtt Gudahtt added Sev1-high High severity; partial loss of service with severe impact upon users, with no workaround. type-bug labels Aug 4, 2020
@0xc0de4c0ffee
Copy link

🙏
I was testing ZWJ in eth-ens-namehash normalize (UTS46 transitional=false, useSTD3AsciiRules=true).

expected: |-
      'vita‍<ZWJ>lik.eth' 
    actual: |-
      'xn--vitalik-k06c.eth'

Current Metamask lookup with Non-ASCII name
zwj

Show normalized xn--name and warning
normal

Warning should focus on all non-ASCII names, check against possible impersonator/scam scenarios and specific homoglyphs without triggering false alarm on all international/emoji names.

This is IDNA mess long before ETH/ENS mess.

While UTS46 tries to bridge the incompatibility, there are four characters which cannot be so bridged: ß (the German sharp s), ς (Greek final sigma), and the ZWJ and ZWNJ characters. These are handled differently depending on the mode; in transitional mode, these strings are mapped to different ones, preserving capability with IDNA2003; in Nontransitional Mode, these strings are mapped to themselves, in accordance with IDNA2008.

Etherscan is warning vita<zwj>lik.eth with "normalized" redirect to real vitalik.eth..

Warning! Found (and Normalized) Zero Width Character in name : [vita<ZeroWidthChar>lik.eth]

Etherscan lookup for xn--vitalik-k06c.eth

Sorry, the name 'xn--vitalik-k06c.eth' does not appear to be valid

IDK if etherscan is using EIP-137 compatible normalization process. OR it's ok for metamask to follow same rules..

UTS46.js Known issues

It also does not try to implement the Bidi and contextual rules for validation: these do not affect any mapping of the domain names; instead, they restrict the set of valid domain names. Since registrars shouldn't be accepting these names in the first place, a domain that violates these rules will simply fail to resolve.

@danfinlay
Copy link
Contributor Author

A problem with enforcing ascii rules is it doesn't represent the scope of ENS, which supports many character sets, including emoji. If we converted all to punycode, we would be effectively limiting the whole of ENS to ASCII-character languages.

@0xc0de4c0ffee
Copy link

If we converted all to punycode, we would be effectively limiting the whole of ENS to ASCII-character languages.

This line from eth-ens-namehash normalise.toAscii function

function normalize(name) {
    return name ? uts46.toAscii(name, {useStd3ASCII: true, transitional: false}) : name
}

I'm bit confused IF ENS spec was asking for uts46.toUnicode(...) instead of uts46.toAscii(...)?

Example from eth-ens-namehash tests
domain: fоо.eth // with cyrillic 'o'
toAscii : xn--f-1tba.eth
namehash : namehash(xn--f-1tba.eth) // NOT namehash(fоо.eth)

So I think it's ok to show full IDNA mess with warning.

fоо.eth
(xn--f-1tba.eth)
0xaddress

Warning: This name contains non-Ascii characters .... (Warn level 1-3)

@lastcanal
Copy link
Contributor

Unicode.org's has a working group that maintains a list of 'confusable' unicode characters along with the character it might be confused with, called confusables.txt

I put together a library that translates the confusables.txt file into a lookup table and then used it to build what I think is a pretty nice warning system.

It is able to detect 'vita‍lik.eth'
vitalik

It can also detect 'faceboоk.eth'
facebook

It might even be a little too strict because it warns on 'math.eth' being so similar to 'rnath.eth'
math

Best of all it doesn't mangle emoji!
nepal-flags

You can see from the tests that it even warns on similar Japanese.

The downside is that 100KB of json (30KB gzipped) gets added to the ui bundle.

@lastcanal
Copy link
Contributor

I should add that one improvement could be to convert the 'confusable' name into it's non confusable version then resolve and display both names in the list, highlighting the differences.

Hopefully this helps!

@rachelcope
Copy link

Design update recommendations:

  1. Update alert copy to:

"We have detected an unusual character (homoglyph) in the ENS name entered. Check the ENS name/address with your recipient to avoid potential scam."

  1. Remove the tooltip.
    (The alert copy and changing the homoglyph character to red text should be sufficient)

@rachelcope rachelcope added the needs-design Needs design support. label Jan 21, 2021
@rachelcope
Copy link

What happens in the use case that an ENS name is created in another language (e.g., Greek, Hebrew, etc) that uses homoglyph characters?

@Gudahtt
Copy link
Member

Gudahtt commented Jan 28, 2021

Removing the tooltip makes sense! That is the only place where the specific character that was confusing is explained though, which is something to consider maybe.

@cjeria
Copy link
Contributor

cjeria commented Jan 29, 2021

@rachelcope @Gudahtt are you letting the user continue in the flow if a homoglyph is detected?

@lastcanal
Copy link
Contributor

Hi @cjeria, I would definitely recommend allowing users to continue the flow if a homoglyph is detected. Languages such as Greek, Hebrew, etc. might have a higher false positive rate than English, but all languages will have false positives.

For example, the Hebrew character 'ן‎' will warn users that it is confusable with 'l', even if the entire ENS domain is in Hebrew. It would be possible to remove the warning for 'ן‎' if all other characters in the ENS domain are in the Hebrew script set; however, this would require building another index from another Unicode working group.

@rachelcope I like the updated copy, but I think it is important to change 'unusual' to 'confusable' if we are sticking with this implementation.

"We have detected a confusable character (homoglyph) in the ENS name entered. Check the ENS name/address with your recipient to avoid potential scam."

The best English example I can find for why this is confusable and not unusual is the letter 'm', which is marked as confusable with 'rn' according to the unicode working group.

If we want only unusual characters to be detected then we could remove the confusable entries that are in the same script set (e.g. m -> rn), but only if the entire ENS domain uses the same script set.

@cjeria
Copy link
Contributor

cjeria commented Feb 1, 2021

@lastcanal Thanks for the feedback. A design pattern we're considering (on the mobile app side at least) is to throw a warning that the users can bypass by acknowledging the alert. I could see this being a nuisance for some languages that use confusable characters but the main intent is to help identify deceptive names. Would this be acceptable in your opinion?

see screenshot
image

@lastcanal
Copy link
Contributor

lastcanal commented Feb 2, 2021

Hi @cjeria I think that would work great; I am in no way annoyed by popups that are trying to save me from some scam :). Would it be possible to gather metrics on how often the popup is displayed? Extra work could be done later to de-escalate the error if the confusable characters are in the same alphabet/script set.

@rachelcope
Copy link

@lastcanal thanks for your copy suggestion.

Here's a small update to the error message to make it more concise: "We have detected an confusable character (homoglyph) in the ENS name. Check the ENS name to avoid a potential scam."

@cjeria Before we add a required acknowledgement (on extension), we need to establish a design pattern and process for determining what errors require additional acknowledgement. For now, I suggest just show the standard/existing error message

@rachelcope rachelcope removed the needs-design Needs design support. label Feb 16, 2021
@rachelcope
Copy link

@Gudahtt here's the updated design and copy. Here's the figma file

image

lastcanal added a commit to lastcanal/metamask-extension that referenced this issue Feb 26, 2021
Uses unicode.org's TR39 confusables.txt to display a warning when
'confusable' unicode points are detected.

Currently only the `AddRecipient` component has been updated, but the new
`Confusable` component could be used elsewhere

The new `unicode-confusables` dependency adds close to 100KB to the
bundle size, and around 30KB when gzipped.

Adds 'tag' prop to the tooltop-v2 component

Use $Red-500 for confusable ens warning

Lint Tooltip component

Update copy for confusing ENS domain warning.
Gudahtt added a commit that referenced this issue Feb 27, 2021
* Add warning system for 'confusable' ENS names (#9129)

Uses unicode.org's TR39 confusables.txt to display a warning when
'confusable' unicode points are detected.

Currently only the `AddRecipient` component has been updated, but the new
`Confusable` component could be used elsewhere

The new `unicode-confusables` dependency adds close to 100KB to the
bundle size, and around 30KB when gzipped.

Adds 'tag' prop to the tooltop-v2 component

Use $Red-500 for confusable ens warning

Lint Tooltip component

Update copy for confusing ENS domain warning.

* Fix prop type

Co-authored-by: Mark Stacey <markjstacey@gmail.com>
@Gudahtt
Copy link
Member

Gudahtt commented Mar 1, 2021

Closing this as completed in #9187, and further improved in #10530.

The designs shown in the most recent comment will be implemented as part of the overall confirmation screen redesign. For now we've just used the existing warning designs with this updated copy.

@Gudahtt Gudahtt closed this as completed Mar 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-name-systems journey-confirmation Sev1-high High severity; partial loss of service with severe impact upon users, with no workaround. type-bug type-security ux-enhancement
Projects
None yet
Development

No branches or pull requests

6 participants