From 3147785eb23bb27080a0b7accbbff46ac471e86c Mon Sep 17 00:00:00 2001 From: Hugo Landau Date: Wed, 18 Jan 2023 15:43:56 +0000 Subject: [PATCH] Fix corruption when searching for CRLs in hashed directories The by_dir certificate/CRL lookup code uses an OPENSSL_STACK to track how many sequentially numbered CRL files have been loaded for a given X509_NAME hash which is being requested. This avoids loading already loaded CRL files and repeated stat() calls. This OPENSSL_STACK is searched using sk_find, however this mutates the OPENSSL_STACK unless it is known to be sorted. This operation therefore requires a write lock, which was not taken. Fix this issue by sorting the OPENSSL_STACK whenever it is mutated. This guarantees no mutation will occur during sk_find. This is chosen over taking a write lock during sk_find as retrieving a CRL by X509_NAME is assumed to be a hotter path than the case where a new CRL is installed. Also optimise the code by avoiding creating the structure to track the last CRL file sequence number in the circumstance where it would match the initial value, namely where no CRL with the given hash is installed. Reviewed-by: Tomas Mraz Reviewed-by: Paul Dale (Merged from https://github.com/openssl/openssl/pull/20076) --- crypto/x509/by_dir.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/crypto/x509/by_dir.c b/crypto/x509/by_dir.c index 68b0b865d3..3fc52be474 100644 --- a/crypto/x509/by_dir.c +++ b/crypto/x509/by_dir.c @@ -354,9 +354,13 @@ static int get_cert_by_subject_ex(X509_LOOKUP *xl, X509_LOOKUP_TYPE type, tmp = sk_X509_OBJECT_value(xl->store_ctx->objs, j); X509_STORE_unlock(xl->store_ctx); - /* If a CRL, update the last file suffix added for this */ - - if (type == X509_LU_CRL) { + /* + * If a CRL, update the last file suffix added for this. + * We don't need to add an entry if k is 0 as this is the initial value. + * This avoids the need for a write lock and sort operation in the + * simple case where no CRL is present for a hash. + */ + if (type == X509_LU_CRL && k > 0) { if (!CRYPTO_THREAD_write_lock(ctx->lock)) goto finish; /* @@ -384,6 +388,12 @@ static int get_cert_by_subject_ex(X509_LOOKUP *xl, X509_LOOKUP_TYPE type, ok = 0; goto finish; } + + /* + * Ensure stack is sorted so that subsequent sk_BY_DIR_HASH_find + * will not mutate the stack and therefore require a write lock. + */ + sk_BY_DIR_HASH_sort(ent->hashes); } else if (hent->suffix < k) { hent->suffix = k; } -- 2.34.1