Skip to content

SSL Certificate Hot-Reload for Mumble Server #6974

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Dinsmoor wants to merge 2 commits into from
+151 −12
Closed

SSL Certificate Hot-Reload for Mumble Server #6974

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
36e7b5c
FEAT(server): Add automatic SSL certificate hot-reload
Dinsmoor Oct 27, 2025
33466da
FIX(server): Use proper thread-safe locking for certificate hot-reload
Dinsmoor Oct 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 81 additions & 0 deletions src/murmur/Cert.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,3 +214,84 @@ void Server::initializeCert() {
const QString Server::getDigest() const {
return QString::fromLatin1(qscCert.digest(QCryptographicHash::Sha1).toHex());
}

void Server::initCertMonitoring() {
connect(&qtCertCheck, SIGNAL(timeout()), this, SLOT(checkCertExpiry()));
// Check certificate expiry every hour
qtCertCheck.start(60 * 60 * 1000);
}

void Server::checkCertExpiry() {
if (qscCert.isNull()) {
return;
}

QDateTime now = QDateTime::currentDateTime();
QDateTime expiryDate = qscCert.expiryDate();
qint64 daysUntilExpiry = now.daysTo(expiryDate);

// Check if certificate expires within 7 days or has already expired
if (daysUntilExpiry <= 7 && daysUntilExpiry >= 0) {
log(QString("Certificate expires in %1 days, attempting to reload from disk").arg(daysUntilExpiry));

if (reloadCertFromDisk()) {
log("Successfully reloaded certificate from disk");
// Update expiry date after reload
expiryDate = qscCert.expiryDate();
daysUntilExpiry = now.daysTo(expiryDate);
log(QString("New certificate expires in %1 days").arg(daysUntilExpiry));
} else {
log("Failed to reload certificate from disk, keeping current certificate");
}
} else if (daysUntilExpiry < 0) {
log(QString("Certificate has expired %1 days ago, attempting to reload from disk").arg(-daysUntilExpiry));

if (reloadCertFromDisk()) {
log("Successfully reloaded certificate from disk");
} else {
log("Failed to reload certificate from disk, keeping current expired certificate");
}
}
}
Comment on lines +224 to +266

@coderabbitai coderabbitai Bot Oct 27, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Critical: Add thread synchronization for certificate access.

This function reads and updates qscCert, qskKey, and qlIntermediates (via reloadCertFromDisk()) without holding any locks. Given that Server inherits from QThread and has qrwlVoiceThread for synchronizing access to shared data, certificate members should also be protected.

Consider:

  1. New client connections (in newClient() around line 1435-1436) read qscCert and qskKey to configure SSL
  2. The voice thread may indirectly access certificate information
  3. Concurrent calls to checkCertExpiry() could cause data races

Wrap certificate access with appropriate locking:

 void Server::checkCertExpiry() {
+	QReadLocker lock(&qrwlVoiceThread);
+	
 	if (qscCert.isNull()) {
 		return;
 	}

 	QDateTime now          = QDateTime::currentDateTime();
 	QDateTime expiryDate   = qscCert.expiryDate();
 	qint64 daysUntilExpiry = now.daysTo(expiryDate);

+	lock.unlock();
+
 	// Check if certificate expires within 7 days or has already expired
 	if (daysUntilExpiry <= 7 && daysUntilExpiry >= 0) {

And update reloadCertFromDisk() to acquire a write lock when modifying certificates.

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents 
In src/murmur/Cert.cpp around lines 224 to 255, checkCertExpiry() accesses and
updates shared certificate state (qscCert, qskKey, qlIntermediates via
reloadCertFromDisk()) without locking; wrap all reads of qscCert/expiryDate and
the logic that may call reloadCertFromDisk() in a read or upgradeable lock using
the existing qrwlVoiceThread (acquire a read lock for expiry checks, upgrade to
a write lock before calling reloadCertFromDisk or modifying certificate
variables), and change reloadCertFromDisk() to acquire the write lock
(qrwlVoiceThread.lockForWrite()/unlock) while it replaces
qscCert/qskKey/qlIntermediates to prevent races with newClient() and other
threads that read these members.


bool Server::reloadCertFromDisk() {
// Only reload if we're using Meta certificate (which is loaded from file)
if (!bUsingMetaCert) {
// This server has a database-stored certificate, cannot reload from disk
return false;
}

// Store current certificate as backup
QSslCertificate oldCert = qscCert;
QSslKey oldKey = qskKey;
QList< QSslCertificate > oldInter = qlIntermediates;

// Force Meta to reload its SSL settings from disk
if (!Meta::mp->loadSSLSettings()) {
log("Failed to reload SSL settings from murmur.ini");
return false;
}

// Try to reload from Meta's certificate
qskKey = Meta::mp->qskKey;
qscCert = Meta::mp->qscCert;
qlIntermediates = Meta::mp->qlIntermediates;

// Verify that the reloaded certificate is valid and different
if (qscCert.isNull() || qskKey.isNull()) {
// Reload failed, restore old certificate
qscCert = oldCert;
qskKey = oldKey;
qlIntermediates = oldInter;
return false;
}

// Check if certificate actually changed
if (qscCert == oldCert) {
// Certificate hasn't changed on disk
return false;
}

// Successfully reloaded new certificate
return true;
}
Comment on lines +268 to +348

@coderabbitai coderabbitai Bot Oct 27, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Critical: Add validation and null pointer checks.

Multiple safety issues in the certificate reload logic:

  1. Null pointer dereference risk: Meta::mp is accessed without null check (lines 270, 276-278). If Meta is not initialized, this will crash.

  2. Missing certificate validation: After reloading, the code only checks if the certificate and key are null (line 281) but doesn't verify:

    • The key actually matches the certificate (should use isKeyForCert() from line 24)
    • The new certificate isn't already expired
    • The certificate is properly formed

  3. Thread safety: Same issue as checkCertExpiry() - certificate members are modified without locking.

Apply these fixes:

 bool Server::reloadCertFromDisk() {
 	// Only reload if we're using Meta certificate (which is loaded from file)
 	if (!bUsingMetaCert) {
 		// This server has a database-stored certificate, cannot reload from disk
 		return false;
 	}

+	if (!Meta::mp) {
+		log("Meta not initialized, cannot reload certificate");
+		return false;
+	}
+
 	// Store current certificate as backup
 	QSslCertificate oldCert           = qscCert;
 	QSslKey oldKey                    = qskKey;
 	QList< QSslCertificate > oldInter = qlIntermediates;

 	// Force Meta to reload its SSL settings from disk
 	if (!Meta::mp->loadSSLSettings()) {
 		log("Failed to reload SSL settings from murmur.ini");
 		return false;
 	}

+	QWriteLocker lock(&qrwlVoiceThread);
+
 	// Try to reload from Meta's certificate
 	qskKey          = Meta::mp->qskKey;
 	qscCert         = Meta::mp->qscCert;
 	qlIntermediates = Meta::mp->qlIntermediates;

 	// Verify that the reloaded certificate is valid and different
 	if (qscCert.isNull() || qskKey.isNull()) {
 		// Reload failed, restore old certificate
 		qscCert         = oldCert;
 		qskKey          = oldKey;
 		qlIntermediates = oldInter;
 		return false;
 	}

+	// Verify the key matches the certificate
+	if (!isKeyForCert(qskKey, qscCert)) {
+		log("Reloaded certificate and key do not match");
+		qscCert         = oldCert;
+		qskKey          = oldKey;
+		qlIntermediates = oldInter;
+		return false;
+	}
+
+	// Check if new certificate is already expired
+	if (qscCert.expiryDate() < QDateTime::currentDateTime()) {
+		log("Reloaded certificate is already expired");
+		qscCert         = oldCert;
+		qskKey          = oldKey;
+		qlIntermediates = oldInter;
+		return false;
+	}
+
 	// Check if certificate actually changed
 	if (qscCert == oldCert) {
 		// Certificate hasn't changed on disk
 		return false;
 	}

 	// Successfully reloaded new certificate
 	return true;
 }
🤖 Prompt for AI Agents 
In src/murmur/Cert.cpp around lines 257-297, add null-pointer, validation and
locking: first check Meta::mp is not null before calling loadSSLSettings or
accessing its members and return false with a log if it is null; wrap all
reads/writes to qscCert, qskKey and qlIntermediates with the same mutex used
elsewhere for certificate state (use the appropriate lock/guard to ensure
thread-safety); after copying Meta::mp->qscCert/qskKey/qlIntermediates validate
the new certificate by ensuring it is not null, the key matches the certificate
(call isKeyForCert or the existing helper), the certificate parses correctly and
is not expired (check its effective/expiry dates), and if any validation fails
restore oldCert/oldKey/oldInter, unlock and return false; only return true after
successful validation and replacing the state while holding the lock.

25 changes: 13 additions & 12 deletions src/murmur/Server.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,8 +135,8 @@ Server::Server(unsigned int snum, const ::mumble::db::ConnectionParameter &conne
int tcpsock = static_cast< int >(ss->socketDescriptor());
socklen_t len = sizeof(addr);
#else
SOCKET tcpsock = ss->socketDescriptor();
int len = sizeof(addr);
SOCKET tcpsock = ss->socketDescriptor();
int len = sizeof(addr);
#endif
memset(&addr, 0, sizeof(addr));
getsockname(tcpsock, reinterpret_cast< struct sockaddr * >(&addr), &len);
Expand Down Expand Up @@ -254,6 +254,7 @@ Server::Server(unsigned int snum, const ::mumble::db::ConnectionParameter &conne
initZeroconf();
#endif
initRegister();
initCertMonitoring();
}
}

Expand Down Expand Up @@ -733,8 +734,8 @@ void Server::udpActivated(int socket) {
int fromlen = static_cast< int >(sizeof(from));
SOCKET sock = static_cast< SOCKET >(socket);
len = ::recvfrom(sock, reinterpret_cast< char * >(m_udpDecoder.getBuffer().data()),
static_cast< int >(m_udpDecoder.getBuffer().size()), 0,
reinterpret_cast< struct sockaddr * >(&from), &fromlen);
static_cast< int >(m_udpDecoder.getBuffer().size()), 0,
reinterpret_cast< struct sockaddr * >(&from), &fromlen);
#endif

std::span< Mumble::Protocol::byte > inputData(&m_udpDecoder.getBuffer()[0], static_cast< std::size_t >(len));
Expand All @@ -752,13 +753,13 @@ void Server::udpActivated(int socket) {
::sendmsg(sock, &msg, 0);
#else
# ifdef Q_OS_WIN
using size_type = int;
using size_type = int;
# else
using size_type = std::size_t;
# endif
::sendto(sock, reinterpret_cast< const char * >(encodedPing.data()),
static_cast< size_type >(encodedPing.size()), 0, reinterpret_cast< struct sockaddr * >(&from),
fromlen);
::sendto(sock, reinterpret_cast< const char * >(encodedPing.data()),
static_cast< size_type >(encodedPing.size()), 0, reinterpret_cast< struct sockaddr * >(&from),
fromlen);
#endif
}
}
Expand Down Expand Up @@ -1068,7 +1069,7 @@ void Server::sendMessage(ServerUser &u, const unsigned char *data, int len, QByt
#else
std::vector< char > bufVec;
bufVec.resize(static_cast< std::size_t >(len + 4));
char *buffer = bufVec.data();
char *buffer = bufVec.data();
#endif
{
QMutexLocker wl(&u.qmCrypt);
Expand Down Expand Up @@ -1669,9 +1670,9 @@ void Server::connectionClosed(QAbstractSocket::SocketError err, const QString &r
qhUsers.remove(u->uiSession);
qhHostUsers[u->haAddress].remove(u);

quint16 port = (u->saiUdpAddress.ss_family == AF_INET6)
? (reinterpret_cast< sockaddr_in6 * >(&u->saiUdpAddress)->sin6_port)
: (reinterpret_cast< sockaddr_in * >(&u->saiUdpAddress)->sin_port);
quint16 port = (u->saiUdpAddress.ss_family == AF_INET6)
? (reinterpret_cast< sockaddr_in6 * >(&u->saiUdpAddress)->sin6_port)
: (reinterpret_cast< sockaddr_in * >(&u->saiUdpAddress)->sin_port);
const QPair< HostAddress, quint16 > &key = QPair< HostAddress, quint16 >(u->haAddress, port);
qhPeerUsers.remove(key);

Expand Down
6 changes: 6 additions & 0 deletions src/murmur/Server.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,10 @@ class Server : public QThread {
QTimer qtTick;
void initRegister();

// Certificate monitoring, implementation in Cert.cpp
QTimer qtCertCheck;
void initCertMonitoring();

WhisperTargetCache createWhisperTargetCacheFor(ServerUser &speaker, const WhisperTarget &target);

private:
Expand All @@ -223,6 +227,7 @@ public slots:
void regSslError(const QList< QSslError > &);
void finished();
void update();
void checkCertExpiry();

// Certificate stuff, implemented partially in Cert.cpp
public:
Expand All @@ -233,6 +238,7 @@ public slots:
/// If no valid private key is found, a null QSslKey is returned.
static QSslKey privateKeyFromPEM(const QByteArray &buf, const QByteArray &pass = QByteArray());
void initializeCert();
bool reloadCertFromDisk();
const QString getDigest() const;

public slots:
Expand Down
Loading