Allow/validate CNAME chains for non-address DNS queries

Go beyond the capabilities of the current address result handling by
allowing results in any order, as long as it creates a complete chain
from query name to the result name(s).  The ordering requirement used in
the current-but-quickly-becoming-legacy address result handling is
generally followed by the DNS ecosystem, but as far as I can tell, not
officially required by any spec.  Plus, we will soon start allowing
similar HTTPS aliasing, which will not require any specific response
ordering.

Bug: 1147247,640596
Change-Id: Id10767d34a2c638b142123145cd41dfe0dee0fd0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2532514Reviewed-by: Matt Menke <mmenke@chromium.org>
Commit-Queue: Eric Orth <ericorth@chromium.org>
Cr-Commit-Position: refs/heads/master@{#828595}

net/dns/dns_response_result_extractor.cc

@@ -7,7 +7,9 @@
 #include <limits.h>
 #include <stdint.h>
 
+#include <map>
 #include <memory>
+#include <string>
 #include <unordered_set>
 #include <vector>
 
@@ -17,6 +19,8 @@
 #include "base/numerics/checked_math.h"
 #include "base/optional.h"
 #include "base/rand_util.h"
+#include "base/strings/string_piece.h"
+#include "base/strings/string_util.h"
 #include "base/time/time.h"
 #include "net/base/host_port_pair.h"
 #include "net/base/net_errors.h"
@@ -33,6 +37,7 @@ namespace net {
 
 namespace {
 
+using AliasMap = std::map<std::string, std::string, DomainNameComparator>;
 using ExtractionError = DnsResponseResultExtractor::ExtractionError;
 
 void SaveMetricsForAdditionalHttpsRecord(const RecordParsed& record,
@@ -122,6 +127,35 @@ std::vector<HostPortPair> SortServiceTargets(
   return sorted_targets;
 }
 
+ExtractionError ValidateNamesAndAliases(
+    base::StringPiece query_name,
+    const AliasMap& aliases,
+    const std::vector<std::unique_ptr<const RecordParsed>>& results) {
+  // Validate that all aliases form a single non-looping chain, starting from
+  // `query_name`.
+  size_t aliases_in_chain = 0;
+  base::StringPiece final_chain_name = query_name;
+  auto alias = aliases.find(std::string(query_name));
+  while (alias != aliases.end() && aliases_in_chain <= aliases.size()) {
+    aliases_in_chain++;
+    final_chain_name = alias->second;
+    alias = aliases.find(alias->second);
+  }
+
+  if (aliases_in_chain != aliases.size())
+    return ExtractionError::kBadAliasChain;
+
+  // All results must match final alias name.
+  for (const auto& result : results) {
+    DCHECK_NE(result->type(), dns_protocol::kTypeCNAME);
+    if (!base::EqualsCaseInsensitiveASCII(final_chain_name, result->name())) {
+      return ExtractionError::kNameMismatch;
+    }
+  }
+
+  return ExtractionError::kOk;
+}
+
 ExtractionError ExtractResponseRecords(
     const DnsResponse& response,
     uint16_t result_qtype,
@@ -130,37 +164,50 @@ ExtractionError ExtractResponseRecords(
   DCHECK(out_records);
   DCHECK(out_response_ttl);
 
-  out_records->clear();
-  out_response_ttl->reset();
+  std::vector<std::unique_ptr<const RecordParsed>> records;
+  base::Optional<base::TimeDelta> response_ttl;
 
   DnsRecordParser parser = response.Parser();
 
   // Expected to be validated by DnsTransaction.
   DCHECK_EQ(result_qtype, response.qtype());
 
+  AliasMap aliases;
   for (unsigned i = 0; i < response.answer_count(); ++i) {
     std::unique_ptr<const RecordParsed> record =
         RecordParsed::CreateFrom(&parser, base::Time::Now());
 
     if (!record)
       return ExtractionError::kMalformedRecord;
-    if (!base::EqualsCaseInsensitiveASCII(record->name(),
-                                          response.GetDottedName())) {
-      return ExtractionError::kNameMismatch;
-    }
 
-    // Ignore any records that are not class Internet and type
-    // |filter_dns_type|.
+    DCHECK_NE(result_qtype, dns_protocol::kTypeCNAME);
     if (record->klass() == dns_protocol::kClassIN &&
+        record->type() == dns_protocol::kTypeCNAME) {
+      // Per RFC2181, multiple CNAME records are not allowed for the same name.
+      if (aliases.find(record->name()) != aliases.end())
+        return ExtractionError::kMultipleCnames;
+
+      const CnameRecordRdata* cname_data = record->rdata<CnameRecordRdata>();
+      if (!cname_data)
+        return ExtractionError::kMalformedCname;
+
+      bool added = aliases.emplace(record->name(), cname_data->cname()).second;
+      DCHECK(added);
+    } else if (record->klass() == dns_protocol::kClassIN &&
                record->type() == result_qtype) {
       base::TimeDelta ttl = base::TimeDelta::FromSeconds(record->ttl());
-      *out_response_ttl =
-          std::min(out_response_ttl->value_or(base::TimeDelta::Max()), ttl);
+      response_ttl =
+          std::min(response_ttl.value_or(base::TimeDelta::Max()), ttl);
 
-      out_records->push_back(std::move(record));
+      records.push_back(std::move(record));
     }
   }
 
+  ExtractionError name_and_alias_validation_error =
+      ValidateNamesAndAliases(response.GetDottedName(), aliases, records);
+  if (name_and_alias_validation_error != ExtractionError::kOk)
+    return name_and_alias_validation_error;
+
   // For NXDOMAIN or NODATA (NOERROR with 0 answers), attempt to find a TTL
   // via an SOA record.
   if (response.rcode() == dns_protocol::kRcodeNXDOMAIN ||
@@ -172,15 +219,15 @@ ExtractionError ExtractResponseRecords(
       if (parser.ReadRecord(&record) && record.type == dns_protocol::kTypeSOA) {
         soa_found = true;
         base::TimeDelta ttl = base::TimeDelta::FromSeconds(record.ttl);
-        *out_response_ttl =
-            std::min(out_response_ttl->value_or(base::TimeDelta::Max()), ttl);
+        response_ttl =
+            std::min(response_ttl.value_or(base::TimeDelta::Max()), ttl);
       }
     }
 
     // Per RFC2308, section 5, never cache negative results unless an SOA
     // record is found.
     if (!soa_found)
-      out_response_ttl->reset();
+      response_ttl.reset();
   }
 
   for (unsigned i = 0; i < response.additional_answer_count(); ++i) {
@@ -193,6 +240,9 @@ ExtractionError ExtractResponseRecords(
     }
   }
 
+  *out_records = std::move(records);
+  *out_response_ttl = response_ttl;
+
   return ExtractionError::kOk;
 }
 

net/dns/dns_response_result_extractor.h

@@ -30,6 +30,10 @@ class NET_EXPORT_PRIVATE DnsResponseResultExtractor {
     kMalformedResult,
     // CNAME record after a result record
     kCnameAfterResult,
+    // Multiple CNAME records for the same owner name.
+    kMultipleCnames,
+    // Invalid alias chain, e.g. contains loops or disjoint aliases.
+    kBadAliasChain,
     // Not expected. Used for DCHECKs.
     kUnexpected,
   };

net/dns/dns_test_util.cc

@@ -93,6 +93,19 @@ DnsResourceRecord BuildTestDnsRecord(std::string name,
   return record;
 }
 
+DnsResourceRecord BuildTestCnameRecord(std::string name,
+                                       base::StringPiece canonical_name,
+                                       base::TimeDelta ttl) {
+  DCHECK(!name.empty());
+  DCHECK(!canonical_name.empty());
+
+  std::string rdata;
+  CHECK(DNSDomainFromDot(canonical_name, &rdata));
+
+  return BuildTestDnsRecord(std::move(name), dns_protocol::kTypeCNAME,
+                            std::move(rdata), ttl);
+}
+
 DnsResourceRecord BuildTestAddressRecord(std::string name,
                                          const IPAddress& ip,
                                          base::TimeDelta ttl) {

net/dns/dns_test_util.h

@@ -198,6 +198,11 @@ DnsResourceRecord BuildTestDnsRecord(
     std::string rdata,
     base::TimeDelta ttl = base::TimeDelta::FromDays(1));
 
+DnsResourceRecord BuildTestCnameRecord(
+    std::string name,
+    base::StringPiece canonical_name,
+    base::TimeDelta ttl = base::TimeDelta::FromDays(1));
+
 DnsResourceRecord BuildTestAddressRecord(
     std::string name,
     const IPAddress& ip,

net/dns/dns_util.h

@@ -10,6 +10,7 @@
 
 #include "base/optional.h"
 #include "base/strings/string_piece.h"
+#include "base/strings/string_util.h"
 #include "base/time/time.h"
 #include "net/base/address_family.h"
 #include "net/base/ip_endpoint.h"
@@ -160,6 +161,15 @@ NET_EXPORT_PRIVATE std::string GetDohProviderIdForHistogramFromNameserver(
 NET_EXPORT_PRIVATE std::string SecureDnsModeToString(
     const SecureDnsMode secure_dns_mode);
 
+// std::map-compliant Compare for two dotted-format domain names. Returns true
+// iff `lhs` is before `rhs` in strict weak ordering.
+class NET_EXPORT_PRIVATE DomainNameComparator {
+ public:
+  bool operator()(base::StringPiece lhs, base::StringPiece rhs) const {
+    return base::CompareCaseInsensitiveASCII(lhs, rhs) < 0;
+  }
+};
+
 }  // namespace net
 
 #endif  // NET_DNS_DNS_UTIL_H_

net/dns/record_rdata.h

@@ -132,7 +132,7 @@ class NET_EXPORT_PRIVATE CnameRecordRdata : public RecordRdata {
   bool IsEqual(const RecordRdata* other) const override;
   uint16_t Type() const override;
 
-  std::string cname() const { return cname_; }
+  const std::string& cname() const { return cname_; }
 
  private:
   CnameRecordRdata();