Enforce the hostname validity check.

Also update test expectations to match the new preferred name syntax policy
enforcement, and add new tests to ensure that the policy is enforced at various 
layers of abstraction/in various net components.

We've been using |DNSDomainFromDotWithValidityCheck|, which returns whether or
not the given name matches the "preferred name syntax"
(https://tools.ietf.org/html/rfc7719#section-2), but using the result only in an
advisory capacity to measure the histogram
Net.SuccessfulResolutionWithValidDNSName. This change still measures the
histogram when the name is not in preferred name syntax, but also returns
|ERR_NAME_NOT_RESOLVED|, effectively enforcing the sanity check.

BUG=496468,695474

Change-Id: Iababe35f0bef37ba07eed8a6edcea43bdec10335
Reviewed-on: https://chromium-review.googlesource.com/569298
Reviewed-by: Mark Pearson <mpearson@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Commit-Queue: Chris Palmer <palmer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#487716}

net/base/host_port_pair.cc

@@ -33,6 +33,7 @@ HostPortPair HostPortPair::FromIPEndPoint(const IPEndPoint& ipe) {
   return HostPortPair(ipe.ToStringWithoutPort(), ipe.port());
 }
 
+// static
 HostPortPair HostPortPair::FromString(const std::string& str) {
   std::vector<base::StringPiece> key_port = base::SplitStringPiece(
       str, ":", base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);

net/dns/dns_hosts_unittest.cc

@@ -87,7 +87,7 @@ TEST(DnsHostsTest, ParseHosts_CommaIsToken) {
   PopulateExpectedHosts(kEntries, arraysize(kEntries), &expected_hosts);
   ParseHostsWithCommaModeForTesting(
       kContents, &actual_hosts, PARSE_HOSTS_COMMA_IS_TOKEN);
-  ASSERT_EQ(expected_hosts, actual_hosts);
+  ASSERT_EQ(0UL, actual_hosts.size());
 }
 
 TEST(DnsHostsTest, ParseHosts_CommaIsWhitespace) {
@@ -108,22 +108,20 @@ TEST(DnsHostsTest, ParseHosts_CommaIsWhitespace) {
 // Test that the right comma mode is used on each platform.
 TEST(DnsHostsTest, ParseHosts_CommaModeByPlatform) {
   std::string kContents = "127.0.0.1 comma1,comma2";
+  DnsHosts actual_hosts;
+  ParseHosts(kContents, &actual_hosts);
 
 #if defined(OS_MACOSX)
   const ExpectedHostsEntry kEntries[] = {
     { "comma1", ADDRESS_FAMILY_IPV4, "127.0.0.1" },
     { "comma2", ADDRESS_FAMILY_IPV4, "127.0.0.1" },
   };
-#else
-  const ExpectedHostsEntry kEntries[] = {
-    { "comma1,comma2", ADDRESS_FAMILY_IPV4, "127.0.0.1" },
-  };
-#endif
-
-  DnsHosts expected_hosts, actual_hosts;
+  DnsHosts expected_hosts;
   PopulateExpectedHosts(kEntries, arraysize(kEntries), &expected_hosts);
-  ParseHosts(kContents, &actual_hosts);
   ASSERT_EQ(expected_hosts, actual_hosts);
+#else
+  ASSERT_EQ(0UL, actual_hosts.size());
+#endif
 }
 
 TEST(DnsHostsTest, HostsParser_Empty) {

net/dns/dns_transaction_unittest.cc

@@ -1001,6 +1001,10 @@ TEST_F(DnsTransactionTest, InvalidQuery) {
 
   TransactionHelper helper0(".", dns_protocol::kTypeA, ERR_INVALID_ARGUMENT);
   EXPECT_TRUE(helper0.Run(transaction_factory_.get()));
+
+  TransactionHelper helper1("foo,bar.com", dns_protocol::kTypeA,
+                            ERR_INVALID_ARGUMENT);
+  EXPECT_TRUE(helper1.Run(transaction_factory_.get()));
 }
 
 }  // namespace

net/dns/dns_util.cc

@@ -45,17 +45,14 @@ const int kMaxLabelLength = 63;
 namespace net {
 
 // Based on DJB's public domain code.
-bool DNSDomainFromDotWithValidityCheck(const base::StringPiece& dotted,
-                                       std::string* out,
-                                       bool* valid_name) {
+bool DNSDomainFromDot(const base::StringPiece& dotted, std::string* out) {
   const char* buf = dotted.data();
   size_t n = dotted.size();
   char label[kMaxLabelLength];
   size_t labellen = 0; /* <= sizeof label */
   char name[dns_protocol::kMaxNameLength];
   size_t namelen = 0; /* <= sizeof name */
   char ch;
-  *valid_name = true;
 
   for (;;) {
     if (!n)
@@ -77,24 +74,11 @@ bool DNSDomainFromDotWithValidityCheck(const base::StringPiece& dotted,
     if (labellen >= sizeof label)
       return false;
     if (!IsValidHostLabelCharacter(ch, labellen == 0)) {
-      // TODO(crbug.com/695474): In the future, when we can remove support for
-      // invalid names, return false here instead (and remove the UMA counter).
-      // And remove the |valid_name| parameter, rename this function back to
-      // |DNSDomainFromDot|, and remove the helper function by that name
-      // (below).
-      *valid_name = false;
+      return false;
     }
     label[labellen++] = ch;
   }
 
-  UMA_HISTOGRAM_BOOLEAN("Net.ValidDNSName", *valid_name);
-  if (*valid_name) {
-    url::CanonHostInfo info;
-    UMA_HISTOGRAM_BOOLEAN("Net.DNSNameCompliantIfValid",
-                          net::IsCanonicalizedHostCompliant(
-                              net::CanonicalizeHost(dotted, &info)));
-  }
-
   // Allow empty label at end of name to disable suffix search.
   if (labellen) {
     if (namelen + labellen + 1 > sizeof name)
@@ -115,11 +99,6 @@ bool DNSDomainFromDotWithValidityCheck(const base::StringPiece& dotted,
   return true;
 }
 
-bool DNSDomainFromDot(const base::StringPiece& dotted, std::string* out) {
-  bool ignored;
-  return DNSDomainFromDotWithValidityCheck(dotted, out, &ignored);
-}
-
 bool IsValidDNSDomain(const base::StringPiece& dotted) {
   std::string dns_formatted;
   return DNSDomainFromDot(dotted, &dns_formatted);

net/dns/dns_util.h

@@ -24,14 +24,6 @@ class AddressList;
 NET_EXPORT_PRIVATE bool DNSDomainFromDot(const base::StringPiece& dotted,
                                          std::string* out);
 
-//   dotted: a string in dotted form: "www.google.com"
-//   out: a result in DNS form: "\x03www\x06google\x03com\x00"
-//   valid_name: whether or not |IsValidHostLabelCharacter| returned true for
-//       all characters in all labels in the name.
-bool DNSDomainFromDotWithValidityCheck(const base::StringPiece& dotted,
-                                       std::string* out,
-                                       bool* valid_name);
-
 // Checks that a hostname is valid. Simple wrapper around DNSDomainFromDot.
 NET_EXPORT_PRIVATE bool IsValidDNSDomain(const base::StringPiece& dotted);
 

net/dns/dns_util_unittest.cc

@@ -45,6 +45,7 @@ TEST_F(DNSUtilTest, DNSDomainFromDot) {
   EXPECT_FALSE(DNSDomainFromDot("", &out));
   EXPECT_FALSE(DNSDomainFromDot(".", &out));
   EXPECT_FALSE(DNSDomainFromDot("..", &out));
+  EXPECT_FALSE(DNSDomainFromDot("foo,bar.com", &out));
 
   EXPECT_TRUE(DNSDomainFromDot("com", &out));
   EXPECT_EQ(out, IncludeNUL("\003com"));

net/dns/host_resolver_impl.cc

@@ -1962,16 +1962,17 @@ int HostResolverImpl::Resolve(const RequestInfo& info,
   DCHECK_EQ(false, callback.is_null());
   DCHECK(out_req);
 
-  // Check that the caller supplied a valid hostname to resolve.
-  if (!IsValidDNSDomain(info.hostname()))
-    return ERR_NAME_NOT_RESOLVED;
-
-  LogStartRequest(source_net_log, info);
-
   IPAddress ip_address;
   IPAddress* ip_address_ptr = nullptr;
-  if (ip_address.AssignFromIPLiteral(info.hostname()))
+  if (ip_address.AssignFromIPLiteral(info.hostname())) {
     ip_address_ptr = &ip_address;
+  } else {
+    // Check that the caller supplied a valid hostname to resolve.
+    if (!IsValidDNSDomain(info.hostname()))
+      return ERR_NAME_NOT_RESOLVED;
+  }
+
+  LogStartRequest(source_net_log, info);
 
   // Build a key that identifies the request in the cache and in the
   // outstanding jobs map.

net/dns/host_resolver_impl_unittest.cc

@@ -1394,7 +1394,7 @@ TEST_F(HostResolverImplTest, ResolveFromCache) {
 
   HostResolver::RequestInfo info(HostPortPair("just.testing", 80));
 
-  // First hit will miss the cache.
+  // First query will miss the cache.
   EXPECT_EQ(ERR_DNS_CACHE_MISS,
             CreateRequest(info, DEFAULT_PRIORITY)->ResolveFromCache());
 
@@ -1409,13 +1409,35 @@ TEST_F(HostResolverImplTest, ResolveFromCache) {
   EXPECT_TRUE(requests_[2]->HasOneAddress("192.168.1.42", 80));
 }
 
+TEST_F(HostResolverImplTest, ResolveFromCacheInvalidName) {
+  proc_->AddRuleForAllFamilies("foo,bar.com", "192.168.1.42");
+  proc_->SignalMultiple(1u);  // Need only one.
+
+  HostResolver::RequestInfo info(HostPortPair("foo,bar.com", 80));
+
+  // First query will miss the cache.
+  EXPECT_EQ(ERR_DNS_CACHE_MISS,
+            CreateRequest(info, DEFAULT_PRIORITY)->ResolveFromCache());
+
+  // This time, we fetch normally.
+  EXPECT_THAT(CreateRequest(info, DEFAULT_PRIORITY)->Resolve(),
+              IsError(ERR_NAME_NOT_RESOLVED));
+  EXPECT_THAT(requests_[1]->WaitForResult(), IsError(ERR_NAME_NOT_RESOLVED));
+
+  // Expect a cache miss, since the query could not have succeeded the first
+  // time.
+  EXPECT_THAT(CreateRequest(info, DEFAULT_PRIORITY)->ResolveFromCache(),
+              IsError(ERR_DNS_CACHE_MISS));
+  EXPECT_FALSE(requests_[2]->HasOneAddress("192.168.1.42", 80));
+}
+
 TEST_F(HostResolverImplTest, ResolveStaleFromCache) {
   proc_->AddRuleForAllFamilies("just.testing", "192.168.1.42");
   proc_->SignalMultiple(1u);  // Need only one.
 
   HostResolver::RequestInfo info(HostPortPair("just.testing", 80));
 
-  // First hit will miss the cache.
+  // First query will miss the cache.
   EXPECT_EQ(ERR_DNS_CACHE_MISS,
             CreateRequest(info, DEFAULT_PRIORITY)->ResolveFromCache());
 

net/dns/host_resolver_proc.cc

@@ -133,7 +133,7 @@ int SystemHostResolverCall(const std::string& host,
   bool valid_hostname = false;
   {
     std::string out_ignored;
-    if (!DNSDomainFromDotWithValidityCheck(host, &out_ignored, &valid_hostname))
+    if (!DNSDomainFromDot(host, &out_ignored))
       return ERR_NAME_NOT_RESOLVED;
   }
 

net/http/transport_security_state_unittest.cc

@@ -694,18 +694,6 @@ TEST_F(TransportSecurityStateTest, Expiration) {
   EXPECT_TRUE(state.HasPublicKeyPins("example2.test"));
 }
 
-TEST_F(TransportSecurityStateTest, InvalidDomains) {
-  TransportSecurityState state;
-  const base::Time current_time(base::Time::Now());
-  const base::Time expiry = current_time + base::TimeDelta::FromSeconds(1000);
-
-  EXPECT_FALSE(state.ShouldUpgradeToSSL("example.test"));
-  bool include_subdomains = true;
-  state.AddHSTS("example.test", expiry, include_subdomains);
-  EXPECT_TRUE(state.ShouldUpgradeToSSL("www-.foo.example.test"));
-  EXPECT_TRUE(state.ShouldUpgradeToSSL("2\x01.foo.example.test"));
-}
-
 // Tests that HPKP and HSTS state are queried independently for subdomain
 // matches.
 TEST_F(TransportSecurityStateTest, IndependentSubdomain) {

net/spdy/chromium/spdy_session_pool_unittest.cc

@@ -800,7 +800,7 @@ TEST_F(SpdySessionPoolTest, IPAddressChanged) {
 }
 
 TEST_F(SpdySessionPoolTest, FindAvailableSession) {
-  SpdySessionKey key(HostPortPair("https://www.example.org", 443),
+  SpdySessionKey key(HostPortPair("www.example.org", 443),
                      ProxyServer::Direct(), PRIVACY_MODE_DISABLED);
 
   MockRead reads[] = {MockRead(SYNCHRONOUS, ERR_IO_PENDING)};
@@ -851,7 +851,7 @@ INSTANTIATE_TEST_CASE_P(
                       base::trace_event::MemoryDumpLevelOfDetail::BACKGROUND));
 
 TEST_P(SpdySessionMemoryDumpTest, DumpMemoryStats) {
-  SpdySessionKey key(HostPortPair("https://www.example.org", 443),
+  SpdySessionKey key(HostPortPair("www.example.org", 443),
                      ProxyServer::Direct(), PRIVACY_MODE_DISABLED);
 
   MockRead reads[] = {MockRead(SYNCHRONOUS, ERR_IO_PENDING)};

tools/metrics/histograms/histograms.xml

@@ -37367,6 +37367,10 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
 </histogram>
 
 <histogram name="Net.DNSNameCompliantIfValid" enum="Boolean">
+  <obsolete>
+    Deprecated 07/2017, not necessary to determine deprecation for invalid DNS
+    names.
+  </obsolete>
   <owner>palmer@chromium.org</owner>
   <summary>
     True if |net::IsCanonicalizedHostCompliant| returns true. Used to see if
@@ -42617,6 +42621,10 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
 </histogram>
 
 <histogram name="Net.ValidDNSName" enum="Boolean">
+  <obsolete>
+    Deprecated 07/2017, not necessary to determine deprecation for invalid DNS
+    names.
+  </obsolete>
   <owner>palmer@chromium.org</owner>
   <summary>
     True if a DNS name contains only characters for which