Fix UAF in DnsTransaction

Starting a TCP DNS attempt cancels and deletes any previous running
attempts.  Fix the filtering to skip retriable errors that will not
retry because we have exhausted available attempts.  Otherwise, if an
attempt has already completed synchronously and posted to handle the
result, it could try to handle the result of a deleted attempt.

Also replace the raw Attempt pointer in AttemptResult with a WeakPtr, so
any future similar bugs will safely DCHECK and crash instead of UAF.

Bug: 860292
Change-Id: I4a012a078b9b35f87b95a9592f4e43573c6a0708
Reviewed-on: https://chromium-review.googlesource.com/1134028
Commit-Queue: Matt Menke <mmenke@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#575085}

net/dns/dns_transaction.cc

@@ -174,19 +174,15 @@ class DnsAttempt {
     return std::move(dict);
   }
 
-  void set_result(int result) {
-    result_ = result;
-  }
+  void set_result(int result) { result_ = result; }
 
   // True if current attempt is pending (waiting for server response).
-  bool is_pending() const {
-    return result_ == ERR_IO_PENDING;
-  }
+  bool is_pending() const { return result_ == ERR_IO_PENDING; }
 
   // True if attempt is completed (received server response).
   bool is_completed() const {
     return (result_ == OK) || (result_ == ERR_NAME_NOT_RESOLVED) ||
-        (result_ == ERR_DNS_SERVER_REQUIRES_TCP);
+           (result_ == ERR_DNS_SERVER_REQUIRES_TCP);
   }
 
  private:
@@ -241,9 +237,7 @@ class DnsUDPAttempt : public DnsAttempt {
     STATE_NONE,
   };
 
-  DatagramClientSocket* socket() {
-    return socket_lease_->socket();
-  }
+  DatagramClientSocket* socket() { return socket_lease_->socket(); }
 
   int DoLoop(int result) {
     CHECK_NE(STATE_NONE, next_state_);
@@ -840,7 +834,7 @@ class DnsTransactionImpl : public DnsTransaction,
   DnsTransactionImpl(DnsSession* session,
                      const std::string& hostname,
                      uint16_t qtype,
-                     DnsTransactionFactory::CallbackType& callback,
+                     DnsTransactionFactory::CallbackType callback,
                      const NetLogWithSource& net_log,
                      const OptRecordRdata* opt_rdata)
       : session_(session),
@@ -895,9 +889,12 @@ class DnsTransactionImpl : public DnsTransaction,
 
     // Must always return result asynchronously, to avoid reentrancy.
     if (result.rv != ERR_IO_PENDING) {
+      // Clear all other non-completed attempts. They are no longer needed and
+      // they may interfere with this posted result.
+      ClearAttempts(result.attempt);
       base::ThreadTaskRunnerHandle::Get()->PostTask(
           FROM_HERE,
-          base::Bind(&DnsTransactionImpl::DoCallback, AsWeakPtr(), result));
+          base::BindOnce(&DnsTransactionImpl::DoCallback, AsWeakPtr(), result));
     }
   }
 
@@ -1122,14 +1119,9 @@ class DnsTransactionImpl : public DnsTransaction,
 
     RecordLostPacketsIfAny();
 
-    // Cancel all other attempts that have not received a response, no point
-    // waiting on them.
-    for (auto it = attempts_.begin(); it != attempts_.end();) {
-      if (!(*it)->is_completed())
-        it = attempts_.erase(it);
-      else
-        ++it;
-    }
+    // Cancel all attempts that have not received a response, no point waiting
+    // on them.
+    ClearAttempts(nullptr);
 
     unsigned attempt_number = attempts_.size();
 
@@ -1302,6 +1294,19 @@ class DnsTransactionImpl : public DnsTransaction,
     return result;
   }
 
