Closed
Bug 1323338
(CVE-2017-5383)
Opened 8 years ago
Closed 8 years ago
Unicode hyphens in domain names are not blacklisted
Categories
(Core :: Networking, defect)
Tracking
()
People
(Reporter: arminius, Assigned: jfkthame)
References
Details
(Keywords: csectype-spoof, reporter-external, sec-moderate, Whiteboard: [post-critsmash-triage][adv-main51+][adv-esr45.7+])
Attachments
(1 file)
3.96 KB,
patch
|
smontagu
:
review+
dveditz
:
approval-mozilla-aurora+
dveditz
:
approval-mozilla-beta+
dveditz
:
approval-mozilla-esr45+
dveditz
:
sec-approval+
|
Details | Diff | Splinter Review |
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•8 years ago
|
||
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)
Keywords: csectype-spoof,
sec-high
Assignee | ||
Comment 2•8 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•8 years ago
|
||
Attachment #8818984 -
Flags: review?(mcmanus)
Comment 4•8 years ago
|
||
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•8 years ago
|
||
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 6•8 years ago
|
||
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?
Updated•8 years ago
|
Flags: needinfo?(smontagu) → needinfo?(jfkthame)
Assignee | ||
Comment 7•8 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)
Comment 8•8 years ago
|
||
There is an actual domain http://‐.com/ which you could try
Reporter | ||
Comment 9•8 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 10•8 years ago
|
||
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•8 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. :)
Comment 12•8 years ago
|
||
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•8 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?
Comment 14•8 years ago
|
||
(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-high → sec-moderate
Updated•8 years ago
|
status-firefox50:
--- → wontfix
status-firefox51:
--- → affected
status-firefox52:
--- → affected
status-firefox53:
--- → affected
status-firefox-esr45:
--- → affected
tracking-firefox51:
--- → +
tracking-firefox52:
--- → +
tracking-firefox53:
--- → +
tracking-firefox-esr45:
--- → 51+
Comment 15•8 years ago
|
||
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•8 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.
Updated•8 years ago
|
Group: core-security → network-core-security
Assignee | ||
Comment 17•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/06429d283d5b465e5c484dc6a7447ad03b2589c0
Bug 1323338 - Update IDN tables. r=smontagu
Assignee | ||
Comment 18•8 years ago
|
||
Assignee | ||
Comment 19•8 years ago
|
||
Comment 20•8 years ago
|
||
(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•8 years ago
|
||
Assignee: nobody → jfkthame
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Updated•8 years ago
|
Group: network-core-security → core-security-release
Assignee | ||
Updated•8 years ago
|
Comment 22•8 years ago
|
||
(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)
Comment 23•8 years ago
|
||
(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)
Comment 24•8 years ago
|
||
Oops, sorry, missed that. Would you care to open a follow-up bug, or shall I?
Gerv
Updated•8 years ago
|
Flags: qe-verify+
Comment 25•8 years ago
|
||
(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•8 years ago
|
||
Could I be CC'd on the bug in case it's relevant to follow up?
Updated•8 years ago
|
Flags: sec-bounty?
Comment 27•8 years ago
|
||
This does not meet the severity criteria for the bug bounty program.
Flags: sec-bounty? → sec-bounty-
Updated•8 years ago
|
Whiteboard: [post-critsmash-triage]
Updated•8 years ago
|
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main51+][adv-esr45.7+]
Updated•8 years ago
|
Alias: CVE-2017-5383
Comment 28•8 years ago
|
||
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•8 years ago
|
||
Removing the qe-verify flag since this bug has been confirmed fixed on Fx52 and Fx45.8esr (see Comment 28).
Flags: qe-verify+
Updated•7 years ago
|
Group: core-security-release
Updated•5 years ago
|
Flags: sec-bounty-hof+
Summary: Unicode hyphens in domain names are not blacklisted → hyphens in domain names
Comment 30•4 years ago
|
||
Whyfor the summary change on this old bug?
Reporter | ||
Updated•4 years ago
|
Summary: hyphens in domain names → Unicode hyphens in domain names are not blacklisted
Updated•8 months ago
|
Keywords: reporter-external
You need to log in
before you can comment on or make changes to this bug.
Description
•