From 2df0061df8b487b4e630694d5e1e1d6a2987d7a8 Mon Sep 17 00:00:00 2001 From: Cedric Caffy Date: Mon, 21 Aug 2023 12:02:12 +0200 Subject: [PATCH 1/2] XrdTls: XrdTlsTempCA - CRLs containing critical extensions are inserted at the end of the bundled CRL file Solves issue #2065 --- src/XrdCrypto/XrdCryptosslX509Crl.cc | 6 ++++ src/XrdCrypto/XrdCryptosslX509Crl.hh | 3 ++ src/XrdTls/XrdTlsTempCA.cc | 47 ++++++++++++++++++++++++++-- 3 files changed, 53 insertions(+), 3 deletions(-) diff --git a/src/XrdCrypto/XrdCryptosslX509Crl.cc b/src/XrdCrypto/XrdCryptosslX509Crl.cc index 653aa38902c..6ece76af59d 100644 --- a/src/XrdCrypto/XrdCryptosslX509Crl.cc +++ b/src/XrdCrypto/XrdCryptosslX509Crl.cc @@ -376,6 +376,12 @@ int XrdCryptosslX509Crl::GetFileType(const char *crlfn) return rc; } +bool XrdCryptosslX509Crl::hasCriticalExtension() { + // If the X509_CRL_get_ext_by_critical() function returns -1, no critical extension + // has been found + return X509_CRL_get_ext_by_critical(crl,1,-1) != -1; +} + //_____________________________________________________________________________ int XrdCryptosslX509Crl::LoadCache() { diff --git a/src/XrdCrypto/XrdCryptosslX509Crl.hh b/src/XrdCrypto/XrdCryptosslX509Crl.hh index 26ca673ec6a..d2dc06b66e5 100644 --- a/src/XrdCrypto/XrdCryptosslX509Crl.hh +++ b/src/XrdCrypto/XrdCryptosslX509Crl.hh @@ -83,6 +83,9 @@ public: // Dump CRL object to a file. bool ToFile(FILE *fh); + //Returns true if the CRL certificate has critical extension, false otherwise + bool hasCriticalExtension(); + private: X509_CRL *crl{nullptr}; // The CRL object time_t lastupdate{-1}; // time of last update diff --git a/src/XrdTls/XrdTlsTempCA.cc b/src/XrdTls/XrdTlsTempCA.cc index 0c1dd52f528..8dabc355338 100644 --- a/src/XrdTls/XrdTlsTempCA.cc +++ b/src/XrdTls/XrdTlsTempCA.cc @@ -161,6 +161,13 @@ class CRLSet { * processFile(...) method, false otherwise */ bool atLeastOneValidCRLFound() const; + /** + * https://github.com/xrootd/xrootd/issues/2065 + * To mitigate that issue, we need to defer the insertion of the CRLs that contain + * critical extensions at the end of the bundled CRL file + * @return true on success. + */ + bool processCRLWithCriticalExt(); private: XrdSysError &m_log; @@ -171,6 +178,9 @@ class CRLSet { std::unordered_set m_known_crls; const int m_output_fd; std::atomic m_atLeastOneValidCRLFound; + //Store the CRLs containing critical extensions to defer their insertion + //at the end of the bundled CRL file. Issue https://github.com/xrootd/xrootd/issues/2065 + std::vector> m_crls_critical_extension; }; @@ -179,7 +189,7 @@ CRLSet::processFile(file_smart_ptr &fp, const std::string &fname) { file_smart_ptr outputfp(fdopen(dup(m_output_fd), "w"), &fclose); if (!outputfp.get()) { - m_log.Emsg("CAset", "Failed to reopen file for output", fname.c_str()); + m_log.Emsg("CRLSet", "Failed to reopen file for output", fname.c_str()); return false; } @@ -202,10 +212,17 @@ CRLSet::processFile(file_smart_ptr &fp, const std::string &fname) //m_log.Emsg("CRLset", "New CRL with hash", fname.c_str(), hash_ptr); m_known_crls.insert(hash_ptr); - if (!xrd_crl->ToFile(outputfp.get())) { + if(xrd_crl->hasCriticalExtension()) { + // Issue https://github.com/xrootd/xrootd/issues/2065 + // This CRL will be put at the end of the bundled file + m_crls_critical_extension.emplace_back(std::move(xrd_crl)); + } else { + // No critical extension found on that CRL, just insert it on the CRL bundled file + if (!xrd_crl->ToFile(outputfp.get())) { m_log.Emsg("CRLset", "Failed to write out CRL", fname.c_str()); fflush(outputfp.get()); return false; + } } } fflush(outputfp.get()); @@ -217,6 +234,26 @@ bool CRLSet::atLeastOneValidCRLFound() const { return m_atLeastOneValidCRLFound; } +bool CRLSet::processCRLWithCriticalExt() { + // Don't open the output file if not necessary + if(!m_crls_critical_extension.empty()) { + file_smart_ptr outputfp(fdopen(dup(m_output_fd), "w"), &fclose); + if (!outputfp.get()) { + m_log.Emsg("CRLSet", "Failed to reopen file for output critical CRLs with critical extension"); + return false; + } + for (const auto &crl: m_crls_critical_extension) { + if (!crl->ToFile(outputfp.get())) { + m_log.Emsg("CRLset", "Failed to write out CRL with critical extension", crl->ParentFile()); + fflush(outputfp.get()); + return false; + } + } + fflush(outputfp.get()); + } + return true; +} + } @@ -417,8 +454,12 @@ XrdTlsTempCA::Maintenance() } closedir(dirp); + if(!crl_builder.processCRLWithCriticalExt()){ + m_log.Emsg("Maintenance", "Failed to insert CRLs with critical extension for CRLs", result->d_name); + } + if (!new_file->commit()) { - m_log.Emsg("Mainteance", "Failed to finalize new CA / CRL files"); + m_log.Emsg("Maintenance", "Failed to finalize new CA / CRL files"); return false; } //m_log.Emsg("Maintenance", "Successfully created CA and CRL files", new_file->getCAFilename().c_str(), From fca07e4e26c99a582de4d45123833b4d50042c77 Mon Sep 17 00:00:00 2001 From: Cedric Caffy Date: Tue, 22 Aug 2023 08:57:21 +0200 Subject: [PATCH 2/2] XrdTls: XrdTlsTempCA - Replaced dup() by XrdSysFD_Dup() in output file open --- src/XrdTls/XrdTlsTempCA.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/XrdTls/XrdTlsTempCA.cc b/src/XrdTls/XrdTlsTempCA.cc index 8dabc355338..392ec914a69 100644 --- a/src/XrdTls/XrdTlsTempCA.cc +++ b/src/XrdTls/XrdTlsTempCA.cc @@ -187,7 +187,7 @@ class CRLSet { bool CRLSet::processFile(file_smart_ptr &fp, const std::string &fname) { - file_smart_ptr outputfp(fdopen(dup(m_output_fd), "w"), &fclose); + file_smart_ptr outputfp(fdopen(XrdSysFD_Dup(m_output_fd), "w"), &fclose); if (!outputfp.get()) { m_log.Emsg("CRLSet", "Failed to reopen file for output", fname.c_str()); return false; @@ -237,7 +237,7 @@ bool CRLSet::atLeastOneValidCRLFound() const { bool CRLSet::processCRLWithCriticalExt() { // Don't open the output file if not necessary if(!m_crls_critical_extension.empty()) { - file_smart_ptr outputfp(fdopen(dup(m_output_fd), "w"), &fclose); + file_smart_ptr outputfp(fdopen(XrdSysFD_Dup(m_output_fd), "w"), &fclose); if (!outputfp.get()) { m_log.Emsg("CRLSet", "Failed to reopen file for output critical CRLs with critical extension"); return false;