Fix use-after-free bug triggered when memory pressure reaches critical

When memory pressure reaches critical, SingleTreeTracker clears the
pending_entries_ map. However, if an inclusion check is in progress for
one or more of those pending entries, LogDnsClient will have a pointer to
a MerkleAuditProof held in that map. This results in it trying to access
freed memory.

The fix is to cancel all inclusion checks when this happens. This is done
by changing LogDnsClient to provide a "resource handle" when it starts a
query, which can be deleted in order to abort the query. Storing this
in pending_entries_ assures that all inclusion checks will be aborted
when pending_entries_ is cleared.

Bug: 811566
Change-Id: I86b7ff880c050b790d219fa0cd50b42839bc0d3e
Reviewed-on: https://chromium-review.googlesource.com/939627Reviewed-by: Ryan Sleevi <rsleevi@chromium.org>
Reviewed-by: Matt Mueller <mattm@chromium.org>
Commit-Queue: Rob Percival <robpercival@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546183}

components/certificate_transparency/log_dns_client.h

@@ -11,6 +11,7 @@
 
 #include "base/callback.h"
 #include "base/macros.h"
+#include "base/single_thread_task_runner.h"
 #include "base/strings/string_piece.h"
 #include "net/base/completion_callback.h"
 #include "net/base/net_errors.h"
