From 1f65c18c0199164197c6515a737ceaad185790e0 Mon Sep 17 00:00:00 2001
From: Remi Gacogne <remi.gacogne@powerdns.com>
Date: Tue, 4 Dec 2018 10:27:13 +0100
Subject: [PATCH] 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.
---
 pdns/dnsdistdist/tcpiohandler.cc | 30 +++++++++++++++++++++++++++---
 pdns/dnsdistdist/tcpiohandler.hh |  6 +++++-
 2 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/pdns/dnsdistdist/tcpiohandler.cc b/pdns/dnsdistdist/tcpiohandler.cc
index e91cf6bd2..1f516f14f 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 ce48a44f9..0d5bfa514 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;
       }
     }
   }
-- 
2.40.0