From 34fdcd5def2c218861037158a677791487cae225 Mon Sep 17 00:00:00 2001 From: Eric Orth Date: Wed, 28 Oct 2020 18:00:47 +0000 Subject: [PATCH] Improve DNS name parsing *Allow optionally enforcing that the name ends with a zero-length label (the root label). *Add additional validation that the name doesn't use DNS name compression, and that the name is within the max DNS name length. *Add an overload that takes a BigEndianReader as input. Better supports parsing within a DNS message when the name needs to be parsed to know its total length and where the next data starts after the name. These improvements should be useful in parsing HTTPS records. Bug: 1138620 Change-Id: Ice66aa880a8c5407af3bfd9a14d720d518ae2136 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2493153 Commit-Queue: Eric Orth Reviewed-by: Dan McArdle Cr-Commit-Position: refs/heads/master@{#821794} --- net/dns/dns_util.cc | 66 ++++--- net/dns/dns_util.h | 21 ++- net/dns/dns_util_unittest.cc | 323 ++++++++++++++++++++++++++++++++-- net/dns/public/dns_protocol.h | 5 + 4 files changed, 371 insertions(+), 44 deletions(-) diff --git a/net/dns/dns_util.cc b/net/dns/dns_util.cc index 74d401bfd262ea..d956b197948e5f 100644 --- a/net/dns/dns_util.cc +++ b/net/dns/dns_util.cc @@ -28,19 +28,6 @@ #include "net/third_party/uri_template/uri_template.h" #include "url/url_canon.h" -namespace { - -// RFC 1035, section 2.3.4: labels 63 octets or less. -// Section 3.1: Each label is represented as a one octet length field followed -// by that number of octets. -const int kMaxLabelLength = 63; - -// RFC 1035, section 4.1.4: the first two bits of a 16-bit name pointer are -// ones. -const uint16_t kFlagNamePointer = 0xc000; - -} // namespace - #if defined(OS_POSIX) #include #if !defined(OS_NACL) @@ -64,7 +51,7 @@ bool DNSDomainFromDot(const base::StringPiece& dotted, std::string* out) { const char* buf = dotted.data(); size_t n = dotted.size(); - char label[kMaxLabelLength]; + char label[dns_protocol::kMaxLabelLength]; size_t labellen = 0; /* <= sizeof label */ char name[dns_protocol::kMaxNameLength]; size_t namelen = 0; /* <= sizeof name */ @@ -162,25 +149,50 @@ bool IsValidHostLabelCharacter(char c, bool is_first_char) { (c >= '0' && c <= '9') || (!is_first_char && c == '-') || c == '_'; } -base::Optional DnsDomainToString(base::StringPiece domain) { +base::Optional DnsDomainToString(base::StringPiece dns_name, + bool require_complete) { + base::BigEndianReader reader(dns_name.data(), dns_name.length()); + return DnsDomainToString(reader, require_complete); +} + +base::Optional DnsDomainToString(base::BigEndianReader& reader, + bool require_complete) { std::string ret; + size_t octets_read = 0; + while (reader.remaining() > 0) { + // DNS name compression not allowed because it does not make sense without + // the context of a full DNS message. + if ((*reader.ptr() & dns_protocol::kLabelMask) == + dns_protocol::kLabelPointer) + return base::nullopt; - for (unsigned i = 0; i < domain.size() && domain[i]; i += domain[i] + 1) { -#if CHAR_MIN < 0 - if (domain[i] < 0) + base::StringPiece label; + if (!reader.ReadU8LengthPrefixed(&label)) return base::nullopt; -#endif - if (domain[i] > kMaxLabelLength) + octets_read += label.size() + 1; + + if (label.size() > dns_protocol::kMaxLabelLength) + return base::nullopt; + if (octets_read > dns_protocol::kMaxNameLength) return base::nullopt; - if (i) - ret += "."; + if (label.size() == 0) + return ret; - if (static_cast(domain[i]) + i + 1 > domain.size()) - return base::nullopt; + if (!ret.empty()) + ret.append("."); - ret.append(domain.data() + i + 1, domain[i]); + ret.append(label.data(), label.size()); } + + if (require_complete) + return base::nullopt; + + // If terminating zero-length label was not included in the input, it still + // counts against the max name length. + if (octets_read + 1 > dns_protocol::kMaxNameLength) + return base::nullopt; + return ret; } @@ -269,10 +281,10 @@ AddressListDeltaType FindAddressListDeltaType(const AddressList& a, } std::string CreateNamePointer(uint16_t offset) { - DCHECK_LE(offset, 0x3fff); - offset |= kFlagNamePointer; + DCHECK_EQ(offset & ~dns_protocol::kOffsetMask, 0); char buf[2]; base::WriteBigEndian(buf, offset); + buf[0] |= dns_protocol::kLabelPointer; return std::string(buf, sizeof(buf)); } diff --git a/net/dns/dns_util.h b/net/dns/dns_util.h index f5e132bf29fc30..b4ba304d714ebd 100644 --- a/net/dns/dns_util.h +++ b/net/dns/dns_util.h @@ -19,6 +19,10 @@ #include "net/dns/public/dns_query_type.h" #include "net/dns/public/secure_dns_mode.h" +namespace base { +class BigEndianReader; +} // namespace base + namespace net { class AddressList; @@ -65,10 +69,21 @@ NET_EXPORT_PRIVATE bool IsValidUnrestrictedDNSDomain( NET_EXPORT_PRIVATE bool IsValidHostLabelCharacter(char c, bool is_first_char); // Converts a domain in DNS format to a dotted string. Excludes the dot at the -// end. Assumes the standard terminating zero-length label at the end if not -// included in the input. Returns nullopt on malformed input. +// end. Returns nullopt on malformed input. +// +// If `require_complete` is true, input will be considered malformed if it does +// not contain a terminating zero-length label. If false, assumes the standard +// terminating zero-length label at the end if not included in the input. +// +// DNS name compression (see RFC 1035, section 4.1.4) is disallowed and +// considered malformed. To handle a potentially compressed name, in a +// DnsResponse object, use DnsRecordParser::ReadName(). +NET_EXPORT base::Optional DnsDomainToString( + base::StringPiece dns_name, + bool require_complete = false); NET_EXPORT base::Optional DnsDomainToString( - base::StringPiece dns_name); + base::BigEndianReader& reader, + bool require_complete = false); // Return the expanded template when no variables have corresponding values. NET_EXPORT_PRIVATE std::string GetURLFromTemplateWithoutParameters( diff --git a/net/dns/dns_util_unittest.cc b/net/dns/dns_util_unittest.cc index c2ce410a92c3e7..63f579cd567fc7 100644 --- a/net/dns/dns_util_unittest.cc +++ b/net/dns/dns_util_unittest.cc @@ -4,6 +4,13 @@ #include "net/dns/dns_util.h" +#include +#include + +#include + +#include "base/big_endian.h" +#include "base/numerics/safe_conversions.h" #include "base/optional.h" #include "base/stl_util.h" #include "net/dns/public/dns_protocol.h" @@ -82,22 +89,310 @@ TEST_F(DNSUtilTest, DNSDomainFromUnrestrictedDot) { &out)); } -TEST_F(DNSUtilTest, DnsDomainToString) { - EXPECT_THAT(DnsDomainToString(IncludeNUL("")), testing::Optional(Eq(""))); - EXPECT_THAT(DnsDomainToString(IncludeNUL("\003foo")), - testing::Optional(Eq("foo"))); - EXPECT_THAT(DnsDomainToString(IncludeNUL("\003foo\003bar")), - testing::Optional(Eq("foo.bar"))); - EXPECT_THAT(DnsDomainToString(IncludeNUL("\003foo\003bar\002uk")), - testing::Optional(Eq("foo.bar.uk"))); +TEST_F(DNSUtilTest, DnsDomainToStringShouldHandleSimpleNames) { + std::string dns_name = "\003foo"; + EXPECT_THAT(DnsDomainToString(dns_name), testing::Optional(Eq("foo"))); + base::BigEndianReader reader(dns_name.c_str(), dns_name.size()); + EXPECT_THAT(DnsDomainToString(reader), testing::Optional(Eq("foo"))); + + dns_name += "\003bar"; + EXPECT_THAT(DnsDomainToString(dns_name), testing::Optional(Eq("foo.bar"))); + base::BigEndianReader reader1(dns_name.c_str(), dns_name.size()); + EXPECT_THAT(DnsDomainToString(reader1), testing::Optional(Eq("foo.bar"))); + + dns_name += "\002uk"; + EXPECT_THAT(DnsDomainToString(dns_name), testing::Optional(Eq("foo.bar.uk"))); + base::BigEndianReader reader2(dns_name.c_str(), dns_name.size()); + EXPECT_THAT(DnsDomainToString(reader2), testing::Optional(Eq("foo.bar.uk"))); + + dns_name += '\0'; + EXPECT_THAT(DnsDomainToString(dns_name), testing::Optional(Eq("foo.bar.uk"))); + base::BigEndianReader reader3(dns_name.c_str(), dns_name.size()); + EXPECT_THAT(DnsDomainToString(reader3), testing::Optional(Eq("foo.bar.uk"))); +} + +TEST_F(DNSUtilTest, DnsDomainToStringShouldHandleEmpty) { + std::string dns_name; + + EXPECT_THAT(DnsDomainToString(dns_name), testing::Optional(Eq(""))); + base::BigEndianReader reader(dns_name.c_str(), dns_name.size()); + EXPECT_THAT(DnsDomainToString(reader), testing::Optional(Eq(""))); + + dns_name += '\0'; + + EXPECT_THAT(DnsDomainToString(dns_name), testing::Optional(Eq(""))); + base::BigEndianReader reader1(dns_name.c_str(), dns_name.size()); + EXPECT_THAT(DnsDomainToString(reader1), testing::Optional(Eq(""))); +} + +TEST_F(DNSUtilTest, DnsDomainToStringShouldRejectEmptyIncomplete) { + std::string dns_name; + + EXPECT_THAT(DnsDomainToString(dns_name, false /* require_complete */), + testing::Optional(Eq(""))); + base::BigEndianReader reader(dns_name.c_str(), dns_name.size()); + EXPECT_THAT(DnsDomainToString(reader, false /* require_complete */), + testing::Optional(Eq(""))); + + EXPECT_EQ(DnsDomainToString(dns_name, true /* require_complete */), + base::nullopt); + base::BigEndianReader reader1(dns_name.c_str(), dns_name.size()); + EXPECT_EQ(DnsDomainToString(reader1, true /* require_complete */), + base::nullopt); +} + +// Test `require_complete` functionality given an input with terminating zero- +// length label. +TEST_F(DNSUtilTest, DnsDomainToStringComplete) { + std::string dns_name("\003foo\004test"); + dns_name += '\0'; + + EXPECT_THAT(DnsDomainToString(dns_name, false /* require_complete */), + testing::Optional(Eq("foo.test"))); + base::BigEndianReader reader(dns_name.c_str(), dns_name.size()); + EXPECT_THAT(DnsDomainToString(reader, false /* require_complete */), + testing::Optional(Eq("foo.test"))); + + EXPECT_THAT(DnsDomainToString(dns_name, true /* require_complete */), + testing::Optional(Eq("foo.test"))); + base::BigEndianReader reader1(dns_name.c_str(), dns_name.size()); + EXPECT_THAT(DnsDomainToString(reader1, true /* require_complete */), + testing::Optional(Eq("foo.test"))); +} + +// Test `require_complete` functionality given an input without terminating +// zero-length label. +TEST_F(DNSUtilTest, DnsDomainToStringNotComplete) { + std::string dns_name("\003boo\004test"); + + EXPECT_THAT(DnsDomainToString(dns_name, false /* require_complete */), + testing::Optional(Eq("boo.test"))); + base::BigEndianReader reader(dns_name.c_str(), dns_name.size()); + EXPECT_THAT(DnsDomainToString(reader, false /* require_complete */), + testing::Optional(Eq("boo.test"))); + + EXPECT_EQ(DnsDomainToString(dns_name, true /* require_complete */), + base::nullopt); + base::BigEndianReader reader2(dns_name.c_str(), dns_name.size()); + EXPECT_EQ(DnsDomainToString(reader2, true /* require_complete */), + base::nullopt); +} + +TEST_F(DNSUtilTest, DnsDomainToStringShouldRejectEmptyWhenRequiringComplete) { + std::string dns_name; + + EXPECT_THAT(DnsDomainToString(dns_name, false /* require_complete */), + testing::Optional(Eq(""))); + base::BigEndianReader reader(dns_name.c_str(), dns_name.size()); + EXPECT_THAT(DnsDomainToString(reader, false /* require_complete */), + testing::Optional(Eq(""))); + + EXPECT_EQ(DnsDomainToString(dns_name, true /* require_complete */), + base::nullopt); + base::BigEndianReader reader1(dns_name.c_str(), dns_name.size()); + EXPECT_EQ(DnsDomainToString(reader1, true /* require_complete */), + base::nullopt); + + dns_name += '\0'; + + EXPECT_THAT(DnsDomainToString(dns_name, true /* require_complete */), + testing::Optional(Eq(""))); + base::BigEndianReader reader2(dns_name.c_str(), dns_name.size()); + EXPECT_THAT(DnsDomainToString(reader2, true /* require_complete */), + testing::Optional(Eq(""))); +} + +TEST_F(DNSUtilTest, DnsDomainToStringShouldRejectCompression) { + std::string dns_name = CreateNamePointer(152); + + EXPECT_EQ(DnsDomainToString(dns_name), base::nullopt); + base::BigEndianReader reader(dns_name.c_str(), dns_name.size()); + EXPECT_EQ(DnsDomainToString(reader), base::nullopt); + + dns_name = "\005hello"; + dns_name += CreateNamePointer(152); + + EXPECT_EQ(DnsDomainToString(dns_name), base::nullopt); + base::BigEndianReader reader1(dns_name.c_str(), dns_name.size()); + EXPECT_EQ(DnsDomainToString(reader1), base::nullopt); +} + +// Test that extra input past the terminating zero-length label are ignored. +TEST_F(DNSUtilTest, DnsDomainToStringShouldHandleExcessInput) { + std::string dns_name("\004cool\004name\004test"); + dns_name += '\0'; + dns_name += "blargh!"; + + EXPECT_THAT(DnsDomainToString(dns_name), + testing::Optional(Eq("cool.name.test"))); + base::BigEndianReader reader(dns_name.c_str(), dns_name.size()); + EXPECT_THAT(DnsDomainToString(reader), + testing::Optional(Eq("cool.name.test"))); + + dns_name = "\002hi"; + dns_name += '\0'; + dns_name += "goodbye"; + + EXPECT_THAT(DnsDomainToString(dns_name), testing::Optional(Eq("hi"))); + base::BigEndianReader reader1(dns_name.c_str(), dns_name.size()); + EXPECT_THAT(DnsDomainToString(reader1), testing::Optional(Eq("hi"))); +} + +// Test that input is malformed if it ends mid label. +TEST_F(DNSUtilTest, DnsDomainToStringShouldRejectTruncatedNames) { + std::string dns_name = "\07cheese"; + + EXPECT_EQ(DnsDomainToString(dns_name), base::nullopt); + base::BigEndianReader reader(dns_name.c_str(), dns_name.size()); + EXPECT_EQ(DnsDomainToString(reader), base::nullopt); + + dns_name = "\006cheesy\05test"; + + EXPECT_EQ(DnsDomainToString(dns_name), base::nullopt); + base::BigEndianReader reader1(dns_name.c_str(), dns_name.size()); + EXPECT_EQ(DnsDomainToString(reader1), base::nullopt); +} + +TEST_F(DNSUtilTest, DnsDomainToStringShouldHandleLongSingleLabel) { + std::string dns_name(1, static_cast(dns_protocol::kMaxLabelLength)); + for (int i = 0; i < dns_protocol::kMaxLabelLength; ++i) { + dns_name += 'a'; + } + + EXPECT_NE(DnsDomainToString(dns_name), base::nullopt); + base::BigEndianReader reader(dns_name.c_str(), dns_name.size()); + EXPECT_NE(DnsDomainToString(reader), base::nullopt); +} + +TEST_F(DNSUtilTest, DnsDomainToStringShouldHandleLongSecondLabel) { + std::string dns_name("\003foo"); + dns_name += static_cast(dns_protocol::kMaxLabelLength); + for (int i = 0; i < dns_protocol::kMaxLabelLength; ++i) { + dns_name += 'a'; + } + + EXPECT_NE(DnsDomainToString(dns_name), base::nullopt); + base::BigEndianReader reader(dns_name.c_str(), dns_name.size()); + EXPECT_NE(DnsDomainToString(reader), base::nullopt); +} + +TEST_F(DNSUtilTest, DnsDomainToStringShouldRejectTooLongSingleLabel) { + std::string dns_name(1, static_cast(dns_protocol::kMaxLabelLength)); + for (int i = 0; i < dns_protocol::kMaxLabelLength + 1; ++i) { + dns_name += 'a'; + } + + EXPECT_EQ(DnsDomainToString(dns_name), base::nullopt); + base::BigEndianReader reader(dns_name.c_str(), dns_name.size()); + EXPECT_EQ(DnsDomainToString(reader), base::nullopt); +} + +TEST_F(DNSUtilTest, DnsDomainToStringShouldRejectTooLongSecondLabel) { + std::string dns_name("\003foo"); + dns_name += static_cast(dns_protocol::kMaxLabelLength); + for (int i = 0; i < dns_protocol::kMaxLabelLength + 1; ++i) { + dns_name += 'a'; + } + + EXPECT_EQ(DnsDomainToString(dns_name), base::nullopt); + base::BigEndianReader reader(dns_name.c_str(), dns_name.size()); + EXPECT_EQ(DnsDomainToString(reader), base::nullopt); +} + +#if CHAR_MIN < 0 +TEST_F(DNSUtilTest, DnsDomainToStringShouldRejectCharMinLabels) { + ASSERT_GT(static_cast(CHAR_MIN), dns_protocol::kMaxLabelLength); - // It should cope with a lack of root label. - EXPECT_THAT(DnsDomainToString("\003foo\003bar"), - testing::Optional(Eq("foo.bar"))); + std::string dns_name; + dns_name += base::checked_cast(CHAR_MIN); + + // Wherever possible, make the name otherwise valid. + if (static_cast(CHAR_MIN) < UINT8_MAX) { + for (uint8_t i = 0; i < static_cast(CHAR_MIN); ++i) { + dns_name += 'a'; + } + } + + EXPECT_EQ(DnsDomainToString(dns_name), base::nullopt); + base::BigEndianReader reader(dns_name.c_str(), dns_name.size()); + EXPECT_EQ(DnsDomainToString(reader), base::nullopt); +} +#endif // if CHAR_MIN < 0 + +TEST_F(DNSUtilTest, DnsDomainToStringShouldHandleLongName) { + std::string dns_name; + for (int i = 0; i < dns_protocol::kMaxNameLength - 1; + i += (dns_protocol::kMaxLabelLength + 1)) { + int label_size = std::min(dns_protocol::kMaxNameLength - 2 - i, + dns_protocol::kMaxLabelLength); + dns_name += static_cast(label_size); + for (int j = 0; j < label_size; ++j) { + dns_name += 'a'; + } + } + ASSERT_EQ(dns_name.size(), + static_cast(dns_protocol::kMaxNameLength - 1)); + + EXPECT_NE(DnsDomainToString(dns_name), base::nullopt); + base::BigEndianReader reader(dns_name.c_str(), dns_name.size()); + EXPECT_NE(DnsDomainToString(reader), base::nullopt); +} + +TEST_F(DNSUtilTest, DnsDomainToStringShouldRejectTooLongName) { + std::string dns_name; + for (int i = 0; i < dns_protocol::kMaxNameLength; + i += (dns_protocol::kMaxLabelLength + 1)) { + int label_size = std::min(dns_protocol::kMaxNameLength - 1 - i, + dns_protocol::kMaxLabelLength); + dns_name += static_cast(label_size); + for (int j = 0; j < label_size; ++j) { + dns_name += 'a'; + } + } + ASSERT_EQ(dns_name.size(), static_cast(dns_protocol::kMaxNameLength)); + + EXPECT_EQ(DnsDomainToString(dns_name), base::nullopt); + base::BigEndianReader reader(dns_name.c_str(), dns_name.size()); + EXPECT_EQ(DnsDomainToString(reader), base::nullopt); +} + +TEST_F(DNSUtilTest, DnsDomainToStringShouldHandleLongCompleteName) { + std::string dns_name; + for (int i = 0; i < dns_protocol::kMaxNameLength - 1; + i += (dns_protocol::kMaxLabelLength + 1)) { + int label_size = std::min(dns_protocol::kMaxNameLength - 2 - i, + dns_protocol::kMaxLabelLength); + dns_name += static_cast(label_size); + for (int j = 0; j < label_size; ++j) { + dns_name += 'a'; + } + } + dns_name += '\0'; + ASSERT_EQ(dns_name.size(), static_cast(dns_protocol::kMaxNameLength)); + + EXPECT_NE(DnsDomainToString(dns_name), base::nullopt); + base::BigEndianReader reader(dns_name.c_str(), dns_name.size()); + EXPECT_NE(DnsDomainToString(reader), base::nullopt); +} + +TEST_F(DNSUtilTest, DnsDomainToStringShouldRejectTooLongCompleteName) { + std::string dns_name; + for (int i = 0; i < dns_protocol::kMaxNameLength; + i += (dns_protocol::kMaxLabelLength + 1)) { + int label_size = std::min(dns_protocol::kMaxNameLength - 1 - i, + dns_protocol::kMaxLabelLength); + dns_name += static_cast(label_size); + for (int j = 0; j < label_size; ++j) { + dns_name += 'a'; + } + } + dns_name += '\0'; + ASSERT_EQ(dns_name.size(), + static_cast(dns_protocol::kMaxNameLength + 1)); - // Invalid inputs should return nullopt. - EXPECT_EQ(DnsDomainToString(IncludeNUL("\x80")), base::nullopt); - EXPECT_EQ(DnsDomainToString("\x06"), base::nullopt); + EXPECT_EQ(DnsDomainToString(dns_name), base::nullopt); + base::BigEndianReader reader(dns_name.c_str(), dns_name.size()); + EXPECT_EQ(DnsDomainToString(reader), base::nullopt); } TEST_F(DNSUtilTest, IsValidDNSDomain) { diff --git a/net/dns/public/dns_protocol.h b/net/dns/public/dns_protocol.h index 3c1757e7264fe7..608b7eca785b81 100644 --- a/net/dns/public/dns_protocol.h +++ b/net/dns/public/dns_protocol.h @@ -114,6 +114,11 @@ static const uint16_t kMDnsClassMask = 0x7FFF; // to 255 octets or less. static const int kMaxNameLength = 255; +// RFC 1035, section 2.3.4: labels 63 octets or less. +// Section 3.1: Each label is represented as a one octet length field followed +// by that number of octets. +const int kMaxLabelLength = 63; + // RFC 1035, section 4.2.1: Messages carried by UDP are restricted to 512 // bytes (not counting the IP nor UDP headers). static const int kMaxUDPSize = 512;