From 4023128be42b18a011dda71ddee9ca79955b89cb Mon Sep 17 00:00:00 2001 From: Julian Brost Date: Wed, 14 May 2025 11:17:22 +0200 Subject: [PATCH] VerifyCertificate: Work around issue in OpenSSL < 1.1.0 causing invalid certifcates being treated as valid Old versions of OpenSSL stored a valid flag in the certificate (see inline code comment for details) that if already set, causes parts of the verification to be skipped and return that the certificate is valid, even if it's not actually signed by the CA in the trust store. This issue was assigned CVE-2025-48057. --- lib/base/tlsutility.cpp | 27 +++++++++++++++++++++++++-- lib/base/tlsutility.hpp | 1 + test/CMakeLists.txt | 1 + test/base-tlsutility.cpp | 20 ++++++++++++++++++++ 4 files changed, 47 insertions(+), 2 deletions(-) diff --git a/lib/base/tlsutility.cpp b/lib/base/tlsutility.cpp index 9942e0921..fce718126 100644 --- a/lib/base/tlsutility.cpp +++ b/lib/base/tlsutility.cpp @@ -985,19 +985,42 @@ String BinaryToHex(const unsigned char* data, size_t length) { bool VerifyCertificate(const std::shared_ptr &caCertificate, const std::shared_ptr &certificate, const String& crlFile) { + 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 copy = StringToCertificate(CertificateToString(certificate)); + VERIFY(copy.get() != certificate); + certificate = copy.get(); +#endif + std::unique_ptr store{X509_STORE_new(), &X509_STORE_free}; if (!store) return false; - X509_STORE_add_cert(store.get(), caCertificate.get()); + X509_STORE_add_cert(store.get(), caCertificate); if (!crlFile.IsEmpty()) { AddCRLToSSLContext(store.get(), crlFile); } std::unique_ptr csc{X509_STORE_CTX_new(), &X509_STORE_CTX_free}; - X509_STORE_CTX_init(csc.get(), store.get(), certificate.get(), nullptr); + X509_STORE_CTX_init(csc.get(), store.get(), certificate, nullptr); int rc = X509_verify_cert(csc.get()); diff --git a/lib/base/tlsutility.hpp b/lib/base/tlsutility.hpp index b06412020..a498eca12 100644 --- a/lib/base/tlsutility.hpp +++ b/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& caCertificate, const std::shared_ptr& certificate, const String& crlFile); +bool VerifyCertificate(X509* caCertificate, X509* certificate, const String& crlFile); bool IsCa(const std::shared_ptr& cacert); int GetCertificateVersion(const std::shared_ptr& cert); String GetSignatureAlgorithm(const std::shared_ptr& cert); diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 0e2a2976e..c87679b08 100644 --- a/test/CMakeLists.txt +++ b/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 diff --git a/test/base-tlsutility.cpp b/test/base-tlsutility.cpp index 2e611e49a..4aa4f646f 100644 --- a/test/base-tlsutility.cpp +++ b/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()