+  // Clears and cancels all non-completed attempts. If |leave_attempt| is not
+  // null, it is not cleared even if complete.
+  void ClearAttempts(const DnsAttempt* leave_attempt) {
+    std::unique_ptr<DnsAttempt> completed_result;
+    for (auto it = attempts_.begin(); it != attempts_.end();) {
+      if (!(*it)->is_completed() && it->get() != leave_attempt) {
+        it = attempts_.erase(it);
+      } else {
+        ++it;
+      }
+    }
+  }
+
   void OnTimeout() {
     if (callback_.is_null())
       return;
@@ -1361,8 +1366,9 @@ class DnsTransactionFactoryImpl : public DnsTransactionFactory {
       uint16_t qtype,
       CallbackType callback,
       const NetLogWithSource& net_log) override {
-    return std::unique_ptr<DnsTransaction>(new DnsTransactionImpl(
-        session_.get(), hostname, qtype, callback, net_log, opt_rdata_.get()));
+    return std::make_unique<DnsTransactionImpl>(session_.get(), hostname, qtype,
+                                                std::move(callback), net_log,
+                                                opt_rdata_.get());
   }
 
   void AddEDNSOption(const OptRecordRdata::Opt& opt) override {

net/dns/dns_transaction_unittest.cc

@@ -8,6 +8,7 @@
 
 #include <limits>
 #include <utility>
+#include <vector>
 
 #include "base/base64url.h"
 #include "base/bind.h"
@@ -192,10 +193,8 @@ class TestSocketFactory;
 // A variant of MockUDPClientSocket which always fails to Connect.
 class FailingUDPClientSocket : public MockUDPClientSocket {
  public:
-  FailingUDPClientSocket(SocketDataProvider* data,
-                         net::NetLog* net_log)
-      : MockUDPClientSocket(data, net_log) {
-  }
+  FailingUDPClientSocket(SocketDataProvider* data, net::NetLog* net_log)
+      : MockUDPClientSocket(data, net_log) {}
   ~FailingUDPClientSocket() override = default;
   int Connect(const IPEndPoint& endpoint) override {
     return ERR_CONNECTION_REFUSED;
@@ -211,8 +210,7 @@ class TestUDPClientSocket : public MockUDPClientSocket {
   TestUDPClientSocket(TestSocketFactory* factory,
                       SocketDataProvider* data,
                       net::NetLog* net_log)
-      : MockUDPClientSocket(data, net_log), factory_(factory) {
-  }
+      : MockUDPClientSocket(data, net_log), factory_(factory) {}
   ~TestUDPClientSocket() override = default;
   int Connect(const IPEndPoint& endpoint) override;
 
@@ -273,9 +271,7 @@ class TransactionHelper {
         completed_(false) {}
 
   // Mark that the transaction shall be destroyed immediately upon callback.
-  void set_cancel_in_callback() {
-    cancel_in_callback_ = true;
-  }
+  void set_cancel_in_callback() { cancel_in_callback_ = true; }
 
   void StartTransaction(DnsTransactionFactory* factory) {
     EXPECT_EQ(NULL, transaction_.get());
@@ -329,9 +325,7 @@ class TransactionHelper {
     }
   }
 
-  bool has_completed() const {
-    return completed_;
-  }
+  bool has_completed() const { return completed_; }
 
   // Shorthands for commonly used commands.
 
@@ -536,9 +530,10 @@ class URLRequestMockDohJob : public URLRequestJob, public AsyncSocket {
   const ResponseModifierCallback response_modifier_;
   IOBuffer* pending_buf_;
   int pending_buf_size_;
-  DISALLOW_COPY_AND_ASSIGN(URLRequestMockDohJob);
 
   base::WeakPtrFactory<URLRequestMockDohJob> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(URLRequestMockDohJob);
 };
 
 class DnsTransactionTestBase : public testing::Test {
@@ -943,10 +938,10 @@ TEST_F(DnsTransactionTest, MismatchedResponseSync) {
   // Attempt receives mismatched response followed by valid response.
   std::unique_ptr<DnsSocketData> data(new DnsSocketData(
       0 /* id */, kT0HostName, kT0Qtype, SYNCHRONOUS, Transport::UDP));
-  data->AddResponseData(kT1ResponseDatagram,
-                        arraysize(kT1ResponseDatagram), SYNCHRONOUS);
-  data->AddResponseData(kT0ResponseDatagram,
-                        arraysize(kT0ResponseDatagram), SYNCHRONOUS);
+  data->AddResponseData(kT1ResponseDatagram, arraysize(kT1ResponseDatagram),
+                        SYNCHRONOUS);
+  data->AddResponseData(kT0ResponseDatagram, arraysize(kT0ResponseDatagram),
+                        SYNCHRONOUS);
   AddSocketData(std::move(data));
 
   TransactionHelper helper0(kT0HostName, kT0Qtype, kT0RecordCount);
@@ -961,10 +956,10 @@ TEST_F(DnsTransactionTest, MismatchedResponseAsync) {
   // Second attempt times out.
   std::unique_ptr<DnsSocketData> data(new DnsSocketData(
       0 /* id */, kT0HostName, kT0Qtype, ASYNC, Transport::UDP));
-  data->AddResponseData(kT1ResponseDatagram,
-                        arraysize(kT1ResponseDatagram), ASYNC);
-  data->AddResponseData(kT0ResponseDatagram,
-                        arraysize(kT0ResponseDatagram), ASYNC);
+  data->AddResponseData(kT1ResponseDatagram, arraysize(kT1ResponseDatagram),
+                        ASYNC);
+  data->AddResponseData(kT0ResponseDatagram, arraysize(kT0ResponseDatagram),
+                        ASYNC);
   AddSocketData(std::move(data));
   AddQueryAndTimeout(kT0HostName, kT0Qtype);
 
@@ -1067,8 +1062,8 @@ TEST_F(DnsTransactionTestWithMockTime, ServerFallbackAndRotate) {
   EXPECT_TRUE(helper1.Run(transaction_factory_.get()));
 
   unsigned kOrder[] = {
-      0, 1, 2, 0, 1,    // The first transaction.
-      1, 2, 0,          // The second transaction starts from the next server.
+      0, 1, 2, 0, 1,  // The first transaction.
+      1, 2, 0,        // The second transaction starts from the next server.
   };
   CheckServerOrder(kOrder, arraysize(kOrder));
 }
@@ -1097,7 +1092,7 @@ TEST_F(DnsTransactionTest, SuffixSearchAboveNdots) {
   EXPECT_TRUE(helper0.Run(transaction_factory_.get()));
 
   // Also check if suffix search causes server rotation.
-  unsigned kOrder0[] = { 0, 1, 0, 1 };
+  unsigned kOrder0[] = {0, 1, 0, 1};
   CheckServerOrder(kOrder0, arraysize(kOrder0));
 }
 
@@ -2026,6 +2021,47 @@ TEST_F(DnsTransactionTest, MismatchedThenOkThenTCP) {
   EXPECT_TRUE(helper0.Run(transaction_factory_.get()));
 }
 
+TEST_F(DnsTransactionTest, MismatchedThenRefusedThenTCP) {
+  // Set up the expected sequence of events:
+  // 1) First attempt (UDP) gets a synchronous mismatched response. On such
+  //    malformed responses, DnsTransaction triggers an immediate retry to read
+  //    again from the socket within the same "attempt".
+  // 2) Second read (within the first attempt) starts. Test is configured to
+  //    give an asynchronous TCP required response which will complete later.
+  //    On asynchronous action after a malformed response, the attempt will
+  //    immediately produce a retriable error result while the retry continues,
+  //    thus forking the running attempts.
+  // 3) Error result triggers a second attempt (UDP) which test gives a
+  //    synchronous ERR_CONNECTION_REFUSED, which is a retriable error, but
+  //    DnsTransaction has exhausted max retries (2 attempts), so this result
+  //    gets posted as the result of the transaction and other running attempts
+  //    should be cancelled.
+  // 4) First attempt should be cancelled when the transaction result is posted,
+  //    so first attempt's second read should never complete. If it did
+  //    complete, it would complete with a TCP-required error, and
+  //    DnsTransaction would start a TCP attempt and clear previous attempts. It
+  //    would be very bad if that then cleared the attempt posted as the final
+  //    result, as result handling does not expect that memory to go away.
+
+  config_.attempts = 2;
+  ConfigureFactory();
+
+  // Attempt 1.
+  std::unique_ptr<DnsSocketData> data(new DnsSocketData(
+      0 /* id */, kT0HostName, kT0Qtype, SYNCHRONOUS, Transport::UDP));
+  data->AddResponseData(kT1ResponseDatagram, arraysize(kT1ResponseDatagram),
+                        SYNCHRONOUS);
+  data->AddRcode(dns_protocol::kFlagTC, ASYNC);
+  AddSocketData(std::move(data));
+
+  // Attempt 2.
+  AddQueryAndErrorResponse(0 /* id */, kT0HostName, kT0Qtype,
+                           ERR_CONNECTION_REFUSED, SYNCHRONOUS, Transport::UDP);
+
+  TransactionHelper helper0(kT0HostName, kT0Qtype, ERR_CONNECTION_REFUSED);
+  EXPECT_TRUE(helper0.Run(transaction_factory_.get()));
+}
+
 TEST_F(DnsTransactionTest, InvalidQuery) {
   ConfigureFactory();