Bug 1502097 - (Part 1) Move pref network.IDN.blacklist_chars to separ…

…ate hardcoded file IDNCharacterBlocklist.inc r=jfkthame,dragana

* Moves the value of the pref and also the fallback definition in nsTextToSubURI.cpp to a separate file.
* The file has better formatting, so we may follow its history more easily. Each range of consecutive values is defined on a separate line.
* Renames `blacklist` to `blocklist` for pref and variable names (for this individual pref. network.IDN.whitelist.* needs to be handled in a separate bug)
* Changes nsIDNService::mIDNBlocklist from being an nsString to sorted nsTArray<char16> and uses mozilla::BinarySearch() to check for characters.

Differential Revision: https://phabricator.services.mozilla.com/D12209

--HG--
extra : moz-landing-system : lando

intl/uconv/nsITextToSubURI.idl

@@ -27,7 +27,7 @@ interface nsITextToSubURI : nsISupports
    *  <li> In case of a conversion error, the URI fragment (escaped) is
    *       assumed to be in UTF-8 and converted to AString (UTF-16)
    *  <li> In case of successful conversion any resulting character listed
-   *       in network.IDN.blacklist_chars (except space) is escaped
+   *       in netwerk/dns/IDNCharacterBlocklist.inc (except space) is escaped
    *  <li> Always succeeeds (callers don't need to do error checking)
    * </ul>
    *

intl/uconv/nsTextToSubURI.cpp

@@ -14,29 +14,8 @@
 
 using namespace mozilla;
 
-// Fallback value for the pref "network.IDN.blacklist_chars".
-// UnEscapeURIForUI allows unescaped space; other than that, this is
-// the same as the default "network.IDN.blacklist_chars" value.
-static const char16_t sNetworkIDNBlacklistChars[] =
-{
-  // clang-format off
-/*0x0020,*/
-          0x00A0, 0x00BC, 0x00BD, 0x00BE, 0x01C3, 0x02D0, 0x0337,
-  0x0338, 0x0589, 0x058A, 0x05C3, 0x05F4, 0x0609, 0x060A, 0x066A, 0x06D4,
-  0x0701, 0x0702, 0x0703, 0x0704, 0x115F, 0x1160, 0x1735, 0x2000,
-  0x2001, 0x2002, 0x2003, 0x2004, 0x2005, 0x2006, 0x2007, 0x2008,
-  0x2009, 0x200A, 0x200B, 0x200E, 0x200F, 0x2010, 0x2019, 0x2024, 0x2027, 0x2028,
-  0x2029, 0x202A, 0x202B, 0x202C, 0x202D, 0x202E, 0x202F, 0x2039,
-  0x203A, 0x2041, 0x2044, 0x2052, 0x205F, 0x2153, 0x2154, 0x2155,
-  0x2156, 0x2157, 0x2158, 0x2159, 0x215A, 0x215B, 0x215C, 0x215D,
-  0x215E, 0x215F, 0x2215, 0x2236, 0x23AE, 0x2571, 0x29F6, 0x29F8,
-  0x2AFB, 0x2AFD, 0x2FF0, 0x2FF1, 0x2FF2, 0x2FF3, 0x2FF4, 0x2FF5,
-  0x2FF6, 0x2FF7, 0x2FF8, 0x2FF9, 0x2FFA, 0x2FFB, /*0x3000,*/ 0x3002,
-  0x3014, 0x3015, 0x3033, 0x30A0, 0x3164, 0x321D, 0x321E, 0x33AE, 0x33AF,
-  0x33C6, 0x33DF, 0xA789, 0xFE14, 0xFE15, 0xFE3F, 0xFE5D, 0xFE5E,
-  0xFEFF, 0xFF0E, 0xFF0F, 0xFF61, 0xFFA0, 0xFFF9, 0xFFFA, 0xFFFB,
-  0xFFFC, 0xFFFD
-  // clang-format on
+static const char16_t sNetworkIDNBlocklistChars[] = {
+#include "../../netwerk/dns/IDNCharacterBlocklist.inc"
 };
 
 nsTextToSubURI::~nsTextToSubURI()
@@ -138,6 +117,7 @@ NS_IMETHODIMP  nsTextToSubURI::UnEscapeURIForUI(const nsACString & aCharset,
                                                 const nsACString &aURIFragment,
                                                 nsAString &_retval)
 {
+  nsresult rv;
   nsAutoCString unescapedSpec;
   // skip control octets (0x00 - 0x1f and 0x7f) when unescaping
   NS_UnescapeURL(PromiseFlatCString(aURIFragment),
@@ -155,24 +135,28 @@ NS_IMETHODIMP  nsTextToSubURI::UnEscapeURIForUI(const nsACString & aCharset,
 
   // If there are any characters that are unsafe for URIs, reescape those.
   if (mUnsafeChars.IsEmpty()) {
-    nsAutoString blacklist;
-    nsresult rv = mozilla::Preferences::GetString("network.IDN.blacklist_chars",
-                                                  blacklist);
-    if (NS_SUCCEEDED(rv)) {
-      // we allow SPACE and IDEOGRAPHIC SPACE in this method
-      blacklist.StripChars(u" \u3000");
-      mUnsafeChars.AppendElements(static_cast<const char16_t*>(blacklist.Data()),
-                                  blacklist.Length());
-    } else {
-      NS_WARNING("Failed to get the 'network.IDN.blacklist_chars' preference");
-    }
-    // We check IsEmpty() intentionally here because an empty (or just spaces)
-    // pref value is likely a mistake/error of some sort.
-    if (mUnsafeChars.IsEmpty()) {
-      mUnsafeChars.AppendElements(sNetworkIDNBlacklistChars,
-                                  mozilla::ArrayLength(sNetworkIDNBlacklistChars));
+    mUnsafeChars.AppendElements(
+      sNetworkIDNBlocklistChars, ArrayLength(sNetworkIDNBlocklistChars));
+
+    nsAutoString extraAllowed;
+    Preferences::GetString("network.IDN.extra_allowed_chars",
+                           extraAllowed);
+    // we allow SPACE and IDEOGRAPHIC SPACE in this method
+    extraAllowed.Append(u' ');
+    extraAllowed.Append(0x3000);
+    mUnsafeChars.RemoveElementsBy([&](char16_t c) {
+      return extraAllowed.FindChar(c, 0) != -1;
+    });
+
+    nsAutoString extraBlocked;
+    rv = Preferences::GetString("network.IDN.extra_blocked_chars",
+                                extraBlocked);
+    if (NS_SUCCEEDED(rv) && !extraBlocked.IsEmpty()) {
+      mUnsafeChars.AppendElements(
+        static_cast<const char16_t*>(extraBlocked.Data()),
+        extraBlocked.Length());
+      mUnsafeChars.Sort();
     }
-    mUnsafeChars.Sort();
   }
   const nsPromiseFlatString& unescapedResult = PromiseFlatString(_retval);
   nsString reescapedSpec;

intl/uconv/nsTextToSubURI.h

@@ -23,8 +23,8 @@ class nsTextToSubURI: public nsITextToSubURI
                                const nsCString& aURI,
                                nsAString &_retval);
 
-  // Characters from the pref "network.IDN.blacklist_chars", or a built-in
-  // fallback if reading the pref fails.
+  // Characters defined in netwerk/dns/IDNCharacterBlocklist.inc or via the
+  // network.IDN.extra_allowed_chars and network.IDN.extra_blocked_chars prefs.
   nsTArray<char16_t> mUnsafeChars;
 };
 

modules/libpref/init/all.js

@@ -2133,12 +2133,21 @@ pref("network.IDN.whitelist.xn--jxalpdlp", true);
 pref("network.IDN.whitelist.xn--kgbechtv", true);
 pref("network.IDN.whitelist.xn--zckzah", true);
 
-// If a domain includes any of the following characters, it may be a spoof
+// If a domain includes any of the blocklist characters, it may be a spoof
 // attempt and so we always display the domain name as punycode. This would
 // override the settings "network.IDN_show_punycode" and
-// "network.IDN.whitelist.*".  (please keep this value in sync with the
-// built-in fallback in intl/uconv/nsTextToSubURI.cpp)
-pref("network.IDN.blacklist_chars", "\u0020\u00A0\u00BC\u00BD\u00BE\u01C3\u02D0\u0337\u0338\u0589\u058A\u05C3\u05F4\u0609\u060A\u066A\u06D4\u0701\u0702\u0703\u0704\u115F\u1160\u1735\u2000\u2001\u2002\u2003\u2004\u2005\u2006\u2007\u2008\u2009\u200A\u200B\u200E\u200F\u2010\u2019\u2024\u2027\u2028\u2029\u202A\u202B\u202C\u202D\u202E\u202F\u2039\u203A\u2041\u2044\u2052\u205F\u2153\u2154\u2155\u2156\u2157\u2158\u2159\u215A\u215B\u215C\u215D\u215E\u215F\u2215\u2236\u23AE\u2571\u29F6\u29F8\u2AFB\u2AFD\u2FF0\u2FF1\u2FF2\u2FF3\u2FF4\u2FF5\u2FF6\u2FF7\u2FF8\u2FF9\u2FFA\u2FFB\u3000\u3002\u3014\u3015\u3033\u30A0\u3164\u321D\u321E\u33AE\u33AF\u33C6\u33DF\uA789\uFE14\uFE15\uFE3F\uFE5D\uFE5E\uFEFF\uFF0E\uFF0F\uFF61\uFFA0\uFFF9\uFFFA\uFFFB\uFFFC\uFFFD");
+// "network.IDN.whitelist.*".
+// For a complete list of the blocked IDN characters see:
+//   netwerk/dns/IDNCharacterBlocklist.inc
+
+// This pref may contain characters that will override the hardcoded blocklist,
+// so their presence in a domain name will not cause it to be displayed as
+// punycode.
+// Note that this only removes the characters from the blocklist, but there may
+// be other rules in place that cause it to be displayed as punycode.
+pref("network.IDN.extra_allowed_chars", "");
+// This pref may contain additional blocklist characters
+pref("network.IDN.extra_blocked_chars", "");
 
 // This preference specifies a list of domains for which DNS lookups will be
 // IPv4 only. Works around broken DNS servers which can't handle IPv6 lookups

netwerk/dns/IDNCharacterBlocklist.inc

@@ -0,0 +1,55 @@
+// ASCII Space
+0x0020,
+0x00A0,
+0x00BC, 0x00BD, 0x00BE,
+0x01C3,
+0x02D0,
+0x0337, 0x0338,
+0x0589, 0x058A,
+0x05C3,
+0x05F4,
+0x0609, 0x060A,
+0x066A,
+0x06D4,
+0x0701, 0x0702, 0x0703, 0x0704,
+0x115F, 0x1160,
+0x1735,
+0x2000, 0x2001, 0x2002, 0x2003, 0x2004, 0x2005, 0x2006, 0x2007, 0x2008, 0x2009, 0x200A, 0x200B,
+0x200E, 0x200F, 0x2010,
+0x2019,
+0x2024,
+0x2027, 0x2028, 0x2029, 0x202A, 0x202B, 0x202C, 0x202D, 0x202E, 0x202F,
+0x2039, 0x203A,
+0x2041,
+0x2044,
+0x2052,
+0x205F,
+0x2153, 0x2154, 0x2155, 0x2156, 0x2157, 0x2158, 0x2159, 0x215A, 0x215B, 0x215C, 0x215D, 0x215E, 0x215F,
+0x2215,
+0x2236,
+0x23AE,
+0x2571,
+0x29F6,
+0x29F8,
+0x2AFB,
+0x2AFD,
+0x2FF0, 0x2FF1, 0x2FF2, 0x2FF3, 0x2FF4, 0x2FF5, 0x2FF6, 0x2FF7, 0x2FF8, 0x2FF9, 0x2FFA, 0x2FFB,
+// Ideographic Space
+0x3000,
+0x3002,
+0x3014, 0x3015,
+0x3033,
+0x30A0,
+0x3164,
+0x321D, 0x321E,
+0x33AE, 0x33AF,
+0x33C6,
+0x33DF,
+0xFE14, 0xFE15,
+0xFE3F,
+0xFE5D, 0xFE5E,
+0xFEFF,
+0xFF0E, 0xFF0F,
+0xFF61,
+0xFFA0,
+0xFFF9, 0xFFFA, 0xFFFB, 0xFFFC, 0xFFFD

netwerk/dns/nsIDNService.cpp

@@ -26,6 +26,10 @@ const bool kIDNA2008_TransitionalProcessing = false;
 #include "ICUUtils.h"
 #include "unicode/uscript.h"
 
+static const char16_t sBlocklistChars[] = {
+#include "IDNCharacterBlocklist.inc"
+};
+
 using namespace mozilla::unicode;
 using mozilla::Preferences;
 
@@ -38,16 +42,30 @@ static const char kACEPrefix[] = "xn--";
 
 //-----------------------------------------------------------------------------
 
-#define NS_NET_PREF_IDNBLACKLIST    "network.IDN.blacklist_chars"
+#define NS_NET_PREF_EXTRAALLOWED "network.IDN.extra_allowed_chars"
+#define NS_NET_PREF_EXTRABLOCKED "network.IDN.extra_blocked_chars"
 #define NS_NET_PREF_SHOWPUNYCODE    "network.IDN_show_punycode"
 #define NS_NET_PREF_IDNWHITELIST    "network.IDN.whitelist."
 #define NS_NET_PREF_IDNUSEWHITELIST "network.IDN.use_whitelist"
 #define NS_NET_PREF_IDNRESTRICTION  "network.IDN.restriction_profile"
 
-inline bool isOnlySafeChars(const nsString& in, const nsString& blacklist)
+static inline bool
+isOnlySafeChars(const nsString& in, const nsTArray<char16_t>& aBlockList)
 {
-  return (blacklist.IsEmpty() ||
-          in.FindCharInSet(blacklist) == kNotFound);
+  if (aBlockList.IsEmpty()) {
+    return true;
+  }
+  const char16_t* cur = in.BeginReading();
+  const char16_t* end = in.EndReading();
+
+  for (; cur < end; ++cur) {
+    size_t unused;
+    if (mozilla::BinarySearch(aBlockList, 0, aBlockList.Length(), *cur,
+                              &unused)) {
+      return false;
+    }
+  }
+  return true;
 }
 
 //-----------------------------------------------------------------------------
@@ -60,7 +78,8 @@ NS_IMPL_ISUPPORTS(nsIDNService,
                   nsISupportsWeakReference)
 
 static const char* gCallbackPrefs[] = {
-  NS_NET_PREF_IDNBLACKLIST,
+  NS_NET_PREF_EXTRAALLOWED,
+  NS_NET_PREF_EXTRABLOCKED,
   NS_NET_PREF_SHOWPUNYCODE,
   NS_NET_PREF_IDNRESTRICTION,
   NS_NET_PREF_IDNUSEWHITELIST,
@@ -78,24 +97,45 @@ nsresult nsIDNService::Init()
 
   Preferences::RegisterPrefixCallbacks(PrefChanged, gCallbackPrefs, this);
   prefsChanged(nullptr);
+  InitializeBlocklist();
 
   return NS_OK;
 }
 
+void
+nsIDNService::InitializeBlocklist()
+{
+  mIDNBlocklist.Clear();
+  mIDNBlocklist.AppendElements(sBlocklistChars,
+                               mozilla::ArrayLength(sBlocklistChars));
+  nsAutoString extraAllowed;
+  nsresult rv = Preferences::GetString(NS_NET_PREF_EXTRAALLOWED, extraAllowed);
+
+  if (NS_SUCCEEDED(rv) && !extraAllowed.IsEmpty()) {
+    mIDNBlocklist.RemoveElementsBy([&](char16_t c) {
+      return extraAllowed.FindChar(c, 0) != -1;
+    });
+  }
+
+  nsAutoString extraBlocked;
+  rv = Preferences::GetString(NS_NET_PREF_EXTRABLOCKED, extraBlocked);
+  if (NS_SUCCEEDED(rv) && !extraBlocked.IsEmpty()) {
+    mIDNBlocklist.AppendElements(
+      static_cast<const char16_t*>(extraBlocked.Data()), extraBlocked.Length());
+    mIDNBlocklist.Sort();
+  }
+}
+
 void nsIDNService::prefsChanged(const char *pref)
 {
   MOZ_ASSERT(NS_IsMainThread());
   mLock.AssertCurrentThreadOwns();
 
-  if (!pref || NS_LITERAL_CSTRING(NS_NET_PREF_IDNBLACKLIST).Equals(pref)) {
-    nsAutoCString blacklist;
-    nsresult rv = Preferences::GetCString(NS_NET_PREF_IDNBLACKLIST,
-                                          blacklist);
-    if (NS_SUCCEEDED(rv)) {
-      CopyUTF8toUTF16(blacklist, mIDNBlacklist);
-    } else {
-      mIDNBlacklist.Truncate();
-    }
+  if (pref && NS_LITERAL_CSTRING(NS_NET_PREF_EXTRAALLOWED).Equals(pref)) {
+    InitializeBlocklist();
+  }
+  if (pref && NS_LITERAL_CSTRING(NS_NET_PREF_EXTRABLOCKED).Equals(pref)) {
+    InitializeBlocklist();
   }
   if (!pref || NS_LITERAL_CSTRING(NS_NET_PREF_SHOWPUNYCODE).Equals(pref)) {
     bool val;
@@ -723,7 +763,7 @@ bool nsIDNService::isLabelSafe(const nsAString &label)
     mLock.AssertCurrentThreadOwns();
   }
 
-  if (!isOnlySafeChars(PromiseFlatString(label), mIDNBlacklist)) {
+  if (!isOnlySafeChars(PromiseFlatString(label), mIDNBlocklist)) {
     return false;
   }
 

netwerk/dns/nsIDNService.h

@@ -95,6 +95,8 @@ class nsIDNService final : public nsIIDNService,
   nsresult ACEtoUTF8(const nsACString& input, nsACString& _retval,
                      stringPrepFlag flag);
 
+  void InitializeBlocklist();
+
   bool isInWhitelist(const nsACString &host);
   void prefsChanged(const char *pref);
 
@@ -164,7 +166,7 @@ class nsIDNService final : public nsIIDNService,
   UIDNA* mIDNA;
 
   // We use this mutex to guard access to:
-  // |mIDNBlacklist|, |mShowPunycode|, |mRestrictionProfile|,
+  // |mIDNBlocklist|, |mShowPunycode|, |mRestrictionProfile|,
   // |mIDNUseWhitelist|.
   //
   // These members can only be updated on the main thread and
@@ -173,7 +175,7 @@ class nsIDNService final : public nsIIDNService,
   mozilla::Mutex mLock;
 
   // guarded by mLock
-  nsString mIDNBlacklist;
+  nsTArray<char16_t> mIDNBlocklist;
 
   /**
    * Flag set by the pref network.IDN_show_punycode. When it is true,

netwerk/test/unit/test_bug464591.js

@@ -27,7 +27,7 @@ let prefData =
   ];
 
  let prefIdnBlackList = {
-      name: "network.IDN.blacklist_chars",
+      name: "network.IDN.extra_blocked_chars",
       minimumList: "\u2215\u0430\u2044",
   };
 

netwerk/test/unit/test_idnservice.js

@@ -1,15 +1,19 @@
 // Tests nsIIDNService
 
-var reference = [
+"use strict";
+ChromeUtils.import("resource://gre/modules/Services.jsm");
+
+const idnService = Cc["@mozilla.org/network/idn-service;1"]
+                     .getService(Ci.nsIIDNService);
+
+add_task(async function test_simple() {
+  let reference = [
                  // The 3rd element indicates whether the second element
                  // is ACE-encoded
                  ["asciihost", "asciihost", false],
                  ["b\u00FCcher", "xn--bcher-kva", true]
                 ];
 
-function run_test() {
-  var idnService = Cc["@mozilla.org/network/idn-service;1"]
-                     .getService(Ci.nsIIDNService);
 
   for (var i = 0; i < reference.length; ++i) {
      dump("Testing " + reference[i] + "\n");
@@ -22,4 +26,12 @@ function run_test() {
      Assert.equal(idnService.convertACEtoUTF8(reference[i][1]), reference[i][0]);
      Assert.equal(idnService.isACE(reference[i][1]), reference[i][2]);
   }
-}
+});
+
+add_task(async function test_extra_blocked() {
+    let isAscii = {};
+    equal(idnService.convertToDisplayIDN("xn--gou-2lb.ro", isAscii), "goșu.ro");
+    Services.prefs.setStringPref("network.IDN.extra_blocked_chars", "ș");
+    equal(idnService.convertToDisplayIDN("xn--gou-2lb.ro", isAscii), "xn--gou-2lb.ro");
+    Services.prefs.clearUserPref("network.IDN.extra_blocked_chars");
+});