New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Long OOO (go/where-is-mgiuca)
Closed: Dec 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Security
Team-Security-UX



Sign in to add a comment
link

Issue 673971: Security: Unicode hyphens in domain names are not blacklisted

Reported by ar...@rawsec.net, Dec 14 2016

Issue description

VULNERABILITY DETAILS
The Unicode glyphs U+2010 (HYPHEN) and U+2011 (NON-BREAKING HYPHEN) are not converted to Punycode when used in domain names, thus allowing spoofing via IDN homograph attacks.

(I notified Mozilla about the same issue.)

VERSION
Chrome Version: 55.0.2883.87 stable
Operating System: Linux 4.8.12-2-ARCH

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

The different hyphen characters are effectively indistinguishable in my browser's URL bar. Instead, the latter domain should be displayed as "xn--deutschebank-n09f.de".

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

Comment 1 by elawrence@chromium.org, Dec 14 2016

Components: UI>Security>UrlFormatting
Status: Untriaged (was: Unconfirmed)
Repros in Chrome 57.2950. Punycode hits the wire, but address bar shows the unencoded character.

Comment 2 by elawrence@chromium.org, Dec 14 2016

url_formatter.cc seems like the likely home for a fix:
bool IDNSpoofChecker::Check(base::StringPiece16 label)

Comment 3 by mbarbe...@chromium.org, Dec 15 2016

Labels: Security_Severity-Medium Security_Impact-Stable OS-All

Comment 4 by elawrence@chromium.org, Dec 15 2016

Cc: js...@chromium.org
jshin@ is fixing this as simple as:

