Bug 1323338 (CVE-2017-5383)

Unicode hyphens in domain names are not blacklisted

RESOLVED FIXED in Firefox -esr45

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: armin, Assigned: jfkthame)

Tracking

({csectype-spoof, sec-moderate})

50 Branch
mozilla53
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty -

Firefox Tracking Flags

(firefox-esr4551+ verified, firefox50 wontfix, firefox51+ fixed, firefox52+ verified, firefox53+ fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main51+][adv-esr45.7+])

Attachments

(1 attachment)

Reporter

Description

3 years ago
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
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(smontagu)
Assignee

Comment 2

3 years ago
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.)
Assignee

Comment 3

3 years ago
Attachment #8818984 - Flags: review?(mcmanus)
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 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..
Attachment #8818984 - Flags: review?(mcmanus) → review?(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?
Flags: needinfo?(smontagu) → needinfo?(jfkthame)
Assignee

Comment 7

3 years ago
(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.
Flags: needinfo?(jfkthame)
There is an actual domain http://‐.com/ which you could try
Reporter

Comment 9

3 years ago
(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 on attachment 8818984 [details] [diff] [review]
Add U+2010 to IDN blacklist

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

Anyway, that all sounds fine.
Attachment #8818984 - Flags: review?(smontagu) → review+
Assignee

Comment 11

3 years ago
(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. :)
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
Assignee

Comment 13

3 years ago
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.
Attachment #8818984 - Flags: sec-approval?
Attachment #8818984 - Flags: approval-mozilla-esr45?
Attachment #8818984 - Flags: approval-mozilla-beta?
Attachment #8818984 - Flags: approval-mozilla-aurora?
(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.
Keywords: sec-highsec-moderate
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.
Attachment #8818984 - Flags: sec-approval?
Attachment #8818984 - Flags: sec-approval+
Attachment #8818984 - Flags: approval-mozilla-esr45?
Attachment #8818984 - Flags: approval-mozilla-esr45+
Attachment #8818984 - Flags: approval-mozilla-beta?
Attachment #8818984 - Flags: approval-mozilla-beta+
Attachment #8818984 - Flags: approval-mozilla-aurora?
Attachment #8818984 - Flags: approval-mozilla-aurora+
Reporter

Comment 16

3 years ago
(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.
Group: core-security → network-core-security
(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
https://hg.mozilla.org/mozilla-central/rev/06429d283d5b
Assignee: nobody → jfkthame
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Group: network-core-security → core-security-release
(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
Flags: needinfo?(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
Flags: needinfo?(smontagu)
Oops, sorry, missed that. Would you care to open a follow-up bug, or shall I?

Gerv
Flags: qe-verify+
Blocks: 1324716
(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
Reporter

Comment 26

3 years ago
Could I be CC'd on the bug in case it's relevant to follow up?
Flags: sec-bounty?
This does not meet the severity criteria for the bug bounty program.
Flags: sec-bounty? → sec-bounty-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main51+][adv-esr45.7+]
Alias: CVE-2017-5383
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.
Removing the qe-verify flag since this bug has been confirmed fixed on Fx52 and Fx45.8esr (see Comment 28).
Flags: qe-verify+
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.