Feature/certificate verification levels (#150)

- Added Certificate verification levels: - None: NO certificate signing check, NO certificate hostname check - Minimal: certificate signing check, NO certificate hostname check - Strict: certificate signing check, certificate hostname check - Tested on a mock nzbget NNTP nserv server with self-signed certificate and got expected results: - "Strict" -> test failed - "Minimal" -> test failed - "None" -> test passed" Co-authored-by: ureyNZB <yuriy@nzbget.com>
nzbgetcom · Feb 2, 2024 · 5956a17 · 5956a17
1 parent 16ab25d
commit 5956a17
Show file tree

Hide file tree

Showing 14 changed files with 100 additions and 21 deletions.
diff --git a/daemon/connect/Connection.cpp b/daemon/connect/Connection.cpp
@@ -23,6 +23,7 @@
 #include "Connection.h"
 #include "Log.h"
 #include "FileSystem.h"
+#include "Options.h"
 
 static const int CONNECTION_READBUFFER_SIZE = 1024;
 #ifndef HAVE_GETADDRINFO
@@ -120,6 +121,9 @@ Connection::Connection(const char* host, int port, bool tls) :
 	debug("Creating Connection");
 
 	m_readBuf.Reserve(CONNECTION_READBUFFER_SIZE + 1);
+#ifndef DISABLE_TLS
+	m_certVerifLevel = Options::ECertVerifLevel::cvStrict;
+#endif
 }
 
 Connection::Connection(SOCKET socket, bool tls)
@@ -138,6 +142,7 @@ Connection::Connection(SOCKET socket, bool tls)
 #ifndef DISABLE_TLS
 	m_tlsSocket = nullptr;
 	m_tlsError = false;
+	m_certVerifLevel = Options::ECertVerifLevel::cvStrict;
 #endif
 }
 
@@ -992,7 +997,7 @@ bool Connection::StartTls(bool isClient, const char* certFile, const char* keyFi
 {
 	debug("Starting TLS");
 
-	m_tlsSocket = std::make_unique<ConTlsSocket>(m_socket, isClient, m_host, certFile, keyFile, m_cipher, this);
+	m_tlsSocket = std::make_unique<ConTlsSocket>(m_socket, isClient, m_host, certFile, keyFile, m_cipher, m_certVerifLevel, this);
 	m_tlsSocket->SetSuppressErrors(m_suppressErrors);
 
 	return m_tlsSocket->Start();

diff --git a/daemon/connect/Connection.h b/daemon/connect/Connection.h
@@ -84,6 +84,7 @@ class Connection
 	void SetForceClose(bool forceClose) { m_forceClose = forceClose; }
 #ifndef DISABLE_TLS
 	bool StartTls(bool isClient, const char* certFile, const char* keyFile);
+	void SetCertVerifLevel(unsigned int level) { m_certVerifLevel = level; }
 #endif
 	int FetchTotalBytesRead();
 
@@ -104,6 +105,9 @@ class Connection
 	int m_totalBytesRead = 0;
 	bool m_gracefull = false;
 	bool m_forceClose = false;
+#ifndef DISABLE_TLS
+	unsigned int m_certVerifLevel;
+#endif
 
 	struct SockAddr
 	{
@@ -119,8 +123,9 @@ class Connection
 	{
 	public:
 		ConTlsSocket(SOCKET socket, bool isClient, const char* host,
-			const char* certFile, const char* keyFile, const char* cipher, Connection* owner) :
-			TlsSocket(socket, isClient, host, certFile, keyFile, cipher), m_owner(owner) {}
+			const char* certFile, const char* keyFile, const char* cipher,
+			unsigned int certVerifLevel, Connection* owner) :
+			TlsSocket(socket, isClient, host, certFile, keyFile, cipher, certVerifLevel), m_owner(owner) {}
 	protected:
 		virtual void PrintError(const char* errMsg) { m_owner->PrintError(errMsg); }
 	private:

diff --git a/daemon/connect/TlsSocket.cpp b/daemon/connect/TlsSocket.cpp
@@ -27,6 +27,7 @@
 #include "Log.h"
 #include "Util.h"
 #include "FileSystem.h"
+#include "Options.h"
 
 CString TlsSocket::m_certStore;
 
@@ -419,8 +420,14 @@ bool TlsSocket::Start()
 			Close();
 			return false;
 		}
-
-		SSL_CTX_set_verify((SSL_CTX*)m_context, SSL_VERIFY_PEER, nullptr);
+		if (m_certVerifLevel > Options::ECertVerifLevel::cvNone)
+		{
+			SSL_CTX_set_verify((SSL_CTX*)m_context, SSL_VERIFY_PEER, nullptr);
+		}
+		else
+		{
+			SSL_CTX_set_verify((SSL_CTX*)m_context, SSL_VERIFY_NONE, nullptr);
+		}
 	}
 
 	m_session = SSL_new((SSL_CTX*)m_context);
@@ -453,7 +460,7 @@ bool TlsSocket::Start()
 	}
 
 	int error_code = m_isClient ? SSL_connect((SSL*)m_session) : SSL_accept((SSL*)m_session);
