Skip to content

Commit 4419412

Browse files
emargolispull[bot]
emargolis
authored and
pull[bot]
committed
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!
1 parent 967397c commit 4419412

9 files changed

+37
-37
lines changed

‎src/tools/chip-cert/CertUtils.cpp

+12-9
Original file line numberDiff line numberDiff line change
@@ -567,13 +567,13 @@ bool AddAuthorityKeyId(X509 * cert, X509 * caCert, bool isAKIDLengthValid)
567567

568568
} // namespace
569569

570-
bool ReadCert(const char * fileNameOrStr, X509 * cert)
570+
bool ReadCert(const char * fileNameOrStr, std::unique_ptr<X509, void (*)(X509 *)> & cert)
571571
{
572572
CertFormat origCertFmt;
573573
return ReadCert(fileNameOrStr, cert, origCertFmt);
574574
}
575575

576-
bool ReadCert(const char * fileNameOrStr, X509 * cert, CertFormat & certFmt)
576+
bool ReadCert(const char * fileNameOrStr, std::unique_ptr<X509, void (*)(X509 *)> & cert, CertFormat & certFmt)
577577
{
578578
bool res = true;
579579
uint32_t certLen = 0;
@@ -628,7 +628,8 @@ bool ReadCert(const char * fileNameOrStr, X509 * cert, CertFormat & certFmt)
628628
std::unique_ptr<BIO, void (*)(BIO *)> certBIO(
629629
BIO_new_mem_buf(static_cast<const void *>(certBuf.get()), static_cast<int>(certLen)), &BIO_free_all);
630630

631-
if (PEM_read_bio_X509(certBIO.get(), &cert, nullptr, nullptr) == nullptr)
631+
cert.reset(PEM_read_bio_X509(certBIO.get(), nullptr, nullptr, nullptr));
632+
if (cert.get() == nullptr)
632633
{
633634
ReportOpenSSLErrorAndExit("PEM_read_bio_X509", res = false);
634635
}
@@ -639,7 +640,8 @@ bool ReadCert(const char * fileNameOrStr, X509 * cert, CertFormat & certFmt)
639640

640641
const uint8_t * outCert = certBuf.get();
641642

642-
if (d2i_X509(&cert, &outCert, static_cast<int>(certLen)) == nullptr)
643+
cert.reset(d2i_X509(nullptr, &outCert, static_cast<int>(certLen)));
644+
if (cert.get() == nullptr)
643645
{
644646
ReportOpenSSLErrorAndExit("d2i_X509", res = false);
645647
}
@@ -667,7 +669,8 @@ bool ReadCert(const char * fileNameOrStr, X509 * cert, CertFormat & certFmt)
667669

668670
VerifyOrReturnError(chip::CanCastTo<int>(x509Cert.size()), false);
669671

670-
if (d2i_X509(&cert, &outCert, static_cast<int>(x509Cert.size())) == nullptr)
672+
cert.reset(d2i_X509(nullptr, &outCert, static_cast<int>(x509Cert.size())));
673+
if (cert.get() == nullptr)
671674
{
672675
ReportOpenSSLErrorAndExit("d2i_X509", res = false);
673676
}
@@ -680,9 +683,9 @@ bool ReadCert(const char * fileNameOrStr, X509 * cert, CertFormat & certFmt)
680683
bool ReadCertDER(const char * fileNameOrStr, MutableByteSpan & cert)
681684
{
682685
bool res = true;
683-
std::unique_ptr<X509, void (*)(X509 *)> certX509(X509_new(), &X509_free);
686+
std::unique_ptr<X509, void (*)(X509 *)> certX509(nullptr, &X509_free);
684687

685-
VerifyOrReturnError(ReadCert(fileNameOrStr, certX509.get()), false);
688+
VerifyOrReturnError(ReadCert(fileNameOrStr, certX509), false);
686689

687690
uint8_t * certPtr = cert.data();
688691
int certLen = i2d_X509(certX509.get(), &certPtr);
@@ -730,9 +733,9 @@ bool LoadChipCert(const char * fileNameOrStr, bool isTrused, ChipCertificateSet
730733
bool res = true;
731734
CHIP_ERROR err;
732735
BitFlags<CertDecodeFlags> decodeFlags;
733-
std::unique_ptr<X509, void (*)(X509 *)> cert(X509_new(), &X509_free);
736+
std::unique_ptr<X509, void (*)(X509 *)> cert(nullptr, &X509_free);
734737

735-
res = ReadCert(fileNameOrStr, cert.get());
738+
res = ReadCert(fileNameOrStr, cert);
736739
VerifyTrueOrExit(res);
737740

738741
res = X509ToChipCert(cert.get(), chipCert);

‎src/tools/chip-cert/Cmd_ConvertCert.cpp

+2-2
Original file line numberDiff line numberDiff line change
@@ -178,7 +178,7 @@ bool HandleNonOptionArgs(const char * progName, int argc, char * const argv[])
178178
bool Cmd_ConvertCert(int argc, char * argv[])
179179
{
180180
bool res = true;
181-
std::unique_ptr<X509, void (*)(X509 *)> cert(X509_new(), &X509_free);
181+
std::unique_ptr<X509, void (*)(X509 *)> cert(nullptr, &X509_free);
182182

183183
if (argc == 1)
184184
{
@@ -192,7 +192,7 @@ bool Cmd_ConvertCert(int argc, char * argv[])
192192
res = InitOpenSSL();
193193
VerifyTrueOrExit(res);
194194

195-
res = ReadCert(gInFileNameOrStr, cert.get());
195+
res = ReadCert(gInFileNameOrStr, cert);
196196
VerifyTrueOrExit(res);
197197

198198
res = WriteCert(gOutFileName, cert.get(), gOutCertFormat);

‎src/tools/chip-cert/Cmd_GenAttCert.cpp

+2-2
Original file line numberDiff line numberDiff line change
@@ -560,10 +560,10 @@ bool Cmd_GenAttCert(int argc, char * argv[])
560560
}
561561
else
562562
{
563-
std::unique_ptr<X509, void (*)(X509 *)> caCert(X509_new(), &X509_free);
563+
std::unique_ptr<X509, void (*)(X509 *)> caCert(nullptr, &X509_free);
564564
std::unique_ptr<EVP_PKEY, void (*)(EVP_PKEY *)> caKey(EVP_PKEY_new(), &EVP_PKEY_free);
565565

566-
res = ReadCert(gCACertFileNameOrStr, caCert.get());
566+
res = ReadCert(gCACertFileNameOrStr, caCert);
567567
VerifyTrueOrExit(res);
568568

569569
res = ReadKey(gCAKeyFileNameOrStr, caKey, gCertConfig.IsErrorTestCaseEnabled());

‎src/tools/chip-cert/Cmd_GenCD.cpp

+4-4
Original file line numberDiff line numberDiff line change
@@ -485,8 +485,8 @@ bool HandleOption(const char * progName, OptionSet * optSet, int id, const char
485485
}
486486
{
487487
const char * fileNameOrStr = arg;
488-
std::unique_ptr<X509, void (*)(X509 *)> cert(X509_new(), &X509_free);
489-
VerifyOrReturnError(ReadCert(fileNameOrStr, cert.get()), false);
488+
std::unique_ptr<X509, void (*)(X509 *)> cert(nullptr, &X509_free);
489+
VerifyOrReturnError(ReadCert(fileNameOrStr, cert), false);
490490

491491
ByteSpan skid;
492492
VerifyOrReturnError(ExtractSKIDFromX509Cert(cert.get(), skid), false);
@@ -1144,10 +1144,10 @@ bool Cmd_GenCD(int argc, char * argv[])
11441144
}
11451145

11461146
{
1147-
std::unique_ptr<X509, void (*)(X509 *)> cert(X509_new(), &X509_free);
1147+
std::unique_ptr<X509, void (*)(X509 *)> cert(nullptr, &X509_free);
11481148
std::unique_ptr<EVP_PKEY, void (*)(EVP_PKEY *)> key(EVP_PKEY_new(), &EVP_PKEY_free);
11491149

1150-
VerifyOrReturnError(ReadCert(gCertFileNameOrStr, cert.get()), false);
1150+
VerifyOrReturnError(ReadCert(gCertFileNameOrStr, cert), false);
11511151
VerifyOrReturnError(ReadKey(gKeyFileNameOrStr, key), false);
11521152

11531153
// Extract the subject key id from the X509 certificate.

‎src/tools/chip-cert/Cmd_GenCert.cpp

+2-2
Original file line numberDiff line numberDiff line change
@@ -992,7 +992,7 @@ bool Cmd_GenCert(int argc, char * argv[])
992992
uint8_t certType = kCertType_NotSpecified;
993993
std::unique_ptr<X509, void (*)(X509 *)> newCert(X509_new(), &X509_free);
994994
std::unique_ptr<EVP_PKEY, void (*)(EVP_PKEY *)> newKey(EVP_PKEY_new(), &EVP_PKEY_free);
995-
std::unique_ptr<X509, void (*)(X509 *)> caCert(X509_new(), &X509_free);
995+
std::unique_ptr<X509, void (*)(X509 *)> caCert(nullptr, &X509_free);
996996
std::unique_ptr<EVP_PKEY, void (*)(EVP_PKEY *)> caKey(EVP_PKEY_new(), &EVP_PKEY_free);
997997
X509 * caCertPtr = nullptr;
998998
EVP_PKEY * caKeyPtr = nullptr;
@@ -1162,7 +1162,7 @@ bool Cmd_GenCert(int argc, char * argv[])
11621162
}
11631163
else
11641164
{
1165-
res = ReadCert(gCACertFileNameOrStr, caCert.get());
1165+
res = ReadCert(gCACertFileNameOrStr, caCert);
11661166
VerifyTrueOrExit(res);
11671167

11681168
res = ReadKey(gCAKeyFileNameOrStr, caKey);

‎src/tools/chip-cert/Cmd_PrintCert.cpp

+3-3
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
/*
22
*
3-
* Copyright (c) 2021 Project CHIP Authors
3+
* Copyright (c) 2021-2022 Project CHIP Authors
44
* Copyright (c) 2013-2017 Nest Labs, Inc.
55
* All rights reserved.
66
*
@@ -371,7 +371,7 @@ bool PrintCert(const char * fileName, X509 * cert)
371371
bool Cmd_PrintCert(int argc, char * argv[])
372372
{
373373
bool res = true;
374-
std::unique_ptr<X509, void (*)(X509 *)> cert(X509_new(), &X509_free);
374+
std::unique_ptr<X509, void (*)(X509 *)> cert(nullptr, &X509_free);
375375

376376
if (argc == 1)
377377
{
@@ -382,7 +382,7 @@ bool Cmd_PrintCert(int argc, char * argv[])
382382
res = ParseArgs(CMD_NAME, argc, argv, gCmdOptionSets, HandleNonOptionArgs);
383383
VerifyTrueOrExit(res);
384384

385-
res = ReadCert(gInFileNameOrStr, cert.get());
385+
res = ReadCert(gInFileNameOrStr, cert);
386386
VerifyTrueOrExit(res);
387387

388388
res = PrintCert(gOutFileName, cert.get());

‎src/tools/chip-cert/Cmd_ResignCert.cpp

+4-4
Original file line numberDiff line numberDiff line change
@@ -137,7 +137,7 @@ bool Cmd_ResignCert(int argc, char * argv[])
137137
{
138138
bool res = true;
139139
CertFormat inCertFmt;
140-
std::unique_ptr<X509, void (*)(X509 *)> cert(X509_new(), &X509_free);
140+
std::unique_ptr<X509, void (*)(X509 *)> cert(nullptr, &X509_free);
141141
std::unique_ptr<EVP_PKEY, void (*)(EVP_PKEY *)> caKey(EVP_PKEY_new(), &EVP_PKEY_free);
142142

143143
if (argc == 1)
@@ -192,17 +192,17 @@ bool Cmd_ResignCert(int argc, char * argv[])
192192
res = InitOpenSSL();
193193
VerifyTrueOrExit(res);
194194

195-
res = ReadCert(gInCertFileNameOrStr, cert.get(), inCertFmt);
195+
res = ReadCert(gInCertFileNameOrStr, cert, inCertFmt);
196196
VerifyTrueOrExit(res);
197197

198198
res = ReadKey(gCAKeyFileNameOrStr, caKey);
199199
VerifyTrueOrExit(res);
200200

201201
if (!gSelfSign)
202202
{
203-
std::unique_ptr<X509, void (*)(X509 *)> caCert(X509_new(), &X509_free);
203+
std::unique_ptr<X509, void (*)(X509 *)> caCert(nullptr, &X509_free);
204204

205-
res = ReadCert(gCACertFileNameOrStr, caCert.get());
205+
res = ReadCert(gCACertFileNameOrStr, caCert);
206206
VerifyTrueOrExit(res);
207207

208208
res = ResignCert(cert.get(), caCert.get(), caKey.get());

‎src/tools/chip-cert/KeyUtils.cpp

+6-9
Original file line numberDiff line numberDiff line change
@@ -264,9 +264,8 @@ bool ReadKey(const char * fileNameOrStr, std::unique_ptr<EVP_PKEY, void (*)(EVP_
264264

265265
if (keyFormat == kKeyFormat_X509_Pubkey_PEM)
266266
{
267-
EC_KEY * ecKey = EC_KEY_new();
268-
269-
if (PEM_read_bio_EC_PUBKEY(keyBIO.get(), &ecKey, nullptr, nullptr) == nullptr)
267+
EC_KEY * ecKey = PEM_read_bio_EC_PUBKEY(keyBIO.get(), nullptr, nullptr, nullptr);
268+
if (ecKey == nullptr)
270269
{
271270
ReportOpenSSLErrorAndExit("PEM_read_bio_EC_PUBKEY", res = false);
272271
}
@@ -278,21 +277,19 @@ bool ReadKey(const char * fileNameOrStr, std::unique_ptr<EVP_PKEY, void (*)(EVP_
278277
}
279278
else if (keyFormat == kKeyFormat_X509_PEM)
280279
{
281-
EVP_PKEY * tmpKeyPtr = nullptr;
282-
if (PEM_read_bio_PrivateKey(keyBIO.get(), &tmpKeyPtr, nullptr, nullptr) == nullptr)
280+
key.reset(PEM_read_bio_PrivateKey(keyBIO.get(), nullptr, nullptr, nullptr));
281+
if (key.get() == nullptr)
283282
{
284283
ReportOpenSSLErrorAndExit("PEM_read_bio_PrivateKey", res = false);
285284
}
286-
key.reset(tmpKeyPtr);
287285
}
288286
else
289287
{
290-
EVP_PKEY * tmpKeyPtr = nullptr;
291-
if (d2i_PrivateKey_bio(keyBIO.get(), &tmpKeyPtr) == nullptr)
288+
key.reset(d2i_PrivateKey_bio(keyBIO.get(), nullptr));
289+
if (key.get() == nullptr)
292290
{
293291
ReportOpenSSLErrorAndExit("d2i_PrivateKey_bio", res = false);
294292
}
295-
key.reset(tmpKeyPtr);
296293
}
297294
}
298295

‎src/tools/chip-cert/chip-cert.h

+2-2
Original file line numberDiff line numberDiff line change
@@ -417,8 +417,8 @@ extern bool Cmd_PrintCert(int argc, char * argv[]);
417417
extern bool Cmd_PrintCD(int argc, char * argv[]);
418418
extern bool Cmd_GenAttCert(int argc, char * argv[]);
419419

420-
extern bool ReadCert(const char * fileNameOrStr, X509 * cert);
421-
extern bool ReadCert(const char * fileNameOrStr, X509 * cert, CertFormat & origCertFmt);
420+
extern bool ReadCert(const char * fileNameOrStr, std::unique_ptr<X509, void (*)(X509 *)> & cert);
421+
extern bool ReadCert(const char * fileNameOrStr, std::unique_ptr<X509, void (*)(X509 *)> & cert, CertFormat & origCertFmt);
422422
extern bool ReadCertDER(const char * fileNameOrStr, chip::MutableByteSpan & cert);
423423
extern bool LoadChipCert(const char * fileNameOrStr, bool isTrused, chip::Credentials::ChipCertificateSet & certSet,
424424
chip::MutableByteSpan & chipCert);

0 commit comments

Comments
 (0)