From: Remi Gacogne <remi.gacogne@powerdns.com>
Date: Tue, 4 Dec 2018 09:27:13 +0000 (+0100)
Subject: dnsdist: Protect GnuTLS tickets key rotation with a read-write lock
X-Git-Tag: auth-4.2.0-alpha1~8^2~1
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=1f65c18c0199164197c6515a737ceaad185790e0;p=thirdparty%2Fpdns.git

dnsdist: Protect GnuTLS tickets key rotation with a read-write lock

Otherwise a thread can replace the shared pointer hold by the
GnuTLSIOCtx while another thread is accessing it. The usage count
is not incremented since no copy is made, so the content might get
deleted while a thread is still accessing it, leading to
use-after-free and possibly a crash.
---

diff --git a/pdns/dnsdistdist/tcpiohandler.cc b/pdns/dnsdistdist/tcpiohandler.cc
index e91cf6bd23..1f516f14f1 100644
--- a/pdns/dnsdistdist/tcpiohandler.cc
+++ b/pdns/dnsdistdist/tcpiohandler.cc
@@ -622,6 +622,7 @@ public:
     catch (const std::exception& e) {
       safe_memory_release(d_key.data, d_key.size);
       gnutls_free(d_key.data);
+      d_key.data = nullptr;
       throw;
     }
   }
@@ -632,6 +633,7 @@ public:
       safe_memory_release(d_key.data, d_key.size);
     }
     gnutls_free(d_key.data);
+    d_key.data = nullptr;
   }
   const gnutls_datum_t& getKey() const
   {
@@ -802,6 +804,8 @@ public:
       warnlog("Error setting up TLS cipher preferences to %s (%s), skipping.", fe.d_ciphers.c_str(), gnutls_strerror(rc));
     }
 
+    pthread_rwlock_init(&d_lock, nullptr);
+
     try {
       if (fe.d_ticketKeyFile.empty()) {
         handleTicketsKeyRotation(time(nullptr));
@@ -811,12 +815,15 @@ public:
       }
     }
     catch(const std::runtime_error& e) {
+      pthread_rwlock_destroy(&d_lock);
       throw std::runtime_error("Error generating tickets key for TLS context on " + fe.d_addr.toStringWithPort() + ": " + e.what());
     }
   }
 
   virtual ~GnuTLSIOCtx() override
   {
+    pthread_rwlock_destroy(&d_lock);
+
     d_creds.reset();
 
     if (d_priorityCache) {
@@ -828,7 +835,13 @@ public:
   {
     handleTicketsKeyRotation(now);
 
-    return std::unique_ptr<GnuTLSConnection>(new GnuTLSConnection(socket, timeout, d_creds.get(), d_priorityCache, d_ticketsKey, d_enableTickets));
+    std::shared_ptr<GnuTLSTicketsKey> ticketsKey;
+    {
+      ReadLock rl(&d_lock);
+      ticketsKey = d_ticketsKey;
+    }
+
+    return std::unique_ptr<GnuTLSConnection>(new GnuTLSConnection(socket, timeout, d_creds.get(), d_priorityCache, ticketsKey, d_enableTickets));
   }
 
   void rotateTicketsKey(time_t now) override
@@ -838,7 +851,12 @@ public:
     }
 
     auto newKey = std::make_shared<GnuTLSTicketsKey>();
-    d_ticketsKey = newKey;
+
+    {
+      WriteLock wl(&d_lock);
+      d_ticketsKey = newKey;
+    }
+
     if (d_ticketsKeyRotationDelay > 0) {
       d_ticketsKeyNextRotation = now + d_ticketsKeyRotationDelay;
     }
@@ -851,7 +869,11 @@ public:
     }
 
     auto newKey = std::make_shared<GnuTLSTicketsKey>(file);
-    d_ticketsKey = newKey;
+    {
+      WriteLock wl(&d_lock);
+      d_ticketsKey = newKey;
+    }
+
     if (d_ticketsKeyRotationDelay > 0) {
       d_ticketsKeyNextRotation = time(nullptr) + d_ticketsKeyRotationDelay;
     }
@@ -859,6 +881,7 @@ public:
 
   size_t getTicketsKeysCount() override
   {
+    ReadLock rl(&d_lock);
     return d_ticketsKey != nullptr ? 1 : 0;
   }
 
@@ -866,6 +889,7 @@ private:
   std::unique_ptr<gnutls_certificate_credentials_st, void(*)(gnutls_certificate_credentials_t)> d_creds;
   gnutls_priority_t d_priorityCache{nullptr};
   std::shared_ptr<GnuTLSTicketsKey> d_ticketsKey{nullptr};
+  pthread_rwlock_t d_lock;
   bool d_enableTickets{true};
 };
 
diff --git a/pdns/dnsdistdist/tcpiohandler.hh b/pdns/dnsdistdist/tcpiohandler.hh
index ce48a44f9f..0d5bfa514e 100644
--- a/pdns/dnsdistdist/tcpiohandler.hh
+++ b/pdns/dnsdistdist/tcpiohandler.hh
@@ -44,7 +44,11 @@ public:
       }
       catch(const std::runtime_error& e) {
         d_rotatingTicketsKey.clear();
-        throw std::runtime_error("Error generating a new tickets key for TLS context");
+        throw std::runtime_error(std::string("Error generating a new tickets key for TLS context:") + e.what());
+      }
+      catch(...) {
+        d_rotatingTicketsKey.clear();
+        throw;
       }
     }
   }