-	if (error_code < 1)
+	if (error_code < 1 && m_certVerifLevel > Options::ECertVerifLevel::cvNone)
 	{
 		long verifyRes = SSL_get_verify_result((SSL*)m_session);
 		if (verifyRes != X509_V_OK)
@@ -567,7 +574,7 @@ bool TlsSocket::ValidateCert()
 
 #ifdef HAVE_X509_CHECK_HOST
 	// hostname verification
-	if (!m_host.Empty() && X509_check_host(cert, m_host, m_host.Length(), 0, nullptr) != 1)
+	if (m_certVerifLevel > Options::ECertVerifLevel::cvMinimal && !m_host.Empty() && X509_check_host(cert, m_host, m_host.Length(), 0, nullptr) != 1)
 	{
 		const unsigned char* certHost = nullptr;
         // Find the position of the CN field in the Subject field of the certificate

diff --git a/daemon/connect/TlsSocket.h b/daemon/connect/TlsSocket.h
@@ -28,9 +28,9 @@ class TlsSocket
 {
 public:
 	TlsSocket(SOCKET socket, bool isClient, const char* host,
-		const char* certFile, const char* keyFile, const char* cipher) :
+		const char* certFile, const char* keyFile, const char* cipher, int certVerifLevel) :
 		m_socket(socket), m_isClient(isClient), m_host(host),
-		m_certFile(certFile), m_keyFile(keyFile), m_cipher(cipher) {}
+		m_certFile(certFile), m_keyFile(keyFile), m_cipher(cipher), m_certVerifLevel(certVerifLevel) {}
 	virtual ~TlsSocket();
 	static void Init();
 	static void InitOptions(const char* certStore) { m_certStore = certStore; }
@@ -56,6 +56,7 @@ class TlsSocket
 	bool m_connected = false;
 	int m_retCode;
 	static CString m_certStore;
+	int m_certVerifLevel;
 
 	// using "void*" to prevent the including of GnuTLS/OpenSSL header files into TlsSocket.h
 	void* m_context = nullptr;

diff --git a/daemon/main/Options.cpp b/daemon/main/Options.cpp
@@ -1011,6 +1011,16 @@ void Options::InitServers()
 			m_tls |= tls;
 		}
 
+		const char* ncertveriflevel = GetOption(BString<100>("Server%i.CertVerification", n));
+		int certveriflevel = ECertVerifLevel::cvStrict;
+		if (ncertveriflevel)
+		{
+			const char* CertVerifNames[] = { "none", "minimal", "strict" };
+			const int CertVerifValues[] = { ECertVerifLevel::cvNone, ECertVerifLevel::cvMinimal, ECertVerifLevel::cvStrict };
+			const int CertVerifCount = ECertVerifLevel::Count;
+			certveriflevel = ParseEnumValue(BString<100>("Server%i.CertVerification", n), CertVerifCount, CertVerifNames, CertVerifValues);
+		}
+
 		const char* nipversion = GetOption(BString<100>("Server%i.IpVersion", n));
 		int ipversion = 0;
 		if (nipversion)
@@ -1048,7 +1058,7 @@ void Options::InitServers()
 					nretention ? atoi(nretention) : 0,
 					nlevel ? atoi(nlevel) : 0,
 					ngroup ? atoi(ngroup) : 0,
-					optional);
+					optional, certveriflevel);
 			}
 		}
 		else
@@ -1539,7 +1549,8 @@ bool Options::ValidateOptionName(const char* optname, const char* optvalue)
 			!strcasecmp(p, ".encryption") || !strcasecmp(p, ".connections") ||
 			!strcasecmp(p, ".cipher") || !strcasecmp(p, ".group") ||
 			!strcasecmp(p, ".retention") || !strcasecmp(p, ".optional") ||
-			!strcasecmp(p, ".notes") || !strcasecmp(p, ".ipversion")))
+			!strcasecmp(p, ".notes") || !strcasecmp(p, ".ipversion") ||
+			!strcasecmp(p, ".certverification")))
 		{
 			return true;
 		}

diff --git a/daemon/main/Options.h b/daemon/main/Options.h
@@ -98,6 +98,13 @@ class Options
 		nfArticle,
 		nfNzb
 	};
