1323338 - (CVE-2017-5383) Unicode hyphens in domain names are not blacklisted

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Armin Ebert [:arminius]]

When used in domain names, the Unicode glyphs U+2010 (HYPHEN) and U+2011 (NON-BREAKING HYPHEN) are not converted to Punycode, thus allowing IDN homograph attacks.

This is a bank: https://www.deutsche-bank.de/
This is not a bank: https://www.deutsche‐bank.de/

In my browser the characters are indistinguishable. Instead, the latter one should be displayed as "xn--deutschebank-n09f.de".

Note that these two seem to be the only remaining instances of hyphen-confusables that don't end up as Punycode, as I checked against this list: http://unicode.org/cldr/utility/confusables.jsp?a=-&r=None

Comment 1

[Daniel Veditz [:dveditz]]

I thought we had that block of punctuation covered, or that NAMEPREP folded it into a regular hyphen. Definitely should not be able to create a separate domain with \u2010 looking like a regular hyphen.

Comment 2

[Jonathan Kew [:jfkthame]]

NAMEPREP doesn't seem to do anything special with it, AFAICT from a quick scan of the RFC; and Unicode NFKC normalization won't touch U+2010, as it has no Unicode decomposition mapping (though U+2011 does have a compatibility mapping to U+2010).

I think it'd make sense to add U+2010 to network.IDN.blacklist_chars; that should help here. Because of the NFKC mapping of U+2011, we shouldn't need to add that to the list.

(AFAICS, Safari and Chrome also don't force Punycode in the address bar when U+2010 is present, so I don't think this is unique to Firefox.)

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: Add U+2010 to IDN blacklist

Comment 4

[Simon Montagu :smontagu]

I would say that this is a registry issue more than a browser issue: registrar's policies shouldn't allow domains that differ only by hyphen (or underbar).

If I can spoof https://www.deutsche-bank.de/ with https://www.deutsche‐bank.de/, I can spoof it almost as effectively with https://www.deutsche_bank.de/ or https://www.deutschebank.de/

FWIW https://www.deutsche‑bank.de/ (with U+2011) should already be mapped to https://www.deutsche‐bank.de/ (with U+2010) by IDNA decomposition processing.

Comment 5

[Patrick McManus [:mcmanus]]

Comment on attachment 8818984 [details] [diff] [review]
Add U+2010 to IDN blacklist

Review of attachment 8818984 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not the right expert here..

Comment 6

[Simon Montagu :smontagu]

Comment on attachment 8818984 [details] [diff] [review]
Add U+2010 to IDN blacklist

Review of attachment 8818984 [details] [diff] [review]:
-----------------------------------------------------------------

What is the effect of the patch in practice, when hovering over the links, and in the URL bar?

Comment 7

[Jonathan Kew [:jfkthame]]

