From c805ee682c229b49a01a7510984afec546f46588 Mon Sep 17 00:00:00 2001 From: AssemblyJohn Date: Wed, 7 Feb 2024 10:21:35 +0200 Subject: [PATCH] Added security limits for certificate deletion to prevent bricking the charger Signed-off-by: AssemblyJohn --- .../evse_security/certificate/x509_bundle.hpp | 26 +++++++++ include/evse_security/evse_security.hpp | 6 ++- lib/evse_security/certificate/x509_bundle.cpp | 4 ++ lib/evse_security/evse_security.cpp | 54 +++++++++++++++---- tests/tests.cpp | 3 ++ 5 files changed, 80 insertions(+), 13 deletions(-) diff --git a/include/evse_security/certificate/x509_bundle.hpp b/include/evse_security/certificate/x509_bundle.hpp index 5d21e18..c3ef4bc 100644 --- a/include/evse_security/certificate/x509_bundle.hpp +++ b/include/evse_security/certificate/x509_bundle.hpp @@ -2,6 +2,7 @@ // Copyright Pionix GmbH and Contributors to EVerest #pragma once +#include #include #include @@ -66,6 +67,9 @@ class X509CertificateBundle { /// @return Contained certificate count int get_certificate_count() const; + /// @return Contained certificate chains count + int get_certificate_chains_count() const; + fs::path get_path() const { return path; } @@ -83,6 +87,28 @@ class X509CertificateBundle { } } + /// @brief Same as 'for_each_chain' but it also uses a predicate for ordering + template void for_each_chain_ordered(function func, ordering order) { + struct Chain { + const fs::path* path; + const std::vector* certificates; + }; + + std::vector ordered; + + for (auto& [path, certs] : certificates) { + ordered.push_back(Chain{&path, &certs}); + } + + std::sort(std::begin(ordered), std::end(ordered), + [&order](Chain& a, Chain& b) { return order(*a.certificates, *b.certificates); }); + + for (const auto& chain : ordered) { + if (!func(*chain.path, *chain.certificates)) + break; + } + } + public: /// @brief Splits the certificate (chain) into single certificates /// @return vector containing single certificates diff --git a/include/evse_security/evse_security.hpp b/include/evse_security/evse_security.hpp index 2c73888..0583f64 100644 --- a/include/evse_security/evse_security.hpp +++ b/include/evse_security/evse_security.hpp @@ -37,13 +37,15 @@ struct FilePaths { LinkPaths links; }; +// Unchangeable security limit for certificate deletion, a min entry count will be always kept (newest) +static constexpr std::size_t DEFAULT_MINIMUM_CERTIFICATE_ENTRIES = 10; // 50 MB default limit for filesystem usage static constexpr std::uintmax_t DEFAULT_MAX_FILESYSTEM_SIZE = 1024 * 1024 * 50; // Default maximum 2000 certificate entries static constexpr std::uintmax_t DEFAULT_MAX_CERTIFICATE_ENTRIES = 2000; -// Expiry for CSRs that did not receive a response CSR -static std::chrono::seconds DEFAULT_CSR_EXPIRY(5 * 60); +// Expiry for CSRs that did not receive a response CSR, 10 minutes or reboot +static std::chrono::seconds DEFAULT_CSR_EXPIRY(10 * 60); /// @brief This class holds filesystem paths to CA bundle file locations and directories for leaf certificates class EvseSecurity { diff --git a/lib/evse_security/certificate/x509_bundle.cpp b/lib/evse_security/certificate/x509_bundle.cpp index 5485ec1..996497f 100644 --- a/lib/evse_security/certificate/x509_bundle.cpp +++ b/lib/evse_security/certificate/x509_bundle.cpp @@ -87,6 +87,10 @@ int X509CertificateBundle::get_certificate_count() const { return count; } +int X509CertificateBundle::get_certificate_chains_count() const { + return certificates.size(); +} + void X509CertificateBundle::add_certificates(const std::string& data, const EncodingFormat encoding, const std::optional& path) { auto loaded = CryptoSupplier::load_certificates(data, encoding); diff --git a/lib/evse_security/evse_security.cpp b/lib/evse_security/evse_security.cpp index d1b1bdc..6fdb207 100644 --- a/lib/evse_security/evse_security.cpp +++ b/lib/evse_security/evse_security.cpp @@ -200,6 +200,17 @@ EvseSecurity::EvseSecurity(const FilePaths& file_paths, const std::optionaldirectories = file_paths.directories; this->links = file_paths.links; @@ -1012,23 +1023,44 @@ void EvseSecurity::garbage_collect(bool delete_expired_certificates) { std::set invalid_certificate_files; - // TODO: Order by latest valid, and keep newest with a safety limit + // Order by latest valid, and keep newest with a safety limit for (auto const& [cert_dir, key_dir] : leaf_paths) { X509CertificateBundle expired_certs(cert_dir, EncodingFormat::PEM); - expired_certs.for_each_chain( - [&invalid_certificate_files](const fs::path& file, const std::vector& chain) { - // If the chain contains the first expired (leafs are the first) - if (chain.size()) { - if (chain[0].is_expired()) { + // Only handle if we have more than the minimum certificates entry + if (expired_certs.get_certificate_chains_count() > DEFAULT_MINIMUM_CERTIFICATE_ENTRIES) { + int skipped = 0; + + // Order by expiry date, and keep even expired certificates with a minimum of 10 certificates + expired_certs.for_each_chain_ordered( + [&invalid_certificate_files, &skipped](const fs::path& file, + const std::vector& chain) { + // By default delete all empty + if (chain.size() <= 0) { invalid_certificate_files.emplace(file); } - } else { - invalid_certificate_files.emplace(file); - } - return true; - }); + if (skipped++ > DEFAULT_MINIMUM_CERTIFICATE_ENTRIES) { + // If the chain contains the first expired (leafs are the first) + if (chain.size()) { + if (chain[0].is_expired()) { + invalid_certificate_files.emplace(file); + } + } + } + + return true; + }, + [](const std::vector& a, const std::vector& b) { + // Order from newest to oldest (newest DEFAULT_MINIMUM_CERTIFICATE_ENTRIES) are kept + // even if they are expired + if (a.size() && b.size()) { + return a.at(0).get_valid_to() > b.at(0).get_valid_to(); + } else { + return false; + } + }); + } } for (const auto& expired_certificate_file : invalid_certificate_files) { diff --git a/tests/tests.cpp b/tests/tests.cpp index 723fe87..19dc85e 100644 --- a/tests/tests.cpp +++ b/tests/tests.cpp @@ -731,6 +731,9 @@ TEST_F(EvseSecurityTests, verify_expired_csr_deletion) { evse_security->generate_certificate_signing_request(LeafCertificateType::CSMS, "DE", "Pionix", "NA"); fs::path csr_key_path = evse_security->managed_csr.begin()->first; + // Simulate a full fs else no deletion will take place + evse_security->max_fs_usage_bytes = 1; + ASSERT_EQ(evse_security->managed_csr.size(), 1); ASSERT_TRUE(fs::exists(csr_key_path));