Use DNS via TCP instead of UDP when DNS entropy low

Add a flag to DnsUdpTracker to signify "low entropy" (setting that flag
in non-test is TODO for a subsequent CL).  Rework DNS attempt code to
allow using TCP instead of UDP when that flag is set.

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

net/dns/dns_transaction.cc

@@ -1056,7 +1056,7 @@ class DnsTransactionImpl : public DnsTransaction,
         net_log_(net_log),
         qnames_initial_size_(0),
         attempts_count_(0),
-        had_tcp_attempt_(false),
+        had_tcp_retry_(false),
         resolve_context_(resolve_context),
         request_priority_(DEFAULT_PRIORITY) {
     DCHECK(session_.get());
@@ -1112,6 +1112,7 @@ class DnsTransactionImpl : public DnsTransaction,
  private:
   // Wrapper for the result of a DnsUDPAttempt.
   struct AttemptResult {
+    AttemptResult() = default;
     AttemptResult(int rv, const DnsAttempt* attempt)
         : rv(rv), attempt(attempt) {}
 
@@ -1202,15 +1203,10 @@ class DnsTransactionImpl : public DnsTransaction,
     }
 
     DCHECK_GT(config.nameservers.size(), 0u);
-    return MakeUDPAttempt();
+    return MakeClassicDnsAttempt();
   }
 
-  // Makes another attempt at the current name, |qnames_.front()|, using the
-  // next nameserver.
-  AttemptResult MakeUDPAttempt() {
-    DCHECK(!secure_);
-    size_t attempt_number = attempts_.size();
-
+  AttemptResult MakeClassicDnsAttempt() {
     uint16_t id = session_->NextQueryId();
     std::unique_ptr<DnsQuery> query;
     if (attempts_.empty()) {
@@ -1219,16 +1215,40 @@ class DnsTransactionImpl : public DnsTransaction,
       query = attempts_[0]->GetQuery()->CloneWithNewId(id);
     }
     DCHECK(dns_server_iterator_->AttemptAvailable());
-    size_t non_doh_server_index = dns_server_iterator_->GetNextAttemptIndex();
+    size_t server_index = dns_server_iterator_->GetNextAttemptIndex();
+
+    size_t attempt_number = attempts_.size();
+    AttemptResult result;
+    if (session_->udp_tracker()->low_entropy()) {
+      result = MakeTcpAttempt(server_index, std::move(query));
+    } else {
+      result = MakeUdpAttempt(server_index, std::move(query));
+    }
+
+    if (result.rv == ERR_IO_PENDING) {
+      base::TimeDelta timeout = resolve_context_->NextClassicTimeout(
+          server_index, attempt_number, session_.get());
+      timer_.Start(FROM_HERE, timeout, this, &DnsTransactionImpl::OnTimeout);
+    }
+
+    return result;
+  }
+
+  // Makes another attempt at the current name, |qnames_.front()|, using the
+  // next nameserver.
+  AttemptResult MakeUdpAttempt(size_t server_index,
+                               std::unique_ptr<DnsQuery> query) {
+    DCHECK(!secure_);
+    size_t attempt_number = attempts_.size();
 
     std::unique_ptr<DnsSession::SocketLease> lease =
-        session_->AllocateSocket(non_doh_server_index, net_log_.source());
+        session_->AllocateSocket(server_index, net_log_.source());
 
     bool got_socket = !!lease.get();
 
     DnsUDPAttempt* attempt =
-        new DnsUDPAttempt(non_doh_server_index, std::move(lease),
-                          std::move(query), session_->udp_tracker());
+        new DnsUDPAttempt(server_index, std::move(lease), std::move(query),
+                          session_->udp_tracker());
 
     attempts_.push_back(base::WrapUnique(attempt));
     ++attempts_count_;
@@ -1242,11 +1262,6 @@ class DnsTransactionImpl : public DnsTransaction,
     int rv = attempt->Start(base::BindOnce(
         &DnsTransactionImpl::OnAttemptComplete, base::Unretained(this),
         attempt_number, true /* record_rtt */, base::TimeTicks::Now()));
-    if (rv == ERR_IO_PENDING) {
-      base::TimeDelta timeout = resolve_context_->NextClassicTimeout(
-          non_doh_server_index, attempt_number, session_.get());
-      timer_.Start(FROM_HERE, timeout, this, &DnsTransactionImpl::OnTimeout);
-    }
     return AttemptResult(rv, attempt);
   }
 
@@ -1272,25 +1287,42 @@ class DnsTransactionImpl : public DnsTransaction,
     return AttemptResult(rv, attempts_.back().get());
   }
 
