Block domain labels made of Cyrillic letters that look alike Latin

components/url_formatter/url_formatter.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/url_formatter/url_formatter.h"
 
 #include <algorithm>
 #include <utility>
 
 #include "base/lazy_instance.h"
@@ skipping 13 matching lines @@
 #include "url/gurl.h"
 #include "url/third_party/mozilla/url_parse.h"
 
 namespace url_formatter {
 
 namespace {
 
 base::string16 IDNToUnicodeWithAdjustments(
     base::StringPiece host,
     base::OffsetAdjuster::Adjustments* adjustments);
-bool IDNToUnicodeOneComponent(const base::char16* comp,
-                              size_t comp_len,
-                              base::string16* out);
+bool IDNToUnicodeOneComponent(const base::char16* comp, size_t comp_len,
+                              bool is_tld_ascii, base::string16* out);
 
 class AppendComponentTransform {
  public:
   AppendComponentTransform() {}
   virtual ~AppendComponentTransform() {}
 
   virtual base::string16 Execute(
       const std::string& component_text,
       base::OffsetAdjuster::Adjustments* adjustments) const = 0;
 
@@ skipping 146 matching lines @@
 // allow unicode UNC hostnames regardless of encodings.
 base::string16 IDNToUnicodeWithAdjustments(
     base::StringPiece host, base::OffsetAdjuster::Adjustments* adjustments) {
   if (adjustments)
     adjustments->clear();
   // Convert the ASCII input to a base::string16 for ICU.
   base::string16 input16;
   input16.reserve(host.length());
   input16.insert(input16.end(), host.begin(), host.end());
 
+  bool is_tld_ascii = true;
+  size_t last_dot = host.rfind('.');
+  static const char* kAcePrefix = "xn--";
+  const size_t kAcePrefixLen = 4;
+  if (last_dot != base::StringPiece::npos &&
+      last_dot + kAcePrefixLen < host.length() &&
+      memcmp(kAcePrefix, host.data() + last_dot + 1, kAcePrefixLen) == 0) {
+    is_tld_ascii = false;

[sffc, 2017/02/15 19:57:30]

I don't really understand what this part of the code is doing.

[jungshik at Google, 2017/02/15 20:55:52]

It's checking if the TLD starts with 'xn--'. Otherwise, it assumes that it's a
non-IDN TLD. Note that |host| is in ACE at this point. 

+  }
+
   // Do each component of the host separately, since we enforce script matching
   // on a per-component basis.
   base::string16 out16;
   for (size_t component_start = 0, component_end;
        component_start < input16.length();
        component_start = component_end + 1) {
     // Find the end of the component.
     component_end = input16.find('.', component_start);
     if (component_end == base::string16::npos)
       component_end = input16.length();  // For getting the last component.
     size_t component_length = component_end - component_start;
     size_t new_component_start = out16.length();
     bool converted_idn = false;
     if (component_end > component_start) {
       // Add the substring that we just found.
       converted_idn =
           IDNToUnicodeOneComponent(input16.data() + component_start,
-                                   component_length, &out16);
+                                   component_length, is_tld_ascii, &out16);
     }
     size_t new_component_length = out16.length() - new_component_start;
 
     if (converted_idn && adjustments) {
       adjustments->push_back(base::OffsetAdjuster::Adjustment(
           component_start, component_length, new_component_length));
     }
 
     // Need to add the dot we just found (if we found one).
     if (component_end < input16.length())
       out16.push_back('.');
   }
   return out16;
 }
 
 // A helper class for IDN Spoof checking, used to ensure that no IDN input is
 // spoofable per Chromium's standard of spoofability. For a more thorough
 // explanation of how spoof checking works in Chromium, see
 // http://dev.chromium.org/developers/design-documents/idn-in-google-chrome .
 class IDNSpoofChecker {
  public:
   IDNSpoofChecker();
 
-  // Returns true if |label| is safe to display as Unicode. In the event of
-  // library failure, all IDN inputs will be treated as unsafe.
-  bool Check(base::StringPiece16 label);
+  // Returns true if |label| is safe to display as Unicode. When
+  // TLD is ASCII, check if a label is entirely made of
+  // Cyrillic letters that look alike Latin letters. In the event of library
+  // failure, all IDN inputs will be treated as unsafe.
+  bool Check(base::StringPiece16 label, bool is_tld_ascii);
 
  private:
   void SetAllowedUnicodeSet(UErrorCode* status);
 
   USpoofChecker* checker_;
   icu::UnicodeSet deviation_characters_;
   icu::UnicodeSet non_ascii_latin_letters_;
   icu::UnicodeSet kana_letters_exceptions_;
+  icu::UnicodeSet cyrillic_letters_latin_alike_;
 
   DISALLOW_COPY_AND_ASSIGN(IDNSpoofChecker);
 };
 
 base::LazyInstance<IDNSpoofChecker>::Leaky g_idn_spoof_checker =
     LAZY_INSTANCE_INITIALIZER;
 base::ThreadLocalStorage::StaticSlot tls_index = TLS_INITIALIZER;
 
 void OnThreadTermination(void* regex_matcher) {
   delete reinterpret_cast<icu::RegexMatcher*>(regex_matcher);
@@ skipping 41 matching lines @@
   // because additional characters pulled in with scx=Latn are not included in
   // the allowed set.
   non_ascii_latin_letters_ = icu::UnicodeSet(
       UNICODE_STRING_SIMPLE("[[:Latin:] - [a-zA-Z]]"), status);
   non_ascii_latin_letters_.freeze();
 
   // These letters are parts of |dangerous_patterns_|.
   kana_letters_exceptions_ = icu::UnicodeSet(UNICODE_STRING_SIMPLE(
       "[\\u3078-\\u307a\\u30d8-\\u30da\\u30fb\\u30fc]"), status);
   kana_letters_exceptions_.freeze();
+  // These Cyrillic letters look alike Latin. A domain label entirely
+  // made of these letters are blocked as a poorman's whole-script-spoofable.
+  cyrillic_letters_latin_alike_ = icu::UnicodeSet(
+      icu::UnicodeString("[аеорсухьѕіјһмӏтнв]"), status);

[jungshik at Google, 2017/02/15 18:50:21]

"м, т, н, в" look like smallcap Latin and it's debatable whether or not to
include them in the set. 

[sffc, 2017/02/15 19:57:30]

I'd look at the capital letters too.  Here's a possible list of strong matches
based on manual inspection of confusables.txt, which includes more characters
than your list:

асԁеһіјӏорԛѕԝхуАВСЕԌНІӀЈКМОРЅТԜХҮ

And here's a possibly incomplete list of weaker matches; up to you whether you
want to include them:

ӕъЬвҽвԍнкпгмтцѵѡүӔѴҲ

[lgarron, 2017/02/15 20:24:49]

Also consider ԁ, Ӏ, and maybe ѵ, listed at the bottom of [1].


[1] https://en.wikipedia.org/wiki/IDN_homograph_attack#Cyrillic

[lgarron, 2017/02/15 20:24:49]

Also consider ԁ, Ӏ, and maybe ѵ, listed at the bottom of [1].


[1] https://en.wikipedia.org/wiki/IDN_homograph_attack#Cyrillic

[lgarron, 2017/02/15 20:25:56]

(Note that Ӏ is a spoof of l, not just I.)

[jungshik at Google, 2017/02/15 20:55:52]

Well, uppercase letters will not 'survive' (they'll all case-mapped to lowercase
letters before being displayed). And, one cannot register domains with uppercase
letters with most (if not all) registrars. For instance, you cannot in
Verisign-controlled TLDs. 
 
Thank you for the list. 

Expanding the set has a risk of having too many false positives. The list is
currently in flux and I'll try various sets and see how they work. 

[jungshik at Google, 2017/02/15 20:55:52]

Thanks. U+0501 and U+0475 I'll consider. As for U+04C0, see below. 
U+04C0 ( 'Ӏ') is uppercase so that it will be normalized away to lowercase
(U+04CF that is already included) before being displayed. 

+  cyrillic_letters_latin_alike_.freeze();
 
   DCHECK(U_SUCCESS(status));
 }
 
-bool IDNSpoofChecker::Check(base::StringPiece16 label) {
+bool IDNSpoofChecker::Check(base::StringPiece16 label,
+                            bool is_tld_ascii) {
   UErrorCode status = U_ZERO_ERROR;
   int32_t result = uspoof_check(checker_, label.data(),
                                 base::checked_cast<int32_t>(label.size()),
                                 NULL, &status);
   // If uspoof_check fails (due to library failure), or if any of the checks
   // fail, treat the IDN as unsafe.
   if (U_FAILURE(status) || (result & USPOOF_ALL_CHECKS))
     return false;
 
   icu::UnicodeString label_string(FALSE, label.data(),
                                   base::checked_cast<int32_t>(label.size()));
 
   // A punycode label with 'xn--' prefix is not subject to the URL
   // canonicalization and is stored as it is in GURL. If it encodes a deviation
   // character (UTS 46; e.g. U+00DF/sharp-s), it should be still shown in
   // punycode instead of Unicode. Without this check, xn--fu-hia for
   // 'fu<sharp-s>' would be converted to 'fu<sharp-s>' for display because
   // "UTS 46 section 4 Processing step 4" applies validity criteria for
   // non-transitional processing (i.e. do not map deviation characters) to any
   // punycode labels regardless of whether transitional or non-transitional is
   // chosen. On the other hand, 'fu<sharp-s>' typed or copy and pasted
   // as Unicode would be canonicalized to 'fuss' by GURL and is displayed as
   // such. See http://crbug.com/595263 .
   if (deviation_characters_.containsSome(label_string))
     return false;
 
   // If there's no script mixing, the input is regarded as safe without any
-  // extra check unless it contains Kana letter exceptions. Note that
+  // extra check unless it contains Kana letter exceptions or it's made enitrely
+  // of Cyrillic letters that look alike Latin letters. Note that
   // the following combinations of scripts are treated as a 'logical' single
   // script.
   //  - Chinese: Han, Bopomofo, Common
   //  - Japanese: Han, Hiragana, Katakana, Common
   //  - Korean: Hangul, Han, Common
   result &= USPOOF_RESTRICTION_LEVEL_MASK;
-  if (result == USPOOF_ASCII ||
-      (result == USPOOF_SINGLE_SCRIPT_RESTRICTIVE &&
-       kana_letters_exceptions_.containsNone(label_string)))
+  if (result == USPOOF_ASCII) return true;
+  // Check Cyrillic confusable only for ASCII TLDs.
+  if (is_tld_ascii && cyrillic_letters_latin_alike_.containsAll(label_string))

[sffc, 2017/02/15 19:57:30]

I think you should compare only the letter characters.  For example, a spoof
string "рох-рох" that contains a non-letter character should still be considered
a spoof attack.

[jungshik at Google, 2017/02/15 20:55:52]

That's a good point. Thanks !

+    return false;
+  if (result == USPOOF_SINGLE_SCRIPT_RESTRICTIVE &&
+      kana_letters_exceptions_.containsNone(label_string))
     return true;
 
   // Additional checks for |label| with multiple scripts, one of which is Latin.
   // Disallow non-ASCII Latin letters to mix with a non-Latin script.
   if (non_ascii_latin_letters_.containsSome(label_string))
     return false;
 
   if (!tls_index.initialized())
     tls_index.Initialize(&OnThreadTermination);
   icu::RegexMatcher* dangerous_pattern =
@@ skipping 106 matching lines @@
   allowed_set.remove(0x2010u);  // Hyphen
   allowed_set.remove(0x2027u);  // Hyphenation Point
 
   uspoof_setAllowedUnicodeSet(checker_, &allowed_set, status);
 }
 
 // Returns true if the given Unicode host component is safe to display to the
 // user. Note that this function does not deal with pure ASCII domain labels at
 // all even though it's possible to make up look-alike labels with ASCII
 // characters alone.
-bool IsIDNComponentSafe(base::StringPiece16 label) {
-  return g_idn_spoof_checker.Get().Check(label);
+bool IsIDNComponentSafe(base::StringPiece16 label, bool is_tld_ascii) {
+  return g_idn_spoof_checker.Get().Check(label, is_tld_ascii);
 }
 
 // A wrapper to use LazyInstance<>::Leaky with ICU's UIDNA, a C pointer to
 // a UTS46/IDNA 2008 handling object opened with uidna_openUTS46().
 //
 // We use UTS46 with BiDiCheck to migrate from IDNA 2003 to IDNA 2008 with the
 // backward compatibility in mind. What it does:
 //
 // 1. Use the up-to-date Unicode data.
 // 2. Define a case folding/mapping with the up-to-date Unicode data as in
@@ skipping 24 matching lines @@
 
 base::LazyInstance<UIDNAWrapper>::Leaky g_uidna = LAZY_INSTANCE_INITIALIZER;
 
 // Converts one component (label) of a host (between dots) to Unicode if safe.
 // The result will be APPENDED to the given output string and will be the
 // same as the input if it is not IDN in ACE/punycode or the IDN is unsafe to
 // display.
 // Returns whether any conversion was performed.
 bool IDNToUnicodeOneComponent(const base::char16* comp,
                               size_t comp_len,
+                              bool is_tld_ascii,
                               base::string16* out) {
   DCHECK(out);
   if (comp_len == 0)
     return false;
 
   // Only transform if the input can be an IDN component.
   static const base::char16 kIdnPrefix[] = {'x', 'n', '-', '-'};
   if ((comp_len > arraysize(kIdnPrefix)) &&
       !memcmp(comp, kIdnPrefix, sizeof(kIdnPrefix))) {
     UIDNA* uidna = g_uidna.Get().value;
@@ skipping 12 matching lines @@
           uidna, comp, static_cast<int32_t>(comp_len), &(*out)[original_length],
           output_length, &info, &status);
     } while ((status == U_BUFFER_OVERFLOW_ERROR && info.errors == 0));
 
     if (U_SUCCESS(status) && info.errors == 0) {
       // Converted successfully. Ensure that the converted component
       // can be safely displayed to the user.
       out->resize(original_length + output_length);
       if (IsIDNComponentSafe(
           base::StringPiece16(out->data() + original_length,
-                              base::checked_cast<size_t>(output_length))))
+                              base::checked_cast<size_t>(output_length)),
+          is_tld_ascii))
         return true;
     }
 
     // Something went wrong. Revert to original string.
     out->resize(original_length);
   }
 
   // We get here with no IDN or on error, in which case we just append the
   // literal input.
   out->append(comp, comp_len);
@@ skipping 226 matching lines @@
   return base::StartsWith(text, www, base::CompareCase::SENSITIVE)
       ? text.substr(www.length()) : text;
 }
 
 base::string16 StripWWWFromHost(const GURL& url) {
   DCHECK(url.is_valid());
   return StripWWW(base::ASCIIToUTF16(url.host_piece()));
 }
 
 }  // namespace url_formatter