Commit 733bf267 authored by Xiangyu Bu's avatar Xiangyu Bu Committed by Facebook Github Bot

OpenSSLCertUtils::readCertsFromBuffer() to throw on malformed cert.

Summary:
Current behavior of OpenSSLCertUtils::readCertsFromBuffer() is that it stops
parsing as soon as it encounters the first error. We don't know if the error
is EOF or something else. Sometimes we want to reject the result
if the buffer is malformed instead of accepting a partially parsed result.
This diff makes the API throw when given malformed cert buffer.

Reviewed By: anirudhvr

Differential Revision: D10467792

fbshipit-source-id: 2c15266e5f00866dfaafe0a5ce88d24459e8b561
parent a3949788
...@@ -23,6 +23,14 @@ ...@@ -23,6 +23,14 @@
namespace folly { namespace folly {
namespace ssl { namespace ssl {
namespace {
std::string getOpenSSLErrorString(unsigned long err) {
std::array<char, 256> errBuff;
ERR_error_string_n(err, errBuff.data(), errBuff.size());
return std::string(errBuff.data());
}
} // namespace
Optional<std::string> OpenSSLCertUtils::getCommonName(X509& x509) { Optional<std::string> OpenSSLCertUtils::getCommonName(X509& x509) {
auto subject = X509_get_subject_name(&x509); auto subject = X509_get_subject_name(&x509);
if (!subject) { if (!subject) {
...@@ -208,13 +216,23 @@ std::vector<X509UniquePtr> OpenSSLCertUtils::readCertsFromBuffer( ...@@ -208,13 +216,23 @@ std::vector<X509UniquePtr> OpenSSLCertUtils::readCertsFromBuffer(
std::vector<X509UniquePtr> certs; std::vector<X509UniquePtr> certs;
while (true) { while (true) {
X509UniquePtr x509(PEM_read_bio_X509(b.get(), nullptr, nullptr, nullptr)); X509UniquePtr x509(PEM_read_bio_X509(b.get(), nullptr, nullptr, nullptr));
if (!x509) { if (x509) {
ERR_clear_error(); certs.push_back(std::move(x509));
continue;
}
auto err = ERR_get_error();
ERR_clear_error();
if (BIO_eof(b.get()) && ERR_GET_LIB(err) == ERR_LIB_PEM &&
ERR_GET_REASON(err) == PEM_R_NO_START_LINE) {
// Reach end of buffer.
break; break;
} }
certs.push_back(std::move(x509)); throw std::runtime_error(folly::to<std::string>(
"Unable to parse cert ",
certs.size(),
": ",
getOpenSSLErrorString(err)));
} }
return certs; return certs;
} }
...@@ -248,8 +266,7 @@ X509StoreUniquePtr OpenSSLCertUtils::readStoreFromFile(std::string caFile) { ...@@ -248,8 +266,7 @@ X509StoreUniquePtr OpenSSLCertUtils::readStoreFromFile(std::string caFile) {
throw std::runtime_error( throw std::runtime_error(
folly::to<std::string>("Could not read store file: ", caFile)); folly::to<std::string>("Could not read store file: ", caFile));
} }
auto certRange = folly::ByteRange(folly::StringPiece(certData)); return readStoreFromBuffer(folly::StringPiece(certData));
return readStoreFromBuffer(std::move(certRange));
} }
X509StoreUniquePtr OpenSSLCertUtils::readStoreFromBuffer(ByteRange certRange) { X509StoreUniquePtr OpenSSLCertUtils::readStoreFromBuffer(ByteRange certRange) {
...@@ -261,11 +278,9 @@ X509StoreUniquePtr OpenSSLCertUtils::readStoreFromBuffer(ByteRange certRange) { ...@@ -261,11 +278,9 @@ X509StoreUniquePtr OpenSSLCertUtils::readStoreFromBuffer(ByteRange certRange) {
auto err = ERR_get_error(); auto err = ERR_get_error();
if (ERR_GET_LIB(err) != ERR_LIB_X509 || if (ERR_GET_LIB(err) != ERR_LIB_X509 ||
ERR_GET_REASON(err) != X509_R_CERT_ALREADY_IN_HASH_TABLE) { ERR_GET_REASON(err) != X509_R_CERT_ALREADY_IN_HASH_TABLE) {
std::array<char, 256> errBuff;
ERR_error_string_n(err, errBuff.data(), errBuff.size());
throw std::runtime_error(folly::to<std::string>( throw std::runtime_error(folly::to<std::string>(
"Could not insert CA certificate into store: ", "Could not insert CA certificate into store: ",
std::string(errBuff.data()))); getOpenSSLErrorString(err)));
} }
} }
} }
......
...@@ -63,24 +63,27 @@ class OpenSSLCertUtils { ...@@ -63,24 +63,27 @@ class OpenSSLCertUtils {
static folly::Optional<std::string> toString(X509& x509); static folly::Optional<std::string> toString(X509& x509);
/** /**
* Decodes the DER representation of an X509 certificate. * Decode the DER representation of an X509 certificate.
* *
* Throws on error (if a valid certificate can't be decoded). * Throws on error (if a valid certificate can't be decoded).
*/ */
static X509UniquePtr derDecode(ByteRange); static X509UniquePtr derDecode(ByteRange);
/** /**
* DER encodes an X509 certificate. * Encode an X509 certificate in DER format.
* *
* Throws on error. * Throws on error.
*/ */
static std::unique_ptr<IOBuf> derEncode(X509&); static std::unique_ptr<IOBuf> derEncode(X509&);
/** /**
* Reads certificates from memory and returns them as a vector of X509 * Read certificates from memory and returns them as a vector of X509
* pointers. * pointers. Throw if there is any malformed cert or memory allocation
* problem.
* @param range Buffer to parse.
* @return A vector of X509 objects.
*/ */
static std::vector<X509UniquePtr> readCertsFromBuffer(ByteRange); static std::vector<X509UniquePtr> readCertsFromBuffer(ByteRange range);
/** /**
* Return the output of the X509_digest for chosen message-digest algo * Return the output of the X509_digest for chosen message-digest algo
...@@ -91,10 +94,20 @@ class OpenSSLCertUtils { ...@@ -91,10 +94,20 @@ class OpenSSLCertUtils {
static std::array<uint8_t, SHA256_DIGEST_LENGTH> getDigestSha256(X509& x509); static std::array<uint8_t, SHA256_DIGEST_LENGTH> getDigestSha256(X509& x509);
/** /**
* Reads a store from a file (or buffer). Throws on error. * Read a store from a file. Throw if unable to read the file, memory
* allocation fails, or any cert can't be parsed or added to the store.
* @param caFile Path to the CA file.
* @return A X509 store that contains certs in the CA file.
*/ */
static X509StoreUniquePtr readStoreFromFile(std::string caFile); static X509StoreUniquePtr readStoreFromFile(std::string caFile);
static X509StoreUniquePtr readStoreFromBuffer(ByteRange);
/**
* Read a store from a PEM buffer. Throw if memory allocation fails, or
* any cert can't be parsed or added to the store.
* @param range A buffer containing certs in PEM format.
* @return A X509 store that contains certs in the CA file.
*/
static X509StoreUniquePtr readStoreFromBuffer(ByteRange range);
private: private:
static std::string getDateTimeStr(const ASN1_TIME* time); static std::string getDateTimeStr(const ASN1_TIME* time);
......
...@@ -31,11 +31,13 @@ const char* kTestCertWithoutSan = "folly/io/async/test/certs/tests-cert.pem"; ...@@ -31,11 +31,13 @@ const char* kTestCertWithoutSan = "folly/io/async/test/certs/tests-cert.pem";
const char* kTestCa = "folly/io/async/test/certs/ca-cert.pem"; const char* kTestCa = "folly/io/async/test/certs/ca-cert.pem";
// Test key // Test key
// -----BEGIN EC PRIVATE KEY----- const std::string kTestKey = folly::stripLeftMargin(R"(
// MHcCAQEEIBskFwVZ9miFN+SKCFZPe9WEuFGmP+fsecLUnsTN6bOcoAoGCCqGSM49 ----BEGIN EC PRIVATE KEY-----
// AwEHoUQDQgAE7/f4YYOYunAM/VkmjDYDg3AWUgyyTIraWmmQZsnu0bYNV/lLLfNz MHcCAQEEIBskFwVZ9miFN+SKCFZPe9WEuFGmP+fsecLUnsTN6bOcoAoGCCqGSM49
// CtHggxGSwEtEe40nNb9C8wQmHUvb7VBBlw== AwEHoUQDQgAE7/f4YYOYunAM/VkmjDYDg3AWUgyyTIraWmmQZsnu0bYNV/lLLfNz
// -----END EC PRIVATE KEY----- CtHggxGSwEtEe40nNb9C8wQmHUvb7VBBlw==
-----END EC PRIVATE KEY-----
)");
const std::string kTestCertWithSan = folly::stripLeftMargin(R"( const std::string kTestCertWithSan = folly::stripLeftMargin(R"(
-----BEGIN CERTIFICATE----- -----BEGIN CERTIFICATE-----
MIIDXDCCAkSgAwIBAgIBAjANBgkqhkiG9w0BAQsFADBQMQswCQYDVQQGEwJVUzEL MIIDXDCCAkSgAwIBAgIBAjANBgkqhkiG9w0BAQsFADBQMQswCQYDVQQGEwJVUzEL
...@@ -155,6 +157,28 @@ static folly::ssl::X509UniquePtr readCertFromData( ...@@ -155,6 +157,28 @@ static folly::ssl::X509UniquePtr readCertFromData(
PEM_read_bio_X509(bio.get(), nullptr, nullptr, nullptr)); PEM_read_bio_X509(bio.get(), nullptr, nullptr, nullptr));
} }
// Validate the certs parsed from kTestCertBundle buffer.
static void validateTestCertBundle(
const std::vector<folly::ssl::X509UniquePtr>& certs) {
EXPECT_EQ(certs.size(), 3);
for (auto i : folly::enumerate(certs)) {
auto identity = folly::ssl::OpenSSLCertUtils::getCommonName(**i);
EXPECT_TRUE(identity);
EXPECT_EQ(*identity, folly::sformat("test cert {}", i.index + 1));
}
}
// Validate parsed cert from kTestCertWithSan.
static void validateTestCertWithSAN(X509* x509) {
ASSERT_NE(nullptr, x509);
auto identity = folly::ssl::OpenSSLCertUtils::getCommonName(*x509);
EXPECT_EQ("127.0.0.1", identity.value());
auto altNames = folly::ssl::OpenSSLCertUtils::getSubjectAltNames(*x509);
EXPECT_EQ(2, altNames.size());
EXPECT_EQ("anotherexample.com", altNames[0]);
EXPECT_EQ("*.thirdexample.com", altNames[1]);
}
TEST_F(OpenSSLCertUtilsTest, TestX509CN) { TEST_F(OpenSSLCertUtilsTest, TestX509CN) {
auto x509 = readCertFromFile(kTestCertWithoutSan); auto x509 = readCertFromFile(kTestCertWithoutSan);
EXPECT_NE(x509, nullptr); EXPECT_NE(x509, nullptr);
...@@ -166,13 +190,7 @@ TEST_F(OpenSSLCertUtilsTest, TestX509CN) { ...@@ -166,13 +190,7 @@ TEST_F(OpenSSLCertUtilsTest, TestX509CN) {
TEST_F(OpenSSLCertUtilsTest, TestX509Sans) { TEST_F(OpenSSLCertUtilsTest, TestX509Sans) {
auto x509 = readCertFromData(kTestCertWithSan); auto x509 = readCertFromData(kTestCertWithSan);
EXPECT_NE(x509, nullptr); validateTestCertWithSAN(x509.get());
auto identity = folly::ssl::OpenSSLCertUtils::getCommonName(*x509);
EXPECT_EQ(identity.value(), "127.0.0.1");
auto altNames = folly::ssl::OpenSSLCertUtils::getSubjectAltNames(*x509);
EXPECT_EQ(altNames.size(), 2);
EXPECT_EQ(altNames[0], "anotherexample.com");
EXPECT_EQ(altNames[1], "*.thirdexample.com");
} }
TEST_F(OpenSSLCertUtilsTest, TestX509IssuerAndSubject) { TEST_F(OpenSSLCertUtilsTest, TestX509IssuerAndSubject) {
...@@ -251,11 +269,20 @@ TEST_F(OpenSSLCertUtilsTest, TestDerDecodeTooShort) { ...@@ -251,11 +269,20 @@ TEST_F(OpenSSLCertUtilsTest, TestDerDecodeTooShort) {
TEST_F(OpenSSLCertUtilsTest, TestReadCertsFromBuffer) { TEST_F(OpenSSLCertUtilsTest, TestReadCertsFromBuffer) {
auto certs = folly::ssl::OpenSSLCertUtils::readCertsFromBuffer( auto certs = folly::ssl::OpenSSLCertUtils::readCertsFromBuffer(
StringPiece(kTestCertBundle)); StringPiece(kTestCertBundle));
EXPECT_EQ(certs.size(), 3); validateTestCertBundle(certs);
for (auto i : folly::enumerate(certs)) { }
auto identity = folly::ssl::OpenSSLCertUtils::getCommonName(**i);
EXPECT_TRUE(identity); // readCertsFromBuffer() should manage to read certs from a buffer that contain
EXPECT_EQ(*identity, folly::sformat("test cert {}", i.index + 1)); // both cert and private key.
TEST_F(OpenSSLCertUtilsTest, TestReadCertsFromMixedBuffer) {
std::vector<std::string> bufs(
{folly::to<std::string>(kTestCertWithSan, "\n\n", kTestKey, "\n"),
folly::to<std::string>(kTestKey, "\n\n", kTestCertWithSan, "\n")});
for (auto& buf : bufs) {
auto certs = folly::ssl::OpenSSLCertUtils::readCertsFromBuffer(
folly::StringPiece(buf));
ASSERT_EQ(1, certs.size());
validateTestCertWithSAN(certs.front().get());
} }
} }
...@@ -285,3 +312,33 @@ TEST_F(OpenSSLCertUtilsTest, TestX509Store) { ...@@ -285,3 +312,33 @@ TEST_F(OpenSSLCertUtilsTest, TestX509Store) {
rc = X509_verify_cert(ctx.get()); rc = X509_verify_cert(ctx.get());
EXPECT_EQ(rc, 1); EXPECT_EQ(rc, 1);
} }
TEST_F(OpenSSLCertUtilsTest, TestProcessMalformedCertBuf) {
std::string badCert =
"-----BEGIN CERTIFICATE-----\n"
"yo\n"
"-----END CERTIFICATE-----\n";
EXPECT_THROW(
folly::ssl::OpenSSLCertUtils::readCertsFromBuffer(
folly::StringPiece(badCert)),
std::runtime_error);
EXPECT_THROW(
folly::ssl::OpenSSLCertUtils::readStoreFromBuffer(
folly::StringPiece(badCert)),
std::runtime_error);
std::string bufWithBadCert =
folly::to<std::string>(badCert, "\n", kTestCertBundle);
EXPECT_THROW(
folly::ssl::OpenSSLCertUtils::readCertsFromBuffer(
folly::StringPiece(bufWithBadCert)),
std::runtime_error);
EXPECT_THROW(
folly::ssl::OpenSSLCertUtils::readStoreFromBuffer(
folly::StringPiece(bufWithBadCert)),
std::runtime_error);
}
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment