From afa0b40f50ed296de95cec59630b73a7c619652a Mon Sep 17 00:00:00 2001 From: Evgeny Margolis Date: Thu, 22 Dec 2022 14:25:15 -0800 Subject: [PATCH] chip-cert tool: Fix OpenSSL Object Reuse and Double-Free (#24166) Don't rely on d2i_X509 object reuse and fix double-free The chip-cert tool is relying on OpenSSL's "object reuse" mode in d2i_X509. d2i_X509 has a very bizarre type signature: X509 *d2i_X509(X509 **out, const unsigned char **inp, long len); The safest way to call this function is to pass NULL into out. The function then straightforwardly hands you a new X509 on success, or NULL on error. However, if out and *out are both NULL, OpenSSL tries to reuse the existing X509 object. This does not work, particular not in the way that chip-cert uses it. When d2i_X509 fails, even in this mode, it will free what's at *out and set *out to NULL. So when ReadCert's d2i_X509 call fails, it will silently free the cert parameter. But the caller doesn't know this and will double-free it! --- src/tools/chip-cert/CertUtils.cpp | 21 ++++++++++++--------- src/tools/chip-cert/Cmd_ConvertCert.cpp | 4 ++-- src/tools/chip-cert/Cmd_GenAttCert.cpp | 4 ++-- src/tools/chip-cert/Cmd_GenCD.cpp | 8 ++++---- src/tools/chip-cert/Cmd_GenCert.cpp | 4 ++-- src/tools/chip-cert/Cmd_PrintCert.cpp | 6 +++--- src/tools/chip-cert/Cmd_ResignCert.cpp | 8 ++++---- src/tools/chip-cert/KeyUtils.cpp | 15 ++++++--------- src/tools/chip-cert/chip-cert.h | 4 ++-- 9 files changed, 37 insertions(+), 37 deletions(-) diff --git a/src/tools/chip-cert/CertUtils.cpp b/src/tools/chip-cert/CertUtils.cpp index ed7f09d040ad26..28b6e0d3951131 100644 --- a/src/tools/chip-cert/CertUtils.cpp +++ b/src/tools/chip-cert/CertUtils.cpp @@ -567,13 +567,13 @@ bool AddAuthorityKeyId(X509 * cert, X509 * caCert, bool isAKIDLengthValid) } // namespace -bool ReadCert(const char * fileNameOrStr, X509 * cert) +bool ReadCert(const char * fileNameOrStr, std::unique_ptr & cert) { CertFormat origCertFmt; return ReadCert(fileNameOrStr, cert, origCertFmt); } -bool ReadCert(const char * fileNameOrStr, X509 * cert, CertFormat & certFmt) +bool ReadCert(const char * fileNameOrStr, std::unique_ptr & cert, CertFormat & certFmt) { bool res = true; uint32_t certLen = 0; @@ -628,7 +628,8 @@ bool ReadCert(const char * fileNameOrStr, X509 * cert, CertFormat & certFmt) std::unique_ptr certBIO( BIO_new_mem_buf(static_cast(certBuf.get()), static_cast(certLen)), &BIO_free_all); - if (PEM_read_bio_X509(certBIO.get(), &cert, nullptr, nullptr) == nullptr) + cert.reset(PEM_read_bio_X509(certBIO.get(), nullptr, nullptr, nullptr)); + if (cert.get() == nullptr) { ReportOpenSSLErrorAndExit("PEM_read_bio_X509", res = false); } @@ -639,7 +640,8 @@ bool ReadCert(const char * fileNameOrStr, X509 * cert, CertFormat & certFmt) const uint8_t * outCert = certBuf.get(); - if (d2i_X509(&cert, &outCert, static_cast(certLen)) == nullptr) + cert.reset(d2i_X509(nullptr, &outCert, static_cast(certLen))); + if (cert.get() == nullptr) { ReportOpenSSLErrorAndExit("d2i_X509", res = false); } @@ -667,7 +669,8 @@ bool ReadCert(const char * fileNameOrStr, X509 * cert, CertFormat & certFmt) VerifyOrReturnError(chip::CanCastTo(x509Cert.size()), false); - if (d2i_X509(&cert, &outCert, static_cast(x509Cert.size())) == nullptr) + cert.reset(d2i_X509(nullptr, &outCert, static_cast(x509Cert.size()))); + if (cert.get() == nullptr) { ReportOpenSSLErrorAndExit("d2i_X509", res = false); } @@ -680,9 +683,9 @@ bool ReadCert(const char * fileNameOrStr, X509 * cert, CertFormat & certFmt) bool ReadCertDER(const char * fileNameOrStr, MutableByteSpan & cert) { bool res = true; - std::unique_ptr certX509(X509_new(), &X509_free); + std::unique_ptr certX509(nullptr, &X509_free); - VerifyOrReturnError(ReadCert(fileNameOrStr, certX509.get()), false); + VerifyOrReturnError(ReadCert(fileNameOrStr, certX509), false); uint8_t * certPtr = cert.data(); int certLen = i2d_X509(certX509.get(), &certPtr); @@ -730,9 +733,9 @@ bool LoadChipCert(const char * fileNameOrStr, bool isTrused, ChipCertificateSet bool res = true; CHIP_ERROR err; BitFlags decodeFlags; - std::unique_ptr cert(X509_new(), &X509_free); + std::unique_ptr cert(nullptr, &X509_free); - res = ReadCert(fileNameOrStr, cert.get()); + res = ReadCert(fileNameOrStr, cert); VerifyTrueOrExit(res); res = X509ToChipCert(cert.get(), chipCert); diff --git a/src/tools/chip-cert/Cmd_ConvertCert.cpp b/src/tools/chip-cert/Cmd_ConvertCert.cpp index 7eba1cc2f8868c..efd5816293ebdd 100644 --- a/src/tools/chip-cert/Cmd_ConvertCert.cpp +++ b/src/tools/chip-cert/Cmd_ConvertCert.cpp @@ -178,7 +178,7 @@ bool HandleNonOptionArgs(const char * progName, int argc, char * const argv[]) bool Cmd_ConvertCert(int argc, char * argv[]) { bool res = true; - std::unique_ptr cert(X509_new(), &X509_free); + std::unique_ptr cert(nullptr, &X509_free); if (argc == 1) { @@ -192,7 +192,7 @@ bool Cmd_ConvertCert(int argc, char * argv[]) res = InitOpenSSL(); VerifyTrueOrExit(res); - res = ReadCert(gInFileNameOrStr, cert.get()); + res = ReadCert(gInFileNameOrStr, cert); VerifyTrueOrExit(res); res = WriteCert(gOutFileName, cert.get(), gOutCertFormat); diff --git a/src/tools/chip-cert/Cmd_GenAttCert.cpp b/src/tools/chip-cert/Cmd_GenAttCert.cpp index 845344860c49cd..270e42cfcbd1d6 100644 --- a/src/tools/chip-cert/Cmd_GenAttCert.cpp +++ b/src/tools/chip-cert/Cmd_GenAttCert.cpp @@ -560,10 +560,10 @@ bool Cmd_GenAttCert(int argc, char * argv[]) } else { - std::unique_ptr caCert(X509_new(), &X509_free); + std::unique_ptr caCert(nullptr, &X509_free); std::unique_ptr caKey(EVP_PKEY_new(), &EVP_PKEY_free); - res = ReadCert(gCACertFileNameOrStr, caCert.get()); + res = ReadCert(gCACertFileNameOrStr, caCert); VerifyTrueOrExit(res); res = ReadKey(gCAKeyFileNameOrStr, caKey, gCertConfig.IsErrorTestCaseEnabled()); diff --git a/src/tools/chip-cert/Cmd_GenCD.cpp b/src/tools/chip-cert/Cmd_GenCD.cpp index a338e19ea6e835..77f77471d93ffa 100644 --- a/src/tools/chip-cert/Cmd_GenCD.cpp +++ b/src/tools/chip-cert/Cmd_GenCD.cpp @@ -485,8 +485,8 @@ bool HandleOption(const char * progName, OptionSet * optSet, int id, const char } { const char * fileNameOrStr = arg; - std::unique_ptr cert(X509_new(), &X509_free); - VerifyOrReturnError(ReadCert(fileNameOrStr, cert.get()), false); + std::unique_ptr cert(nullptr, &X509_free); + VerifyOrReturnError(ReadCert(fileNameOrStr, cert), false); ByteSpan skid; VerifyOrReturnError(ExtractSKIDFromX509Cert(cert.get(), skid), false); @@ -1144,10 +1144,10 @@ bool Cmd_GenCD(int argc, char * argv[]) } { - std::unique_ptr cert(X509_new(), &X509_free); + std::unique_ptr cert(nullptr, &X509_free); std::unique_ptr key(EVP_PKEY_new(), &EVP_PKEY_free); - VerifyOrReturnError(ReadCert(gCertFileNameOrStr, cert.get()), false); + VerifyOrReturnError(ReadCert(gCertFileNameOrStr, cert), false); VerifyOrReturnError(ReadKey(gKeyFileNameOrStr, key), false); // Extract the subject key id from the X509 certificate. diff --git a/src/tools/chip-cert/Cmd_GenCert.cpp b/src/tools/chip-cert/Cmd_GenCert.cpp index c17780eec584d4..07317d414c673f 100644 --- a/src/tools/chip-cert/Cmd_GenCert.cpp +++ b/src/tools/chip-cert/Cmd_GenCert.cpp @@ -992,7 +992,7 @@ bool Cmd_GenCert(int argc, char * argv[]) uint8_t certType = kCertType_NotSpecified; std::unique_ptr newCert(X509_new(), &X509_free); std::unique_ptr newKey(EVP_PKEY_new(), &EVP_PKEY_free); - std::unique_ptr caCert(X509_new(), &X509_free); + std::unique_ptr caCert(nullptr, &X509_free); std::unique_ptr caKey(EVP_PKEY_new(), &EVP_PKEY_free); X509 * caCertPtr = nullptr; EVP_PKEY * caKeyPtr = nullptr; @@ -1162,7 +1162,7 @@ bool Cmd_GenCert(int argc, char * argv[]) } else { - res = ReadCert(gCACertFileNameOrStr, caCert.get()); + res = ReadCert(gCACertFileNameOrStr, caCert); VerifyTrueOrExit(res); res = ReadKey(gCAKeyFileNameOrStr, caKey); diff --git a/src/tools/chip-cert/Cmd_PrintCert.cpp b/src/tools/chip-cert/Cmd_PrintCert.cpp index 76110b8dca5981..0c7f696bdb8a5d 100644 --- a/src/tools/chip-cert/Cmd_PrintCert.cpp +++ b/src/tools/chip-cert/Cmd_PrintCert.cpp @@ -1,6 +1,6 @@ /* * - * Copyright (c) 2021 Project CHIP Authors + * Copyright (c) 2021-2022 Project CHIP Authors * Copyright (c) 2013-2017 Nest Labs, Inc. * All rights reserved. * @@ -371,7 +371,7 @@ bool PrintCert(const char * fileName, X509 * cert) bool Cmd_PrintCert(int argc, char * argv[]) { bool res = true; - std::unique_ptr cert(X509_new(), &X509_free); + std::unique_ptr cert(nullptr, &X509_free); if (argc == 1) { @@ -382,7 +382,7 @@ bool Cmd_PrintCert(int argc, char * argv[]) res = ParseArgs(CMD_NAME, argc, argv, gCmdOptionSets, HandleNonOptionArgs); VerifyTrueOrExit(res); - res = ReadCert(gInFileNameOrStr, cert.get()); + res = ReadCert(gInFileNameOrStr, cert); VerifyTrueOrExit(res); res = PrintCert(gOutFileName, cert.get()); diff --git a/src/tools/chip-cert/Cmd_ResignCert.cpp b/src/tools/chip-cert/Cmd_ResignCert.cpp index 053b5ff917d0b9..425d295c918170 100644 --- a/src/tools/chip-cert/Cmd_ResignCert.cpp +++ b/src/tools/chip-cert/Cmd_ResignCert.cpp @@ -137,7 +137,7 @@ bool Cmd_ResignCert(int argc, char * argv[]) { bool res = true; CertFormat inCertFmt; - std::unique_ptr cert(X509_new(), &X509_free); + std::unique_ptr cert(nullptr, &X509_free); std::unique_ptr caKey(EVP_PKEY_new(), &EVP_PKEY_free); if (argc == 1) @@ -192,7 +192,7 @@ bool Cmd_ResignCert(int argc, char * argv[]) res = InitOpenSSL(); VerifyTrueOrExit(res); - res = ReadCert(gInCertFileNameOrStr, cert.get(), inCertFmt); + res = ReadCert(gInCertFileNameOrStr, cert, inCertFmt); VerifyTrueOrExit(res); res = ReadKey(gCAKeyFileNameOrStr, caKey); @@ -200,9 +200,9 @@ bool Cmd_ResignCert(int argc, char * argv[]) if (!gSelfSign) { - std::unique_ptr caCert(X509_new(), &X509_free); + std::unique_ptr caCert(nullptr, &X509_free); - res = ReadCert(gCACertFileNameOrStr, caCert.get()); + res = ReadCert(gCACertFileNameOrStr, caCert); VerifyTrueOrExit(res); res = ResignCert(cert.get(), caCert.get(), caKey.get()); diff --git a/src/tools/chip-cert/KeyUtils.cpp b/src/tools/chip-cert/KeyUtils.cpp index 9cc7fc484cdc80..fcc2e567724a9a 100644 --- a/src/tools/chip-cert/KeyUtils.cpp +++ b/src/tools/chip-cert/KeyUtils.cpp @@ -264,9 +264,8 @@ bool ReadKey(const char * fileNameOrStr, std::unique_ptr & cert); +extern bool ReadCert(const char * fileNameOrStr, std::unique_ptr & cert, CertFormat & origCertFmt); extern bool ReadCertDER(const char * fileNameOrStr, chip::MutableByteSpan & cert); extern bool LoadChipCert(const char * fileNameOrStr, bool isTrused, chip::Credentials::ChipCertificateSet & certSet, chip::MutableByteSpan & chipCert);