+	enum ECertVerifLevel
+	{
+		cvNone,
+		cvMinimal,
+		cvStrict,
+		Count
+	};
 
 	class OptEntry
 	{
@@ -169,7 +176,7 @@ class Options
 		virtual void AddNewsServer(int id, bool active, const char* name, const char* host,
 			int port, int ipVersion, const char* user, const char* pass, bool joinGroup,
 			bool tls, const char* cipher, int maxConnections, int retention,
-			int level, int group, bool optional) = 0;
+			int level, int group, bool optional, unsigned int certVerificationfLevel) = 0;
 		virtual void AddFeed(int id, const char* name, const char* url, int interval,
 			const char* filter, bool backlog, bool pauseNzb, const char* category,
 			int priority, const char* extensions) {}

diff --git a/daemon/main/nzbget.cpp b/daemon/main/nzbget.cpp
@@ -164,7 +164,7 @@ class NZBGet : public Options::Extender
 	virtual void AddNewsServer(int id, bool active, const char* name, const char* host,
 		int port, int ipVersion, const char* user, const char* pass, bool joinGroup,
 		bool tls, const char* cipher, int maxConnections, int retention,
-		int level, int group, bool optional);
+		int level, int group, bool optional, unsigned int certVerificationfLevel);
 	virtual void AddFeed(int id, const char* name, const char* url, int interval,
 		const char* filter, bool backlog, bool pauseNzb, const char* category,
 		int priority, const char* feedScript);
@@ -991,10 +991,11 @@ void NZBGet::Daemonize()
 
 void NZBGet::AddNewsServer(int id, bool active, const char* name, const char* host,
 	int port, int ipVersion, const char* user, const char* pass, bool joinGroup, bool tls,
-	const char* cipher, int maxConnections, int retention, int level, int group, bool optional)
+	const char* cipher, int maxConnections, int retention, int level, int group, bool optional,
+	unsigned int certVerificationfLevel)
 {
 	m_serverPool->AddServer(std::make_unique<NewsServer>(id, active, name, host, port, ipVersion, user, pass, joinGroup,
-		tls, cipher, maxConnections, retention, level, group, optional));
+		tls, cipher, maxConnections, retention, level, group, optional, certVerificationfLevel));
 }
 
 void NZBGet::AddFeed(int id, const char* name, const char* url, int interval, const char* filter,

diff --git a/daemon/nntp/ArticleDownloader.cpp b/daemon/nntp/ArticleDownloader.cpp
@@ -116,6 +116,10 @@ void ArticleDownloader::Run()
 
 		m_connection->SetSuppressErrors(false);
 
+#ifndef DISABLE_TLS
+		m_connection->SetCertVerifLevel(lastServer->GetCertVerificationLevel());
+#endif
+
 		m_connectionName.Format("%s (%s)",
 			m_connection->GetNewsServer()->GetName(), m_connection->GetHost());
 

diff --git a/daemon/nntp/NewsServer.cpp b/daemon/nntp/NewsServer.cpp
@@ -24,11 +24,11 @@
 
 NewsServer::NewsServer(int id, bool active, const char* name, const char* host, int port, int ipVersion,
 	const char* user, const char* pass, bool joinGroup, bool tls, const char* cipher,
-	int maxConnections, int retention, int level, int group, bool optional) :
+	int maxConnections, int retention, int level, int group, bool optional, unsigned int certVerificationfLevel) :
 		m_id(id), m_active(active), m_name(name), m_host(host ? host : ""), m_port(port), m_ipVersion(ipVersion),
 		m_user(user ? user : ""), m_password(pass ? pass : ""), m_joinGroup(joinGroup), m_tls(tls),
 		m_cipher(cipher ? cipher : ""), m_maxConnections(maxConnections), m_retention(retention),
-		m_level(level), m_normLevel(level), m_group(group), m_optional(optional)
+		m_level(level), m_normLevel(level), m_group(group), m_optional(optional), m_certVerificationfLevel(certVerificationfLevel)
 {
 	if (m_name.Empty())
 	{

diff --git a/daemon/nntp/NewsServer.h b/daemon/nntp/NewsServer.h
@@ -35,7 +35,7 @@ class NewsServer
 	NewsServer(int id, bool active, const char* name, const char* host, int port, int ipVersion,
 		const char* user, const char* pass, bool joinGroup,
 		bool tls, const char* cipher, int maxConnections, int retention,
-		int level, int group, bool optional);
+		int level, int group, bool optional, unsigned int certVerificationfLevel);
 	int GetId() { return m_id; }
 	int GetStateId() { return m_stateId; }
 	void SetStateId(int stateId) { m_stateId = stateId; }
@@ -59,6 +59,7 @@ class NewsServer
 	bool GetOptional() { return m_optional; }
 	time_t GetBlockTime() { return m_blockTime; }
 	void SetBlockTime(time_t blockTime) { m_blockTime = blockTime; }
+	unsigned int GetCertVerificationLevel() { return m_certVerificationfLevel; }
 
 private:
 	int m_id;
@@ -80,6 +81,7 @@ class NewsServer
 	int m_group;
 	bool m_optional = false;
 	time_t m_blockTime = 0;
+	unsigned int m_certVerificationfLevel;
 };
 
 typedef std::vector<std::unique_ptr<NewsServer>> Servers;

