Implement DNS padding for DoH

All DoH queries will be padded to the next multiple of 128 bytes.
Implemented as a feature of DnsQuery to add the padding to the OPT
record, rather than just having it specified in the |opt_rdata| passed
to the DnsQuery constructor because adding the correct amount of padding
requires knowledge of the DnsQuery being constructed.

Bug: 967848
Change-Id: I6be13c24c770e80d06ac3f7e988cdf814ce5c00e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1614454
Commit-Queue: Eric Orth <ericorth@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#665935}

net/dns/dns_query.cc

@@ -4,10 +4,13 @@
 
 #include "net/dns/dns_query.h"
 
+#include <utility>
+
 #include "base/big_endian.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "base/numerics/safe_conversions.h"
+#include "base/optional.h"
 #include "base/sys_byteorder.h"
 #include "net/base/io_buffer.h"
 #include "net/dns/dns_util.h"
@@ -28,10 +31,63 @@ static const size_t kOptRRFixedSize = 11;
 // TODO(robpercival): Determine a good value for this programmatically.
 const uint16_t kMaxUdpPayloadSize = 4096;
 
+size_t QuestionSize(size_t qname_size) {
+  // QNAME + QTYPE + QCLASS
+  return qname_size + sizeof(uint16_t) + sizeof(uint16_t);
+}
+
+// Buffer size of Opt record for |rdata| (does not include Opt record or RData
+// added for padding).
 size_t OptRecordSize(const OptRecordRdata* rdata) {
   return rdata == nullptr ? 0 : kOptRRFixedSize + rdata->buf().size();
 }
 
+// Padding size includes Opt header for the padding.  Does not include OptRecord
+// header (kOptRRFixedSize) even when added just for padding.
+size_t DeterminePaddingSize(size_t unpadded_size,
+                            DnsQuery::PaddingStrategy padding_strategy) {
+  switch (padding_strategy) {
+    case DnsQuery::PaddingStrategy::NONE:
+      return 0;
+    case DnsQuery::PaddingStrategy::BLOCK_LENGTH_128:
+      size_t padding_size = OptRecordRdata::Opt::kHeaderSize;
+      size_t remainder = (padding_size + unpadded_size) % 128;
+      padding_size += (128 - remainder) % 128;
+      DCHECK_EQ((unpadded_size + padding_size) % 128, 0u);
+      return padding_size;
+  }
+}
+
+base::Optional<OptRecordRdata> AddPaddingIfNecessary(
+    const OptRecordRdata* opt_rdata,
+    DnsQuery::PaddingStrategy padding_strategy,
+    size_t no_opt_buffer_size) {
+  // If no input OPT record rdata and no padding, no OPT record rdata needed.
+  if (!opt_rdata && padding_strategy == DnsQuery::PaddingStrategy::NONE)
+    return base::nullopt;
+
+  OptRecordRdata merged_opt_rdata;
+  if (opt_rdata)
+    merged_opt_rdata.AddOpts(*opt_rdata);
+
+  size_t unpadded_size = no_opt_buffer_size + OptRecordSize(&merged_opt_rdata);
+  size_t padding_size = DeterminePaddingSize(unpadded_size, padding_strategy);
+
+  if (padding_size > 0) {
+    // |opt_rdata| must not already contain padding if DnsQuery is to add
+    // padding.
+    DCHECK(!merged_opt_rdata.ContainsOptCode(dns_protocol::kEdnsPadding));
+    // OPT header is the minimum amount of padding.
+    DCHECK(padding_size >= OptRecordRdata::Opt::kHeaderSize);
+
+    merged_opt_rdata.AddOpt(OptRecordRdata::Opt(
+        dns_protocol::kEdnsPadding,
+        std::string(padding_size - OptRecordRdata::Opt::kHeaderSize, 0)));
+  }
+
+  return merged_opt_rdata;
+}
+
 }  // namespace
 
 // DNS query consists of a 12-byte header followed by a question section.
