Add some tests for X509_LOOKUP_hash_dir
Writing these tests revealed that actually this has been broken on
Windows this whole time!
First, the APIs to configure a directory actually configure a
colon-separated list of directories. But file paths contain colons on
Windows, so the actual separator on Windows is semicolon. But that got
lost in the fork at some point. This CL fixes that.
Second, X509_FILETYPE_ASN1 is broken because of a text vs binary mode
mixup. The following CL will fix this.
Some of the behaviors tested here around CRLs are not quite reasonable.
See https://crbug.com/boringssl/690 for details. For now, I've tried to
capture the existing behavior. As BY_DIR actually maintains some shared
mutable state, I've also added TSAn tests.
Another subtlety is that multiple CAs with the same name work, but the
reason they work is pretty messy because OpenSSL's internal interfaces
are incompatible with it. Instead, OpenSSL works around itself with the
X509_STORE cache. These tests do not cover this case, but a subsequent
CL will add tests for it.
Change-Id: Ifd8f2faea164edb0eda771350cd9bf6dc94104e7
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66008
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
diff --git a/crypto/test/file_util.cc b/crypto/test/file_util.cc
index ede0d76..f593e1d 100644
--- a/crypto/test/file_util.cc
+++ b/crypto/test/file_util.cc
@@ -161,3 +161,79 @@
return ScopedFD(open(path_.c_str(), flags));
#endif
}
+
+TemporaryDirectory::~TemporaryDirectory() {
+ if (path_.empty()) {
+ return;
+ }
+
+#if defined(OPENSSL_WINDOWS)
+ for (const auto &file : files_) {
+ if (!DeleteFileA(GetFilePath(file).c_str())) {
+ PrintLastError("Could not delete file");
+ }
+ }
+ if (!RemoveDirectoryA(path_.c_str())) {
+ PrintLastError("Could not delete directory");
+ }
+#else
+ for (const auto &file : files_) {
+ if (unlink(GetFilePath(file).c_str()) != 0) {
+ perror("Could not delete file");
+ }
+ }
+ if (rmdir(path_.c_str()) != 0) {
+ perror("Could not delete directory");
+ }
+#endif
+}
+
+bool TemporaryDirectory::Init() {
+ std::string path = GetTempDir();
+ if (path.empty()) {
+ return false;
+ }
+
+#if defined(OPENSSL_WINDOWS)
+ // For simplicity, just try the first candidate and assume the directory
+ // doesn't exist. 128 bits of cryptographically secure randomness is plenty.
+ uint8_t buf[16];
+ RAND_bytes(buf, sizeof(buf));
+ path += "bssl_tmp_dir." + EncodeHex(buf);
+ if (!CreateDirectoryA(path.c_str(), /*lpSecurityAttributes=*/nullptr)) {
+ perror("Could not make temporary directory");
+ return false;
+ }
+#else
+ path += "bssl_tmp_dir.XXXXXX";
+ // TODO(davidben): Use |path.data()| when we require C++17.
+ if (mkdtemp(&path[0]) == nullptr) {
+ perror("Could not make temporary directory");
+ return false;
+ }
+#endif
+
+ path_ = std::move(path);
+ return true;
+}
+
+bool TemporaryDirectory::AddFile(const std::string &filename,
+ bssl::Span<const uint8_t> content) {
+ if (path_.empty()) {
+ return false;
+ }
+
+ ScopedFILE file(fopen(GetFilePath(filename).c_str(), "wb"));
+ if (file == nullptr) {
+ perror("Could not open temporary file");
+ return false;
+ }
+ if (!content.empty() &&
+ fwrite(content.data(), content.size(), /*nitems=*/1, file.get()) != 1) {
+ perror("Could not write temporary file");
+ return false;
+ }
+
+ files_.insert(filename);
+ return true;
+}
diff --git a/crypto/test/file_util.h b/crypto/test/file_util.h
index 51752b4..0fd8d85 100644
--- a/crypto/test/file_util.h
+++ b/crypto/test/file_util.h
@@ -18,6 +18,7 @@
#include <stdio.h>
#include <memory>
+#include <set>
#include <string>
#include <utility>
@@ -110,4 +111,52 @@
std::string path_;
};
+// TemporaryDirectory manages a temporary directory for testing.
+class TemporaryDirectory {
+ public:
+ TemporaryDirectory() = default;
+ ~TemporaryDirectory();
+
+ TemporaryDirectory(TemporaryDirectory &other) { *this = std::move(other); }
+ TemporaryDirectory& operator=(TemporaryDirectory&&other) {
+ // Ensure |other_| is empty so it doesn't try to delete the directory.
+ path_ = std::exchange(other.path_, {});
+ files_ = std::exchange(other.files_, {});
+ return *this;
+ }
+
+ // Init initializes the temporary directory. It returns true on success and
+ // false on error. On error, callers should call |IgnoreTempFileErrors| to
+ // determine whether to ignore the error.
+ bool Init();
+
+ // path returns the path to the temporary directory.
+ const std::string &path() const { return path_; }
+
+ // AddFile adds a file to the temporary directory with the specified content.
+ // It returns true on success and false on error. Subdirectories in the
+ // temporary directory are not currently supported.
+ bool AddFile(const std::string &filename, bssl::Span<const uint8_t> content);
+ bool AddFile(const std::string &filename, const std::string &content) {
+ return AddFile(
+ filename,
+ bssl::MakeConstSpan(reinterpret_cast<const uint8_t *>(content.data()),
+ content.size()));
+ }
+
+ // GetFilePath returns the path to the speciifed file within the temporary
+ // directory.
+ std::string GetFilePath(const std::string &filename) {
+#if defined(OPENSSL_WINDOWS)
+ return path_ + '\\' + filename;
+#else
+ return path_ + '/' + filename;
+#endif
+ }
+
+ private:
+ std::string path_;
+ std::set<std::string> files_;
+};
+
#endif // OPENSSL_HEADER_CRYPTO_TEST_FILE_UTIL_H
diff --git a/crypto/x509/by_dir.c b/crypto/x509/by_dir.c
index f5e4787..d675f29 100644
--- a/crypto/x509/by_dir.c
+++ b/crypto/x509/by_dir.c
@@ -172,6 +172,12 @@
}
}
+#if defined(OPENSSL_WINDOWS)
+#define DIR_HASH_SEPARATOR ';'
+#else
+#define DIR_HASH_SEPARATOR ':'
+#endif
+
static int add_cert_dir(BY_DIR *ctx, const char *dir, int type) {
size_t j, len;
const char *s, *ss, *p;
@@ -184,7 +190,7 @@
s = dir;
p = s;
do {
- if ((*p == ':') || (*p == '\0')) {
+ if (*p == DIR_HASH_SEPARATOR || *p == '\0') {
BY_DIR_ENTRY *ent;
ss = s;
s = p + 1;
diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc
index 6b06b7c..61bc54d 100644
--- a/crypto/x509/x509_test.cc
+++ b/crypto/x509/x509_test.cc
@@ -36,6 +36,7 @@
#include "internal.h"
#include "../internal.h"
+#include "../test/file_util.h"
#include "../test/test_util.h"
#if defined(OPENSSL_THREADS)
@@ -1701,18 +1702,27 @@
return name;
}
+static bssl::UniquePtr<X509_NAME> MakeTestName(const char *common_name) {
+ bssl::UniquePtr<X509_NAME> name(X509_NAME_new());
+ if (name == nullptr ||
+ !X509_NAME_add_entry_by_txt(
+ name.get(), "CN", MBSTRING_UTF8,
+ reinterpret_cast<const uint8_t *>(common_name), -1, -1, 0)) {
+ return nullptr;
+ }
+ return name;
+}
+
static bssl::UniquePtr<X509> MakeTestCert(const char *issuer,
const char *subject, EVP_PKEY *key,
bool is_ca) {
+ bssl::UniquePtr<X509_NAME> issuer_name = MakeTestName(issuer);
+ bssl::UniquePtr<X509_NAME> subject_name = MakeTestName(subject);
bssl::UniquePtr<X509> cert(X509_new());
- if (!cert || //
+ if (issuer_name == nullptr || subject_name == nullptr || cert == nullptr ||
!X509_set_version(cert.get(), X509_VERSION_3) ||
- !X509_NAME_add_entry_by_txt(
- X509_get_issuer_name(cert.get()), "CN", MBSTRING_UTF8,
- reinterpret_cast<const uint8_t *>(issuer), -1, -1, 0) ||
- !X509_NAME_add_entry_by_txt(
- X509_get_subject_name(cert.get()), "CN", MBSTRING_UTF8,
- reinterpret_cast<const uint8_t *>(subject), -1, -1, 0) ||
+ !X509_set_issuer_name(cert.get(), issuer_name.get()) ||
+ !X509_set_subject_name(cert.get(), subject_name.get()) ||
!X509_set_pubkey(cert.get(), key) ||
!ASN1_TIME_adj(X509_getm_notBefore(cert.get()), kReferenceTime, -1, 0) ||
!ASN1_TIME_adj(X509_getm_notAfter(cert.get()), kReferenceTime, 1, 0)) {
@@ -1770,6 +1780,45 @@
/*flags=*/0);
}
+static bssl::UniquePtr<X509_CRL> MakeTestCRL(const char *issuer,
+ int this_update_offset_day,
+ int next_update_offset_day) {
+ bssl::UniquePtr<X509_NAME> issuer_name = MakeTestName(issuer);
+ bssl::UniquePtr<X509_CRL> crl(X509_CRL_new());
+ bssl::UniquePtr<ASN1_TIME> this_update(ASN1_TIME_adj(
+ nullptr, kReferenceTime, this_update_offset_day, /*offset_sec=*/0));
+ bssl::UniquePtr<ASN1_TIME> next_update(ASN1_TIME_adj(
+ nullptr, kReferenceTime, next_update_offset_day, /*offset_sec=*/0));
+ if (crl == nullptr || issuer_name == nullptr || this_update == nullptr ||
+ next_update == nullptr ||
+ !X509_CRL_set_version(crl.get(), X509_CRL_VERSION_2) ||
+ !X509_CRL_set_issuer_name(crl.get(), issuer_name.get()) ||
+ // OpenSSL's API is named incorrectly. The field is called thisUpdate.
+ !X509_CRL_set1_lastUpdate(crl.get(), this_update.get()) ||
+ !X509_CRL_set1_nextUpdate(crl.get(), next_update.get())) {
+ return nullptr;
+ }
+
+ return crl;
+}
+
+static bool AddRevokedSerialU64(X509_CRL *crl, uint64_t serial,
+ int offset_day) {
+ bssl::UniquePtr<X509_REVOKED> rev(X509_REVOKED_new());
+ bssl::UniquePtr<ASN1_INTEGER> serial_asn1(ASN1_INTEGER_new());
+ bssl::UniquePtr<ASN1_TIME> rev_date(
+ ASN1_TIME_adj(nullptr, kReferenceTime, offset_day, /*offset_sec=*/0));
+ if (rev == nullptr || serial_asn1 == nullptr || rev_date == nullptr ||
+ !ASN1_INTEGER_set_uint64(serial_asn1.get(), serial) ||
+ !X509_REVOKED_set_serialNumber(rev.get(), serial_asn1.get()) ||
+ !X509_REVOKED_set_revocationDate(rev.get(), rev_date.get()) ||
+ !X509_CRL_add0_revoked(crl, rev.get())) {
+ return false;
+ }
+ rev.release(); // X509_CRL_add0_revoked takes ownership on success.
+ return true;
+}
+
TEST(X509Test, NameConstraints) {
bssl::UniquePtr<EVP_PKEY> key = PrivateKeyFromPEM(kP256Key);
ASSERT_TRUE(key);
@@ -7789,3 +7838,551 @@
EXPECT_EQ(X509_V_OK, Verify(leaf.get(), {root.get()}, {}, {},
X509_V_FLAG_IGNORE_CRITICAL));
}
+
+enum NameHash { kOldHash, kNewHash };
+
+// TemporaryHashDir constructs a temporary directory in the format of
+// |X509_LOOKUP_hash_dir|.
+class TemporaryHashDir {
+ public:
+ explicit TemporaryHashDir(int type) : type_(type) {}
+
+ bool Init() { return dir_.Init(); }
+ const std::string &path() const { return dir_.path(); }
+
+ size_t num_cert_hashes() const { return next_cert_.size(); }
+ size_t num_crl_hashes() const { return next_crl_.size(); }
+
+ bool AddCert(X509 *x509, NameHash name_hash) {
+ return AddCertWithHash(HashName(name_hash, X509_get_subject_name(x509)),
+ x509);
+ }
+
+ bool AddCRL(X509_CRL *crl, NameHash name_hash) {
+ return AddCRLWithHash(HashName(name_hash, X509_CRL_get_issuer(crl)), crl);
+ }
+
+ bool AddCertWithHash(uint32_t hash, X509 *cert) {
+ std::vector<uint8_t> data = EncodeCert(cert);
+ if (data.empty()) {
+ return false;
+ }
+ auto &num = next_cert_[hash];
+ char path[32];
+ snprintf(path, sizeof(path), "%08x.%d", hash, num);
+ if (!dir_.AddFile(path, data)) {
+ return false;
+ }
+ num++;
+ return true;
+ }
+
+ bool AddCRLWithHash(uint32_t hash, X509_CRL *crl) {
+ std::vector<uint8_t> data = EncodeCRL(crl);
+ if (data.empty()) {
+ return false;
+ }
+ auto &num = next_crl_[hash];
+ char path[32];
+ snprintf(path, sizeof(path), "%08x.r%d", hash, num);
+ if (!dir_.AddFile(path, data)) {
+ return false;
+ }
+ num++;
+ return true;
+ }
+
+ bool ReplaceLastCRL(X509_CRL *crl, NameHash name_hash) {
+ uint32_t hash = HashName(name_hash, X509_CRL_get_issuer(crl));
+ auto iter = next_crl_.find(hash);
+ if (iter == next_crl_.end()) {
+ return false;
+ }
+ std::vector<uint8_t> data = EncodeCRL(crl);
+ if (data.empty()) {
+ return false;
+ }
+ char path[32];
+ snprintf(path, sizeof(path), "%08x.r%d", hash, iter->second - 1);
+ return dir_.AddFile(path, data);
+ }
+
+ private:
+ static uint32_t HashName(NameHash name_hash, X509_NAME *name) {
+ return name_hash == kOldHash ? X509_NAME_hash_old(name)
+ : X509_NAME_hash(name);
+ }
+
+ std::vector<uint8_t> EncodeCert(X509 *cert) {
+ if (type_ == X509_FILETYPE_ASN1) {
+ uint8_t *der = nullptr;
+ int der_len = i2d_X509(cert, &der);
+ if (der_len < 0) {
+ return {};
+ }
+ bssl::UniquePtr<uint8_t> free_der(der);
+ return std::vector<uint8_t>(der, der + der_len);
+ }
+
+ bssl::UniquePtr<BIO> bio(BIO_new(BIO_s_mem()));
+ const uint8_t *pem;
+ size_t pem_len;
+ if (bio == nullptr || //
+ !PEM_write_bio_X509(bio.get(), cert) ||
+ !BIO_mem_contents(bio.get(), &pem, &pem_len)) {
+ return {};
+ }
+ return std::vector<uint8_t>(pem, pem + pem_len);
+ }
+
+ std::vector<uint8_t> EncodeCRL(X509_CRL *crl) {
+ if (type_ == X509_FILETYPE_ASN1) {
+ uint8_t *der = nullptr;
+ int der_len = i2d_X509_CRL(crl, &der);
+ if (der_len < 0) {
+ return {};
+ }
+ bssl::UniquePtr<uint8_t> free_der(der);
+ return std::vector<uint8_t>(der, der + der_len);
+ }
+
+ bssl::UniquePtr<BIO> bio(BIO_new(BIO_s_mem()));
+ const uint8_t *pem;
+ size_t pem_len;
+ if (bio == nullptr || //
+ !PEM_write_bio_X509_CRL(bio.get(), crl) ||
+ !BIO_mem_contents(bio.get(), &pem, &pem_len)) {
+ return {};
+ }
+ return std::vector<uint8_t>(pem, pem + pem_len);
+ }
+
+ int type_;
+ TemporaryDirectory dir_;
+ std::map<uint32_t, int> next_cert_;
+ std::map<uint32_t, int> next_crl_;
+};
+
+// TODO(davidben): Also test CRL handling. There are some interesting behaviors
+// in here.
+TEST(X509Test, DirHash) {
+ if (SkipTempFileTests()) {
+ GTEST_SKIP();
+ }
+
+ bssl::UniquePtr<EVP_PKEY> key = PrivateKeyFromPEM(kP256Key);
+ ASSERT_TRUE(key);
+
+ // Test both formats.
+ for (int type : {X509_FILETYPE_PEM, X509_FILETYPE_ASN1}) {
+ SCOPED_TRACE(type);
+
+#if defined(OPENSSL_WINDOWS)
+ // TODO(davidben): X509_FILETYPE_ASN1 is currently broken on Windows due to
+ // some text vs binary mixup.
+ if (type == X509_FILETYPE_ASN1) {
+ continue;
+ }
+#endif
+
+ // Generate some roots and fill a directory with OpenSSL's directory hash
+ // format. The hash depends only on the name, so we do not need to
+ // pre-generate the certificates. Test both DER and PEM.
+ TemporaryHashDir dir(type);
+ ASSERT_TRUE(dir.Init());
+
+ auto add_root = [&](const std::string &name, NameHash name_hash) -> bool {
+ bssl::UniquePtr<X509> ca =
+ MakeTestCert(name.c_str(), name.c_str(), key.get(), /*is_ca=*/true);
+ if (ca == nullptr || !X509_sign(ca.get(), key.get(), EVP_sha256())) {
+ return false;
+ }
+ return dir.AddCert(ca.get(), name_hash);
+ };
+
+ auto issue_crl =
+ [&](const std::string &name, int this_update_offset_day,
+ const std::vector<uint64_t> &serials) -> bssl::UniquePtr<X509_CRL> {
+ bssl::UniquePtr<X509_CRL> crl = MakeTestCRL(
+ name.c_str(), this_update_offset_day, /*next_update_offset_day=*/1);
+ if (crl == nullptr) {
+ return nullptr;
+ }
+ for (uint64_t serial : serials) {
+ // The revocation time does not matter for this test. Pretend the
+ // certificate was just revoked.
+ if (!AddRevokedSerialU64(crl.get(), serial,
+ /*offset_day=*/this_update_offset_day)) {
+ return nullptr;
+ }
+ }
+ if (!X509_CRL_sign(crl.get(), key.get(), EVP_sha256())) {
+ return nullptr;
+ }
+ return crl;
+ };
+
+ auto add_crl = [&](const std::string &name, NameHash name_hash,
+ int this_update_offset_day,
+ const std::vector<uint64_t> &serials) -> bool {
+ bssl::UniquePtr<X509_CRL> crl =
+ issue_crl(name, this_update_offset_day, serials);
+ return crl != nullptr && dir.AddCRL(crl.get(), name_hash);
+ };
+
+ std::string ca1 = "Test CA 1";
+ ASSERT_TRUE(add_root(ca1, kNewHash));
+
+ std::string ca2 = "Test CA 2";
+ ASSERT_TRUE(add_root(ca2, kOldHash));
+
+ // Install CA 3 at its new hash. CA 3's name is not canonical, but the new
+ // hash runs after canonicalization, so OpenSSL should be able to find it.
+ std::string ca3 = "Test CA 3";
+ std::string ca3_noncanonical = " test ca 3 ";
+ ASSERT_TRUE(add_root(ca3_noncanonical, kNewHash));
+
+ // These two CAs collide under |X509_NAME_hash|.
+ std::string collide_name1 = "Test CA 1191514847";
+ std::string collide_name2 = "Test CA 1570301806";
+ size_t num_cert_hashes = dir.num_cert_hashes();
+ ASSERT_TRUE(add_root(collide_name1, kNewHash));
+ EXPECT_EQ(dir.num_cert_hashes(), num_cert_hashes + 1);
+ ASSERT_TRUE(add_root(collide_name2, kNewHash));
+ EXPECT_EQ(dir.num_cert_hashes(), num_cert_hashes + 1);
+
+ // These two CAs collide under |X509_NAME_hash_old|.
+ std::string old_collide_name1 = "Test CA 1069881739";
+ std::string old_collide_name2 = "Test CA 940754110";
+ num_cert_hashes = dir.num_cert_hashes();
+ ASSERT_TRUE(add_root(old_collide_name1, kOldHash));
+ EXPECT_EQ(dir.num_cert_hashes(), num_cert_hashes + 1);
+ ASSERT_TRUE(add_root(old_collide_name2, kOldHash));
+ EXPECT_EQ(dir.num_cert_hashes(), num_cert_hashes + 1);
+
+ // Make an |X509_STORE| that gets CAs from |dir|.
+ bssl::UniquePtr<X509_STORE> store(X509_STORE_new());
+ ASSERT_TRUE(store);
+ X509_LOOKUP *lookup =
+ X509_STORE_add_lookup(store.get(), X509_LOOKUP_hash_dir());
+ ASSERT_TRUE(lookup);
+ ASSERT_TRUE(X509_LOOKUP_add_dir(lookup, dir.path().c_str(), type));
+
+ auto test_issuer_flags = [&](const std::string &issuer, uint64_t serial,
+ unsigned long flags) -> int {
+ bssl::UniquePtr<X509> cert =
+ MakeTestCert(issuer.c_str(), "Leaf", key.get(), /*is_ca=*/false);
+ bssl::UniquePtr<ASN1_INTEGER> serial_asn1(ASN1_INTEGER_new());
+ if (cert == nullptr || serial_asn1 == nullptr ||
+ !ASN1_INTEGER_set_uint64(serial_asn1.get(), serial) ||
+ !X509_set_serialNumber(cert.get(), serial_asn1.get()) ||
+ !X509_sign(cert.get(), key.get(), EVP_sha256())) {
+ return X509_V_ERR_UNSPECIFIED;
+ }
+
+ bssl::UniquePtr<X509_STORE_CTX> ctx(X509_STORE_CTX_new());
+ if (ctx == nullptr ||
+ !X509_STORE_CTX_init(ctx.get(), store.get(), cert.get(),
+ /*chain=*/nullptr)) {
+ return X509_V_ERR_UNSPECIFIED;
+ }
+ X509_STORE_CTX_set_flags(ctx.get(), flags);
+ X509_STORE_CTX_set_time_posix(ctx.get(), /*flags=*/0, kReferenceTime);
+
+ return X509_verify_cert(ctx.get()) ? X509_V_OK
+ : X509_STORE_CTX_get_error(ctx.get());
+ };
+
+ auto test_issuer = [&](const std::string &issuer) -> int {
+ return test_issuer_flags(issuer, /*serial=*/0, /*flags=*/0);
+ };
+ auto test_issuer_crl = [&](const std::string &issuer,
+ uint64_t serial) -> int {
+ return test_issuer_flags(issuer, serial, X509_V_FLAG_CRL_CHECK);
+ };
+
+ // All these roots are in the store and should be found. Although Test CA
+ // 3 was installed under a non-canonical name, the new hash accounts for
+ // this.
+ EXPECT_EQ(X509_V_OK, test_issuer(ca1));
+ EXPECT_EQ(X509_V_OK, test_issuer(ca2));
+ EXPECT_EQ(X509_V_OK, test_issuer(ca3));
+ EXPECT_EQ(X509_V_OK, test_issuer(collide_name1));
+ EXPECT_EQ(X509_V_OK, test_issuer(collide_name2));
+ EXPECT_EQ(X509_V_OK, test_issuer(old_collide_name1));
+ EXPECT_EQ(X509_V_OK, test_issuer(old_collide_name2));
+
+ // Repeat the tests. This time it will pick up the certificate from the
+ // cache.
+ EXPECT_EQ(X509_V_OK, test_issuer(ca1));
+ EXPECT_EQ(X509_V_OK, test_issuer(ca2));
+ EXPECT_EQ(X509_V_OK, test_issuer(ca3));
+ EXPECT_EQ(X509_V_OK, test_issuer(collide_name1));
+ EXPECT_EQ(X509_V_OK, test_issuer(collide_name2));
+ EXPECT_EQ(X509_V_OK, test_issuer(old_collide_name1));
+ EXPECT_EQ(X509_V_OK, test_issuer(old_collide_name2));
+
+ // Test a certificate not in the store.
+ EXPECT_EQ(X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY,
+ test_issuer("Not In Store"));
+
+ // Test CRL handling. First, if we cannot find a CRL, verification will
+ // fail.
+ //
+ // TODO(crbug.com/boringssl/690): We should test both the old and new hash,
+ // but the CRL reloading process does not work for the old hash due to a
+ // bug. It notices the cached old CRL, mistakes it for something loaded via
+ // the new hash, and never bothers checking the old hash.
+ EXPECT_EQ(X509_V_ERR_UNABLE_TO_GET_CRL,
+ test_issuer_crl(collide_name1, /*serial=*/1));
+
+ // Install an empty CRL. Verification should now succeed.
+ ASSERT_TRUE(add_crl(collide_name1, kNewHash,
+ /*this_update_offset_day=*/-10, /*serials=*/{}));
+ EXPECT_EQ(X509_V_OK, test_issuer_crl(collide_name1, /*serial=*/1));
+
+ // Verify again. Unlike roots, which are cached, this will query the
+ // directory again.
+ EXPECT_EQ(X509_V_OK, test_issuer_crl(collide_name1, /*serial=*/1));
+
+ // The extra query is so that a newer CRL is picked up, at an incrementing
+ // number. This feature is less useful than it sounds because all CRLs are
+ // persistently cached.
+ ASSERT_TRUE(add_crl(collide_name1, kNewHash,
+ /*this_update_offset_day=*/-9, /*serials=*/{1}));
+ EXPECT_EQ(X509_V_ERR_CERT_REVOKED,
+ test_issuer_crl(collide_name1, /*serial=*/1));
+
+ // Serial number 2 is not revoked.
+ EXPECT_EQ(X509_V_OK, test_issuer_crl(collide_name1, /*serial=*/2));
+
+ // A new CRL at an already loaded name is ignored because OpenSSL skips
+ // loading the older ones and relies on them being persistently cached in
+ // memory.
+ //
+ // TODO(crbug.com/boringssl/690): This behavior is almost certainly not what
+ // anyone wants. Rework this.
+ bssl::UniquePtr<X509_CRL> crl = issue_crl(
+ collide_name1, /*this_update_offset_day=*/-8, /*serials=*/{1, 2});
+ ASSERT_TRUE(crl);
+ ASSERT_TRUE(dir.ReplaceLastCRL(crl.get(), kNewHash));
+ EXPECT_EQ(X509_V_OK, test_issuer_crl(collide_name1, /*serial=*/2));
+
+ // If there are many new CRLs, they are all loaded and the newest is wins.
+ ASSERT_TRUE(add_crl(collide_name1, kNewHash,
+ /*this_update_offset_day=*/-7, /*serials=*/{1, 2}));
+ ASSERT_TRUE(add_crl(collide_name1, kNewHash,
+ /*this_update_offset_day=*/-5, /*serials=*/{1, 2, 3}));
+ ASSERT_TRUE(add_crl(collide_name1, kNewHash,
+ /*this_update_offset_day=*/-6, /*serials=*/{1, 2}));
+
+ // r3 should have won, which revokes all three serials:
+ EXPECT_EQ(X509_V_ERR_CERT_REVOKED,
+ test_issuer_crl(collide_name1, /*serial=*/1));
+ EXPECT_EQ(X509_V_ERR_CERT_REVOKED,
+ test_issuer_crl(collide_name1, /*serial=*/2));
+ EXPECT_EQ(X509_V_ERR_CERT_REVOKED,
+ test_issuer_crl(collide_name1, /*serial=*/3));
+
+ // If the new CRL is older than a previously loaded one, it is ignored.
+ ASSERT_TRUE(add_crl(collide_name1, kNewHash,
+ /*this_update_offset_day=*/-100, /*serials=*/{}));
+
+ // Finally, test hash collisions. The internal book-keeping for where to
+ // start loading should be compatible with a second CA whose hash collides.
+ EXPECT_EQ(X509_V_ERR_UNABLE_TO_GET_CRL,
+ test_issuer_crl(collide_name2, /*serial=*/1));
+ EXPECT_EQ(X509_V_ERR_UNABLE_TO_GET_CRL,
+ test_issuer_crl(collide_name2, /*serial=*/2));
+ ASSERT_TRUE(add_crl(collide_name2, kNewHash,
+ /*this_update_offset_day=*/-10, /*serials=*/{1}));
+ EXPECT_EQ(X509_V_ERR_CERT_REVOKED,
+ test_issuer_crl(collide_name2, /*serial=*/1));
+ EXPECT_EQ(X509_V_OK, test_issuer_crl(collide_name2, /*serial=*/2));
+
+ // Confirm all CRLs we added had the same hash.
+ EXPECT_EQ(dir.num_crl_hashes(), 1u);
+ }
+}
+
+// Test that two directory hash paths are treated as a sequence of paths,
+// separated by a separator.
+TEST(X509Test, DirHashSeparator) {
+#if defined(OPENSSL_WINDOWS)
+ const char kSeparator = ';';
+#else
+ const char kSeparator = ':';
+#endif
+
+ if (SkipTempFileTests()) {
+ GTEST_SKIP();
+ }
+
+ bssl::UniquePtr<EVP_PKEY> key = PrivateKeyFromPEM(kP256Key);
+ ASSERT_TRUE(key);
+
+ // Make two directories and place one CA in each.
+ TemporaryHashDir dir1(X509_FILETYPE_PEM), dir2(X509_FILETYPE_PEM);
+ ASSERT_TRUE(dir1.Init());
+ ASSERT_TRUE(dir2.Init());
+
+ bssl::UniquePtr<X509> ca1 =
+ MakeTestCert("Test CA 1", "Test CA 1", key.get(), /*is_ca=*/true);
+ ASSERT_TRUE(ca1);
+ ASSERT_TRUE(X509_sign(ca1.get(), key.get(), EVP_sha256()));
+ ASSERT_TRUE(dir1.AddCert(ca1.get(), kNewHash));
+
+ bssl::UniquePtr<X509> ca2 =
+ MakeTestCert("Test CA 2", "Test CA 2", key.get(), /*is_ca=*/true);
+ ASSERT_TRUE(ca2);
+ ASSERT_TRUE(X509_sign(ca2.get(), key.get(), EVP_sha256()));
+ ASSERT_TRUE(dir1.AddCert(ca2.get(), kNewHash));
+
+ // Make an |X509_STORE| that gets CAs from |dir1| and |dir2|.
+ bssl::UniquePtr<X509_STORE> store(X509_STORE_new());
+ ASSERT_TRUE(store);
+ std::string paths = dir1.path() + kSeparator + dir2.path();
+ ASSERT_TRUE(
+ X509_STORE_load_locations(store.get(), /*file=*/nullptr, paths.c_str()));
+
+ // Both CAs should work.
+ {
+ bssl::UniquePtr<X509> cert =
+ MakeTestCert("Test CA 1", "Leaf", key.get(), /*is_ca=*/false);
+ ASSERT_TRUE(cert);
+ ASSERT_TRUE(X509_sign(cert.get(), key.get(), EVP_sha256()));
+ bssl::UniquePtr<X509_STORE_CTX> ctx(X509_STORE_CTX_new());
+ ASSERT_TRUE(ctx);
+ ASSERT_TRUE(X509_STORE_CTX_init(ctx.get(), store.get(), cert.get(),
+ /*chain=*/nullptr));
+ X509_STORE_CTX_set_time_posix(ctx.get(), /*flags=*/0, kReferenceTime);
+ EXPECT_TRUE(X509_verify_cert(ctx.get()))
+ << X509_verify_cert_error_string(X509_STORE_CTX_get_error(ctx.get()));
+ }
+
+ {
+ bssl::UniquePtr<X509> cert =
+ MakeTestCert("Test CA 2", "Leaf", key.get(), /*is_ca=*/false);
+ ASSERT_TRUE(cert);
+ ASSERT_TRUE(X509_sign(cert.get(), key.get(), EVP_sha256()));
+ bssl::UniquePtr<X509_STORE_CTX> ctx(X509_STORE_CTX_new());
+ ASSERT_TRUE(ctx);
+ ASSERT_TRUE(X509_STORE_CTX_init(ctx.get(), store.get(), cert.get(),
+ /*chain=*/nullptr));
+ X509_STORE_CTX_set_time_posix(ctx.get(), /*flags=*/0, kReferenceTime);
+ EXPECT_TRUE(X509_verify_cert(ctx.get()))
+ << X509_verify_cert_error_string(X509_STORE_CTX_get_error(ctx.get()));
+ }
+}
+
+#if defined(OPENSSL_THREADS)
+// Test that directory hash lookup is thread-safe.
+TEST(X509Test, DirHashThreads) {
+ if (SkipTempFileTests()) {
+ GTEST_SKIP();
+ }
+
+ bssl::UniquePtr<EVP_PKEY> key = PrivateKeyFromPEM(kP256Key);
+ ASSERT_TRUE(key);
+
+ // Generate some roots and fill a directory with OpenSSL's directory hash
+ // format. The hash depends only on the name, so we do not need to
+ // pre-generate the certificates. Test both DER and PEM.
+ TemporaryHashDir dir(X509_FILETYPE_PEM);
+ ASSERT_TRUE(dir.Init());
+
+ auto add_root = [&](const std::string &name, NameHash name_hash) -> bool {
+ bssl::UniquePtr<X509> ca =
+ MakeTestCert(name.c_str(), name.c_str(), key.get(), /*is_ca=*/true);
+ return ca != nullptr && //
+ X509_sign(ca.get(), key.get(), EVP_sha256()) &&
+ dir.AddCert(ca.get(), name_hash);
+ };
+
+ auto issue_cert = [&](const std::string &issuer) -> bssl::UniquePtr<X509> {
+ bssl::UniquePtr<X509> cert =
+ MakeTestCert(issuer.c_str(), "Leaf", key.get(), /*is_ca=*/false);
+ if (cert == nullptr || !X509_sign(cert.get(), key.get(), EVP_sha256())) {
+ return nullptr;
+ }
+ return cert;
+ };
+
+ auto add_crl = [&](const std::string &name,
+ int this_update_offset_day, NameHash name_hash) -> bool {
+ bssl::UniquePtr<X509_CRL> crl = MakeTestCRL(
+ name.c_str(), this_update_offset_day, /*next_update_offset_day=*/1);
+ return crl != nullptr &&
+ X509_CRL_sign(crl.get(), key.get(), EVP_sha256()) &&
+ dir.AddCRL(crl.get(), name_hash);
+ };
+
+ // These two CAs collide under |X509_NAME_hash|.
+ std::string ca1 = "Test CA 1191514847";
+ std::string ca2 = "Test CA 1570301806";
+ ASSERT_TRUE(add_root(ca1, kNewHash));
+ ASSERT_TRUE(add_root(ca2, kNewHash));
+ ASSERT_TRUE(add_crl(ca1, -2, kNewHash));
+ ASSERT_TRUE(add_crl(ca2, -1, kNewHash));
+ ASSERT_TRUE(add_crl(ca2, -2, kNewHash));
+ ASSERT_TRUE(add_crl(ca1, -1, kNewHash));
+ // Verify the hashes collided.
+ ASSERT_EQ(dir.num_cert_hashes(), 1u);
+ ASSERT_EQ(dir.num_crl_hashes(), 1u);
+ bssl::UniquePtr<X509> leaf1 = issue_cert(ca1);
+ ASSERT_TRUE(leaf1);
+ bssl::UniquePtr<X509> leaf2 = issue_cert(ca2);
+ ASSERT_TRUE(leaf2);
+
+ // These two CAs collide under |X509_NAME_hash_old|.
+ std::string old_ca1 = "Test CA 1069881739";
+ std::string old_ca2 = "Test CA 940754110";
+ ASSERT_TRUE(add_root(old_ca1, kOldHash));
+ ASSERT_TRUE(add_root(old_ca2, kOldHash));
+ ASSERT_TRUE(add_crl(old_ca1, -2, kOldHash));
+ ASSERT_TRUE(add_crl(old_ca2, -1, kOldHash));
+ ASSERT_TRUE(add_crl(old_ca2, -2, kOldHash));
+ ASSERT_TRUE(add_crl(old_ca1, -1, kOldHash));
+ // Verify the hashes collided.
+ ASSERT_EQ(dir.num_cert_hashes(), 2u);
+ ASSERT_EQ(dir.num_crl_hashes(), 2u);
+ bssl::UniquePtr<X509> old_leaf1 = issue_cert(old_ca1);
+ ASSERT_TRUE(old_leaf1);
+ bssl::UniquePtr<X509> old_leaf2 = issue_cert(old_ca2);
+ ASSERT_TRUE(old_leaf2);
+
+ // Make an |X509_STORE| that gets CAs from |dir|.
+ bssl::UniquePtr<X509_STORE> store(X509_STORE_new());
+ ASSERT_TRUE(store);
+ ASSERT_TRUE(X509_STORE_load_locations(store.get(), /*file=*/nullptr,
+ dir.path().c_str()));
+
+ auto verify = [&](X509 *cert, bool crl_check) {
+ bssl::UniquePtr<X509_STORE_CTX> ctx(X509_STORE_CTX_new());
+ ASSERT_TRUE(ctx);
+ ASSERT_TRUE(X509_STORE_CTX_init(ctx.get(), store.get(), cert,
+ /*chain=*/nullptr));
+ X509_STORE_CTX_set_flags(ctx.get(), crl_check ? X509_V_FLAG_CRL_CHECK : 0);
+ X509_STORE_CTX_set_time_posix(ctx.get(), /*flags=*/0, kReferenceTime);
+ EXPECT_TRUE(X509_verify_cert(ctx.get()))
+ << X509_verify_cert_error_string(X509_STORE_CTX_get_error(ctx.get()));
+ };
+
+ const size_t kNumThreads = 10;
+ std::vector<std::thread> threads;
+ for (size_t i = 0; i < kNumThreads; i++) {
+ threads.emplace_back([&] { verify(leaf1.get(), false); });
+ threads.emplace_back([&] { verify(leaf1.get(), true); });
+ threads.emplace_back([&] { verify(leaf2.get(), false); });
+ threads.emplace_back([&] { verify(leaf2.get(), true); });
+
+ threads.emplace_back([&] { verify(old_leaf1.get(), false); });
+ threads.emplace_back([&] { verify(old_leaf1.get(), true); });
+ threads.emplace_back([&] { verify(old_leaf2.get(), false); });
+ threads.emplace_back([&] { verify(old_leaf2.get(), true); });
+ }
+ for (auto &thread : threads) {
+ thread.join();
+ }
+}
+#endif // OPENSSL_THREADS
diff --git a/include/openssl/x509.h b/include/openssl/x509.h
index 38e367a..b2eafcc 100644
--- a/include/openssl/x509.h
+++ b/include/openssl/x509.h
@@ -4536,6 +4536,13 @@
// should be one of the |X509_FILETYPE_*| constants to determine if the contents
// are PEM or DER. If |type| is |X509_FILETYPE_DEFAULT|, |path| is ignored and
// instead some default system path is used.
+//
+// WARNING: |path| is interpreted as a colon-separated (semicolon-separated on
+// Windows) list of paths. It is not possible to configure a path containing the
+// separator character.
+//
+// TODO(crbug.com/boringssl/691): The colon handling is surprising. Consider
+// removing it.
OPENSSL_EXPORT int X509_LOOKUP_add_dir(X509_LOOKUP *lookup, const char *path,
int type);