@@ -33,6 +34,12 @@ namespace certificate_transparency {
 // It must be created and deleted on the same thread. It is not thread-safe.
 class LogDnsClient : public net::NetworkChangeNotifier::DNSObserver {
  public:
+  class AuditProofQuery {
+   public:
+    virtual ~AuditProofQuery() = default;
+    virtual const net::ct::MerkleAuditProof& GetProof() const = 0;
+  };
+
   // Creates a log client that will take ownership of |dns_client| and use it
   // to perform DNS queries. Queries will be logged to |net_log|.
   // The |dns_client| does not need to be configured first - this will be done
@@ -60,7 +67,8 @@ class LogDnsClient : public net::NetworkChangeNotifier::DNSObserver {
   // constructor of LogDnsClient). This callback will fire once and then be
   // unregistered. Should only be used if QueryAuditProof() returns
   // net::ERR_TEMPORARILY_THROTTLED.
-  void NotifyWhenNotThrottled(const base::Closure& callback);
+  // The callback will be run on the same thread that created the LogDnsClient.
+  void NotifyWhenNotThrottled(base::OnceClosure callback);
 
   // Queries a CT log to retrieve an audit proof for the leaf with |leaf_hash|.
   // The log is identified by |domain_for_log|, which is the DNS name used as a
@@ -68,10 +76,12 @@ class LogDnsClient : public net::NetworkChangeNotifier::DNSObserver {
   // The |leaf_hash| is the SHA-256 Merkle leaf hash (see RFC6962, section 2.1).
   // The size of the CT log tree, for which the proof is requested, must be
   // provided in |tree_size|.
-  // The leaf index and audit proof obtained from the CT log will be placed in
-  // |out_proof|.
+  // A handle to the query will be placed in |out_query|. The audit proof can be
+  // obtained from that once the query completes. Deleting this handle before
+  // the query completes will cancel it. It must not outlive the LogDnsClient.
   // If the proof cannot be obtained synchronously, this method will return
   // net::ERR_IO_PENDING and invoke |callback| once the query is complete.
+  // The callback will be run on the same thread that created the LogDnsClient.
   // Returns:
   // - net::OK if the query was successful.
   // - net::ERR_IO_PENDING if the query was successfully started and is
@@ -85,24 +95,23 @@ class LogDnsClient : public net::NetworkChangeNotifier::DNSObserver {
   net::Error QueryAuditProof(base::StringPiece domain_for_log,
                              std::string leaf_hash,
                              uint64_t tree_size,
-                             net::ct::MerkleAuditProof* out_proof,
+                             std::unique_ptr<AuditProofQuery>* out_query,
                              const net::CompletionCallback& callback);
 
  private:
-  class AuditProofQuery;
-
   // Invoked when an audit proof query completes.
-  // |query| is the query that has completed.
   // |callback| is the user-provided callback that should be notified.
   // |net_error| is a net::Error indicating success or failure.
-  void QueryAuditProofComplete(AuditProofQuery* query,
-                               const net::CompletionCallback& callback,
+  void QueryAuditProofComplete(const net::CompletionCallback& callback,
                                int net_error);
 
-  // Returns true if the maximum number of queries are currently in flight.
-  // If the maximum number of concurrency queries is set to 0, this will always
+  // Invoked when an audit proof query is cancelled.
+  void QueryAuditProofCancelled();
+
+  // Returns true if the maximum number of queries are currently in-flight.
+  // If the maximum number of in-flight queries is set to 0, this will always
   // return false.
-  bool HasMaxConcurrentQueriesInProgress() const;
+  bool HasMaxQueriesInFlight() const;
 
   // Updates the |dns_client_| config using NetworkChangeNotifier.
   void UpdateDnsConfig();
@@ -111,16 +120,12 @@ class LogDnsClient : public net::NetworkChangeNotifier::DNSObserver {
   std::unique_ptr<net::DnsClient> dns_client_;
   // Passed to the DNS client for logging.
   net::NetLogWithSource net_log_;
-  // A FIFO queue of ongoing queries. Since entries will always be appended to
-  // the end and lookups will typically yield entries at the beginning,
-  // std::list is an efficient choice.
-  std::list<std::unique_ptr<AuditProofQuery>> audit_proof_queries_;
-  // The maximum number of queries that can be in flight at one time.
-  size_t max_concurrent_queries_;
-  // Callbacks to invoke when the number of concurrent queries is at its limit.
-  std::list<base::Closure> not_throttled_callbacks_;
-  // Creates weak_ptrs to this, for callback purposes.
-  base::WeakPtrFactory<LogDnsClient> weak_ptr_factory_;
+  // The number of queries that are currently in-flight.
+  size_t in_flight_queries_;
+  // The maximum number of queries that can be in-flight at one time.
+  size_t max_in_flight_queries_;
+  // Callbacks to invoke when the number of in-flight queries is at its limit.
+  std::list<base::OnceClosure> not_throttled_callbacks_;
 
   DISALLOW_COPY_AND_ASSIGN(LogDnsClient);
 };

components/certificate_transparency/single_tree_tracker.cc

@@ -202,8 +202,9 @@ struct SingleTreeTracker::EntryAuditState {
   // Current phase of inclusion check.
   AuditState state;
 
-  // The proof to be filled in by the LogDnsClient
-  MerkleAuditProof proof;
+  // The audit proof query performed by LogDnsClient.
+  // It is null unless a query has been started.
+  std::unique_ptr<LogDnsClient::AuditProofQuery> audit_proof_query;
 
   // The root hash of the tree for which an inclusion proof was requested.
   // The root hash is needed after the inclusion proof is fetched for validating
@@ -414,9 +415,9 @@ void SingleTreeTracker::ProcessPendingEntries() {
         crypto::kSHA256Length);
     net::Error result = dns_client_->QueryAuditProof(
         ct_log_->dns_domain(), leaf_hash, verified_sth_.tree_size,
-        &(it->second.proof),
+        &(it->second.audit_proof_query),
         base::Bind(&SingleTreeTracker::OnAuditProofObtained,
-                   weak_factory_.GetWeakPtr(), it->first));
+                   base::Unretained(this), it->first));
     // Handling proofs returned synchronously is not implemeted.
     DCHECK_NE(result, net::OK);
     if (result == net::ERR_IO_PENDING) {
@@ -424,8 +425,11 @@ void SingleTreeTracker::ProcessPendingEntries() {
       // and continue to the next one.
       it->second.state = INCLUSION_PROOF_REQUESTED;
     } else if (result == net::ERR_TEMPORARILY_THROTTLED) {
+      // Need to use a weak pointer here, as this callback could be triggered
+      // when the SingleTreeTracker is deleted (and pending queries are
+      // cancelled).
       dns_client_->NotifyWhenNotThrottled(
-          base::Bind(&SingleTreeTracker::ProcessPendingEntries,
+          base::BindOnce(&SingleTreeTracker::ProcessPendingEntries,
                          weak_factory_.GetWeakPtr()));
       // Exit the loop since all subsequent calls to QueryAuditProof
       // will be throttled.
@@ -494,7 +498,8 @@ void SingleTreeTracker::OnAuditProofObtained(const EntryToAudit& entry,
   std::string leaf_hash(reinterpret_cast<const char*>(entry.leaf_hash.data),
                         crypto::kSHA256Length);
 
-  bool verified = ct_log_->VerifyAuditProof(it->second.proof,
+  bool verified =
+      ct_log_->VerifyAuditProof(it->second.audit_proof_query->GetProof(),
                                 it->second.root_hash, leaf_hash);
   LogAuditResultToNetLog(entry, verified);
 
@@ -514,7 +519,7 @@ void SingleTreeTracker::OnMemoryPressure(
     case base::MemoryPressureListener::MEMORY_PRESSURE_LEVEL_NONE:
       break;
     case base::MemoryPressureListener::MEMORY_PRESSURE_LEVEL_CRITICAL:
-      pending_entries_.clear();
+      ResetPendingQueue();
       // Fall through to clearing the other cache.
       FALLTHROUGH;
     case base::MemoryPressureListener::MEMORY_PRESSURE_LEVEL_MODERATE:

components/certificate_transparency/single_tree_tracker_unittest.cc

@@ -655,6 +655,45 @@ TEST_F(SingleTreeTrackerTest, TestEntryIncludedAfterInclusionCheckSuccess) {
       net_log_, LeafHash(chain_.get(), cert_sct_.get()), true));
 }
 
+// Tests that inclusion checks are aborted and SCTs discarded if under critical
+// memory pressure.
+TEST_F(SingleTreeTrackerTest,
+       TestInclusionCheckCancelledIfUnderMemoryPressure) {
+  CreateTreeTracker();
+  AddCacheEntry(host_resolver_.GetHostCache(), kHostname,
+                net::HostCache::Entry::SOURCE_DNS, kZeroTTL);
+
+  tree_tracker_->OnSCTVerified(kHostname, chain_.get(), cert_sct_.get());
+  EXPECT_EQ(
+      SingleTreeTracker::SCT_PENDING_NEWER_STH,
+      tree_tracker_->GetLogEntryInclusionStatus(chain_.get(), cert_sct_.get()));
+
+  // Provide with a fresh STH, which is for a tree of size 2.
+  SignedTreeHead sth;
+  ASSERT_TRUE(GetSignedTreeHeadForTreeOfSize2(&sth));
+  ASSERT_TRUE(log_->VerifySignedTreeHead(sth));
+
+  // Make the first event that is processed a critical memory pressure
+  // notification. This should be handled before the response to the first DNS
+  // request, so no requests after the first one should be sent (the leaf index
+  // request).
+  base::MemoryPressureListener::NotifyMemoryPressure(
+      base::MemoryPressureListener::MEMORY_PRESSURE_LEVEL_CRITICAL);
+
+  ASSERT_TRUE(mock_dns_.ExpectLeafIndexRequestAndResponse(
+      Base32LeafHash(chain_.get(), cert_sct_.get()) + ".hash." +
+          kDNSRequestSuffix,
+      0));
+
+  tree_tracker_->NewSTHObserved(sth);
+  base::RunLoop().RunUntilIdle();
+
+  // Expect the SCT to have been discarded.
+  EXPECT_EQ(
+      SingleTreeTracker::SCT_NOT_OBSERVED,
+      tree_tracker_->GetLogEntryInclusionStatus(chain_.get(), cert_sct_.get()));
+}
+
 // Test that pending entries transition states correctly according to the
 // STHs provided:
 // * Start without an STH.