(In reply to Simon Montagu :smontagu from comment #4)
> FWIW https://www.deutsche‑bank.de/ (with U+2011) should already be mapped to
> https://www.deutsche‐bank.de/ (with U+2010) by IDNA decomposition processing.

Right, these both map to the same domain due to NFKC.

(In reply to Simon Montagu :smontagu from comment #6)
> Comment on attachment 8818984 [details] [diff] [review]
> Add U+2010 to IDN blacklist
> 
> Review of attachment 8818984 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> What is the effect of the patch in practice, when hovering over the links,
> and in the URL bar?

When hovering over the links, it causes them to show the punycode form xn--deutschebank-n09f.de in the status panel at the bottom of the window (whereas current Firefox shows the original Unicode). Similarly, doing Copy Link Location results in the punycode form (current Firefox copies the raw Unicode).

I'm not sure what the URL bar would display if the user actually went to such a site, as I don't know of a valid address to try (ideally, registrars shouldn't have allowed one to exist, but who knows...). The patch does, however, lead to the domain appearing in punycode in the connection failure page when I try to go to such a URL.

Comment 8

[Simon Montagu :smontagu]

There is an actual domain http://‐.com/ which you could try

Comment 9

[Armin Ebert [:arminius]]

(In reply to Jonathan Kew (:jfkthame) from comment #7)
> I'm not sure what the URL bar would display if the user actually went to
> such a site, as I don't know of a valid address to try (ideally, registrars
> shouldn't have allowed one to exist, but who knows...).

Start a local server and add this entry to /etc/hosts:

127.0.0.1 www.xn--deutschebank-n09f.de

Then visit http://www.xn--deutschebank-n09f.de/. It will be un-punycoded.

In conclusion you just need to register the Punycode domain xn--deutschebank-n09f.de to execute the spoof.

Comment 10

[Simon Montagu :smontagu]

Comment on attachment 8818984 [details] [diff] [review]
Add U+2010 to IDN blacklist

Review of attachment 8818984 [details] [diff] [review]:
-----------------------------------------------------------------

Anyway, that all sounds fine.

Comment 11

[Jonathan Kew [:jfkthame]]

(In reply to Simon Montagu :smontagu from comment #8)
> There is an actual domain http://‐.com/ which you could try

There is?! Sheesh.

Well, with the patch applied, that appears as http://xn--4ug.com/ in the URL bar. :)

Comment 12

[Gervase Markham [:gerv]]

Yes, I agree this should be added to the character blacklist, at least until we work out what's going on. I'm not sure why it's not mapped or otherwise dealt with by IDNA2008; seems like a no-brainer.

Gerv

Comment 13

[Jonathan Kew [:jfkthame]]

Comment on attachment 8818984 [details] [diff] [review]
Add U+2010 to IDN blacklist

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
The patch adds a character to the network.IDN.blacklist_chars pref, so that's a pretty clear hint that prior to the patch, that character might be used to attempt a spoof. Whether that could succeed might depend on whether an attacker can get a suitable spoof domain registered.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The current checkin comment refers to the IDN blacklist, which could be considered "painting a bulls-eye". I suggest we land this with a minimal "Bug 1323338." comment, though that in itself hints at a security issue, and there's no way to disguise what the patch itself does (as above).

Which older supported branches are affected by this flaw?
All.

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Trivial to backport (branches earlier than 51 may have a conflict with bug 1143644 in the context, but it's trivial to resolve).

How likely is this patch to cause regressions; how much testing does it need?
Minimal, it's just adding one more character to the blacklist.


Also nominating for aurora/beta/esr45, as the issue (and trivial fix) is common to all releases.

Comment 14

[Daniel Veditz [:dveditz]]

(In reply to Gervase Markham [:gerv] from comment #12)
> Yes, I agree this should be added to the character blacklist, at least until
> we work out what's going on. I'm not sure why it's not mapped or otherwise
> dealt with by IDNA2008; seems like a no-brainer.

Have we implemented IDNA2008, or just some tweaks on top of IDN2003? according to rfc5892 \u2010 is "DISALLOWED" along with the rest of that punctuation block (except ZWNJ and ZWJ which are CONTEXTJ).

Lowering severity to sec-moderate because as Simon points out there's some responsibility on registrars to prevent homographic spoofs like this. Also the fact that other browsers have this problem makes me wonder if the older IDNA2003 allowed this character.

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 8818984 [details] [diff] [review]
Add U+2010 to IDN blacklist

sec-approval=dveditz, a=dveditz for uplifts.

Don't contort yourself trying to come up with an obscure check-in comment since it's obvious from the patch itself. "Bug 1323338 update IDN tables" is fine.

Comment 16

[Armin Ebert [:arminius]]

(In reply to Daniel Veditz [:dveditz] from comment #14)
> Lowering severity to sec-moderate because as Simon points out there's some
> responsibility on registrars to prevent homographic spoofs like this. Also
> the fact that other browsers have this problem makes me wonder if the older
> IDNA2003 allowed this character.

It's indeed a little odd that the bug exists in multiple browsers but I couldn't trace back the cause. I should have mentioned that I initially notified Chromium too and they're in the process of fixing it.

Comment 17

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/06429d283d5b465e5c484dc6a7447ad03b2589c0
Bug 1323338 - Update IDN tables. r=smontagu

Comment 18

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0041f901b9e41b0cf52873b5f9255b0c8bb14983
https://hg.mozilla.org/releases/mozilla-beta/rev/8f5198636104f839d4f5021590cad684ba4773fa

Comment 19

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/releases/mozilla-esr45/rev/b21dee058b2c92cb69d5e95e94fa63f09bbb8c7c

Comment 20

[Simon Montagu :smontagu]

(In reply to Daniel Veditz [:dveditz] from comment #14)
> Have we implemented IDNA2008, or just some tweaks on top of IDN2003?
> according to rfc5892 \u2010 is "DISALLOWED" along with the rest of that
> punctuation block (except ZWNJ and ZWJ which are CONTEXTJ).

In http://www.unicode.org/Public/idna/latest/IdnaMappingTable.txt U+2010 is marked as "valid" with the annotation "NV8". According to http://www.unicode.org/reports/tr46/#Table_Data_File_Fields this means "the status is valid but the character is excluded by IDNA2008 from all domain names for all versions of Unicode".

I think further investigation is needed to be sure how ICU's implementation of IDNA2008 treats such characters, i.e. whether they just accept them as valid, or whether they are setting some flag which we aren't testing for. This covers other characters as well, as long as they don't trigger punicode by being marked as "restricted" in xidmodifications.txt/, for example U+058A ARMENIAN HYPHEN

Comment 21

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/06429d283d5b

Comment 22

[Gervase Markham [:gerv]]

(In reply to Daniel Veditz [:dveditz] from comment #14)
> Have we implemented IDNA2008, or just some tweaks on top of IDN2003?
> according to rfc5892 \u2010 is "DISALLOWED" along with the rest of that
> punctuation block (except ZWNJ and ZWJ which are CONTEXTJ).

Well, I hope we did! Bug 479520 and bug 1218179.

smontagu: do you have a moment to look into why this ever worked since we implemented 2008?

Gerv

Comment 23

[Simon Montagu :smontagu]

(In reply to Gervase Markham [:gerv] from comment #22)
> smontagu: do you have a moment to look into why this ever worked since we
> implemented 2008?

See comment 20

Comment 24

[Gervase Markham [:gerv]]

Oops, sorry, missed that. Would you care to open a follow-up bug, or shall I?

Gerv

Comment 25

[Simon Montagu :smontagu]

(In reply to Gervase Markham [:gerv] from comment #24)
> Oops, sorry, missed that. Would you care to open a follow-up bug, or shall I?

Bug 1324716

Comment 26

[Armin Ebert [:arminius]]

Could I be CC'd on the bug in case it's relevant to follow up?

Comment 27

[Daniel Veditz [:dveditz]]

This does not meet the severity criteria for the bug bounty program.

Comment 28

[ovidiu boca[:Ovidiu]]

Tested on Mac OS X 10.10, Windows 10 x64, Ubuntu 16.04 x64 with FF Beta 52.0b9 and FF ESR 45.8.0 and I can confirm the fix.

Comment 29

[Andrei Vaida [:avaida]]

Removing the qe-verify flag since this bug has been confirmed fixed on Fx52 and Fx45.8esr (see Comment 28).

Comment 30

[Mark Straver]

Whyfor the summary change on this old bug?