diff --git a/daemon/nntp/NntpConnection.cpp b/daemon/nntp/NntpConnection.cpp
@@ -34,6 +34,10 @@ NntpConnection::NntpConnection(NewsServer* newsServer) :
 	SetCipher(newsServer->GetCipher());
 	SetIPVersion(newsServer->GetIpVersion() == 4 ? Connection::ipV4 :
 		newsServer->GetIpVersion() == 6 ? Connection::ipV6 : Connection::ipAuto);
+
+#ifndef DISABLE_TLS
+	SetCertVerifLevel(newsServer->GetCertVerificationLevel());
+#endif
 }
 
 const char* NntpConnection::Request(const char* req)

diff --git a/daemon/remote/XmlRpc.cpp b/daemon/remote/XmlRpc.cpp
@@ -3296,16 +3296,25 @@ void TestServerXmlCommand::Execute()
 	bool encryption;
 	char* cipher;
 	int timeout;
+	int certVerifLevel;
 
 	if (!NextParamAsStr(&host) || !NextParamAsInt(&port) || !NextParamAsStr(&username) ||
 		!NextParamAsStr(&password) || !NextParamAsBool(&encryption) ||
-		!NextParamAsStr(&cipher) || !NextParamAsInt(&timeout))
+		!NextParamAsStr(&cipher) || !NextParamAsInt(&timeout) ||
+		!NextParamAsInt(&certVerifLevel))
 	{
 		BuildErrorResponse(2, "Invalid parameter");
 		return;
 	}
 
-	NewsServer server(0, true, "test server", host, port, 0, username, password, false, encryption, cipher, 1, 0, 0, 0, false);
+	if (certVerifLevel < 0 || certVerifLevel >= Options::ECertVerifLevel::Count)
+	{
+		BuildErrorResponse(2, "Invalid parameter (Certificate Verification Level).");
+		return;
+	}
+
+	NewsServer server(0, true, "test server", host, port, 0, username, password, false, 
+					encryption, cipher, 1, 0, 0, 0, false, certVerifLevel);
 	TestConnection connection(&server, this);
 	connection.SetTimeout(timeout == 0 ? g_Options->GetArticleTimeout() : timeout);
 	connection.SetSuppressErrors(false);

diff --git a/nzbget.conf b/nzbget.conf
@@ -243,6 +243,15 @@ Server1.Connections=8
 # Value "0" disables retention check.
 Server1.Retention=0
 
+# Certificate verification level (Strict, Minimal, None).
+#
+# None - NO certificate signing check, NO certificate hostname check
+#
+# Minimal - certificate signing check, NO certificate hostname check
+#
+# Strict - certificate signing check, certificate hostname check
+Server1.CertVerification=strict
+
 # IP protocol version (auto, ipv4, ipv6).
 Server1.IpVersion=auto
 

diff --git a/webui/config.js b/webui/config.js
@@ -1581,14 +1581,16 @@ var Config = (new function($)
 		$('#Notif_Config_TestConnectionProgress').fadeIn(function() {
 			var multiid = parseInt($(control).attr('data-multiid'));
 			var timeout = Math.min(parseInt(getOptionValue(findOptionByName('ArticleTimeout'))), 10);
+			var certStrictLevel = getCertStrictLevel(getOptionValue(findOptionByName('Server' + multiid + '.CertVerification')));
 			RPC.call('testserver', [
 				getOptionValue(findOptionByName('Server' + multiid + '.Host')),
 				parseInt(getOptionValue(findOptionByName('Server' + multiid + '.Port'))),
 				getOptionValue(findOptionByName('Server' + multiid + '.Username')),
 				getOptionValue(findOptionByName('Server' + multiid + '.Password')),
 				getOptionValue(findOptionByName('Server' + multiid + '.Encryption')) === 'yes',
 				getOptionValue(findOptionByName('Server' + multiid + '.Cipher')),
-				timeout
+				timeout,
+				certStrictLevel
 				],
 				function(errtext) {
 					$('#Notif_Config_TestConnectionProgress').fadeOut(function() {
@@ -1799,6 +1801,18 @@ var Config = (new function($)
 		}
 	}
 
+	function getCertStrictLevel(strictLevel)
+	{
+		var level = strictLevel.toLowerCase();
+		switch(level)
+		{
+			case "none": return 0;
+			case "minimal": return 1;
+			case "strict": return 2;
+			default: return 2;
+		}
+	}
+
 	function showSaveBanner()
 	{
 		$('#Config_Save').attr('disabled', 'disabled');