--- a/components/url_formatter/url_formatter.cc
+++ b/components/url_formatter/url_formatter.cc
@@ -469,6 +469,8 @@ void IDNSpoofChecker::SetAllowedUnicodeSet(UErrorCode* status) {
   // should be safe. When used with a non-Hebrew script, it'd be filtered by
   // other checks in place.
   allowed_set.remove(0x338u);   // Combining Long Solidus Overlay
+  allowed_set.remove(0x2010u);  // Hyphen
+  allowed_set.remove(0x2011u);  // Non-breaking hyphen
   allowed_set.remove(0x2027u);  // Hyphenation Point

   uspoof_setAllowedUnicodeSet(checker_, &allowed_set, status);

... or is it more subtle than that?

Comment 5 by sheriffbot@chromium.org, Dec 16 2016

Project Member
Labels: M-56

Comment 6 by sheriffbot@chromium.org, Dec 16 2016

Project Member
Labels: Pri-1

Comment 7 by sleevi@google.com, Dec 19 2016

Cc: rsleevi@chromium.org

Comment 8 by mgiuca@chromium.org, Dec 19 2016

Owner: mgiuca@chromium.org
Since creis is OOO, I will take this. Looks straightforward. #4 should be sufficient.

Comment 9 by mgiuca@chromium.org, Dec 19 2016

Status: Assigned (was: Untriaged)

Comment 10 by rsleevi@chromium.org, Dec 19 2016

mgiuca: Should we coordinate with Mozilla to make sure we take the same/similar approach (or close to it)? jshin@ tried to sync up and align with FF folks, and to communicate&track where we diverged (as mentioned in https://www.chromium.org/developers/design-documents/idn-in-google-chrome )

armin: Do you have a bug number reference for Mozilla so that we can use that for discussions?

Comment 11 by mgiuca@chromium.org, Dec 19 2016

Hmm, if this needs to block on coordinating with Moz then maybe jshin should do it. He has a lot of experience with confusables and coordinating efforts with Mozilla.

Jungshik you there?

Comment 12 by rsleevi@chromium.org, Dec 20 2016

mgiuca@: I wasn't thinking block so much as having a parallel chat with them to make sure we don't miss anything :)

Comment 13 by ar...@rawsec.net, Dec 20 2016

rsleevi: It's bug #1323338: https://bugzilla.mozilla.org/show_bug.cgi?id=1323338

Approaching Mozilla on this is probably a good idea as they did some additional investigation on the implementation of IDNA2008 which might be relevant here, too.

Comment 14 by mgiuca@chromium.org, Dec 21 2016

CL to fix: https://codereview.chromium.org/2597523004/

Thanks for the suggestion and poking, elawrence@. It turns out we did not need to add U+2011 as that character is automatically normalized to U+2010 (and I added a test for this). So only U+2010 needed to be added to the blacklist.

Comment 15 by mgiuca@chromium.org, Dec 21 2016

Status: Started (was: Assigned)

Comment 16 by js...@chromium.org, Dec 21 2016

Sorry that I'm late here. A fix suggested seems good to me. I'll mark in the CL and add it to CQ.

Comment 17 by js...@chromium.org, Dec 21 2016

BTW, thanks a lot for catching this, armin@

Comment 18 by js...@chromium.org, Dec 21 2016

Cc: pkasting@chromium.org
To land a CL, I need Peter's owner approval. 

In the meantime, offline, I'll ask armin@ to add my mozilla account to the mozilla bug (comment 13) so that I can coordinate with them.

Comment 19 by bugdroid1@chromium.org, Dec 22 2016

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/60f6c0ff8de89ed3a4e4a7c182a0477edfacbfee

commit 60f6c0ff8de89ed3a4e4a7c182a0477edfacbfee
Author: Jungshik Shin <jshin@chromium.org>
Date: Thu Dec 22 00:30:36 2016

url_formatter: Remove U+2010 from the displayable character set.

Fix suggested by elawrence.
CL by mgiucia@ (landed by jshin@ on his behalf).

BUG= 673971 
TEST=components_unittests --gtest_filter=*IDN*Uni*
R=jshin@chromium.org, pkasting@chromium.org

Review-Url: https://codereview.chromium.org/2597523004 .
Cr-Commit-Position: refs/heads/master@{#440282}

[modify] https://crrev.com/60f6c0ff8de89ed3a4e4a7c182a0477edfacbfee/components/url_formatter/url_formatter.cc
[modify] https://crrev.com/60f6c0ff8de89ed3a4e4a7c182a0477edfacbfee/components/url_formatter/url_formatter_unittest.cc

Comment 20 by js...@chromium.org, Dec 22 2016

Labels: Merge-Request-56
Status: Fixed (was: Started)
Requesting Merge to 56.

Comment 21 by sheriffbot@chromium.org, Dec 22 2016

Project Member
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify

Comment 22 by dimu@chromium.org, Dec 22 2016

Labels: -Merge-Request-56 Merge-Approved-56 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M56 (branch: 2924)

Comment 23 by sheriffbot@chromium.org, Dec 26 2016

Project Member
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 24 by sheriffbot@chromium.org, Dec 29 2016

Project Member
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 25 by awhalley@chromium.org, Jan 2 2017

Labels: reward-topanel

Comment 26 by js...@chromium.org, Jan 3 2017

I'm merging it to M56 branch now. Sorry for missing the merge approval on Dec 22.

Comment 27 by bugdroid1@chromium.org, Jan 3 2017

Project Member
Labels: -merge-approved-56 merge-merged-2924
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ebb863dc1f62c21c578834de595a2ba54a36f1f1

commit ebb863dc1f62c21c578834de595a2ba54a36f1f1
Author: Jungshik Shin <jshin@chromium.org>
Date: Tue Jan 03 21:23:41 2017

url_formatter: Remove U+2010 from the displayable character set.

Fix suggested by elawrence.
CL by mgiucia@ (landed by jshin@ on his behalf).

BUG= 673971 
TEST=components_unittests --gtest_filter=*IDN*Uni*
R=jshin@chromium.org, pkasting@chromium.org

Review-Url: https://codereview.chromium.org/2597523004 .
Cr-Commit-Position: refs/heads/master@{#440282}
(cherry picked from commit 60f6c0ff8de89ed3a4e4a7c182a0477edfacbfee)

Review-Url: https://codereview.chromium.org/2613603002 .
Cr-Commit-Position: refs/branch-heads/2924@{#654}
Cr-Branched-From: 3a87aecc31cd1ffe751dd72c04e5a96a1fc8108a-refs/heads/master@{#433059}

[modify] https://crrev.com/ebb863dc1f62c21c578834de595a2ba54a36f1f1/components/url_formatter/url_formatter.cc
[modify] https://crrev.com/ebb863dc1f62c21c578834de595a2ba54a36f1f1/components/url_formatter/url_formatter_unittest.cc

Comment 28 by js...@chromium.org, Jan 3 2017

I came across bug 633922 (git drover issue), but merge seems to have succeeded.

Comment 29 by mgiuca@chromium.org, Jan 5 2017

Jungshik: Thanks for dealing with landing and merging while I was away!

Comment 30 by js...@chromium.org, Jan 5 2017

You're welcome, Matt ! (and thank you for the CL). 

As for Mozilla, they also blocked U+2010 (Thanks, armin@ for copying me to the two mozilla bugs on this). And, one related ICU bug was filed by a DENic folk: 

http://bugs.icu-project.org/trac/ticket/12906

Comment 31 by awhalley@chromium.org, Jan 9 2017

Labels: -reward-topanel reward-unpaid reward-2000

Comment 32 by awhalley@google.com, Jan 10 2017

Nice one!  The panel decided to reward $2,000 for this report.

Comment 33 by awhalley@chromium.org, Jan 10 2017

Labels: -reward-unpaid reward-inprocess

Comment 34 by awhalley@chromium.org, Jan 13 2017

Labels: -Hotlist-Merge-Approved

Comment 35 by awhalley@chromium.org, Jan 24 2017

Labels: Release-0-M56

Comment 36 by awhalley@chromium.org, Jan 25 2017

Labels: CVE-2017-5015

Comment 37 by sheriffbot@chromium.org, Mar 30 2017

Project Member
Labels: -Restrict-View-SecurityNotify allpublic
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 38 by awhalley@chromium.org, Apr 25 2018

Labels: CVE_description-submitted

Comment 39 by mea...@chromium.org, Oct 19

Labels: idn-spoof

Sign in to add a comment