Update HTTPS parsing to draft-ietf-dnsop-svcb-https-01

*Allow empty alias names (indicates that a service does not exist).
*Add the "mandatory" service param. Also added an IsCompatible() method
 to indicate if any listed keys are not handled by the parser. Normally
 I would consider that level of handling to be higher level than the
 wire-format stuff the parser should deal with, but the parser is the
 code that knows what params it can handle.
*Also added some extra IP hint validation in the fuzzer while I was
 poking at it for the other changes.

Bug: 1138620
Change-Id: I8172fb5cc9b6ba06d42e6f68c2d11517115f80e2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2508429Reviewed-by: Dan McArdle <dmcardle@chromium.org>
Commit-Queue: Eric Orth <ericorth@chromium.org>
Cr-Commit-Position: refs/heads/master@{#823281}

net/data/fuzzer_dictionaries/net_dns_record_fuzzer.dict

@@ -31,6 +31,7 @@
 "\x00\x19"
 "\x00\x1B"
 "\x00\x1C"
+"\x00\x1D"
 "\x00\x1F"
 "\x00\x20"
 "\x00\x21"
@@ -100,6 +101,7 @@
 "\x00\x41\x00\x01\x00\x00\x14\x00\x00\x03\x00\x00\x00"
 
 # HTTPS service params
+"\x00\x00\x00\x0C\x00\x01\x00\x02\x00\x03\x00\x04\x00\x05\x00\x06"
 "\x00\x01\x00\x08\x03foo\x03bar"
 "\x00\x02\x00\x00"
 "\x00\x03\x00\x02\x14\xC3"

net/dns/https_record_rdata.cc

@@ -5,8 +5,11 @@
 #include "net/dns/https_record_rdata.h"
 
 #include <stdint.h>
+
+#include <algorithm>
 #include <map>
 #include <memory>
+#include <set>
 #include <string>
 #include <utility>
 #include <vector>
@@ -14,6 +17,7 @@
 #include "base/big_endian.h"
 #include "base/check.h"
 #include "base/dcheck_is_on.h"
+#include "base/immediate_crash.h"
 #include "base/memory/ptr_util.h"
 #include "base/optional.h"
 #include "base/strings/string_piece.h"
@@ -47,6 +51,33 @@ bool ReadNextServiceParam(base::Optional<uint16_t> last_key,
   return true;
 }
 
+bool ParseMandatoryKeys(base::StringPiece param_value,
+                        std::set<uint16_t>* out_parsed) {
+  DCHECK(out_parsed);
+
+  base::BigEndianReader reader(param_value.data(), param_value.size());
+
+  std::set<uint16_t> mandatory_keys;
+  // Do/while to require at least one key.
+  do {
+    uint16_t key;
+    if (!reader.ReadU16(&key))
+      return false;
+
+    // Mandatory key itself is disallowed from its list.
+    if (key == dns_protocol::kHttpsServiceParamKeyMandatory)
+      return false;
+    // Keys required to be listed in ascending order.
+    if (!mandatory_keys.empty() && key <= *mandatory_keys.rbegin())
+      return false;
+
+    CHECK(mandatory_keys.insert(key).second);
+  } while (reader.remaining() > 0);
+
+  *out_parsed = std::move(mandatory_keys);
+  return true;
+}
+
 bool ParseAlpnIds(base::StringPiece param_value,
                   std::vector<std::string>* out_parsed) {
   DCHECK(out_parsed);
@@ -161,7 +192,7 @@ std::unique_ptr<AliasFormHttpsRecordRdata> AliasFormHttpsRecordRdata::Parse(
 
   base::Optional<std::string> alias_name =
       DnsDomainToString(reader, true /* require_complete */);
-  if (!alias_name.has_value() || alias_name.value().empty())
+  if (!alias_name.has_value())
     return nullptr;
 
   if (reader.remaining() != 0)
@@ -186,9 +217,13 @@ bool AliasFormHttpsRecordRdata::IsAlias() const {
   return true;
 }
 
+// static
+constexpr uint16_t ServiceFormHttpsRecordRdata::kSupportedKeys[];
+
 ServiceFormHttpsRecordRdata::ServiceFormHttpsRecordRdata(
     uint16_t priority,
     std::string service_name,
+    std::set<uint16_t> mandatory_keys,
     std::vector<std::string> alpn_ids,
     bool default_alpn,
     base::Optional<uint16_t> port,
@@ -198,6 +233,7 @@ ServiceFormHttpsRecordRdata::ServiceFormHttpsRecordRdata(
     std::map<uint16_t, std::string> unparsed_params)
     : priority_(priority),
       service_name_(std::move(service_name)),
+      mandatory_keys_(std::move(mandatory_keys)),
       alpn_ids_(std::move(alpn_ids)),
       default_alpn_(default_alpn),
       port_(port),
@@ -206,14 +242,19 @@ ServiceFormHttpsRecordRdata::ServiceFormHttpsRecordRdata(
       ipv6_hint_(std::move(ipv6_hint)),
       unparsed_params_(std::move(unparsed_params)) {
   DCHECK_NE(priority_, 0);
+  DCHECK(mandatory_keys_.find(dns_protocol::kHttpsServiceParamKeyMandatory) ==
+         mandatory_keys_.end());
 
 #if DCHECK_IS_ON()
-  for (const IPAddress& address : ipv4_hint) {
+  for (const IPAddress& address : ipv4_hint_) {
     DCHECK(address.IsIPv4());
   }
-  for (const IPAddress& address : ipv6_hint) {
+  for (const IPAddress& address : ipv6_hint_) {
     DCHECK(address.IsIPv6());
   }
+  for (const auto& unparsed_param : unparsed_params_) {
+    DCHECK(!IsSupportedKey(unparsed_param.first));
+  }
 #endif  // DCHECK_IS_ON()
 }
 
@@ -229,6 +270,7 @@ bool ServiceFormHttpsRecordRdata::IsEqual(const HttpsRecordRdata* other) const {
       static_cast<const ServiceFormHttpsRecordRdata*>(other);
   return priority_ == service->priority_ &&
          service_name_ == service->service_name_ &&
+         mandatory_keys_ == service->mandatory_keys_ &&
          alpn_ids_ == service->alpn_ids_ &&
          default_alpn_ == service->default_alpn_ && port_ == service->port_ &&
          ipv4_hint_ == service->ipv4_hint_ &&
@@ -259,6 +301,7 @@ std::unique_ptr<ServiceFormHttpsRecordRdata> ServiceFormHttpsRecordRdata::Parse(
   if (reader.remaining() == 0) {
     return std::make_unique<ServiceFormHttpsRecordRdata>(
         priority, std::move(service_name).value(),
+        std::set<uint16_t>() /* mandatory_keys */,
         std::vector<std::string>() /* alpn_ids */, true /* default_alpn */,
         base::nullopt /* port */, std::vector<IPAddress>() /* ipv4_hint */,
         std::string() /* ech_config */,
@@ -272,19 +315,23 @@ std::unique_ptr<ServiceFormHttpsRecordRdata> ServiceFormHttpsRecordRdata::Parse(
                             &param_value))
     return nullptr;
 
-  std::map<uint16_t, std::string> unparsed_params;
-  while (param_key < dns_protocol::kHttpsServiceParamKeyAlpn) {
-    CHECK(unparsed_params
-              .emplace(param_key, static_cast<std::string>(param_value))
-              .second);
-    if (reader.remaining() == 0)
-      break;
-    if (!ReadNextServiceParam(param_key, reader, &param_key, &param_value))
+  // Assume keys less than Mandatory are not possible.
+  DCHECK_GE(param_key, dns_protocol::kHttpsServiceParamKeyMandatory);
+
+  std::set<uint16_t> mandatory_keys;
+  if (param_key == dns_protocol::kHttpsServiceParamKeyMandatory) {
+    DCHECK(IsSupportedKey(param_key));
+    if (!ParseMandatoryKeys(param_value, &mandatory_keys))
       return nullptr;
+    if (reader.remaining() > 0 &&
+        !ReadNextServiceParam(param_key, reader, &param_key, &param_value)) {
+      return nullptr;
+    }
   }
 
   std::vector<std::string> alpn_ids;
   if (param_key == dns_protocol::kHttpsServiceParamKeyAlpn) {
+    DCHECK(IsSupportedKey(param_key));
     if (!ParseAlpnIds(param_value, &alpn_ids))
       return nullptr;
     if (reader.remaining() > 0 &&
@@ -295,6 +342,7 @@ std::unique_ptr<ServiceFormHttpsRecordRdata> ServiceFormHttpsRecordRdata::Parse(
 
   bool default_alpn = true;
   if (param_key == dns_protocol::kHttpsServiceParamKeyNoDefaultAlpn) {
+    DCHECK(IsSupportedKey(param_key));
     if (!param_value.empty())
       return nullptr;
     default_alpn = false;
@@ -306,6 +354,7 @@ std::unique_ptr<ServiceFormHttpsRecordRdata> ServiceFormHttpsRecordRdata::Parse(
 
   base::Optional<uint16_t> port;
   if (param_key == dns_protocol::kHttpsServiceParamKeyPort) {
+    DCHECK(IsSupportedKey(param_key));
     if (param_value.size() != 2)
       return nullptr;
     uint16_t port_val;
@@ -319,6 +368,7 @@ std::unique_ptr<ServiceFormHttpsRecordRdata> ServiceFormHttpsRecordRdata::Parse(
 
   std::vector<IPAddress> ipv4_hint;
   if (param_key == dns_protocol::kHttpsServiceParamKeyIpv4Hint) {
+    DCHECK(IsSupportedKey(param_key));
     if (!ParseIpAddresses<IPAddress::kIPv4AddressSize>(param_value, &ipv4_hint))
       return nullptr;
     if (reader.remaining() > 0 &&
@@ -329,6 +379,7 @@ std::unique_ptr<ServiceFormHttpsRecordRdata> ServiceFormHttpsRecordRdata::Parse(
 
   std::string ech_config;
   if (param_key == dns_protocol::kHttpsServiceParamKeyEchConfig) {
+    DCHECK(IsSupportedKey(param_key));
     ech_config = std::string(param_value.data(), param_value.size());
     if (reader.remaining() > 0 &&
         !ReadNextServiceParam(param_key, reader, &param_key, &param_value)) {
@@ -338,6 +389,7 @@ std::unique_ptr<ServiceFormHttpsRecordRdata> ServiceFormHttpsRecordRdata::Parse(
 
   std::vector<IPAddress> ipv6_hint;
   if (param_key == dns_protocol::kHttpsServiceParamKeyIpv6Hint) {
+    DCHECK(IsSupportedKey(param_key));
     if (!ParseIpAddresses<IPAddress::kIPv6AddressSize>(param_value, &ipv6_hint))
       return nullptr;
     if (reader.remaining() > 0 &&
@@ -348,8 +400,10 @@ std::unique_ptr<ServiceFormHttpsRecordRdata> ServiceFormHttpsRecordRdata::Parse(
 
   // Note that if parsing has already reached the end of the rdata, `param_key`
   // is still set for whatever param was read last.
+  std::map<uint16_t, std::string> unparsed_params;
   if (param_key > dns_protocol::kHttpsServiceParamKeyIpv6Hint) {
     for (;;) {
+      DCHECK(!IsSupportedKey(param_key));
       CHECK(unparsed_params
                 .emplace(param_key, static_cast<std::string>(param_value))
                 .second);
@@ -361,9 +415,40 @@ std::unique_ptr<ServiceFormHttpsRecordRdata> ServiceFormHttpsRecordRdata::Parse(
   }
 
   return std::make_unique<ServiceFormHttpsRecordRdata>(
-      priority, std::move(service_name).value(), std::move(alpn_ids),
-      default_alpn, port, std::move(ipv4_hint), std::move(ech_config),
-      std::move(ipv6_hint), std::move(unparsed_params));
+      priority, std::move(service_name).value(), std::move(mandatory_keys),
+      std::move(alpn_ids), default_alpn, port, std::move(ipv4_hint),
+      std::move(ech_config), std::move(ipv6_hint), std::move(unparsed_params));
+}
+
+bool ServiceFormHttpsRecordRdata::IsCompatible() const {
+  std::set<uint16_t> supported_keys(std::begin(kSupportedKeys),
+                                    std::end(kSupportedKeys));
+
+  for (uint16_t mandatory_key : mandatory_keys_) {
+    DCHECK_NE(mandatory_key, dns_protocol::kHttpsServiceParamKeyMandatory);
+
+    if (supported_keys.find(mandatory_key) == supported_keys.end())
+      return false;
+  }
+
+#if DCHECK_IS_ON()
+  for (const auto& unparsed_param : unparsed_params_) {
+    DCHECK(mandatory_keys_.find(unparsed_param.first) == mandatory_keys_.end());
+  }
+#endif  // DCHECK_IS_ON()
+
+  return true;
+}
+
+// static
+bool ServiceFormHttpsRecordRdata::IsSupportedKey(uint16_t key) {
+#if DCHECK_IS_ON()
+  return std::find(std::begin(kSupportedKeys), std::end(kSupportedKeys), key) !=
+         std::end(kSupportedKeys);
+#else
+  // Only intended for DCHECKs.
+  IMMEDIATE_CRASH();
+#endif  // DCHECK_IS_ON()
 }
 
 }  // namespace net

net/dns/https_record_rdata.h

@@ -8,6 +8,7 @@
 #include <stdint.h>
 #include <map>
 #include <memory>
+#include <set>
 #include <string>
 #include <vector>
 
@@ -57,7 +58,7 @@ class NET_EXPORT_PRIVATE AliasFormHttpsRecordRdata : public HttpsRecordRdata {
   bool IsEqual(const HttpsRecordRdata* other) const override;
   bool IsAlias() const override;
 
-  base::StringPiece alias_name() { return alias_name_; }
+  base::StringPiece alias_name() const { return alias_name_; }
 
  private:
   AliasFormHttpsRecordRdata() = default;
@@ -67,8 +68,18 @@ class NET_EXPORT_PRIVATE AliasFormHttpsRecordRdata : public HttpsRecordRdata {
 
 class NET_EXPORT_PRIVATE ServiceFormHttpsRecordRdata : public HttpsRecordRdata {
  public:
+  static constexpr uint16_t kSupportedKeys[] = {
+      dns_protocol::kHttpsServiceParamKeyMandatory,
+      dns_protocol::kHttpsServiceParamKeyAlpn,
+      dns_protocol::kHttpsServiceParamKeyNoDefaultAlpn,
+      dns_protocol::kHttpsServiceParamKeyPort,
+      dns_protocol::kHttpsServiceParamKeyIpv4Hint,
+      dns_protocol::kHttpsServiceParamKeyEchConfig,
+      dns_protocol::kHttpsServiceParamKeyIpv6Hint};
+
   ServiceFormHttpsRecordRdata(uint16_t priority,
                               std::string service_name,
+                              std::set<uint16_t> mandatory_keys,
                               std::vector<std::string> alpn_ids,
                               bool default_alpn,
                               base::Optional<uint16_t> port,
@@ -84,23 +95,33 @@ class NET_EXPORT_PRIVATE ServiceFormHttpsRecordRdata : public HttpsRecordRdata {
   bool IsEqual(const HttpsRecordRdata* other) const override;
   bool IsAlias() const override;
 
-  uint16_t priority() { return priority_; }
-  base::StringPiece service_name() { return service_name_; }
-  const std::vector<std::string>& alpn_ids() { return alpn_ids_; }
-  bool default_alpn() { return default_alpn_; }
-  base::Optional<uint16_t> port() { return port_; }
-  const std::vector<IPAddress>& ipv4_hint() { return ipv4_hint_; }
-  base::StringPiece ech_config() { return ech_config_; }
-  const std::vector<IPAddress>& ipv6_hint() { return ipv6_hint_; }
-  const std::map<uint16_t, std::string>& unparsed_params() {
+  uint16_t priority() const { return priority_; }
+  base::StringPiece service_name() const { return service_name_; }
+  const std::set<uint16_t>& mandatory_keys() const { return mandatory_keys_; }
+  const std::vector<std::string>& alpn_ids() const { return alpn_ids_; }
+  bool default_alpn() const { return default_alpn_; }
+  base::Optional<uint16_t> port() const { return port_; }
+  const std::vector<IPAddress>& ipv4_hint() const { return ipv4_hint_; }
+  base::StringPiece ech_config() const { return ech_config_; }
+  const std::vector<IPAddress>& ipv6_hint() const { return ipv6_hint_; }
+  const std::map<uint16_t, std::string>& unparsed_params() const {
     return unparsed_params_;
   }
 
+  // Returns whether or not this rdata parser is considered "compatible" with
+  // the parsed rdata. That is that all keys listed by mandatory_keys() (and all
+  // keys considered default mandatory for HTTPS records) are parsable by this
+  // parser.
+  bool IsCompatible() const;
+
  private:
+  static bool IsSupportedKey(uint16_t key);
+
   const uint16_t priority_;
   const std::string service_name_;
 
   // Supported service parameters.
+  const std::set<uint16_t> mandatory_keys_;
   const std::vector<std::string> alpn_ids_;
   const bool default_alpn_;
   const base::Optional<uint16_t> port_;

net/dns/https_record_rdata_fuzzer.cc

@@ -7,9 +7,12 @@
 #include <stdint.h>
 
 #include <memory>
+#include <set>
+#include <vector>
 
 #include "base/check.h"
 #include "base/strings/string_piece.h"
+#include "net/base/ip_address.h"
 #include "net/dns/public/dns_protocol.h"
 
 namespace net {
@@ -30,7 +33,7 @@ void ParseAndExercise(base::StringPiece data) {
   CHECK_EQ(parsed->Type(), dns_protocol::kTypeHttps);
   if (parsed->IsAlias()) {
     AliasFormHttpsRecordRdata* alias = parsed->AsAliasForm();
-    CHECK(!alias->alias_name().empty());
+    alias->alias_name();
   } else {
     ServiceFormHttpsRecordRdata* service = parsed->AsServiceForm();
     CHECK_GT(service->priority(), 0);
@@ -38,10 +41,23 @@ void ParseAndExercise(base::StringPiece data) {
     service->alpn_ids();
     service->default_alpn();
     service->port();
-    service->ipv4_hint();
     service->ech_config();
-    service->ipv6_hint();
     service->unparsed_params();
+    service->IsCompatible();
+
+    std::set<uint16_t> mandatory_keys = service->mandatory_keys();
+    CHECK(mandatory_keys.find(dns_protocol::kHttpsServiceParamKeyMandatory) ==
+          mandatory_keys.end());
+
+    std::vector<IPAddress> ipv4_hint = service->ipv4_hint();
+    for (const IPAddress& address : ipv4_hint) {
+      CHECK(address.IsIPv4());
+    }
+
+    std::vector<IPAddress> ipv6_hint = service->ipv6_hint();
+    for (const IPAddress& address : ipv6_hint) {
+      CHECK(address.IsIPv6());
+    }
   }
 }
 

net/dns/https_record_rdata_unittest.cc

@@ -43,6 +43,8 @@ TEST(HttpsRecordRdataTest, ParsesService) {
       "\000\001"
       // Service name: chromium.org
       "\010chromium\003org\000"
+      // mandatory=alpn,no-default-alpn,port,ipv4hint,echconfig,ipv6hint
+      "\000\000\000\014\000\001\000\002\000\003\000\004\000\005\000\006"
       // alpn=foo,bar
       "\000\001\000\010\003foo\003bar"
       // no-default-alpn
@@ -66,7 +68,7 @@ TEST(HttpsRecordRdataTest, ParsesService) {
   IPAddress expected_ipv6;
   ASSERT_TRUE(expected_ipv6.AssignFromIPLiteral("2001:4860:4860::8888"));
   ServiceFormHttpsRecordRdata expected(
-      1 /* priority */, "chromium.org",
+      1 /* priority */, "chromium.org", std::set<uint16_t>({1, 2, 3, 4, 5, 6}),
       std::vector<std::string>({"foo", "bar"}) /* alpn_ids */,
       false /* default_alpn */, base::Optional<uint16_t>(46) /* port */,
       std::vector<IPAddress>({IPAddress(8, 8, 8, 8)}) /* ipv4_hint */,
@@ -80,6 +82,8 @@ TEST(HttpsRecordRdataTest, ParsesService) {
   ASSERT_TRUE(service_rdata);
   EXPECT_EQ(service_rdata->priority(), 1);
   EXPECT_EQ(service_rdata->service_name(), "chromium.org");
+  EXPECT_THAT(service_rdata->mandatory_keys(),
+              testing::ElementsAre(1, 2, 3, 4, 5, 6));
   EXPECT_THAT(service_rdata->alpn_ids(), testing::ElementsAre("foo", "bar"));
   EXPECT_FALSE(service_rdata->default_alpn());
   EXPECT_THAT(service_rdata->port(), testing::Optional(46));
@@ -89,6 +93,7 @@ TEST(HttpsRecordRdataTest, ParsesService) {
   EXPECT_THAT(service_rdata->ipv6_hint(), testing::ElementsAre(expected_ipv6));
   EXPECT_THAT(service_rdata->unparsed_params(),
               testing::ElementsAre(testing::Pair(7, "foo")));
+  EXPECT_TRUE(service_rdata->IsCompatible());
 }
 
 TEST(HttpsRecordRdataTest, RejectCorruptRdata) {

net/dns/public/dns_protocol.h

@@ -189,7 +189,8 @@ static const uint16_t kFlagTC = 0x200;  // Truncated - server flag.
 
 // SVCB/HTTPS ServiceParamKey
 //
-// IANA registration pending. Values from draft-ietf-dnsop-svcb-httpssvc-03.
+// IANA registration pending. Values from draft-ietf-dnsop-svcb-https-01.
+static constexpr uint16_t kHttpsServiceParamKeyMandatory = 0;
 static constexpr uint16_t kHttpsServiceParamKeyAlpn = 1;
 static constexpr uint16_t kHttpsServiceParamKeyNoDefaultAlpn = 2;
 static constexpr uint16_t kHttpsServiceParamKeyPort = 3;