Merge pull request from GHSA-pcmr-2p2f-r7j6

Verify certificates against CRL before renewing them (2.12)

CHANGELOG.md

@@ -9,9 +9,25 @@ Released closed milestones can be found on [GitHub](https://github.com/Icinga/ic
 
 ## 2.12.3 (2020-12-15)
 
-Version 2.12.3 mainly focuses on resolving issues with high load on Windows regarding the config sync
+Version 2.12.3 resolves a security vulnerability with revoked certificates being
+renewed automatically ignoring the CRL.
+
+This version also resolves issues with high load on Windows regarding the config sync
 and not being able to disable/enable Icinga 2 features over the API.
 
+### Security
+
+* Fix that revoked certificates due for renewal will automatically be renewed ignoring the CRL (CVE-2020-29663)
+
+When a CRL is specified in the ApiListener configuration, Icinga 2 only used it
+when connections were established so far, but not when a certificate is requested.
+This allows a node to automatically renew a revoked certificate if it meets the
+other conditions for auto renewal (issued before 2017 or expires in less than 30 days).
+
+Because Icinga 2 currently (v2.12.3 and earlier) uses a validity duration of 15 years,
+this only affects setups with external certificate signing and revoked certificates
+that expire in less then 30 days.
+
 ### Bugfixes
 
 * Improve config sync locking - resolves high load issues on Windows #8511

lib/base/tlsutility.cpp

@@ -248,15 +248,26 @@ void SetTlsProtocolminToSSLContext(const Shared<boost::asio::ssl::context>::Ptr&
 }
 
 /**
- * Loads a CRL and appends its certificates to the specified SSL context.
+ * Loads a CRL and appends its certificates to the specified Boost SSL context.
  *
  * @param context The SSL context.
  * @param crlPath The path to the CRL file.
  */
 void AddCRLToSSLContext(const Shared<boost::asio::ssl::context>::Ptr& context, const String& crlPath)
 {
-	char errbuf[256];
 	X509_STORE *x509_store = SSL_CTX_get_cert_store(context->native_handle());
+	AddCRLToSSLContext(x509_store, crlPath);
+}
+
+/**
+ * Loads a CRL and appends its certificates to the specified OpenSSL X509 store.
+ *
+ * @param context The SSL context.
+ * @param crlPath The path to the CRL file.
+ */
+void AddCRLToSSLContext(X509_STORE *x509_store, const String& crlPath)
+{
+	char errbuf[256];
 
 	X509_LOOKUP *lookup;
 	lookup = X509_STORE_add_lookup(x509_store, X509_LOOKUP_file());
@@ -833,7 +844,7 @@ String RandomString(int length)
 	return result;
 }
 
-bool VerifyCertificate(const std::shared_ptr<X509>& caCertificate, const std::shared_ptr<X509>& certificate)
+bool VerifyCertificate(const std::shared_ptr<X509> &caCertificate, const std::shared_ptr<X509> &certificate, const String& crlFile)
 {
 	X509_STORE *store = X509_STORE_new();
 
@@ -842,6 +853,10 @@ bool VerifyCertificate(const std::shared_ptr<X509>& caCertificate, const std::sh
 
 	X509_STORE_add_cert(store, caCertificate.get());
 
+	if (!crlFile.IsEmpty()) {
+		AddCRLToSSLContext(store, crlFile);
+	}
+
 	X509_STORE_CTX *csc = X509_STORE_CTX_new();
 	X509_STORE_CTX_init(csc, store, certificate.get(), nullptr);
 

lib/base/tlsutility.hpp

@@ -30,6 +30,7 @@ String GetOpenSSLVersion();
 
 Shared<boost::asio::ssl::context>::Ptr MakeAsioSslContext(const String& pubkey = String(), const String& privkey = String(), const String& cakey = String());
 void AddCRLToSSLContext(const Shared<boost::asio::ssl::context>::Ptr& context, const String& crlPath);
+void AddCRLToSSLContext(X509_STORE *x509_store, const String& crlPath);
 void SetCipherListToSSLContext(const Shared<boost::asio::ssl::context>::Ptr& context, const String& cipherList);
 void SetTlsProtocolminToSSLContext(const Shared<boost::asio::ssl::context>::Ptr& context, const String& tlsProtocolmin);
 
@@ -51,7 +52,7 @@ String SHA1(const String& s, bool binary = false);
 String SHA256(const String& s);
 String RandomString(int length);
 
-bool VerifyCertificate(const std::shared_ptr<X509>& caCertificate, const std::shared_ptr<X509>& certificate);
+bool VerifyCertificate(const std::shared_ptr<X509>& caCertificate, const std::shared_ptr<X509>& certificate, const String& crlFile);
 bool IsCa(const std::shared_ptr<X509>& cacert);
 int GetCertificateVersion(const std::shared_ptr<X509>& cert);
 String GetSignatureAlgorithm(const std::shared_ptr<X509>& cert);

lib/cli/pkiverifycommand.cpp

@@ -28,12 +28,13 @@ void PKIVerifyCommand::InitParameters(boost::program_options::options_descriptio
 	visibleDesc.add_options()
 		("cn", po::value<std::string>(), "Common Name (optional). Use with '--cert' to check the CN in the certificate.")
 		("cert", po::value<std::string>(), "Certificate file path (optional). Standalone: print certificate. With '--cacert': Verify against CA.")
-		("cacert", po::value<std::string>(), "CA certificate file path (optional). If passed standalone, verifies whether this is a CA certificate");
+		("cacert", po::value<std::string>(), "CA certificate file path (optional). If passed standalone, verifies whether this is a CA certificate")
+		("crl", po::value<std::string>(), "CRL file path (optional). Check the certificate against this revocation list when verifying against CA.");
 }
 
 std::vector<String> PKIVerifyCommand::GetArgumentSuggestions(const String& argument, const String& word) const
 {
-	if (argument == "cert" || argument == "cacert")
+	if (argument == "cert" || argument == "cacert" || argument == "crl")
 		return GetBashCompletionSuggestions("file", word);
 	else
 		return CLICommand::GetArgumentSuggestions(argument, word);
@@ -46,7 +47,7 @@ std::vector<String> PKIVerifyCommand::GetArgumentSuggestions(const String& argum
  */
 int PKIVerifyCommand::Run(const boost::program_options::variables_map& vm, const std::vector<std::string>& ap) const
 {
-	String cn, certFile, caCertFile;
+	String cn, certFile, caCertFile, crlFile;
 
 	if (vm.count("cn"))
 		cn = vm["cn"].as<std::string>();
@@ -57,6 +58,9 @@ int PKIVerifyCommand::Run(const boost::program_options::variables_map& vm, const
 	if (vm.count("cacert"))
 		caCertFile = vm["cacert"].as<std::string>();
 
+	if (vm.count("crl"))
+		crlFile = vm["crl"].as<std::string>();
+
 	/* Verify CN in certificate. */
 	if (!cn.IsEmpty() && !certFile.IsEmpty()) {
 		std::shared_ptr<X509> cert;
@@ -126,10 +130,15 @@ int PKIVerifyCommand::Run(const boost::program_options::variables_map& vm, const
 		bool signedByCA;
 
 		try {
-			signedByCA = VerifyCertificate(cacert, cert);
+			signedByCA = VerifyCertificate(cacert, cert, crlFile);
 		} catch (const std::exception& ex) {
-			Log(LogCritical, "cli")
-				<< "CRITICAL: Certificate with CN '" << certCN << "' is NOT signed by CA: " << DiagnosticInformation(ex, false);
+			Log logmsg (LogCritical, "cli");
+			logmsg << "CRITICAL: Certificate with CN '" << certCN << "' is NOT signed by CA: ";
+			if (const unsigned long *openssl_code = boost::get_error_info<errinfo_openssl_error>(ex)) {
+				logmsg << X509_verify_cert_error_string(*openssl_code) << " (code " << *openssl_code << ")";
+			} else {
+				logmsg << DiagnosticInformation(ex, false);
+			}
 
 			return ServiceCritical;
 		}

lib/remote/jsonrpcconnection-pki.cpp

@@ -55,13 +55,25 @@ Value RequestCertificateHandler(const MessageOrigin::Ptr& origin, const Dictiona
 
 	bool signedByCA = false;
 
-	try {
-		signedByCA = VerifyCertificate(cacert, cert);
-	} catch (const std::exception&) { } /* Swallow the exception on purpose, cacert will never be a non-CA certificate. */
-
-	Log(LogInformation, "JsonRpcConnection")
-		<< "Received certificate request for CN '" << cn << "'"
-		<< (signedByCA ? "" : " not") << " signed by our CA.";
+	{
+		Log logmsg(LogInformation, "JsonRpcConnection");
+		logmsg << "Received certificate request for CN '" << cn << "'";
+
+		try {
+			signedByCA = VerifyCertificate(cacert, cert, listener->GetCrlPath());
+			if (!signedByCA) {
+				logmsg << " not";
+			}
+			logmsg << " signed by our CA.";
+		} catch (const std::exception &ex) {
+			logmsg << " not signed by our CA";
+			if (const unsigned long *openssl_code = boost::get_error_info<errinfo_openssl_error>(ex)) {
+				logmsg << ": " << X509_verify_cert_error_string(long(*openssl_code)) << " (code " << *openssl_code << ")";
+			} else {
+				logmsg << ".";
+			}
+		}
+	}
 
 	if (signedByCA) {
 		time_t now;
@@ -204,7 +216,7 @@ Value RequestCertificateHandler(const MessageOrigin::Ptr& origin, const Dictiona
 	 * we're using for cluster connections (there's no point in sending a client
 	 * a certificate it wouldn't be able to use to connect to us anyway) */
 	try {
-		if (!VerifyCertificate(cacert, newcert)) {
+		if (!VerifyCertificate(cacert, newcert, listener->GetCrlPath())) {
 			Log(LogWarning, "JsonRpcConnection")
 				<< "The CA in '" << listener->GetDefaultCaPath() << "' does not match the CA which Icinga uses "
 				<< "for its own cluster connections. This is most likely a configuration problem.";