-  AttemptResult MakeTCPAttempt(const DnsAttempt* previous_attempt) {
-    DCHECK(!secure_);
+  AttemptResult RetryUdpAttemptAsTcp(const DnsAttempt* previous_attempt) {
     DCHECK(previous_attempt);
-    DCHECK(!had_tcp_attempt_);
+    DCHECK(!had_tcp_retry_);
 
-    size_t server_index = previous_attempt->server_index();
+    // Only allow a single TCP retry per query.
+    had_tcp_retry_ = true;
 
-    std::unique_ptr<StreamSocket> socket(
-        session_->CreateTCPSocket(server_index, net_log_.source()));
-
-    // TODO(szym): Reuse the same id to help the server?
-    uint16_t id = session_->NextQueryId();
+    size_t server_index = previous_attempt->server_index();
+    // Use a new query ID instead of reusing the same one from the UDP attempt.
+    // RFC5452, section 9.2 requires an unpredictable ID for all outgoing
+    // queries, with no distinction made between queries made via TCP or UDP.
     std::unique_ptr<DnsQuery> query =
-        previous_attempt->GetQuery()->CloneWithNewId(id);
+        previous_attempt->GetQuery()->CloneWithNewId(session_->NextQueryId());
 
-    // Cancel all attempts that have not received a response, no point waiting
-    // on them.
+    // Cancel all attempts that have not received a response, as they will
+    // likely similarly require TCP retry.
     ClearAttempts(nullptr);
 
+    AttemptResult result = MakeTcpAttempt(server_index, std::move(query));
+
+    if (result.rv == ERR_IO_PENDING) {
+      // On TCP upgrade, use 2x the upgraded timeout.
+      base::TimeDelta timeout = timer_.GetCurrentDelay() * 2;
+      timer_.Start(FROM_HERE, timeout, this, &DnsTransactionImpl::OnTimeout);
+    }
+
+    return result;
+  }
+
+  AttemptResult MakeTcpAttempt(size_t server_index,
+                               std::unique_ptr<DnsQuery> query) {
+    DCHECK(!secure_);
+
+    std::unique_ptr<StreamSocket> socket(
+        session_->CreateTCPSocket(server_index, net_log_.source()));
+
     unsigned attempt_number = attempts_.size();
 
     DnsTCPAttempt* attempt =
@@ -1298,7 +1330,6 @@ class DnsTransactionImpl : public DnsTransaction,
 
     attempts_.push_back(base::WrapUnique(attempt));
     ++attempts_count_;
-    had_tcp_attempt_ = true;
 
     net_log_.AddEventReferencingSource(
         NetLogEventType::DNS_TRANSACTION_TCP_ATTEMPT,
@@ -1307,11 +1338,6 @@ class DnsTransactionImpl : public DnsTransaction,
     int rv = attempt->Start(base::BindOnce(
         &DnsTransactionImpl::OnAttemptComplete, base::Unretained(this),
         attempt_number, false /* record_rtt */, base::TimeTicks::Now()));
-    if (rv == ERR_IO_PENDING) {
-      // Custom timeout for TCP attempt.
-      base::TimeDelta timeout = timer_.GetCurrentDelay() * 2;
-      timer_.Start(FROM_HERE, timeout, this, &DnsTransactionImpl::OnTimeout);
-    }
     return AttemptResult(rv, attempt);
   }
 
@@ -1322,7 +1348,7 @@ class DnsTransactionImpl : public DnsTransaction,
                                         "qname", dotted_qname);
 
     attempts_.clear();
-    had_tcp_attempt_ = false;
+    had_tcp_retry_ = false;
     if (secure_) {
       dns_server_iterator_ = resolve_context_->GetDohIterator(
           session_->config(), secure_dns_mode_, session_.get());
@@ -1365,7 +1391,7 @@ class DnsTransactionImpl : public DnsTransaction,
   }
 
   bool MoreAttemptsAllowed() const {
-    if (had_tcp_attempt_)
+    if (had_tcp_retry_)
       return false;
 
     return dns_server_iterator_->AttemptAvailable();
@@ -1419,7 +1445,7 @@ class DnsTransactionImpl : public DnsTransaction,
           }
           break;
         case ERR_DNS_SERVER_REQUIRES_TCP:
-          result = MakeTCPAttempt(result.attempt);
+          result = RetryUdpAttemptAsTcp(result.attempt);
           break;
         case ERR_BLOCKED_BY_CLIENT:
           net_log_.EndEventWithNetErrorCode(
@@ -1493,7 +1519,9 @@ class DnsTransactionImpl : public DnsTransaction,
   std::vector<std::unique_ptr<DnsAttempt>> attempts_;
   // Count of attempts, not reset when |attempts_| vector is cleared.
   int attempts_count_;
-  bool had_tcp_attempt_;
+
+  // Records when an attempt was retried via TCP due to a truncation error.
+  bool had_tcp_retry_;
 
   // Iterator to get the index of the DNS server for each search query.
   std::unique_ptr<DnsServerIterator> dns_server_iterator_;

net/dns/dns_transaction_unittest.cc

@@ -2512,7 +2512,7 @@ TEST_F(DnsTransactionTestWithMockTime, SlowHttpsResponse_TwoAttempts) {
   EXPECT_TRUE(helper.has_completed());
 }
 
-TEST_F(DnsTransactionTest, TCPLookup) {
+TEST_F(DnsTransactionTest, TcpLookup_UdpRetry) {
   AddAsyncQueryAndRcode(kT0HostName, kT0Qtype,
                         dns_protocol::kRcodeNOERROR | dns_protocol::kFlagTC);
   AddQueryAndResponse(0 /* id */, kT0HostName, kT0Qtype, kT0ResponseDatagram,
@@ -2523,6 +2523,17 @@ TEST_F(DnsTransactionTest, TCPLookup) {
   EXPECT_TRUE(helper0.Run(transaction_factory_.get()));
 }
 
+TEST_F(DnsTransactionTest, TcpLookup_LowEntropy) {
+  session_->udp_tracker()->set_low_entropy_for_testing(true);
+
+  AddQueryAndResponse(0 /* id */, kT0HostName, kT0Qtype, kT0ResponseDatagram,
+                      base::size(kT0ResponseDatagram), ASYNC, Transport::TCP);
+
+  TransactionHelper helper0(kT0HostName, kT0Qtype, false /* secure */,
+                            kT0RecordCount, resolve_context_.get());
+  EXPECT_TRUE(helper0.Run(transaction_factory_.get()));
+}
+
 TEST_F(DnsTransactionTest, TCPFailure) {
   AddAsyncQueryAndRcode(kT0HostName, kT0Qtype,
                         dns_protocol::kRcodeNOERROR | dns_protocol::kFlagTC);
@@ -2557,7 +2568,7 @@ TEST_F(DnsTransactionTest, TCPMalformed) {
   EXPECT_TRUE(helper0.Run(transaction_factory_.get()));
 }
 
-TEST_F(DnsTransactionTestWithMockTime, TCPTimeout) {
+TEST_F(DnsTransactionTestWithMockTime, TcpTimeout_UdpRetry) {
   ConfigureFactory();
   AddAsyncQueryAndRcode(kT0HostName, kT0Qtype,
                         dns_protocol::kRcodeNOERROR | dns_protocol::kFlagTC);
@@ -2571,6 +2582,20 @@ TEST_F(DnsTransactionTestWithMockTime, TCPTimeout) {
   EXPECT_TRUE(helper0.has_completed());
 }
 
+TEST_F(DnsTransactionTestWithMockTime, TcpTimeout_LowEntropy) {
+  ConfigureFactory();
+  session_->udp_tracker()->set_low_entropy_for_testing(true);
+
+  AddSocketData(std::make_unique<DnsSocketData>(
+      1 /* id */, kT0HostName, kT0Qtype, ASYNC, Transport::TCP));
+
+  TransactionHelper helper0(kT0HostName, kT0Qtype, false /* secure */,
+                            ERR_DNS_TIMED_OUT, resolve_context_.get());
+  EXPECT_FALSE(helper0.Run(transaction_factory_.get()));
+  FastForwardUntilNoTasksRemain();
+  EXPECT_TRUE(helper0.has_completed());
+}
+
 TEST_F(DnsTransactionTest, TCPReadReturnsZeroAsync) {
   AddAsyncQueryAndRcode(kT0HostName, kT0Qtype,
                         dns_protocol::kRcodeNOERROR | dns_protocol::kFlagTC);

net/dns/dns_udp_tracker.h

@@ -38,6 +38,13 @@ class NET_EXPORT_PRIVATE DnsUdpTracker {
   void RecordQuery(uint16_t port, uint16_t query_id);
   void RecordResponseId(uint16_t query_id, uint16_t response_id);
 
+  // If true, the entropy from random UDP port and DNS ID has been detected to
+  // potentially be low, e.g. due to exhaustion of the port pool or mismatches
+  // on IDs.
+  bool low_entropy() const { return low_entropy_; }
+
+  void set_low_entropy_for_testing(bool value) { low_entropy_ = value; }
+
   void set_tick_clock_for_testing(base::TickClock* tick_clock) {
     tick_clock_ = tick_clock;
   }
@@ -48,6 +55,8 @@ class NET_EXPORT_PRIVATE DnsUdpTracker {
   void PurgeOldQueries();
   void SaveQuery(QueryData query);
 
+  // TODO(crbug.com/1093361): Set based on recorded queries and responses.
+  bool low_entropy_ = false;
   base::circular_deque<QueryData> recent_queries_;
 
   const base::TickClock* tick_clock_ = base::DefaultTickClock::GetInstance();