Ignore unknown record types in DNS responses

Previously worked only for address requests. Other request types (e.g.
HTTPS), on seeing any unrecognized (and therefore unparsable) record
type in the response, would fail the whole response as malformed.

Bug: 1147247
Change-Id: Icd9e5ab2e39cf09ffe7c3945b011ebf1d4b45778
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2529619
Commit-Queue: Eric Orth <ericorth@chromium.org>
Auto-Submit: Eric Orth <ericorth@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#826462}

net/dns/dns_test_util.cc

@@ -4,13 +4,19 @@
 
 #include "net/dns/dns_test_util.h"
 
+#include <string>
+#include <utility>
+#include <vector>
+
 #include "base/big_endian.h"
 #include "base/bind.h"
+#include "base/check.h"
 #include "base/location.h"
 #include "base/numerics/safe_conversions.h"
 #include "base/single_thread_task_runner.h"
 #include "base/sys_byteorder.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "base/time/time.h"
 #include "net/base/io_buffer.h"
 #include "net/base/ip_address.h"
 #include "net/base/net_errors.h"
@@ -99,6 +105,23 @@ DnsResourceRecord BuildTestAddressRecord(std::string name,
       net::IPAddressToPackedString(ip), ttl);
 }
 
+DnsResourceRecord BuildTestTextRecord(std::string name,
+                                      std::vector<std::string> text_strings,
+                                      base::TimeDelta ttl) {
+  DCHECK(!text_strings.empty());
+
+  std::string rdata;
+  for (const std::string& text_string : text_strings) {
+    DCHECK(!text_string.empty());
+
+    rdata += base::checked_cast<unsigned char>(text_string.size());
+    rdata += text_string;
+  }
+
+  return BuildTestDnsRecord(std::move(name), dns_protocol::kTypeTXT,
+                            std::move(rdata));
+}
+
 DnsResourceRecord BuildTestHttpsAliasRecord(std::string name,
                                             base::StringPiece alias_name,
                                             base::TimeDelta ttl) {
@@ -215,18 +238,7 @@ DnsResponse BuildTestDnsTextResponse(
 
   std::vector<DnsResourceRecord> answers;
   for (std::vector<std::string>& text_record : text_records) {
-    DCHECK(!text_record.empty());
-
-    std::string rdata;
-    for (std::string text_string : text_record) {
-      DCHECK(!text_string.empty());
-
-      rdata += base::checked_cast<unsigned char>(text_string.size());
-      rdata += std::move(text_string);
-    }
-
-    answers.push_back(BuildTestDnsRecord(answer_name, dns_protocol::kTypeTXT,
-                                         std::move(rdata)));
+    answers.push_back(BuildTestTextRecord(answer_name, std::move(text_record)));
   }
 
   return BuildTestDnsResponse(std::move(name), dns_protocol::kTypeTXT, answers);

net/dns/dns_test_util.h

@@ -203,6 +203,11 @@ DnsResourceRecord BuildTestAddressRecord(
     const IPAddress& ip,
     base::TimeDelta ttl = base::TimeDelta::FromDays(1));
 
+DnsResourceRecord BuildTestTextRecord(
+    std::string name,
+    std::vector<std::string> text_strings,
+    base::TimeDelta ttl = base::TimeDelta::FromDays(1));
+
 DnsResourceRecord BuildTestHttpsAliasRecord(
     std::string name,
     base::StringPiece alias_name,

net/dns/host_resolver_manager_unittest.cc

@@ -7950,6 +7950,39 @@ TEST_F(HostResolverManagerDnsTest, TxtQuery) {
                                        bar_records.begin(), bar_records.end()));
 }
 
+// Test that TXT records can be extracted from a response that also contains
+// unrecognized record types.
+TEST_F(HostResolverManagerDnsTest, TxtQuery_MixedWithUnrecognizedType) {
+  std::vector<std::string> text_strings = {"foo"};
+
+  MockDnsClientRuleList rules;
+  rules.emplace_back(
+      "host", dns_protocol::kTypeTXT, false /* secure */,
+      MockDnsClientRule::Result(BuildTestDnsResponse(
+          "host", dns_protocol::kTypeTXT,
+          {BuildTestDnsRecord("host", 3u /* type */, "fake rdata 1"),
+           BuildTestTextRecord("host", std::move(text_strings)),
+           BuildTestDnsRecord("host", 3u /* type */, "fake rdata 2")})),
+      false /* delay */);
+
+  CreateResolver();
+  UseMockDnsClient(CreateValidDnsConfig(), std::move(rules));
+
+  HostResolver::ResolveHostParameters parameters;
+  parameters.dns_query_type = DnsQueryType::TXT;
+
+  ResolveHostResponseHelper response(resolver_->CreateRequest(
+      HostPortPair("host", 108), NetworkIsolationKey(), NetLogWithSource(),
+      parameters, resolve_context_.get(), resolve_context_->host_cache()));
+  EXPECT_THAT(response.result_error(), IsOk());
+  EXPECT_FALSE(response.request()->GetAddressResults());
+  EXPECT_FALSE(response.request()->GetHostnameResults());
+  EXPECT_FALSE(response.request()->GetExperimentalResultsForTesting());
+
+  EXPECT_THAT(response.request()->GetTextResults(),
+              testing::Optional(testing::ElementsAre("foo")));
+}
+
 TEST_F(HostResolverManagerDnsTest, TxtQuery_InvalidConfig) {
   set_allow_fallback_to_proctask(false);
   // Set empty DnsConfig.
@@ -8869,6 +8902,39 @@ TEST_F(HostResolverManagerDnsTest, HttpsQuery) {
   EXPECT_FALSE(response.request()->GetExperimentalResultsForTesting());
 }
 
+// Test that HTTPS records can be extracted from a response that also contains
+// unrecognized record types.
+TEST_F(HostResolverManagerDnsTest, HttpsQuery_MixedWithUnrecognizedType) {
+  const std::string kName = "https.test";
+
+  MockDnsClientRuleList rules;
+  std::vector<DnsResourceRecord> records = {
+      BuildTestDnsRecord(kName, 3u /* type */, "fake rdata 1"),
+      BuildTestHttpsAliasRecord(kName, "alias.test"),
+      BuildTestDnsRecord(kName, 3u /* type */, "fake rdata 2")};
+  rules.emplace_back(kName, dns_protocol::kTypeHttps, false /* secure */,
+                     MockDnsClientRule::Result(BuildTestDnsResponse(
+                         kName, dns_protocol::kTypeHttps, records)),
+                     false /* delay */);
+
+  CreateResolver();
+  UseMockDnsClient(CreateValidDnsConfig(), std::move(rules));
+
+  HostResolver::ResolveHostParameters parameters;
+  parameters.dns_query_type = DnsQueryType::HTTPS;
+
+  ResolveHostResponseHelper response(resolver_->CreateRequest(
+      HostPortPair(kName, 108), NetworkIsolationKey(), NetLogWithSource(),
+      parameters, resolve_context_.get(), resolve_context_->host_cache()));
+  // Experimental type, so does not affect overall result.
+  EXPECT_THAT(response.result_error(), IsError(ERR_NAME_NOT_RESOLVED));
+  EXPECT_FALSE(response.request()->GetAddressResults());
+  EXPECT_FALSE(response.request()->GetHostnameResults());
+  EXPECT_FALSE(response.request()->GetTextResults());
+  // Results never saved when overall result not OK.
+  EXPECT_FALSE(response.request()->GetExperimentalResultsForTesting());
+}
+
 TEST_F(HostResolverManagerDnsTest, HttpsQuery_MultipleResults) {
   const std::string kName = "https.test";
 

net/dns/record_parsed.cc

@@ -38,6 +38,7 @@ std::unique_ptr<const RecordParsed> RecordParsed::CreateFrom(
   if (!parser->ReadRecord(&record))
     return std::unique_ptr<const RecordParsed>();
 
+  bool unrecognized_type = false;
   switch (record.type) {
     case ARecordRdata::kType:
       rdata = ARecordRdata::Create(record.rdata, *parser);
@@ -71,10 +72,14 @@ std::unique_ptr<const RecordParsed> RecordParsed::CreateFrom(
       break;
     default:
       DVLOG(1) << "Unknown RData type for received record: " << record.type;
-      return std::unique_ptr<const RecordParsed>();
+      rdata = nullptr;
+      unrecognized_type = true;
+      break;
   }
 
-  if (!rdata.get())
+  // If a recognized type has a malformed rdata, consider the whole record
+  // malformed.
+  if (!rdata.get() && !unrecognized_type)
     return std::unique_ptr<const RecordParsed>();
 
   return std::unique_ptr<const RecordParsed>(

net/dns/record_parsed.h

@@ -39,9 +39,16 @@ class NET_EXPORT_PRIVATE RecordParsed {
   template <class T> const T* rdata() const {
     if (T::kType != type_)
       return nullptr;
+
+    // Expect RData should always be parsed for recognized types.
+    DCHECK(rdata_);
+
     return static_cast<const T*>(rdata_.get());
   }
 
+  // Gets the rdata without casting to a known RData type.
+  const RecordRdata* rdata_for_testing() const { return rdata_.get(); }
+
   // Check if two records have the same data. Ignores time_created and ttl.
   // If |is_mdns| is true, ignore the top bit of the class
   // (the cache flush bit).

net/dns/record_parsed_unittest.cc

@@ -4,6 +4,9 @@
 
 #include "net/dns/record_parsed.h"
 
+#include <memory>
+
+#include "base/time/time.h"
 #include "net/dns/dns_response.h"
 #include "net/dns/dns_test_util.h"
 #include "net/dns/public/dns_protocol.h"
@@ -65,4 +68,55 @@ TEST(RecordParsedTest, CacheFlushBitCompare) {
   EXPECT_TRUE(record2->IsEqual(record1.get(), true));
 }
 
-}  //namespace net
+TEST(RecordParsedTest, ParseUnknownRdata) {
+  static const char kRecordData[] =
+      // NAME="foo.test"
+      "\003foo\004test\000"
+      // TYPE=MD (an obsolete type that will likely never be recognized by
+      // Chrome)
+      "\000\003"
+      // CLASS=IN
+      "\000\001"
+      // TTL=30 seconds
+      "\000\000\000\036"
+      // RDLENGTH=12 bytes
+      "\000\014"
+      // RDATA="garbage data"
+      "garbage data";
+  DnsRecordParser parser(kRecordData, sizeof(kRecordData) - 1, 0 /* offset */);
+
+  std::unique_ptr<const RecordParsed> record =
+      RecordParsed::CreateFrom(&parser, base::Time());
+
+  ASSERT_TRUE(record);
+  EXPECT_EQ(record->name(), "foo.test");
+  EXPECT_EQ(record->type(), 3u);
+  EXPECT_EQ(record->klass(), dns_protocol::kClassIN);
+  EXPECT_EQ(record->ttl(), 30u);
+  EXPECT_FALSE(record->rdata<ARecordRdata>());
+  EXPECT_FALSE(record->rdata_for_testing());
+}
+
+TEST(RecordParsedTest, RejectMalformedRdata) {
+  static const char kRecordData[] =
+      // NAME="foo.test"
+      "\003foo\004test\000"
+      // TYPE=PTR
+      "\000\014"
+      // CLASS=IN
+      "\000\001"
+      // TTL=31 seconds
+      "\000\000\000\037"
+      // RDLENGTH=1 byte
+      "\000\001"
+      // RDATA=truncated name
+      "\001";
+  DnsRecordParser parser(kRecordData, sizeof(kRecordData) - 1, 0 /* offset */);
+
+  std::unique_ptr<const RecordParsed> record =
+      RecordParsed::CreateFrom(&parser, base::Time());
+
+  EXPECT_FALSE(record);
+}
+
+}  // namespace net

net/dns/record_rdata.cc

@@ -48,8 +48,8 @@ bool RecordRdata::HasValidSize(const base::StringPiece& data, uint16_t type) {
     case dns_protocol::kTypeSOA:
       return true;
     default:
-      VLOG(1) << "Unsupported RDATA type.";
-      return false;
+      VLOG(1) << "Unrecognized RDATA type.";
+      return true;
   }
 }
 

net/dns/record_rdata.h

@@ -32,8 +32,9 @@ class NET_EXPORT RecordRdata {
  public:
   virtual ~RecordRdata() {}
 
-  // Return true if |data| represents RDATA in the wire format with a valid size
-  // for the give |type|.
+  // Return true if `data` represents RDATA in the wire format with a valid size
+  // for the give `type`. Always returns true for unrecognized `type`s as the
+  // size is never known to be invalid.
   static bool HasValidSize(const base::StringPiece& data, uint16_t type);
 
   virtual bool IsEqual(const RecordRdata* other) const = 0;