Added DnsQuery/DnsResponse parsing checks.

Adds a check in the DnsQuery to ensure we don't parse extra bytes
from the buffer. Also adds a check in DnsResponse that ensures we
don't leak any uninitialized bytes if we allocated the buffer too
large, after constructing the DnsResponse from a DnsQuery.

Bug: 891521
Test: ./net_unittests
Change-Id: Ia0fd4c677be71efec55e452e5219c34c8112a5e5
Reviewed-on: https://chromium-review.googlesource.com/c/1263280
Commit-Queue: Matthew Denton <mpdenton@chromium.org>
Reviewed-by: Eric Orth <ericorth@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#600893}

net/dns/dns_query.cc

@@ -5,7 +5,9 @@
 #include "net/dns/dns_query.h"
 
 #include "base/big_endian.h"
+#include "base/logging.h"
 #include "base/memory/ptr_util.h"
+#include "base/numerics/safe_conversions.h"
 #include "base/sys_byteorder.h"
 #include "net/base/io_buffer.h"
 #include "net/dns/dns_protocol.h"
@@ -84,15 +86,16 @@ std::unique_ptr<DnsQuery> DnsQuery::CloneWithNewId(uint16_t id) const {
   return base::WrapUnique(new DnsQuery(*this, id));
 }
 
-bool DnsQuery::Parse() {
+bool DnsQuery::Parse(size_t valid_bytes) {
   if (io_buffer_ == nullptr || io_buffer_->data() == nullptr) {
     return false;
   }
+  CHECK(valid_bytes <= base::checked_cast<size_t>(io_buffer_->size()));
   // We should only parse the query once if the query is constructed from a raw
   // buffer. If we have constructed the query from data or the query is already
   // parsed after constructed from a raw buffer, |header_| is not null.
   DCHECK(header_ == nullptr);
-  base::BigEndianReader reader(io_buffer_->data(), io_buffer_->size());
+  base::BigEndianReader reader(io_buffer_->data(), valid_bytes);
   dns_protocol::Header header;
   if (!ReadHeader(&reader, &header)) {
     return false;

net/dns/dns_query.h

@@ -54,7 +54,13 @@ class NET_EXPORT_PRIVATE DnsQuery {
   // Returns true and populates the query if the internally stored raw packet
   // can be parsed. This should only be called when DnsQuery is constructed from
   // the raw buffer.
-  bool Parse();
+  // |valid_bytes| indicates the number of initialized bytes in the raw buffer.
+  // E.g. if the buffer holds a packet received from the network, the buffer may
+  // be allocated with the maximum size of a UDP packet, but |valid_bytes|
+  // indicates the number of bytes actually received from the network. If the
+  // parsing requires reading more than the number of initialized bytes, this
+  // method fails and returns false.
+  bool Parse(size_t valid_bytes);
 
   // DnsQuery field accessors.
   uint16_t id() const;

net/dns/dns_query_parse_fuzzer.cc

@@ -15,6 +15,6 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) {
   auto packet = base::MakeRefCounted<net::IOBufferWithSize>(size);
   memcpy(packet->data(), data, size);
   auto out = std::make_unique<net::DnsQuery>(packet);
-  out->Parse();
+  out->Parse(size);
   return 0;
 }

net/dns/dns_query_unittest.cc

@@ -27,7 +27,7 @@ bool ParseAndCreateDnsQueryFromRawPacket(const uint8_t* data,
   auto packet = base::MakeRefCounted<IOBufferWithSize>(length);
   memcpy(packet->data(), data, length);
   out->reset(new DnsQuery(packet));
-  return (*out)->Parse();
+  return (*out)->Parse(length);
 }
 
 // This includes \0 at the end.

net/dns/dns_response.cc

@@ -9,6 +9,8 @@
 #include <vector>
 
 #include "base/big_endian.h"
+#include "base/logging.h"
+#include "base/numerics/safe_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/sys_byteorder.h"
 #include "net/base/address_list.h"
@@ -227,6 +229,9 @@ DnsResponse::DnsResponse(
     io_buffer_size_ = 0;
     return;
   }
+  // Ensure we don't have any remaining uninitialized bytes in the buffer.
+  DCHECK(!writer.remaining());
+  memset(writer.ptr(), 0, writer.remaining());
   if (has_query) {
     InitParse(io_buffer_size_, query.value());
   } else {
@@ -257,7 +262,7 @@ DnsResponse::~DnsResponse() = default;
 
 bool DnsResponse::InitParse(size_t nbytes, const DnsQuery& query) {
   // Response includes query, it should be at least that size.
-  if (nbytes < static_cast<size_t>(query.io_buffer()->size()) ||
+  if (nbytes < base::checked_cast<size_t>(query.io_buffer()->size()) ||
       nbytes > io_buffer_size_) {
     return false;
   }

net/dns/dns_response_fuzzer.cc

@@ -18,7 +18,7 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) {
   memcpy(packet->data(), data, size);
   base::Optional<net::DnsQuery> query;
   query.emplace(packet);
-  if (!query->Parse()) {
+  if (!query->Parse(size)) {
     return 0;
   }
   net::DnsResponse response(query->id(), true /* is_authoritative */,

net/dns/dns_response_unittest.cc

@@ -732,7 +732,7 @@ TEST(DnsResponseWriteTest,
   // buf contains 10 extra zero bytes.
   base::Optional<DnsQuery> query;
   query.emplace(buf);
-  query->Parse();
+  query->Parse(buf_size);
   net::DnsResourceRecord answer;
   answer.name = dotted_name;
   answer.type = dns_protocol::kTypeA;