@@ -41,12 +97,20 @@ size_t OptRecordSize(const OptRecordRdata* rdata) {
 DnsQuery::DnsQuery(uint16_t id,
                    const base::StringPiece& qname,
                    uint16_t qtype,
-                   const OptRecordRdata* opt_rdata)
-    : qname_size_(qname.size()),
-      io_buffer_(base::MakeRefCounted<IOBufferWithSize>(
-          kHeaderSize + question_size() + OptRecordSize(opt_rdata))),
-      header_(reinterpret_cast<dns_protocol::Header*>(io_buffer_->data())) {
+                   const OptRecordRdata* opt_rdata,
+                   PaddingStrategy padding_strategy)
+    : qname_size_(qname.size()) {
   DCHECK(!DNSDomainToString(qname).empty());
+
+  size_t buffer_size = kHeaderSize + QuestionSize(qname_size_);
+  base::Optional<OptRecordRdata> merged_opt_rdata =
+      AddPaddingIfNecessary(opt_rdata, padding_strategy, buffer_size);
+  if (merged_opt_rdata)
+    buffer_size += OptRecordSize(&merged_opt_rdata.value());
+
+  io_buffer_ = base::MakeRefCounted<IOBufferWithSize>(buffer_size);
+
+  header_ = reinterpret_cast<dns_protocol::Header*>(io_buffer_->data());
   *header_ = {};
   header_->id = base::HostToNet16(id);
   header_->flags = base::HostToNet16(dns_protocol::kFlagRD);
@@ -59,7 +123,9 @@ DnsQuery::DnsQuery(uint16_t id,
   writer.WriteU16(qtype);
   writer.WriteU16(dns_protocol::kClassIN);
 
-  if (opt_rdata != nullptr) {
+  if (merged_opt_rdata) {
+    DCHECK(!merged_opt_rdata.value().opts().empty());
+
     header_->arcount = base::HostToNet16(1);
     // Write OPT pseudo-resource record.
     writer.WriteU8(0);                       // empty domain name (root domain)
@@ -71,9 +137,11 @@ DnsQuery::DnsQuery(uint16_t id,
     // TODO(robpercival): Set "DNSSEC OK" flag if/when DNSSEC is supported:
     // https://tools.ietf.org/html/rfc3225#section-3
     writer.WriteU16(0);  // flags
+
     // rdata
-    writer.WriteU16(opt_rdata->buf().size());  // rdata length
-    writer.WriteBytes(opt_rdata->buf().data(), opt_rdata->buf().size());
+    writer.WriteU16(merged_opt_rdata.value().buf().size());  // rdata length
+    writer.WriteBytes(merged_opt_rdata.value().buf().data(),
+                      merged_opt_rdata.value().buf().size());
   }
 }
 
@@ -140,7 +208,12 @@ uint16_t DnsQuery::qtype() const {
 }
 
 base::StringPiece DnsQuery::question() const {
-  return base::StringPiece(io_buffer_->data() + kHeaderSize, question_size());
+  return base::StringPiece(io_buffer_->data() + kHeaderSize,
+                           QuestionSize(qname_size_));
+}
+
+size_t DnsQuery::question_size() const {
+  return QuestionSize(qname_size_);
 }
 
 void DnsQuery::set_flags(uint16_t flags) {

net/dns/dns_query.h

@@ -9,6 +9,7 @@
 #include <stdint.h>
 
 #include <memory>
+#include <string>
 
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
@@ -32,19 +33,31 @@ class IOBufferWithSize;
 // Represents on-the-wire DNS query message as an object.
 class NET_EXPORT_PRIVATE DnsQuery {
  public:
+  enum class PaddingStrategy {
+    // Query will not be padded. Recommended strategy when query will not be
+    // encrypted.
+    NONE,
+
+    // Query will be padded to the next multiple of 128 octets. Recommended
+    // strategy (per RFC 8467) when query will be encrypted, e.g. through
+    // DNS-over-HTTPS.
+    BLOCK_LENGTH_128,
+  };
+
   // Constructs a query message from |qname| which *MUST* be in a valid
   // DNS name format, and |qtype|. The qclass is set to IN.
-  // If opt_rdata is not null, an OPT record will be added to the "Additional"
+  // If |opt_rdata| is not null, an OPT record will be added to the "Additional"
   // section of the query.
   DnsQuery(uint16_t id,
            const base::StringPiece& qname,
            uint16_t qtype,
-           const OptRecordRdata* opt_rdata = nullptr);
+           const OptRecordRdata* opt_rdata = nullptr,
+           PaddingStrategy padding_strategy = PaddingStrategy::NONE);
 
   // Constructs an empty query from a raw packet in |buffer|. If the raw packet
   // represents a valid DNS query in the wire format (RFC 1035), Parse() will
   // populate the empty query.
-  DnsQuery(scoped_refptr<IOBufferWithSize> buffer);
+  explicit DnsQuery(scoped_refptr<IOBufferWithSize> buffer);
 
   ~DnsQuery();
 
@@ -72,10 +85,7 @@ class NET_EXPORT_PRIVATE DnsQuery {
   base::StringPiece question() const;
 
   // Returns the size of the question section.
-  size_t question_size() const {
-    // QNAME + QTYPE + QCLASS
-    return qname_size_ + sizeof(uint16_t) + sizeof(uint16_t);
-  }
+  size_t question_size() const;
 
   // IOBuffer accessor to be used for writing out the query. The buffer has
   // the same byte layout as the DNS query wire format.

net/dns/dns_query_unittest.cc

@@ -4,8 +4,11 @@
 
 #include "net/dns/dns_query.h"
 
+#include <tuple>
+
 #include "base/stl_util.h"
 #include "net/base/io_buffer.h"
+#include "net/dns/dns_util.h"
 #include "net/dns/public/dns_protocol.h"
 #include "net/dns/record_rdata.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -38,6 +41,7 @@ const char kQNameData[] =
     "example"
     "\x03"
     "com";
+const base::StringPiece kQName(kQNameData, sizeof(kQNameData));
 
 TEST(DnsQueryTest, Constructor) {
   // This includes \0 at the end.
@@ -56,11 +60,10 @@ TEST(DnsQueryTest, Constructor) {
       0x00, 0x01,  // QCLASS: IN class.
   };
 
-  base::StringPiece qname(kQNameData, sizeof(kQNameData));
-  DnsQuery q1(0xbeef, qname, dns_protocol::kTypeA);
+  DnsQuery q1(0xbeef, kQName, dns_protocol::kTypeA);
   EXPECT_EQ(dns_protocol::kTypeA, q1.qtype());
   EXPECT_THAT(AsTuple(q1.io_buffer()), ElementsAreArray(query_data));
-  EXPECT_EQ(qname, q1.qname());
+  EXPECT_EQ(kQName, q1.qname());
 
   base::StringPiece question(reinterpret_cast<const char*>(query_data) + 12,
                              21);
@@ -117,6 +120,42 @@ TEST(DnsQueryTest, EDNS0) {
   EXPECT_EQ(question, q1.question());
 }
 
+TEST(DnsQueryTest, Block128Padding) {
+  DnsQuery query(46 /* id */, kQName, dns_protocol::kTypeAAAA,
+                 nullptr /* opt_rdata */,
+                 DnsQuery::PaddingStrategy::BLOCK_LENGTH_128);
+
+  // Query is expected to be short and fit in a single 128-byte padded block.
+  EXPECT_EQ(128, query.io_buffer()->size());
+
+  // Ensure created query still parses as expected.
+  DnsQuery parsed_query(query.io_buffer());
+  ASSERT_TRUE(parsed_query.Parse(query.io_buffer()->size()));
+  EXPECT_EQ(kQName, parsed_query.qname());
+  EXPECT_EQ(dns_protocol::kTypeAAAA, parsed_query.qtype());
+}
+
+TEST(DnsQueryTest, Block128Padding_LongName) {
+  std::string qname;
+  DNSDomainFromDot(
+      "really.long.domain.name.that.will.push.us.past.the.128.byte.block.size."
+      "because.it.would.be.nice.to.test.something.realy.long.like.that.com",
+      &qname);
+  DnsQuery query(112 /* id */, qname, dns_protocol::kTypeAAAA,
+                 nullptr /* opt_rdata */,
+                 DnsQuery::PaddingStrategy::BLOCK_LENGTH_128);
+
+  // Query is expected to pad into a second 128-byte block.
+  EXPECT_EQ(256, query.io_buffer()->size());
+  EXPECT_EQ(qname, query.qname());
+
+  // Ensure created query still parses as expected.
+  DnsQuery parsed_query(query.io_buffer());
+  ASSERT_TRUE(parsed_query.Parse(query.io_buffer()->size()));
+  EXPECT_EQ(qname, parsed_query.qname());
+  EXPECT_EQ(dns_protocol::kTypeAAAA, parsed_query.qtype());
+}
+
 TEST(DnsQueryParseTest, SingleQuestionForTypeARecord) {
   const uint8_t query_data[] = {
       0x12, 0x34,  // ID

net/dns/dns_response.cc

@@ -363,9 +363,10 @@ DnsResponse::DnsResponse(const void* data, size_t length, size_t answer_offset)
 DnsResponse::~DnsResponse() = default;
 
 bool DnsResponse::InitParse(size_t nbytes, const DnsQuery& query) {
-  // Response includes query, it should be at least that size.
-  if (nbytes < base::checked_cast<size_t>(query.io_buffer()->size()) ||
-      nbytes > io_buffer_size_) {
+  const base::StringPiece question = query.question();
+
+  // Response includes question, it should be at least that size.
+  if (nbytes < kHeaderSize + question.size() || nbytes > io_buffer_size_) {
     return false;
   }
 
@@ -382,7 +383,6 @@ bool DnsResponse::InitParse(size_t nbytes, const DnsQuery& query) {
     return false;
 
   // Match the question section.
-  const base::StringPiece question = query.question();
   if (question !=
       base::StringPiece(io_buffer_->data() + kHeaderSize, question.size())) {
     return false;

net/dns/dns_transaction.cc

@@ -1005,7 +1005,8 @@ class DnsTransactionImpl : public DnsTransaction,
     uint16_t id = session_->NextQueryId();
     std::unique_ptr<DnsQuery> query;
     if (attempts_.empty()) {
-      query.reset(new DnsQuery(id, qnames_.front(), qtype_, opt_rdata_));
+      query.reset(new DnsQuery(id, qnames_.front(), qtype_, opt_rdata_,
+                               DnsQuery::PaddingStrategy::BLOCK_LENGTH_128));
     } else {
       query = attempts_[0]->GetQuery()->CloneWithNewId(id);
     }

net/dns/public/dns_protocol.h

@@ -151,6 +151,11 @@ static const uint8_t kRcodeNXDOMAIN = 3;
 static const uint8_t kRcodeNOTIMP = 4;
 static const uint8_t kRcodeREFUSED = 5;
 
+// DNS EDNS(0) option codes (OPT)
+//
+// https://www.iana.org/assignments/dns-parameters/dns-parameters.xhtml#dns-parameters-11
+static const uint16_t kEdnsPadding = 12;
+
 // DNS header flags.
 //
 // https://www.iana.org/assignments/dns-parameters/dns-parameters.xhtml#dns-parameters-12

net/dns/record_rdata.cc

@@ -4,6 +4,7 @@
 
 #include "net/dns/record_rdata.h"
 
+#include <algorithm>
 #include <numeric>
 
 #include "base/big_endian.h"
@@ -14,8 +15,6 @@ namespace net {
 
 static const size_t kSrvRecordMinimumSize = 6;
 
-RecordRdata::RecordRdata() = default;
-
 bool RecordRdata::HasValidSize(const base::StringPiece& data, uint16_t type) {
   switch (type) {
     case dns_protocol::kTypeSRV:
@@ -288,8 +287,12 @@ bool NsecRecordRdata::GetBit(unsigned i) const {
 
 OptRecordRdata::OptRecordRdata() = default;
 
+OptRecordRdata::OptRecordRdata(OptRecordRdata&& other) = default;
+
 OptRecordRdata::~OptRecordRdata() = default;
 
+OptRecordRdata& OptRecordRdata::operator=(OptRecordRdata&& other) = default;
+
 // static
 std::unique_ptr<OptRecordRdata> OptRecordRdata::Create(
     const base::StringPiece& data,
@@ -340,6 +343,17 @@ void OptRecordRdata::AddOpt(const Opt& opt) {
   opts_.push_back(opt);
 }
 
+void OptRecordRdata::AddOpts(const OptRecordRdata& other) {
+  buf_.insert(buf_.end(), other.buf_.begin(), other.buf_.end());
+  opts_.insert(opts_.end(), other.opts_.begin(), other.opts_.end());
+}
+
+bool OptRecordRdata::ContainsOptCode(uint16_t opt_code) const {
+  return std::any_of(
+      opts_.begin(), opts_.end(),
+      [=](const OptRecordRdata::Opt& opt) { return opt.code() == opt_code; });
+}
+
 OptRecordRdata::Opt::Opt(uint16_t code, base::StringPiece data) : code_(code) {
   data.CopyToString(&data_);
 }

net/dns/record_rdata.h

@@ -36,11 +36,6 @@ class NET_EXPORT RecordRdata {
 
   virtual bool IsEqual(const RecordRdata* other) const = 0;
   virtual uint16_t Type() const = 0;
-
- protected:
-  RecordRdata();
-
-  DISALLOW_COPY_AND_ASSIGN(RecordRdata);
 };
 
 // SRV record format (http://www.ietf.org/rfc/rfc2782.txt):
@@ -228,7 +223,7 @@ class NET_EXPORT_PRIVATE OptRecordRdata : public RecordRdata {
  public:
   class NET_EXPORT_PRIVATE Opt {
    public:
-    static const size_t kHeaderSize = 4;  // sizeof(code) + sizeof(size)
+    static constexpr size_t kHeaderSize = 4;  // sizeof(code) + sizeof(size)
 
     Opt(uint16_t code, base::StringPiece data);
 
@@ -245,7 +240,11 @@ class NET_EXPORT_PRIVATE OptRecordRdata : public RecordRdata {
   static const uint16_t kType = dns_protocol::kTypeOPT;
 
   OptRecordRdata();
+  OptRecordRdata(OptRecordRdata&& other);
   ~OptRecordRdata() override;
+
+  OptRecordRdata& operator=(OptRecordRdata&& other);
+
   static std::unique_ptr<OptRecordRdata> Create(const base::StringPiece& data,
                                                 const DnsRecordParser& parser);
   bool IsEqual(const RecordRdata* other) const override;
@@ -256,6 +255,11 @@ class NET_EXPORT_PRIVATE OptRecordRdata : public RecordRdata {
   const std::vector<Opt>& opts() const { return opts_; }
   void AddOpt(const Opt& opt);
 
+  // Add all Opts from |other| to |this|.
+  void AddOpts(const OptRecordRdata& other);
+
+  bool ContainsOptCode(uint16_t opt_code) const;
+
  private:
   std::vector<Opt> opts_;
   std::vector<char> buf_;

net/url_request/http_with_dns_over_https_unittest.cc

@@ -2,12 +2,16 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/big_endian.h"
 #include "base/bind.h"
+#include "base/logging.h"
+#include "base/memory/scoped_refptr.h"
 #include "net/base/privacy_mode.h"
 #include "net/base/proxy_server.h"
 #include "net/dns/context_host_resolver.h"
 #include "net/dns/dns_client.h"
 #include "net/dns/dns_config.h"
+#include "net/dns/dns_query.h"
 #include "net/dns/dns_transaction.h"
 #include "net/dns/host_resolver.h"
 #include "net/dns/host_resolver_manager.h"
@@ -87,19 +91,24 @@ class HttpWithDnsOverHttpsTest : public TestWithScopedTaskEnvironment {
       const test_server::HttpRequest& request) {
     if (request.relative_url.compare("/dns_query") == 0) {
       doh_queries_served_++;
-      uint8_t id1 = request.content[0];
-      uint8_t id2 = request.content[1];
-      std::unique_ptr<test_server::BasicHttpResponse> http_response(
-          new test_server::BasicHttpResponse);
-      const uint8_t header_data[] = {
-          id1,  id2,   // - Same ID as before
-          0x81, 0x80,  // - Different flags, we'll look at this below
-          0x00, 0x01,  // - 1 question
-          0x00, 0x01,  // - 1 answer
-          0x00, 0x00,  // - No authority records
-          0x00, 0x00,  // - No additional records
-      };
-      std::string question = request.content.substr(kHeaderSize);
+
+      // Parse request content as a DnsQuery to access the question.
+      auto request_buffer =
+          base::MakeRefCounted<IOBufferWithSize>(request.content.size());
+      memcpy(request_buffer->data(), request.content.data(),
+             request.content.size());
+      DnsQuery query(std::move(request_buffer));
+      EXPECT_TRUE(query.Parse(request.content.size()));
+
+      char header_data[kHeaderSize];
+      base::BigEndianWriter header_writer(header_data, kHeaderSize);
+      header_writer.WriteU16(query.id());  // Same ID as before
+      char flags[] = {0x81, 0x80};
+      header_writer.WriteBytes(flags, 2);
+      header_writer.WriteU16(1);  // 1 question
+      header_writer.WriteU16(1);  // 1 answer
+      header_writer.WriteU16(0);  // No authority records
+      header_writer.WriteU16(0);  // No additional records
 
       const uint8_t answer_data[]{0xC0, 0x0C,  // - NAME
                                   0x00, 0x01,  // - TYPE
@@ -109,8 +118,12 @@ class HttpWithDnsOverHttpsTest : public TestWithScopedTaskEnvironment {
                                   0x00, 0x04,  // - RDLENGTH = 4 bytes
                                   0x7f, 0x00,  // - RDDATA, IP is 127.0.0.1
                                   0x00, 0x01};
+
+      std::unique_ptr<test_server::BasicHttpResponse> http_response(
+          new test_server::BasicHttpResponse);
       http_response->set_content(
-          std::string((char*)header_data, sizeof(header_data)) + question +
+          std::string(header_data, sizeof(header_data)) +
+          query.question().as_string() +
           std::string((char*)answer_data, sizeof(answer_data)));
       http_response->set_content_type("application/dns-message");
       return std::move(http_response);