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!
project-chip · Jul 21, 2023 · afa0b40 · afa0b40
1 parent 01b4b03
commit afa0b40
Show file tree

Hide file tree

Showing 9 changed files with 37 additions and 37 deletions.
diff --git 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<X509, void (*)(X509 *)> & 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<X509, void (*)(X509 *)> & 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<BIO, void (*)(BIO *)> certBIO(
             BIO_new_mem_buf(static_cast<const void *>(certBuf.get()), static_cast<int>(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<int>(certLen)) == nullptr)
+        cert.reset(d2i_X509(nullptr, &outCert, static_cast<int>(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<int>(x509Cert.size()), false);
 
-        if (d2i_X509(&cert, &outCert, static_cast<int>(x509Cert.size())) == nullptr)
+        cert.reset(d2i_X509(nullptr, &outCert, static_cast<int>(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<X509, void (*)(X509 *)> certX509(X509_new(), &X509_free);
+    std::unique_ptr<X509, void (*)(X509 *)> 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<CertDecodeFlags> decodeFlags;
-    std::unique_ptr<X509, void (*)(X509 *)> cert(X509_new(), &X509_free);
+    std::unique_ptr<X509, void (*)(X509 *)> 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
@@ -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<X509, void (*)(X509 *)> cert(X509_new(), &X509_free);
+    std::unique_ptr<X509, void (*)(X509 *)> 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
@@ -560,10 +560,10 @@ bool Cmd_GenAttCert(int argc, char * argv[])
     }
     else
     {
-        std::unique_ptr<X509, void (*)(X509 *)> caCert(X509_new(), &X509_free);
+        std::unique_ptr<X509, void (*)(X509 *)> caCert(nullptr, &X509_free);
         std::unique_ptr<EVP_PKEY, void (*)(EVP_PKEY *)> 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
@@ -485,8 +485,8 @@ bool HandleOption(const char * progName, OptionSet * optSet, int id, const char
         }
         {
             const char * fileNameOrStr = arg;
-            std::unique_ptr<X509, void (*)(X509 *)> cert(X509_new(), &X509_free);
-            VerifyOrReturnError(ReadCert(fileNameOrStr, cert.get()), false);
+            std::unique_ptr<X509, void (*)(X509 *)> 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<X509, void (*)(X509 *)> cert(X509_new(), &X509_free);
+        std::unique_ptr<X509, void (*)(X509 *)> cert(nullptr, &X509_free);
         std::unique_ptr<EVP_PKEY, void (*)(EVP_PKEY *)> 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
@@ -992,7 +992,7 @@ bool Cmd_GenCert(int argc, char * argv[])
     uint8_t certType = kCertType_NotSpecified;
     std::unique_ptr<X509, void (*)(X509 *)> newCert(X509_new(), &X509_free);
     std::unique_ptr<EVP_PKEY, void (*)(EVP_PKEY *)> newKey(EVP_PKEY_new(), &EVP_PKEY_free);
-    std::unique_ptr<X509, void (*)(X509 *)> caCert(X509_new(), &X509_free);
+    std::unique_ptr<X509, void (*)(X509 *)> caCert(nullptr, &X509_free);
     std::unique_ptr<EVP_PKEY, void (*)(EVP_PKEY *)> 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
@@ -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<X509, void (*)(X509 *)> cert(X509_new(), &X509_free);
+    std::unique_ptr<X509, void (*)(X509 *)> 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
@@ -137,7 +137,7 @@ bool Cmd_ResignCert(int argc, char * argv[])
 {
     bool res = true;
     CertFormat inCertFmt;
-    std::unique_ptr<X509, void (*)(X509 *)> cert(X509_new(), &X509_free);
+    std::unique_ptr<X509, void (*)(X509 *)> cert(nullptr, &X509_free);
     std::unique_ptr<EVP_PKEY, void (*)(EVP_PKEY *)> caKey(EVP_PKEY_new(), &EVP_PKEY_free);
 
     if (argc == 1)
@@ -192,17 +192,17 @@ 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);
     VerifyTrueOrExit(res);
 
     if (!gSelfSign)
     {
-        std::unique_ptr<X509, void (*)(X509 *)> caCert(X509_new(), &X509_free);
+        std::unique_ptr<X509, void (*)(X509 *)> 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
@@ -264,9 +264,8 @@ bool ReadKey(const char * fileNameOrStr, std::unique_ptr<EVP_PKEY, void (*)(EVP_
 
         if (keyFormat == kKeyFormat_X509_Pubkey_PEM)
         {
-            EC_KEY * ecKey = EC_KEY_new();
-
-            if (PEM_read_bio_EC_PUBKEY(keyBIO.get(), &ecKey, nullptr, nullptr) == nullptr)
+            EC_KEY * ecKey = PEM_read_bio_EC_PUBKEY(keyBIO.get(), nullptr, nullptr, nullptr);
+            if (ecKey == nullptr)
             {
                 ReportOpenSSLErrorAndExit("PEM_read_bio_EC_PUBKEY", res = false);
             }
@@ -278,21 +277,19 @@ bool ReadKey(const char * fileNameOrStr, std::unique_ptr<EVP_PKEY, void (*)(EVP_
         }
         else if (keyFormat == kKeyFormat_X509_PEM)
         {
-            EVP_PKEY * tmpKeyPtr = nullptr;
-            if (PEM_read_bio_PrivateKey(keyBIO.get(), &tmpKeyPtr, nullptr, nullptr) == nullptr)
+            key.reset(PEM_read_bio_PrivateKey(keyBIO.get(), nullptr, nullptr, nullptr));
+            if (key.get() == nullptr)
             {
                 ReportOpenSSLErrorAndExit("PEM_read_bio_PrivateKey", res = false);
             }
-            key.reset(tmpKeyPtr);
         }
         else
         {
-            EVP_PKEY * tmpKeyPtr = nullptr;
-            if (d2i_PrivateKey_bio(keyBIO.get(), &tmpKeyPtr) == nullptr)
+            key.reset(d2i_PrivateKey_bio(keyBIO.get(), nullptr));
+            if (key.get() == nullptr)
             {
                 ReportOpenSSLErrorAndExit("d2i_PrivateKey_bio", res = false);
             }
-            key.reset(tmpKeyPtr);
         }
     }
 

diff --git a/src/tools/chip-cert/chip-cert.h b/src/tools/chip-cert/chip-cert.h
@@ -417,8 +417,8 @@ extern bool Cmd_PrintCert(int argc, char * argv[]);
 extern bool Cmd_PrintCD(int argc, char * argv[]);
 extern bool Cmd_GenAttCert(int argc, char * argv[]);
 
-extern bool ReadCert(const char * fileNameOrStr, X509 * cert);
-extern bool ReadCert(const char * fileNameOrStr, X509 * cert, CertFormat & origCertFmt);
+extern bool ReadCert(const char * fileNameOrStr, std::unique_ptr<X509, void (*)(X509 *)> & cert);
+extern bool ReadCert(const char * fileNameOrStr, std::unique_ptr<X509, void (*)(X509 *)> & 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);