Mitigate spoofing attempt using Latin letters.

components/url_formatter/url_formatter.cc

Index: components/url_formatter/url_formatter.cc
diff --git a/components/url_formatter/url_formatter.cc b/components/url_formatter/url_formatter.cc
index d54b67913ce76d400907266963475bae6920cdd9..0bd994a92461497b9d59cff6766aca3b13ef84ad 100644
--- a/components/url_formatter/url_formatter.cc
+++ b/components/url_formatter/url_formatter.cc
@@ -6,21 +6,26 @@
 
 #include <algorithm>
 #include <utility>
+#include <vector>
 
 #include "base/lazy_instance.h"
 #include "base/macros.h"
 #include "base/numerics/safe_conversions.h"
 #include "base/strings/string_piece.h"
+#include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_offset_string_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread_local_storage.h"
+#include "net/base/lookup_string_in_fixed_set.h"
 #include "third_party/icu/source/common/unicode/schriter.h"
 #include "third_party/icu/source/common/unicode/uidna.h"
 #include "third_party/icu/source/common/unicode/uniset.h"
 #include "third_party/icu/source/common/unicode/uscript.h"
+#include "third_party/icu/source/common/unicode/utypes.h"
 #include "third_party/icu/source/common/unicode/uvernum.h"
 #include "third_party/icu/source/i18n/unicode/regex.h"
+#include "third_party/icu/source/i18n/unicode/translit.h"
 #include "third_party/icu/source/i18n/unicode/uspoof.h"
 #include "url/gurl.h"
 #include "url/third_party/mozilla/url_parse.h"
@@ -191,6 +196,56 @@ base::string16 FormatViewSourceUrl(
   return result;
 }
 
+// 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 {

[Peter Kasting, 2017/05/09 01:37:03]

Nit: It might be nice to pull this class out to its own .h/.cc for maximum
readability.

[jungshik at Google, 2017/05/10 18:05:13]

Ok. pulled it out. 

+ public:
+  IDNSpoofChecker();
+
+  // Returns true if |label| is safe to display as Unicode. When the TLD is

[Peter Kasting, 2017/05/09 01:37:03]

Nit: Does the second sentence here really need to be here?  It seems like it
only describes a portion of the functionality of the function.  Maybe we should
just say "See the function body for details on the specific safety checks
performed"?

[jungshik at Google, 2017/05/10 18:05:13]

Yeah, that's better. 

+  // ASCII, check if a label is entirely made of Cyrillic letters that look like
+  // 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);

[Peter Kasting, 2017/05/09 01:37:03]

Nit: This is a poor function name; how about something like
SafeToDisplayAsUnicode()?

[jungshik at Google, 2017/05/10 18:05:13]

Done.

+
+  // Returns true if |hostname| or the last few components of |hostname| looks
+  // similar to one of top domains listed in top_domains/alexa_domains.list. Two
+  // checks are done:
+  //   1. Calculate the skeleton of |hostname| based on the Unicode confusable
+  //   character list and look it up in the pre-calculated skeleton list of
+  //   top domains.
+  //   2. Look up the diacritic-free version of |hostname| in the list of
+  //   top domains. Note that non-IDN hostnames will not get here.
+  bool SimilarToTopDomains(base::StringPiece16 hostname);
+
+ private:
+  void SetAllowedUnicodeSet(UErrorCode* status);

[Peter Kasting, 2017/05/09 01:37:03]

Nit: I suggest adding comments for these even though they're private.

[jungshik at Google, 2017/05/10 18:05:13]

Done.

