Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Issue 673971 Security: Unicode hyphens in domain names are not blacklisted
Starred by 3 users Reported by ar...@rawsec.net, Dec 14 2016 Back to list
Status: Fixed
Owner:
Closed: Dec 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Security
Team-Security-UX



Sign in to add a comment
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
 
Components: UI>Security>UrlFormatting
Status: Untriaged
Repros in Chrome 57.2950. Punycode hits the wire, but address bar shows the unencoded character.
url_formatter.cc seems like the likely home for a fix:
bool IDNSpoofChecker::Check(base::StringPiece16 label)
Labels: Security_Severity-Medium Security_Impact-Stable OS-All
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?

Project Member Comment 5 by sheriffbot@chromium.org, Dec 16 2016
Labels: M-56
Project Member Comment 6 by sheriffbot@chromium.org, Dec 16 2016
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
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?
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?
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.
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.
Status: Started
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. 
Project Member Comment 19 by bugdroid1@chromium.org, Dec 22 2016
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
Requesting Merge to 56. 

Project Member Comment 21 by sheriffbot@chromium.org, Dec 22 2016
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)
Project Member Comment 23 by sheriffbot@chromium.org, Dec 26 2016
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
Project Member Comment 24 by sheriffbot@chromium.org, Dec 29 2016
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
Labels: reward-topanel
I'm merging it to M56 branch now. Sorry for missing the merge approval on Dec 22. 


Project Member Comment 27 by bugdroid1@chromium.org, Jan 3
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

I came across bug 633922 (git drover issue), but merge seems to have succeeded. 
Jungshik: Thanks for dealing with landing and merging while I was away!
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


Labels: -reward-topanel reward-unpaid reward-2000
Nice one!  The panel decided to reward $2,000 for this report.
Labels: -reward-unpaid reward-inprocess
Labels: -Hotlist-Merge-Approved
Labels: Release-0-M56
Labels: CVE-2017-5015
Project Member Comment 37 by sheriffbot@chromium.org, Mar 30
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
Sign in to add a comment