Move DNS address extraction to shared Extractor

Should be capable of handling strictly more cases than the previous
logic in DnsResponse, because CNAME validation has less assumptions on
the order of received records.

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

net/base/address_list.cc

@@ -22,6 +22,10 @@ AddressList::AddressList(const AddressList&) = default;
 
 AddressList& AddressList::operator=(const AddressList&) = default;
 
+AddressList::AddressList(AddressList&&) = default;
+
+AddressList& AddressList::operator=(AddressList&&) = default;
+
 AddressList::~AddressList() = default;
 
 AddressList::AddressList(const IPEndPoint& endpoint) {

net/base/address_list.h

@@ -29,6 +29,8 @@ class NET_EXPORT AddressList {
   AddressList();
   AddressList(const AddressList&);
   AddressList& operator=(const AddressList&);
+  AddressList(AddressList&&);
+  AddressList& operator=(AddressList&&);
   ~AddressList();
 
   // Creates an address list for a single IP literal.

net/dns/dns_response.cc

@@ -12,11 +12,10 @@
 #include "base/big_endian.h"
 #include "base/logging.h"
 #include "base/numerics/safe_conversions.h"
+#include "base/optional.h"
 #include "base/strings/string_util.h"
 #include "base/sys_byteorder.h"
-#include "net/base/address_list.h"
 #include "net/base/io_buffer.h"
-#include "net/base/ip_address.h"
 #include "net/base/net_errors.h"
 #include "net/dns/dns_query.h"
 #include "net/dns/dns_response_result_extractor.h"
@@ -262,7 +261,7 @@ DnsResponse::DnsResponse(
     const std::vector<DnsResourceRecord>& additional_records,
     const base::Optional<DnsQuery>& query,
     uint8_t rcode,
-    bool validate_answers_match_query) {
+    bool validate_records) {
   bool has_query = query.has_value();
   dns_protocol::Header header;
   header.id = id;
@@ -311,18 +310,17 @@ DnsResponse::DnsResponse(
   }
   // Start the Answer section.
   for (const auto& answer : answers) {
-    success &=
-        WriteAnswer(&writer, answer, query, validate_answers_match_query);
+    success &= WriteAnswer(&writer, answer, query, validate_records);
     DCHECK(success);
   }
   // Start the Authority section.
   for (const auto& record : authority_records) {
-    success &= WriteRecord(&writer, record);
+    success &= WriteRecord(&writer, record, validate_records);
     DCHECK(success);
   }
   // Start the Additional section.
   for (const auto& record : additional_records) {
-    success &= WriteRecord(&writer, record);
+    success &= WriteRecord(&writer, record, validate_records);
     DCHECK(success);
   }
   if (!success) {
@@ -492,92 +490,6 @@ const dns_protocol::Header* DnsResponse::header() const {
   return reinterpret_cast<const dns_protocol::Header*>(io_buffer_->data());
 }
 
-DnsResponse::Result DnsResponse::ParseToAddressList(
-    AddressList* out_addr_list,
-    base::Optional<base::TimeDelta>* out_ttl) const {
-  DCHECK(IsValid());
-  // DnsTransaction already verified that |response| matches the issued query.
-  // We still need to determine if there is a valid chain of CNAMEs from the
-  // query name to the RR owner name.
-  // We err on the side of caution with the assumption that if we are too picky,
-  // we can always fall back to the system getaddrinfo.
-
-  // Expected owner of record. No trailing dot.
-  std::string expected_name = GetDottedName();
-
-  uint16_t expected_type = qtype();
-  DCHECK(expected_type == dns_protocol::kTypeA ||
-         expected_type == dns_protocol::kTypeAAAA);
-
-  size_t expected_size = (expected_type == dns_protocol::kTypeAAAA)
-                             ? IPAddress::kIPv6AddressSize
-                             : IPAddress::kIPv4AddressSize;
-
-  base::Optional<base::TimeDelta> ttl;
-  IPAddressList ip_addresses;
-  DnsRecordParser parser = Parser();
-  DnsResourceRecord record;
-  unsigned ancount = answer_count();
-
-  for (unsigned i = 0; i < ancount; ++i) {
-    if (!parser.ReadRecord(&record))
-      return Result::kMalformedRecord;
-
-    base::TimeDelta record_ttl = base::TimeDelta::FromSeconds(record.ttl);
-    if (record.type == dns_protocol::kTypeCNAME) {
-      // Following the CNAME chain, only if no addresses seen.
-      if (!ip_addresses.empty())
-        return Result::kCnameAfterResult;
-
-      if (!base::EqualsCaseInsensitiveASCII(record.name, expected_name))
-        return Result::kNameMismatch;
-
-      if (record.rdata.size() !=
-          parser.ReadName(record.rdata.begin(), &expected_name))
-        return Result::kMalformedCname;
-
-      ttl = std::min(ttl.value_or(base::TimeDelta::Max()), record_ttl);
-    } else if (record.type == expected_type) {
-      if (record.rdata.size() != expected_size)
-        return Result::kMalformedResult;
-
-      if (!base::EqualsCaseInsensitiveASCII(record.name, expected_name))
-        return Result::kNameMismatch;
-
-      ttl = std::min(ttl.value_or(base::TimeDelta::Max()), record_ttl);
-      ip_addresses.push_back(
-          IPAddress(reinterpret_cast<const uint8_t*>(record.rdata.data()),
-                    record.rdata.length()));
-    }
-  }
-
-  // NXDOMAIN or NODATA cases respectively.
-  if (rcode() == dns_protocol::kRcodeNXDOMAIN ||
-      (ancount == 0 && rcode() == dns_protocol::kRcodeNOERROR)) {
-    bool soa_found = false;
-    unsigned nscount = base::NetToHost16(header()->nscount);
-    for (unsigned i = 0; i < nscount; ++i) {
-      if (parser.ReadRecord(&record) && record.type == dns_protocol::kTypeSOA) {
-        soa_found = true;
-        base::TimeDelta record_ttl = base::TimeDelta::FromSeconds(record.ttl);
-        ttl = std::min(ttl.value_or(base::TimeDelta::Max()), record_ttl);
-      }
-    }
-
-    // Per RFC2308, section 5, never cache negative results unless an SOA
-    // record is found.
-    if (!soa_found)
-      ttl.reset();
-  }
-
-  // getcanonname in eglibc returns the first owner name of an A or AAAA RR.
-  // If the response passed all the checks so far, then |expected_name| is it.
-  *out_addr_list =
-      AddressList::CreateFromIPAddressList(ip_addresses, expected_name);
-  *out_ttl = ttl;
-  return Result::kOk;
-}
-
 bool DnsResponse::WriteHeader(base::BigEndianWriter* writer,
                               const dns_protocol::Header& header) {
   return writer->WriteU16(header.id) && writer->WriteU16(header.flags) &&
@@ -592,13 +504,15 @@ bool DnsResponse::WriteQuestion(base::BigEndianWriter* writer,
 }
 
 bool DnsResponse::WriteRecord(base::BigEndianWriter* writer,
-                              const DnsResourceRecord& record) {
+                              const DnsResourceRecord& record,
+                              bool validate_record) {
   if (record.rdata != base::StringPiece(record.owned_rdata)) {
     VLOG(1) << "record.rdata should point to record.owned_rdata.";
     return false;
   }
 
-  if (!RecordRdata::HasValidSize(record.owned_rdata, record.type)) {
+  if (validate_record &&
+      !RecordRdata::HasValidSize(record.owned_rdata, record.type)) {
     VLOG(1) << "Invalid RDATA size for a record.";
     return false;
   }
@@ -619,16 +533,16 @@ bool DnsResponse::WriteRecord(base::BigEndianWriter* writer,
 bool DnsResponse::WriteAnswer(base::BigEndianWriter* writer,
                               const DnsResourceRecord& answer,
                               const base::Optional<DnsQuery>& query,
-                              bool validate_answer_matches_query) {
+                              bool validate_record) {
   // Generally assumed to be a mistake if we write answers that don't match the
   // query type, except CNAME answers which can always be added.
-  if (validate_answer_matches_query && query.has_value() &&
+  if (validate_record && query.has_value() &&
       answer.type != query.value().qtype() &&
       answer.type != dns_protocol::kTypeCNAME) {
     VLOG(1) << "Mismatched answer resource record type and qtype.";
     return false;
   }
-  return WriteRecord(writer, answer);
+  return WriteRecord(writer, answer, validate_record);
 }
 
 }  // namespace net

net/dns/dns_response.h

@@ -15,7 +15,6 @@
 #include "base/memory/ref_counted.h"
 #include "base/optional.h"
 #include "base/strings/string_piece.h"
-#include "base/time/time.h"
 #include "net/base/net_export.h"
 #include "net/dns/dns_response_result_extractor.h"
 #include "net/dns/public/dns_protocol.h"
@@ -26,7 +25,6 @@ class BigEndianWriter;
 
 namespace net {
 
-class AddressList;
 class DnsQuery;
 class IOBuffer;
 
@@ -112,15 +110,16 @@ class NET_EXPORT_PRIVATE DnsRecordParser {
 // position the RR parser.
 class NET_EXPORT_PRIVATE DnsResponse {
  public:
-  // Possible results from ParseToAddressList.
-  using Result = DnsResponseResultExtractor::ExtractionError;
-
   // Constructs a response buffer large enough to store one byte more than
   // largest possible response, to detect malformed responses.
   DnsResponse();
 
-  // Constructs a response message from |answers| and the originating |query|.
+  // Constructs a response message from `answers` and the originating `query`.
   // After the successful construction, and the parser is also initialized.
+  //
+  // If `validate_records` is false, DCHECKs validating the correctness of
+  // records will be skipped. Intended for tests to allow creation of malformed
+  // responses.
   DnsResponse(uint16_t id,
               bool is_authoritative,
               const std::vector<DnsResourceRecord>& answers,
@@ -128,7 +127,7 @@ class NET_EXPORT_PRIVATE DnsResponse {
               const std::vector<DnsResourceRecord>& additional_records,
               const base::Optional<DnsQuery>& query,
               uint8_t rcode = dns_protocol::kRcodeNOERROR,
-              bool validate_answers_match_query = true);
+              bool validate_records = true);
 
   // Constructs a response buffer of given length. Used for TCP transactions.
   explicit DnsResponse(size_t length);
@@ -195,21 +194,17 @@ class NET_EXPORT_PRIVATE DnsResponse {
   // This operation is idempotent.
   DnsRecordParser Parser() const;
 
-  // Extracts an AddressList from this response.
-  // TODO(crbug.com/1147247): Remove in favor of DnsResponseResultExtractor.
-  Result ParseToAddressList(AddressList* out_addr_list,
-                            base::Optional<base::TimeDelta>* out_ttl) const;
-
  private:
   bool WriteHeader(base::BigEndianWriter* writer,
                    const dns_protocol::Header& header);
   bool WriteQuestion(base::BigEndianWriter* writer, const DnsQuery& query);
   bool WriteRecord(base::BigEndianWriter* writer,
-                   const DnsResourceRecord& record);
+                   const DnsResourceRecord& record,
+                   bool validate_record);
   bool WriteAnswer(base::BigEndianWriter* writer,
                    const DnsResourceRecord& answer,
                    const base::Optional<DnsQuery>& query,
-                   bool validate_answer_matches_query);
+                   bool validate_record);
 
   // Convenience for header access.
   const dns_protocol::Header* header() const;

net/dns/dns_response_result_extractor.cc

@@ -22,7 +22,10 @@
 #include "base/strings/string_piece.h"
 #include "base/strings/string_util.h"
 #include "base/time/time.h"
+#include "net/base/address_list.h"
 #include "net/base/host_port_pair.h"
+#include "net/base/ip_address.h"
+#include "net/base/ip_endpoint.h"
 #include "net/base/net_errors.h"
 #include "net/dns/dns_response.h"
 #include "net/dns/dns_util.h"
@@ -191,6 +194,10 @@ ExtractionError ExtractResponseRecords(
       if (!cname_data)
         return ExtractionError::kMalformedCname;
 
+      base::TimeDelta ttl = base::TimeDelta::FromSeconds(record->ttl());
+      response_ttl =
+          std::min(response_ttl.value_or(base::TimeDelta::Max()), ttl);
+
       bool added = aliases.emplace(record->name(), cname_data->cname()).second;
       DCHECK(added);
     } else if (record->klass() == dns_protocol::kClassIN &&
@@ -249,9 +256,48 @@ ExtractionError ExtractResponseRecords(
 ExtractionError ExtractAddressResults(const DnsResponse& response,
                                       uint16_t address_qtype,
                                       HostCache::Entry* out_results) {
-  // TODO(crbug.com/1147247): Move address result handling from DnsResponse.
-  NOTIMPLEMENTED();
-  return ExtractionError::kUnexpected;
+  DCHECK(address_qtype == dns_protocol::kTypeA ||
+         address_qtype == dns_protocol::kTypeAAAA);
+  DCHECK(out_results);
+
+  std::vector<std::unique_ptr<const RecordParsed>> records;
+  base::Optional<base::TimeDelta> response_ttl;
+  ExtractionError extraction_error =
+      ExtractResponseRecords(response, address_qtype, &records, &response_ttl);
+
+  if (extraction_error != ExtractionError::kOk) {
+    *out_results = HostCache::Entry(ERR_DNS_MALFORMED_RESPONSE,
+                                    HostCache::Entry::SOURCE_DNS);
+    return extraction_error;
+  }
+
+  AddressList addresses;
+  for (const auto& record : records) {
+    if (addresses.empty())
+      addresses.set_canonical_name(record->name());
+
+    // Expect that ExtractResponseRecords validates that all results correctly
+    // have the same name.
+    DCHECK_EQ(addresses.canonical_name(), record->name());
+
+    IPAddress address;
+    if (address_qtype == dns_protocol::kTypeA) {
+      const ARecordRdata* rdata = record->rdata<ARecordRdata>();
+      address = rdata->address();
+      DCHECK(address.IsIPv4());
+    } else {
+      DCHECK_EQ(address_qtype, dns_protocol::kTypeAAAA);
+      const AAAARecordRdata* rdata = record->rdata<AAAARecordRdata>();
+      address = rdata->address();
+      DCHECK(address.IsIPv6());
+    }
+    addresses.push_back(IPEndPoint(address, 0 /* port */));
+  }
+
+  *out_results = HostCache::Entry(
+      addresses.empty() ? ERR_NAME_NOT_RESOLVED : OK, std::move(addresses),
+      HostCache::Entry::SOURCE_DNS, response_ttl);
+  return ExtractionError::kOk;
 }
 
 ExtractionError ExtractTxtResults(const DnsResponse& response,

net/dns/dns_test_util.cc

@@ -132,7 +132,7 @@ DnsResourceRecord BuildTestTextRecord(std::string name,
   }
 
   return BuildTestDnsRecord(std::move(name), dns_protocol::kTypeTXT,
-                            std::move(rdata));
+                            std::move(rdata), ttl);
 }
 
 DnsResourceRecord BuildTestHttpsAliasRecord(std::string name,
@@ -200,7 +200,7 @@ DnsResponse BuildTestDnsResponse(
   return DnsResponse(0, true /* is_authoritative */, answers,
                      authority /* authority_records */,
                      additional /* additional_records */, query, rcode,
-                     false /* validate_answers_match_query */);
+                     false /* validate_records */);
 }
 
 DnsResponse BuildTestDnsAddressResponse(std::string name,

net/dns/dns_transaction.cc

@@ -54,8 +54,10 @@
 #include "net/dns/dns_socket_allocator.h"
 #include "net/dns/dns_udp_tracker.h"
 #include "net/dns/dns_util.h"
+#include "net/dns/host_cache.h"
 #include "net/dns/public/dns_over_https_server_config.h"
 #include "net/dns/public/dns_protocol.h"
+#include "net/dns/public/dns_query_type.h"
 #include "net/dns/resolve_context.h"
 #include "net/http/http_request_headers.h"
 #include "net/log/net_log.h"
@@ -976,26 +978,30 @@ class DnsOverHttpsProbeRunner : public DnsProbeRunner {
       const DnsAttempt* attempt =
           probe_stats->probe_attempts[attempt_number].get();
       const DnsResponse* response = attempt->GetResponse();
-      AddressList addresses;
-      base::Optional<base::TimeDelta> ttl;
-      if (response &&
-          attempt->GetResponse()->ParseToAddressList(&addresses, &ttl) ==
-              DnsResponseResultExtractor::ExtractionError::kOk &&
-          !addresses.empty()) {
-        // The DoH probe queries don't go through the standard DnsAttempt path,
-        // so the ServerStats have not been updated yet.
-        context_->RecordServerSuccess(doh_server_index,
-                                      true /* is_doh_server */, session_.get());
-        context_->RecordRtt(doh_server_index, true /* is_doh_server */,
-                            base::TimeTicks::Now() - query_start_time, rv,
-                            session_.get());
-        success = true;
-
-        // Do not delete the ProbeStats and cancel the probe sequence. It will
-        // cancel itself on the next scheduled ContinueProbe() call if the
-        // server is still available. This way, the backoff schedule will be
-        // maintained if a server quickly becomes unavailable again before that
-        // scheduled call.
+      if (response) {
+        DnsResponseResultExtractor extractor(response);
+        HostCache::Entry results(ERR_FAILED, HostCache::Entry::SOURCE_UNKNOWN);
+        DnsResponseResultExtractor::ExtractionError extraction_error =
+            extractor.ExtractDnsResults(DnsQueryType::A, &results);
+
+        if (extraction_error ==
+                DnsResponseResultExtractor::ExtractionError::kOk &&
+            results.addresses() && !results.addresses().value().empty()) {
+          // The DoH probe queries don't go through the standard DnsAttempt
+          // path, so the ServerStats have not been updated yet.
+          context_->RecordServerSuccess(
+              doh_server_index, true /* is_doh_server */, session_.get());
+          context_->RecordRtt(doh_server_index, true /* is_doh_server */,
+                              base::TimeTicks::Now() - query_start_time, rv,
+                              session_.get());
+          success = true;
+
+          // Do not delete the ProbeStats and cancel the probe sequence. It will
+          // cancel itself on the next scheduled ContinueProbe() call if the
+          // server is still available. This way, the backoff schedule will be
+          // maintained if a server quickly becomes unavailable again before
+          // that scheduled call.
+        }
       }
     }
 

net/dns/host_resolver_manager.cc

@@ -1261,15 +1261,9 @@ class HostResolverManager::DnsTask : public base::SupportsWeakPtr<DnsTask> {
     // ERR_NAME_NOT_RESOLVED) results with a valid response to parse.
     if (net_error == OK || (net_error == ERR_NAME_NOT_RESOLVED && response &&
                             response->IsValid())) {
-      DnsResponseResultExtractor::ExtractionError extraction_error;
-      if (dns_query_type == DnsQueryType::A ||
-          dns_query_type == DnsQueryType::AAAA) {
-        extraction_error = ParseAddressDnsResponse(response, &results);
-      } else {
-        DnsResponseResultExtractor extractor(response);
-        extraction_error =
-            extractor.ExtractDnsResults(dns_query_type, &results);
-      }
+      DnsResponseResultExtractor extractor(response);
+      DnsResponseResultExtractor::ExtractionError extraction_error =
+          extractor.ExtractDnsResults(dns_query_type, &results);
       DCHECK_NE(extraction_error,
                 DnsResponseResultExtractor::ExtractionError::kUnexpected);
 
@@ -1391,28 +1385,6 @@ class HostResolverManager::DnsTask : public base::SupportsWeakPtr<DnsTask> {
     OnSuccess(results);
   }
 
-  DnsResponseResultExtractor::ExtractionError ParseAddressDnsResponse(
-      const DnsResponse* response,
-      HostCache::Entry* out_results) {
-    DCHECK(response);
-    AddressList addresses;
-    base::Optional<base::TimeDelta> ttl;
-    DnsResponseResultExtractor::ExtractionError parse_result =
-        response->ParseToAddressList(&addresses, &ttl);
-
-    if (parse_result != DnsResponseResultExtractor::ExtractionError::kOk) {
-      *out_results = GetMalformedResponseResult();
-    } else if (addresses.empty()) {
-      *out_results = HostCache::Entry(ERR_NAME_NOT_RESOLVED, AddressList(),
-                                      HostCache::Entry::SOURCE_DNS, ttl);
-    } else {
-      addresses.Deduplicate();
-      *out_results = HostCache::Entry(OK, std::move(addresses),
-                                      HostCache::Entry::SOURCE_DNS, ttl);
-    }
-    return parse_result;
-  }
-
   void OnSortComplete(base::TimeTicks sort_start_time,
                       HostCache::Entry results,
                       bool secure,