When veryfing a certificate the intermediary subca-s are not trusted …

…any more

Signed-off-by: AssemblyJohn <[email protected]>

include/evse_security/crypto/interface/crypto_supplier.hpp

@@ -57,11 +57,10 @@ class AbstractCryptoSupplier {
     /// @param dir_path     Optional directory path that can be used for certificate store lookup
     /// @param file_path    Optional certificate file path that can be used for certificate store lookup
     /// @return
-    static CertificateValidationResult x509_verify_certificate_chain(X509Handle* target,
-                                                                     const std::vector<X509Handle*>& parents,
-                                                                     bool allow_future_certificates,
-                                                                     const std::optional<fs::path> dir_path,
-                                                                     const std::optional<fs::path> file_path);
+    static CertificateValidationResult
+    x509_verify_certificate_chain(X509Handle* target, const std::vector<X509Handle*>& parents,
+                                  const std::vector<X509Handle*>& untrusted_subcas, bool allow_future_certificates,
+                                  const std::optional<fs::path> dir_path, const std::optional<fs::path> file_path);
 
     /// @brief Checks if the private key is consistent with the provided handle
     static KeyValidationResult x509_check_private_key(X509Handle* handle, std::string private_key,

include/evse_security/crypto/openssl/openssl_supplier.hpp

@@ -31,11 +31,10 @@ class OpenSSLSupplier : public AbstractCryptoSupplier {
     static bool x509_is_child(X509Handle* child, X509Handle* parent);
     static bool x509_is_equal(X509Handle* a, X509Handle* b);
     static X509Handle_ptr x509_duplicate_unique(X509Handle* handle);
-    static CertificateValidationResult x509_verify_certificate_chain(X509Handle* target,
-                                                                     const std::vector<X509Handle*>& parents,
-                                                                     bool allow_future_certificates,
-                                                                     const std::optional<fs::path> dir_path,
-                                                                     const std::optional<fs::path> file_path);
+    static CertificateValidationResult
+    x509_verify_certificate_chain(X509Handle* target, const std::vector<X509Handle*>& parents,
+                                  const std::vector<X509Handle*>& untrusted_subcas, bool allow_future_certificates,
+                                  const std::optional<fs::path> dir_path, const std::optional<fs::path> file_path);
     static KeyValidationResult x509_check_private_key(X509Handle* handle, std::string private_key,
                                                       std::optional<std::string> password);
     static bool x509_verify_signature(X509Handle* handle, const std::vector<std::byte>& signature,

include/evse_security/detail/openssl/openssl_types.hpp

@@ -33,6 +33,13 @@ template <> class std::default_delete<X509_REQ> {
     }
 };
 
+template <> class std::default_delete<STACK_OF(X509)> {
+public:
+    void operator()(STACK_OF(X509) * ptr) const {
+        sk_X509_free(ptr);
+    }
+};
+
 template <> class std::default_delete<EVP_PKEY> {
 public:
     void operator()(EVP_PKEY* ptr) const {
@@ -87,6 +94,9 @@ namespace evse_security {
 using X509_ptr = std::unique_ptr<X509>;
 using X509_STORE_ptr = std::unique_ptr<X509_STORE>;
 using X509_STORE_CTX_ptr = std::unique_ptr<X509_STORE_CTX>;
+// Unsafe since it does not free contained elements, only the stack, the element
+// cleanup has to be done manually
+using X509_STACK_UNSAFE_ptr = std::unique_ptr<STACK_OF(X509)>;
 using X509_REQ_ptr = std::unique_ptr<X509_REQ>;
 using EVP_PKEY_ptr = std::unique_ptr<EVP_PKEY>;
 using EVP_PKEY_CTX_ptr = std::unique_ptr<EVP_PKEY_CTX>;

lib/evse_security/crypto/interface/crypto_supplier.cpp

@@ -76,8 +76,8 @@ static X509Handle_ptr x509_duplicate_unique(X509Handle* handle) {
 }
 
 CertificateValidationResult AbstractCryptoSupplier::x509_verify_certificate_chain(
-    X509Handle* target, const std::vector<X509Handle*>& parents, bool allow_future_certificates,
-    const std::optional<fs::path> dir_path, const std::optional<fs::path> file_path) {
+    X509Handle* target, const std::vector<X509Handle*>& parents, const std::vector<X509Handle*>& untrusted_subcas,
+    bool allow_future_certificates, const std::optional<fs::path> dir_path, const std::optional<fs::path> file_path) {
     default_crypto_supplier_usage_error() return CertificateValidationResult::Unknown;
 }
 

lib/evse_security/crypto/openssl/openssl_supplier.cpp

@@ -560,13 +560,12 @@ X509Handle_ptr OpenSSLSupplier::x509_duplicate_unique(X509Handle* handle) {
     return std::make_unique<X509HandleOpenSSL>(X509_dup(get(handle)));
 }
 
-CertificateValidationResult OpenSSLSupplier::x509_verify_certificate_chain(X509Handle* target,
-                                                                           const std::vector<X509Handle*>& parents,
-                                                                           bool allow_future_certificates,
-                                                                           const std::optional<fs::path> dir_path,
-                                                                           const std::optional<fs::path> file_path) {
+CertificateValidationResult OpenSSLSupplier::x509_verify_certificate_chain(
+    X509Handle* target, const std::vector<X509Handle*>& parents, const std::vector<X509Handle*>& untrusted_subcas,
+    bool allow_future_certificates, const std::optional<fs::path> dir_path, const std::optional<fs::path> file_path) {
     OpenSSLProvider provider;
     provider.set_global_mode(OpenSSLProvider::mode_t::default_provider);
+
     X509_STORE_ptr store_ptr(X509_STORE_new());
     X509_STORE_CTX_ptr store_ctx_ptr(X509_STORE_CTX_new());
 
@@ -591,7 +590,24 @@ CertificateValidationResult OpenSSLSupplier::x509_verify_certificate_chain(X509H
         }
     }
 
-    X509_STORE_CTX_init(store_ctx_ptr.get(), store_ptr.get(), get(target), NULL);
+    X509_STACK_UNSAFE_ptr untrusted = nullptr;
+
+    // Build potentially untrusted intermediary (subca) certificates
+    if (false == untrusted_subcas.empty()) {
+        untrusted = X509_STACK_UNSAFE_ptr(sk_X509_new_null());
+
+        for (auto& untrusted_cert : untrusted_subcas) {
+            if (false == sk_X509_push(untrusted.get(), get(untrusted_cert))) {
+                EVLOG_error << "X509 could not create untrusted store stack!";
+                return CertificateValidationResult::Unknown;
+            }
+        }
+    }
+
+    if (false == X509_STORE_CTX_init(store_ctx_ptr.get(), store_ptr.get(), get(target), untrusted.get())) {
+        EVLOG_error << "X509 could not init x509 store ctx!";
+        return CertificateValidationResult::Unknown;
+    }
 
     if (allow_future_certificates) {
         // Manually check if cert is expired

lib/evse_security/evse_security.cpp

@@ -1261,27 +1261,35 @@ CertificateValidationResult EvseSecurity::verify_certificate_internal(const std:
 
     try {
         X509CertificateBundle certificate(certificate_chain, EncodingFormat::PEM);
+
         std::vector<X509Wrapper> _certificate_chain = certificate.split();
         if (_certificate_chain.empty()) {
             return CertificateValidationResult::Unknown;
         }
 
+        // The leaf is to be verified
         const auto leaf_certificate = _certificate_chain.at(0);
-        std::vector<X509Handle*> parent_certificates;
 
         // Retrieve the hierarchy in order to check if the chain contains a root certificate
         X509CertificateHierarchy& hierarchy = certificate.get_certficate_hierarchy();
 
-        // Make sure that an added root certificate is excluded and taken from the bundle
-        for (size_t i = 1; i < _certificate_chain.size(); i++) {
-            const auto& cert = _certificate_chain[i];
-            if (hierarchy.is_root(cert)) {
-                EVLOG_warning << "Ignore root certificate: " << cert.get_common_name();
-            } else {
-                parent_certificates.emplace_back(cert.get());
+        // Build all untrusted intermediary certificates, and exclude any root
+        std::vector<X509Handle*> untrusted_subcas;
+
+        if (_certificate_chain.size() > 1) {
+            for (size_t i = 1; i < _certificate_chain.size(); i++) {
+                const auto& cert = _certificate_chain[i];
+                if (hierarchy.is_root(cert)) {
+                    EVLOG_warning << "Ignore root certificate: " << cert.get_common_name();
+                } else {
+                    untrusted_subcas.emplace_back(cert.get());
+                }
             }
         }
 
+        // Build the trusted parent certificates from our internal store
+        std::vector<X509Handle*> trusted_parent_certificates;
+
         fs::path root_store = this->ca_bundle_path_map.at(ca_certificate_type);
         CertificateValidationResult validated{};
 
@@ -1296,16 +1304,17 @@ CertificateValidationResult EvseSecurity::verify_certificate_internal(const std:
             std::vector<X509Wrapper> root_chain{roots.split()};
 
             for (size_t i = 0; i < root_chain.size(); i++) {
-                parent_certificates.emplace_back(root_chain[i].get());
+                trusted_parent_certificates.emplace_back(root_chain[i].get());
             }
 
             // The root_chain stores the X509Handler pointers, if this goes out of scope then
             // parent_certificates will point to nothing.
-            validated = CryptoSupplier::x509_verify_certificate_chain(leaf_certificate.get(), parent_certificates, true,
-                                                                      std::nullopt, std::nullopt);
+            validated =
+                CryptoSupplier::x509_verify_certificate_chain(leaf_certificate.get(), trusted_parent_certificates,
+                                                              untrusted_subcas, true, std::nullopt, std::nullopt);
         } else {
-            validated = CryptoSupplier::x509_verify_certificate_chain(leaf_certificate.get(), parent_certificates, true,
-                                                                      std::nullopt, root_store);
+            validated = CryptoSupplier::x509_verify_certificate_chain(
+                leaf_certificate.get(), trusted_parent_certificates, untrusted_subcas, true, std::nullopt, root_store);
         }
 
         return validated;

tests/openssl_supplier_test.cpp

@@ -91,12 +91,13 @@ TEST_F(OpenSSLSupplierTest, x509_verify_certificate_chain) {
     auto res_leaf = OpenSSLSupplier::load_certificates(cert_leaf, EncodingFormat::PEM);
 
     std::vector<X509Handle*> parents;
+    std::vector<X509Handle*> empty_untrusted;
 
     for (auto& i : res_path) {
         parents.push_back(i.get());
     }
 
-    auto res = OpenSSLSupplier::x509_verify_certificate_chain(res_leaf[0].get(), parents, true, std::nullopt,
+    auto res = OpenSSLSupplier::x509_verify_certificate_chain(res_leaf[0].get(), parents, empty_untrusted, true, std::nullopt,
                                                               "pki/root_cert.pem");
     ASSERT_EQ(res, CertificateValidationResult::Valid);
 }