Merge commit from fork

Fix for master

Author: yhabteab

lib/base/tlsutility.cpp

@@ -985,27 +985,47 @@ String BinaryToHex(const unsigned char* data, size_t length) {
 
 bool VerifyCertificate(const std::shared_ptr<X509> &caCertificate, const std::shared_ptr<X509> &certificate, const String& crlFile)
 {
-	X509_STORE *store = X509_STORE_new();
+	return VerifyCertificate(caCertificate.get(), certificate.get(), crlFile);
+}
+
+bool VerifyCertificate(X509* caCertificate, X509* certificate, const String& crlFile)
+{
+#if OPENSSL_VERSION_NUMBER < 0x10100000L
+	/*
+	 * OpenSSL older than version 1.1.0 stored a valid flag in the struct behind X509* which leads to certain validation
+	 * steps to be skipped on subsequent verification operations. If a certificate is verified multiple times with a
+	 * different configuration, for example with different trust anchors, this can result in the certificate
+	 * incorrectly being treated as valid.
+	 *
+	 * This issue is worked around by serializing and deserializing the certificate which creates a new struct instance
+	 * with the valid flag cleared, hence performing the full validation.
+	 *
+	 * The flag in question was removed in OpenSSL 1.1.0, so this extra step isn't necessary for more recent versions:
+	 * https://github.com/openssl/openssl/commit/0e76014e584ba78ef1d6ecb4572391ef61c4fb51
+	 */
+	std::shared_ptr<X509> copy = StringToCertificate(CertificateToString(certificate));
+	VERIFY(copy.get() != certificate);
+	certificate = copy.get();
+#endif
+
+	std::unique_ptr<X509_STORE, decltype(&X509_STORE_free)> store{X509_STORE_new(), &X509_STORE_free};
 
 	if (!store)
 		return false;
 
-	X509_STORE_add_cert(store, caCertificate.get());
+	X509_STORE_add_cert(store.get(), caCertificate);
 
 	if (!crlFile.IsEmpty()) {
-		AddCRLToSSLContext(store, crlFile);
+		AddCRLToSSLContext(store.get(), crlFile);
 	}
 
-	X509_STORE_CTX *csc = X509_STORE_CTX_new();
-	X509_STORE_CTX_init(csc, store, certificate.get(), nullptr);
-
-	int rc = X509_verify_cert(csc);
+	std::unique_ptr<X509_STORE_CTX, decltype(&X509_STORE_CTX_free)> csc{X509_STORE_CTX_new(), &X509_STORE_CTX_free};
+	X509_STORE_CTX_init(csc.get(), store.get(), certificate, nullptr);
 
-	X509_STORE_CTX_free(csc);
-	X509_STORE_free(store);
+	int rc = X509_verify_cert(csc.get());
 
 	if (rc == 0) {
-		int err = X509_STORE_CTX_get_error(csc);
+		int err = X509_STORE_CTX_get_error(csc.get());
 
 		BOOST_THROW_EXCEPTION(openssl_error()
 			<< boost::errinfo_api_function("X509_verify_cert")

lib/base/tlsutility.hpp

@@ -79,6 +79,7 @@ String RandomString(int length);
 String BinaryToHex(const unsigned char* data, size_t length);
 
 bool VerifyCertificate(const std::shared_ptr<X509>& caCertificate, const std::shared_ptr<X509>& certificate, const String& crlFile);
+bool VerifyCertificate(X509* caCertificate, 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);

test/CMakeLists.txt

@@ -178,6 +178,7 @@ add_boost_test(base
     base_tlsutility/iscertuptodate_ok
     base_tlsutility/iscertuptodate_expiring
     base_tlsutility/iscertuptodate_old
+    base_tlsutility/VerifyCertificate_revalidate
     base_utility/parse_version
     base_utility/compare_version
     base_utility/comparepasswords_works

test/base-tlsutility.cpp

@@ -132,4 +132,24 @@ BOOST_AUTO_TEST_CASE(iscertuptodate_old)
 	})));
 }
 
+BOOST_AUTO_TEST_CASE(VerifyCertificate_revalidate)
+{
+	X509_NAME *caSubject = X509_NAME_new();
+	X509_NAME_add_entry_by_txt(caSubject, "CN", MBSTRING_ASC, (const unsigned char*)"Icinga CA", -1, -1, 0);
+
+	auto signingCaKey = GenKeypair();
+	auto signingCaCert = CreateCert(signingCaKey, caSubject, caSubject, signingCaKey, true);
+
+	X509_NAME *leafSubject = X509_NAME_new();
+	X509_NAME_add_entry_by_txt(leafSubject, "CN", MBSTRING_ASC, (const unsigned char*)"Leaf Certificate", -1, -1, 0);
+	auto leafKey = GenKeypair();
+	auto leafCert = CreateCert(leafKey, leafSubject, caSubject, signingCaKey, false);
+	BOOST_CHECK(VerifyCertificate(signingCaCert, leafCert, ""));
+
+	// Create a second CA with a different key, the leaf certificate is supposed to fail validation against that CA.
+	auto otherCaKey = GenKeypair();
+	auto otherCaCert = CreateCert(otherCaKey, caSubject, caSubject, otherCaKey, true);
+	BOOST_CHECK_THROW(VerifyCertificate(otherCaCert, leafCert, ""), openssl_error);
+}
+
 BOOST_AUTO_TEST_SUITE_END()