diff --git a/lib/base/tlsutility.cpp b/lib/base/tlsutility.cpp index fb60e0221..fce718126 100644 --- a/lib/base/tlsutility.cpp +++ b/lib/base/tlsutility.cpp @@ -985,27 +985,47 @@ String BinaryToHex(const unsigned char* data, size_t length) { bool VerifyCertificate(const std::shared_ptr &caCertificate, const std::shared_ptr &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 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, 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); + std::unique_ptr csc{X509_STORE_CTX_new(), &X509_STORE_CTX_free}; + X509_STORE_CTX_init(csc.get(), store.get(), certificate, nullptr); - int rc = X509_verify_cert(csc); - - 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") 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()