+  bool IsMadeOfLatinAlikeCyrillic(const icu::UnicodeString& label_string);
+  bool GetSkeleton(base::StringPiece16 hostname, std::string* skeleton);
+
+  USpoofChecker* checker_;
+  icu::UnicodeSet deviation_characters_;
+  icu::UnicodeSet non_ascii_latin_letters_;
+  icu::UnicodeSet kana_letters_exceptions_;
+  icu::UnicodeSet combining_diacritics_exceptions_;
+  icu::UnicodeSet cyrillic_letters_;
+  icu::UnicodeSet cyrillic_letters_latin_alike_;
+  icu::UnicodeSet lgc_letters_n_ascii_;
+  icu::Transliterator* transliterator_;
+
+  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) {

[Peter Kasting, 2017/05/09 01:37:03]

Let me guess: the RegexMatcher uses internal state, so it's not possible to
simultaneously use it from multiple threads, hence the need to stick it in TLS.

+  delete reinterpret_cast<icu::RegexMatcher*>(regex_matcher);
+}
+
 // TODO(brettw): We may want to skip this step in the case of file URLs to
 // allow unicode UNC hostnames regardless of encodings.
 base::string16 IDNToUnicodeWithAdjustments(
@@ -212,6 +267,7 @@ base::string16 IDNToUnicodeWithAdjustments(
   // Do each component of the host separately, since we enforce script matching
   // on a per-component basis.
   base::string16 out16;
+  bool has_idn_component = false;

[Peter Kasting, 2017/05/09 01:37:03]

Can we reach this function with an input that doesn't cause the loop below to
set this to true?  It seems unlikely.  If not, we could eliminate this variable.

[jungshik at Google, 2017/05/10 18:05:13]

IDNToUnicode (which calls this function) is called with any host name out of
GURL. So, has_idn_component can be false for all labels/components. Then,
has_idn_component would be false, too. 

   for (size_t component_start = 0, component_end;
        component_start < input16.length();
        component_start = component_end + 1) {
@@ -227,6 +283,7 @@ base::string16 IDNToUnicodeWithAdjustments(
       converted_idn =
           IDNToUnicodeOneComponent(input16.data() + component_start,
                                    component_length, is_tld_ascii, &out16);
+      has_idn_component = has_idn_component || converted_idn;

[Peter Kasting, 2017/05/09 01:37:03]

Nit: Or use |=

[jungshik at Google, 2017/05/10 18:05:13]

Changed. 

     }
     size_t new_component_length = out16.length() - new_component_start;
 
@@ -239,43 +296,14 @@ base::string16 IDNToUnicodeWithAdjustments(
     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. When the TLD is
-  // ASCII, check if a label is entirely made of Cyrillic letters that look like
-  // 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);
-  bool IsMadeOfLatinAlikeCyrillic(const icu::UnicodeString& label_string);
-
-  USpoofChecker* checker_;
-  icu::UnicodeSet deviation_characters_;
-  icu::UnicodeSet non_ascii_latin_letters_;
-  icu::UnicodeSet kana_letters_exceptions_;
-  icu::UnicodeSet cyrillic_letters_;
-  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);
+  if (has_idn_component &&

[Peter Kasting, 2017/05/09 01:37:03]

Nit: Might want a comment above this block like "Leave as punycode any inputs
that spoof top domains."

[jungshik at Google, 2017/05/10 18:05:13]

Done.

+      g_idn_spoof_checker.Get().SimilarToTopDomains(out16)) {
+    if (adjustments)
+      adjustments->clear();
+    return input16;
+  }

[Peter Kasting, 2017/05/09 01:37:03]

Nit: Blank line after this?

[jungshik at Google, 2017/05/10 18:05:13]

Done.

+  return out16;
 }
 
 IDNSpoofChecker::IDNSpoofChecker() {
@@ -323,11 +351,14 @@ IDNSpoofChecker::IDNSpoofChecker() {
       UNICODE_STRING_SIMPLE("[[:Latin:] - [a-zA-Z]]"), status);
   non_ascii_latin_letters_.freeze();
 
-  // These letters are parts of |dangerous_patterns_|.
+  // The following two sets are parts of |dangerous_patterns_|.
   kana_letters_exceptions_ = icu::UnicodeSet(
       UNICODE_STRING_SIMPLE("[\\u3078-\\u307a\\u30d8-\\u30da\\u30fb-\\u30fe]"),
       status);
   kana_letters_exceptions_.freeze();
+  combining_diacritics_exceptions_ = icu::UnicodeSet(
+      UNICODE_STRING_SIMPLE("[\\u0300-\\u0339]"), status);
+  combining_diacritics_exceptions_.freeze();
 
   // These Cyrillic letters look like Latin. A domain label entirely made of
   // these letters is blocked as a simplified whole-script-spoofable.
@@ -339,7 +370,37 @@ IDNSpoofChecker::IDNSpoofChecker() {
       icu::UnicodeSet(UNICODE_STRING_SIMPLE("[[:Cyrl:]]"), status);
   cyrillic_letters_.freeze();
 
-  DCHECK(U_SUCCESS(status));
+  // This set is used to determine whether or not to apply a slow
+  // transliteration to remove diacritics to a given hostname before the
+  // confusable skeleton calculation for comparison with top domain names. If
+  // it has any character outside the set, the expensive step will be skipped
+  // because it cannot match any of top domain names.
+  // The last ([\u0300-\u0339] is a shorthand for "[:Identifier_Status=Allowed:]
+  // & [:Script_Extensions=Inherited:] - [\\u200C\\u200D]". The latter is a
+  // subset of the former but it does not matter because hostnames with
+  // characters outside the latter set would be rejected in an earlier step.
+  lgc_letters_n_ascii_ = icu::UnicodeSet(UNICODE_STRING_SIMPLE(
+      "[[:Latin:][:Greek:][:Cyrillic:][0-9\\u002e_\\u002d][\\u0300-\\u0339]]"),
+      status);
+  lgc_letters_n_ascii_.freeze();
+
+  // Used for diacritics-removal before the skeleton calculation. Add
+  // "ł > l; ø > o; đ > d" that are not handled by "NFD; Nonspacing mark
+  // removal; NFC". On top of that, supplement the Unicode confusable list by
+  // replacing {U+043A (к), U+0138(ĸ), U+03BA(κ)}, U+04CF (ӏ) and U+043F(п) by
+  // 'k', 'l' and 'n', respectively.
+  // TODO(jshin): Revisit "ł > l; ø > o" mapping.

[Peter Kasting, 2017/05/09 01:37:03]

Nit: Might want to link this TODO to a bug or otherwise expand on why/how you'd
revisit, or this won't be actionable by others.

+  UParseError parse_error;
+  transliterator_ = icu::Transliterator::createFromRules(
+      UNICODE_STRING_SIMPLE("DropAcc"),
+      icu::UnicodeString("::NFD; ::[:Nonspacing Mark:] Remove; ::NFC;"
+                         " ł > l; ø > o; đ > d; ӏ > l; [кĸκ] > k; п > n;"),
+      UTRANS_FORWARD, parse_error, status);
+  DCHECK(U_SUCCESS(status))
+      << "Spoofchecker initalization failed due to an error: "
+      << u_errorName(status);
+  if (U_FAILURE(status))

[Peter Kasting, 2017/05/09 01:37:03]

Do not handle DCHECK failure; assume DCHECKs cannot fail.  If they can, they
should be conditionals, not DCHECKs.

[jungshik at Google, 2017/05/10 18:05:13]

Done.

+    transliterator_ = nullptr;
 }
 
 bool IDNSpoofChecker::Check(base::StringPiece16 label, bool is_tld_ascii) {
@@ -370,23 +431,31 @@ bool IDNSpoofChecker::Check(base::StringPiece16 label, bool is_tld_ascii) {
     return false;
 
   // If there's no script mixing, the input is regarded as safe without any
-  // extra check unless it contains Kana letter exceptions or it's made entirely
-  // of Cyrillic letters that look like Latin letters. Note that the following
-  // combinations of scripts are treated as a 'logical' single script.
+  // extra check unless it falls into one of three categories:
+  //   - contains Kana letter exceptions
+  //   - it's made entirely of Cyrillic letters that look like Latin letters.

[Peter Kasting, 2017/05/09 01:37:03]

Nit: it's -> the TLD is ASCII, and the input is ?

[jungshik at Google, 2017/05/10 18:05:13]

Done.

+  //   - it has combining diacritic marks.
+  // 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) return true;
   if (result == USPOOF_SINGLE_SCRIPT_RESTRICTIVE &&
-      kana_letters_exceptions_.containsNone(label_string)) {
+      kana_letters_exceptions_.containsNone(label_string) &&
+      combining_diacritics_exceptions_.containsNone(label_string)) {
     // Check Cyrillic confusable only for ASCII TLDs.
     return !is_tld_ascii || !IsMadeOfLatinAlikeCyrillic(label_string);
   }
 
   // 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))
+  // Note that non-ASCII Latin check should not be applied when the entire label

[Peter Kasting, 2017/05/09 01:37:03]

Nit: that -> that the ?

[jungshik at Google, 2017/05/10 18:05:13]

Done.

+  // is made of Latin. Checking with lgc_letters set here should be fine because
+  // script mixing of LGC is already rejected.
+  if (non_ascii_latin_letters_.containsSome(label_string) &&
+      !lgc_letters_n_ascii_.containsAll(label_string))
     return false;
 
   if (!tls_index.initialized())
@@ -415,6 +484,9 @@ bool IDNSpoofChecker::Check(base::StringPiece16 label, bool is_tld_ascii) {
     // - Disallow U+0585 (Armenian Small Letter Oh) and U+0581 (Armenian Small
     //   Letter Co) to be next to Latin.
     // - Disallow Latin 'o' and 'g' next to Armenian.
+    // - Disallow combining diacritical mark (U+0300-U+0339) after a non-LGC
+    //   character. Other combining diacritical marks are not in the allowed
+    //   character set.
     dangerous_pattern = new icu::RegexMatcher(
         icu::UnicodeString(
             "[^\\p{scx=kana}\\p{scx=hira}\\p{scx=hani}]"
@@ -428,7 +500,8 @@ bool IDNSpoofChecker::Check(base::StringPiece16 label, bool is_tld_ascii) {
             "^[\\u0585\\u0581]+[a-z]|[a-z][\\u0585\\u0581]+$|"
             "[a-z][\\u0585\\u0581]+[a-z]|"
             "^[og]+[\\p{scx=armn}]|[\\p{scx=armn}][og]+$|"
-            "[\\p{scx=armn}][og]+[\\p{scx=armn}]",
+            "[\\p{scx=armn}][og]+[\\p{scx=armn}]|"
+            "[^\\p{scx=latn}\\p{scx=grek}\\p{scx=cyrl}][\\u0300-\\u0339]",
             -1, US_INV),
         0, status);
     tls_index.Set(dangerous_pattern);
@@ -437,6 +510,59 @@ bool IDNSpoofChecker::Check(base::StringPiece16 label, bool is_tld_ascii) {
   return !dangerous_pattern->find();
 }
 
+#include "components/url_formatter/top_domains/alexa_skeletons-inc.cc"
+// All the domains in the above file have 3 or fewer labels.
+const size_t kNumberOfLabelsToCheck = 3;

[Peter Kasting, 2017/05/09 01:37:03]

Can we write this value into the file so we don't need to hardcode it here?

[jungshik at Google, 2017/05/10 18:05:13]

make_top_domain_gperf  can write that out to another file (the only line other
than license boilerplate would be the above line). Do you like that? 

[Peter Kasting, 2017/05/10 22:38:46]

Can't we write it to the same file?  Having a separate file for this works, I
guess, it just feels inelegant.  Dunno why I'm worrying.  It'd still probably be
better than hardcoding this.

+
+bool LookupStringInSet(base::StringPiece needle,

[Peter Kasting, 2017/05/09 01:37:04]

Nit: If you're not going to use boring names for your params, I'd copy the ones
from the underlying net:: declaration rather than using |needle|.

That said, this wrapper is so short, and is called only once, that I'd just
inline the body of this at the callsite below.

[jungshik at Google, 2017/05/10 18:05:13]

Inlined it.

+                       const unsigned char* fixed_set,
+                       size_t set_len) {
+  return net::LookupStringInFixedSet(fixed_set, set_len, needle.data(),
+                                     needle.length()) != net::kDafsaNotFound;
+}
+
+bool LookupMatchInTopDomains(base::StringPiece hostname) {
+  // When 'hostname' is a skeleton instead of actual hostname, it's assumed
+  // that no character other than '.' among those allowed in IDN will have
+  // '.' as its skeleton.
+  DCHECK(hostname[hostname.length() - 1] != '.');

[Peter Kasting, 2017/05/09 01:37:03]

Nit: hostname.back()

[jungshik at Google, 2017/05/10 18:05:13]

Done.

+  auto labels = base::SplitStringPiece(hostname, ".", base::KEEP_WHITESPACE,
+                                       base::SPLIT_WANT_ALL);
+
+  while (labels.size() > kNumberOfLabelsToCheck)
+    labels.erase(labels.begin());

[Peter Kasting, 2017/05/09 01:37:04]

Nit: Seems like a single call to vector::erase could be more efficient than a
while loop.

[jungshik at Google, 2017/05/10 18:05:13]

Done.

+
+  while (labels.size() > 1) {

[Peter Kasting, 2017/05/09 01:37:03]

Is this naive loop faster than computing the actual eTLD+1 length using the RCDS
and then doing a single DAFSA lookup?

[jungshik at Google, 2017/05/10 18:05:13]

'hostname' is not a good name (at one point, it's either a hostname or its
skeleton, but now it's always a skeleton). Changed it to |skeleton|. eTLD match
cannot be done because even 'com' is turned to 'c o r n' (without spaces).  

We can try the eTLD match before the skeleton calculation.  Hmm,   it appears
that RCDS canonicalizes 'hostname' before finding eTLD+1.   The canonicalization
would turn an IDN to punycode That would not work here. 

Because # of labels is limited to 3 here, at most two look ups are done here.
So, I'd expect little difference even if RCDS works without canonicalization.. 

OTOH, thanks to your suggestion, it occurred to me that I can change 2-step
(python + C++) into one step (C++ using RCDS that accepts URLs to extract eTLD
+1)

+    std::string partial_hostname = base::JoinString(labels, ".");
+    if (LookupStringInSet(partial_hostname, kDafsa, arraysize(kDafsa)))
+      return true;
+    labels.erase(labels.begin());
+  }
+  return false;
+}
+
+bool IDNSpoofChecker::SimilarToTopDomains(base::StringPiece16 hostname) {
+  size_t hostname_length = hostname.length() -
+      (*(hostname.rbegin()) == '.' ? 1 : 0);

[Peter Kasting, 2017/05/09 01:37:03]

Nit: Use .back() instead of *rbegin()

[jungshik at Google, 2017/05/10 18:05:13]

Done.

+  icu::UnicodeString ustr_host(FALSE, hostname.data(), hostname_length);
+  // If input has any characters outside Latin-Greek-Cyrillic and [0-9._-],
+  // there is no point in getting rid of diacritics because combining marks
+  // attached to non-LGC characters are already blocked.
+  if (lgc_letters_n_ascii_.span(ustr_host, 0, USET_SPAN_CONTAINED) ==
+      ustr_host.length() && transliterator_)

[Peter Kasting, 2017/05/09 01:37:03]

Note that if the DCHECK earlier is assumed not to fail, this null-check can
disappear.

[jungshik at Google, 2017/05/10 18:05:13]

removed it. 

+    transliterator_->transliterate(ustr_host);
+
+  UErrorCode status = U_ZERO_ERROR;
+  icu::UnicodeString ustr_skeleton;
+  uspoof_getSkeletonUnicodeString(checker_, 0, /* not used. deprecated. */

[Peter Kasting, 2017/05/09 01:37:03]

Nit: If you're going to add /* */ (which I'm not sure is necessary), do so
before the comma to make it very clear which parameter this is on.

"deprecated." is also probably unnecessary here.

[jungshik at Google, 2017/05/10 18:05:13]

ok. just removed it. 

+                                  ustr_host, ustr_skeleton, &status);
+  if (U_FAILURE(status))
+    return false;
+  std::string skeleton;
+  ustr_skeleton.toUTF8String(skeleton);
+  return LookupMatchInTopDomains(skeleton);
+}
+
 bool IDNSpoofChecker::IsMadeOfLatinAlikeCyrillic(
     const icu::UnicodeString& label_string) {
   // Collect all the Cyrillic letters in |label_string| and see if they're