From: Nikola Pajkovsky Date: Thu, 18 Sep 2025 09:13:45 +0000 (+0200) Subject: x509store: reduce lock contention in X509_STORE X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=04589b59ef50b1f8a4cec5d15ce172c32e0aa01c;p=thirdparty%2Fopenssl.git x509store: reduce lock contention in X509_STORE X509_STORE was using STACK_OF(X509_OBJECT) which is not ideal structure. The better solution is to use hashmap. The performance gains come from the fact that sorting was removed and therefore read lock is just enough for looking up objects/cert/crls from hashmap. When X509_STORE_get0_objects() is called, the hashmap converts back to the STACK_OF(X509_OBJECT), and goes back to the original implementation with the performance hit on lookup side because stack is not sorted anymore. Note, hashmap maps X509_NAME to STACK_OF(X509_OBJECT), and the stack is never sorted which may lead to performance impact if stack contains a huge of objects. Before the change | Threads | mean/us | var/us | |---------+-----------+---------| | 1 | 2.434803 | .034190 | | 2 | 3.033588 | .247471 | | 4 | 6.551132 | .150209 | | 6 | 12.548113 | .258445 | | 8 | 17.566257 | .168508 | | 10 | 22.782846 | .182674 | | 12 | 27.928990 | .426779 | | 14 | 32.844572 | .307754 | | 16 | 37.816247 | .660630 | | 18 | 42.662465 | .434926 | After the change | Threads | mean/us | var/us | |---------+----------+---------| | 1 | 2.385398 | .015329 | | 2 | 2.775794 | .172223 | | 4 | 3.071882 | .126400 | | 6 | 3.174147 | .139685 | | 8 | 3.479235 | .297154 | | 10 | 4.206260 | .149006 | | 12 | 5.044039 | .194108 | | 14 | 5.890640 | .185817 | | 16 | 6.447808 | .256179 | | 18 | 7.489261 | .149204 | Resolves: https://github.com/openssl/project/issues/1275 Signed-off-by: Nikola Pajkovsky Reviewed-by: Neil Horman Reviewed-by: Saša Nedvědický (Merged from https://github.com/openssl/openssl/pull/28599) --- diff --git a/crypto/x509/by_dir.c b/crypto/x509/by_dir.c index b3731b01d15..20ad8062fc7 100644 --- a/crypto/x509/by_dir.c +++ b/crypto/x509/by_dir.c @@ -232,7 +232,7 @@ static int get_cert_by_subject_ex(X509_LOOKUP *xl, X509_LOOKUP_TYPE type, int i, j, k; unsigned long h; BUF_MEM *b = NULL; - X509_OBJECT stmp, *tmp; + X509_OBJECT stmp, *tmp = NULL; const char *postfix = ""; if (name == NULL) @@ -348,10 +348,18 @@ static int get_cert_by_subject_ex(X509_LOOKUP *xl, X509_LOOKUP_TYPE type, * sorted and sorting the would result in O(n^2 log n) complexity. */ if (k > 0) { - if (!X509_STORE_lock(xl->store_ctx)) + STACK_OF(X509_OBJECT) *objs; + + if (ossl_x509_store_read_lock(xl->store_ctx) == 0) goto finish; - j = sk_X509_OBJECT_find(xl->store_ctx->objs, &stmp); - tmp = sk_X509_OBJECT_value(xl->store_ctx->objs, j); + if (xl->store_ctx->objs_ht) + objs = ossl_x509_store_ht_get_by_name(xl->store_ctx, name); + else + objs = xl->store_ctx->objs; + if (objs != NULL) { + j = sk_X509_OBJECT_find(objs, &stmp); + tmp = sk_X509_OBJECT_value(objs, j); + } X509_STORE_unlock(xl->store_ctx); } else { tmp = NULL; @@ -419,14 +427,6 @@ static int get_cert_by_subject_ex(X509_LOOKUP *xl, X509_LOOKUP_TYPE type, } } finish: - /* If we changed anything, resort the objects for faster lookup */ - if (X509_STORE_lock(xl->store_ctx)) { - if (!sk_X509_OBJECT_is_sorted(xl->store_ctx->objs)) { - sk_X509_OBJECT_sort(xl->store_ctx->objs); - } - X509_STORE_unlock(xl->store_ctx); - } - BUF_MEM_free(b); return ok; } diff --git a/crypto/x509/by_store.c b/crypto/x509/by_store.c index a969e3aa05b..043801c2efc 100644 --- a/crypto/x509/by_store.c +++ b/crypto/x509/by_store.c @@ -218,18 +218,22 @@ static int by_store_subject(X509_LOOKUP *ctx, X509_LOOKUP_TYPE type, OSSL_STORE_SEARCH *criterion = OSSL_STORE_SEARCH_by_name((X509_NAME *)name); /* won't modify it */ int ok = by_store(ctx, type, criterion, ret); - STACK_OF(X509_OBJECT) *store_objects = - X509_STORE_get0_objects(X509_LOOKUP_get_store(ctx)); X509_OBJECT *tmp = NULL; OSSL_STORE_SEARCH_free(criterion); if (ok) { + STACK_OF(X509_OBJECT) *store_objects; X509_STORE *store = X509_LOOKUP_get_store(ctx); if (!ossl_x509_store_read_lock(store)) return 0; - tmp = X509_OBJECT_retrieve_by_subject(store_objects, type, name); + if (store->objs_ht != NULL) + store_objects = ossl_x509_store_ht_get_by_name(store, name); + else + store_objects = store->objs; + if (store_objects != NULL) + tmp = X509_OBJECT_retrieve_by_subject(store_objects, type, name); X509_STORE_unlock(store); } diff --git a/crypto/x509/x509_local.h b/crypto/x509/x509_local.h index ca56f478874..649bd32405c 100644 --- a/crypto/x509/x509_local.h +++ b/crypto/x509/x509_local.h @@ -8,6 +8,7 @@ */ #include "internal/refcount.h" +#include "internal/hashtable.h" #define X509V3_conf_add_error_name_value(val) \ ERR_add_error_data(4, "name=", (val)->name, ", value=", (val)->value) @@ -106,6 +107,11 @@ struct x509_lookup_st { X509_STORE *store_ctx; /* who owns us */ }; +HT_START_KEY_DEFN(objs_key) +HT_DEF_KEY_FIELD(xn_canon, unsigned char *) +HT_DEF_KEY_FIELD(xn_canon_enclen, int) +HT_END_KEY_DEFN(OBJS_KEY) + /* * This is used to hold everything. It is used for all certificate * validation. Once we have a certificate chain, the 'verify' function is @@ -114,7 +120,13 @@ struct x509_lookup_st { struct x509_store_st { /* The following is a cache of trusted certs */ int cache; /* if true, stash any hits */ - STACK_OF(X509_OBJECT) *objs; /* Cache of all objects */ + /* Maps X509_NAME -> STACK_OF(X509_OBJECT) */ + HT *objs_ht; + /* + * Deprecated. Used only in the X509_STORE_get0_objects() for backward + * compatibility. + */ + STACK_OF(X509_OBJECT) *objs; /* These are external lookup methods */ STACK_OF(X509_LOOKUP) *get_cert_methods; X509_VERIFY_PARAM *param; @@ -159,4 +171,6 @@ int ossl_x509_likely_issued(X509 *issuer, X509 *subject); int ossl_x509_signing_allowed(const X509 *issuer, const X509 *subject); int ossl_x509_store_ctx_get_by_subject(const X509_STORE_CTX *ctx, X509_LOOKUP_TYPE type, const X509_NAME *name, X509_OBJECT *ret); -int ossl_x509_store_read_lock(X509_STORE *xs); +__owur int ossl_x509_store_read_lock(X509_STORE *xs); +STACK_OF(X509_OBJECT) *ossl_x509_store_ht_get_by_name(const X509_STORE *store, + const X509_NAME *xn); diff --git a/crypto/x509/x509_lu.c b/crypto/x509/x509_lu.c index eb2d47955b2..f724bf1e2d5 100644 --- a/crypto/x509/x509_lu.c +++ b/crypto/x509/x509_lu.c @@ -9,12 +9,16 @@ #include #include "internal/cryptlib.h" +#include "internal/hashtable.h" +#include "internal/hashfunc.h" #include "internal/refcount.h" #include #include "crypto/x509.h" #include #include "x509_local.h" +#define X509_OBJS_HT_BUCKETS 512 + X509_LOOKUP *X509_LOOKUP_new(X509_LOOKUP_METHOD *method) { X509_LOOKUP *ret = OPENSSL_zalloc(sizeof(*ret)); @@ -179,12 +183,36 @@ static int x509_object_cmp(const X509_OBJECT *const *a, return ret; } +static void objs_ht_free(HT_VALUE *v) +{ + STACK_OF(X509_OBJECT) *objs = v->value; + + sk_X509_OBJECT_pop_free(objs, X509_OBJECT_free); +} + +static uint64_t obj_ht_hash(HT_KEY *key) +{ + OBJS_KEY *k = (OBJS_KEY *)key; + + return ossl_fnv1a_hash(k->keyfields.xn_canon, k->keyfields.xn_canon_enclen); +} + X509_STORE *X509_STORE_new(void) { X509_STORE *ret = OPENSSL_zalloc(sizeof(*ret)); + HT_CONFIG htconf = { + .ht_free_fn = objs_ht_free, + .ht_hash_fn = obj_ht_hash, + .init_neighborhoods = X509_OBJS_HT_BUCKETS, + .no_rcu = 1, + }; if (ret == NULL) return NULL; + if ((ret->objs_ht = ossl_ht_new(&htconf)) == NULL) { + ERR_raise(ERR_LIB_X509, ERR_R_CRYPTO_LIB); + goto err; + } if ((ret->objs = sk_X509_OBJECT_new(x509_object_cmp)) == NULL) { ERR_raise(ERR_LIB_X509, ERR_R_CRYPTO_LIB); goto err; @@ -218,6 +246,7 @@ err: X509_VERIFY_PARAM_free(ret->param); sk_X509_OBJECT_free(ret->objs); sk_X509_LOOKUP_free(ret->get_cert_methods); + ossl_ht_free(ret->objs_ht); CRYPTO_THREAD_lock_free(ret->lock); OPENSSL_free(ret); return NULL; @@ -250,6 +279,7 @@ void X509_STORE_free(X509_STORE *xs) X509_VERIFY_PARAM_free(xs->param); CRYPTO_THREAD_lock_free(xs->lock); CRYPTO_FREE_REF(&xs->references); + ossl_ht_free(xs->objs_ht); OPENSSL_free(xs); } @@ -310,6 +340,93 @@ X509_OBJECT *X509_STORE_CTX_get_obj_by_subject(X509_STORE_CTX *ctx, return ret; } +STACK_OF(X509_OBJECT) *ossl_x509_store_ht_get_by_name(const X509_STORE *store, + const X509_NAME *xn) +{ + HT_VALUE *v; + OBJS_KEY key; + int ret; + + if (xn->canon_enc == NULL || xn->modified) { + ret = i2d_X509_NAME((X509_NAME *)xn, NULL); + if (ret < 0) + return NULL; + } + + HT_INIT_KEY(&key); + HT_SET_KEY_FIELD(&key, xn_canon, xn->canon_enc); + HT_SET_KEY_FIELD(&key, xn_canon_enclen, xn->canon_enclen); + v = ossl_ht_get(store->objs_ht, TO_HT_KEY(&key)); + if (v == NULL) + return NULL; + v = ossl_rcu_deref(&v); + + return v->value; +} + +static int x509_name_objs_ht_insert(const X509_STORE *store, const X509_NAME *xn, + STACK_OF(X509_OBJECT) *objs, X509_OBJECT *obj) +{ + int ret = 0, added = 0; + OBJS_KEY key; + HT_VALUE val = {0}; + + if (objs != NULL) { + added = sk_X509_OBJECT_push(objs, obj) != 0; + return added; + } + + if (xn->canon_enc == NULL || xn->modified) { + ret = i2d_X509_NAME((X509_NAME *)xn, NULL); + if (ret < 0) + return 0; + } + + objs = sk_X509_OBJECT_new(x509_object_cmp); + if (objs == NULL) + return 0; + + added = sk_X509_OBJECT_push(objs, obj) != 0; + if (added == 0) { + sk_X509_OBJECT_free(objs); + return 0; + } + + HT_INIT_KEY(&key); + HT_SET_KEY_FIELD(&key, xn_canon, xn->canon_enc); + HT_SET_KEY_FIELD(&key, xn_canon_enclen, xn->canon_enclen); + val.value = (void *)objs; + ret = ossl_ht_insert(store->objs_ht, TO_HT_KEY(&key), &val, NULL); + if (ret != 1) { + sk_X509_OBJECT_free(objs); + return 0; + } + + return 1; +} + +static int obj_ht_foreach_certs(HT_VALUE *v, void *arg) +{ + STACK_OF(X509) **sk = arg; + STACK_OF(X509_OBJECT) *objs = v->value; + int i, r; + + for (i = 0; i < sk_X509_OBJECT_num(objs); i++) { + X509 *cert = X509_OBJECT_get0_X509(sk_X509_OBJECT_value(objs, i)); + + if (cert == NULL) + continue; + r = X509_add_cert(*sk, cert, X509_ADD_FLAG_UP_REF); + if (r == 0) { + OSSL_STACK_OF_X509_free(*sk); + *sk = NULL; + return 0; + } + } + + return 1; +} + /* * May be called with |ret| == NULL just for the side effect of * caching all certs matching the given subject DN in |ctx->store->objs|. @@ -322,7 +439,8 @@ int ossl_x509_store_ctx_get_by_subject(const X509_STORE_CTX *ctx, X509_LOOKUP_TY { X509_STORE *store = ctx->store; X509_LOOKUP *lu; - X509_OBJECT stmp, *tmp; + X509_OBJECT stmp, *tmp = NULL; + STACK_OF(X509_OBJECT) *objs; int i, j; if (store == NULL) @@ -333,19 +451,13 @@ int ossl_x509_store_ctx_get_by_subject(const X509_STORE_CTX *ctx, X509_LOOKUP_TY if (!ossl_x509_store_read_lock(store)) return 0; - /* Should already be sorted...but just in case */ - if (!sk_X509_OBJECT_is_sorted(store->objs)) { - X509_STORE_unlock(store); - /* Take a write lock instead of a read lock */ - if (!X509_STORE_lock(store)) - return 0; - /* - * Another thread might have sorted it in the meantime. But if so, - * sk_X509_OBJECT_sort() exits early. - */ - sk_X509_OBJECT_sort(store->objs); + if (store->objs_ht != NULL) { + objs = ossl_x509_store_ht_get_by_name(store, name); + if (objs != NULL) + tmp = X509_OBJECT_retrieve_by_subject(objs, type, name); + } else { + tmp = X509_OBJECT_retrieve_by_subject(store->objs, type, name); } - tmp = X509_OBJECT_retrieve_by_subject(store->objs, type, name); X509_STORE_unlock(store); if (tmp == NULL || type == X509_LU_CRL) { @@ -387,6 +499,8 @@ static int x509_store_add(X509_STORE *store, void *x, int crl) { X509_OBJECT *obj; int ret = 0, added = 0; + X509_NAME *xn; + STACK_OF(X509_OBJECT) *objs = NULL; if (x == NULL) return 0; @@ -397,10 +511,19 @@ static int x509_store_add(X509_STORE *store, void *x, int crl) if (crl) { obj->type = X509_LU_CRL; obj->data.crl = (X509_CRL *)x; + xn = obj->data.crl->crl.issuer; } else { obj->type = X509_LU_X509; obj->data.x509 = (X509 *)x; + xn = obj->data.x509->cert_info.subject; + } + + if (xn == NULL) { + obj->type = X509_LU_NONE; + X509_OBJECT_free(obj); + return 0; } + if (!X509_OBJECT_up_ref_count(obj)) { obj->type = X509_LU_NONE; X509_OBJECT_free(obj); @@ -412,11 +535,21 @@ static int x509_store_add(X509_STORE *store, void *x, int crl) return 0; } - if (X509_OBJECT_retrieve_match(store->objs, obj)) { - ret = 1; + if (store->objs_ht != NULL) { + objs = ossl_x509_store_ht_get_by_name(store, xn); + if (objs != NULL && X509_OBJECT_retrieve_match(objs, obj)) { + ret = 1; + } else { + added = x509_name_objs_ht_insert(store, xn, objs, obj); + ret = added != 0; + } } else { - added = sk_X509_OBJECT_push(store->objs, obj); - ret = added != 0; + if (X509_OBJECT_retrieve_match(store->objs, obj)) { + ret = 1; + } else { + added = sk_X509_OBJECT_push(store->objs, obj); + ret = added != 0; + } } X509_STORE_unlock(store); @@ -577,11 +710,6 @@ X509_OBJECT *X509_OBJECT_retrieve_by_subject(STACK_OF(X509_OBJECT) *h, return sk_X509_OBJECT_value(h, idx); } -STACK_OF(X509_OBJECT) *X509_STORE_get0_objects(const X509_STORE *xs) -{ - return xs->objs; -} - static X509_OBJECT *x509_object_dup(const X509_OBJECT *obj) { X509_OBJECT *ret = X509_OBJECT_new(); @@ -594,6 +722,44 @@ static X509_OBJECT *x509_object_dup(const X509_OBJECT *obj) return ret; } +static int obj_ht_foreach_object(HT_VALUE *v, void *arg) +{ + STACK_OF(X509_OBJECT) **sk = arg; + STACK_OF(X509_OBJECT) *objs = v->value; + X509_OBJECT *dup, *obj; + int i; + + for (i = 0; i < sk_X509_OBJECT_num(objs); i++) { + obj = sk_X509_OBJECT_value(objs, i); + dup = x509_object_dup(obj); + if (dup == NULL) + goto err; + if (sk_X509_OBJECT_push(*sk, dup) == 0) + goto err; + } + + return 1; + + err: + sk_X509_OBJECT_pop_free(*sk, X509_OBJECT_free); + *sk = NULL; + + return 0; +} + +STACK_OF(X509_OBJECT) *X509_STORE_get0_objects(const X509_STORE *xs) +{ + X509_STORE *store = (X509_STORE *)xs; + + if (xs->objs_ht != NULL) { + ossl_ht_foreach_until(xs->objs_ht, obj_ht_foreach_object, &store->objs); + ossl_ht_free(xs->objs_ht); + store->objs_ht = NULL; + } + + return xs->objs; +} + STACK_OF(X509_OBJECT) *X509_STORE_get1_objects(X509_STORE *store) { STACK_OF(X509_OBJECT) *objs; @@ -606,17 +772,24 @@ STACK_OF(X509_OBJECT) *X509_STORE_get1_objects(X509_STORE *store) if (!ossl_x509_store_read_lock(store)) return NULL; - objs = sk_X509_OBJECT_deep_copy(store->objs, x509_object_dup, - X509_OBJECT_free); + if (store->objs_ht != NULL) { + if ((objs = sk_X509_OBJECT_new(x509_object_cmp)) == NULL) { + X509_STORE_unlock(store); + return NULL; + } + ossl_ht_foreach_until(store->objs_ht, obj_ht_foreach_object, &objs); + } else { + objs = sk_X509_OBJECT_deep_copy(store->objs, x509_object_dup, + X509_OBJECT_free); + } X509_STORE_unlock(store); + return objs; } STACK_OF(X509) *X509_STORE_get1_all_certs(X509_STORE *store) { - STACK_OF(X509) *sk; - STACK_OF(X509_OBJECT) *objs; - int i; + STACK_OF(X509) *sk = NULL; if (store == NULL) { ERR_raise(ERR_LIB_X509, ERR_R_PASSED_NULL_PARAMETER); @@ -624,19 +797,23 @@ STACK_OF(X509) *X509_STORE_get1_all_certs(X509_STORE *store) } if ((sk = sk_X509_new_null()) == NULL) return NULL; - if (!X509_STORE_lock(store)) + + if (!ossl_x509_store_read_lock(store)) goto out_free; - sk_X509_OBJECT_sort(store->objs); - objs = X509_STORE_get0_objects(store); - for (i = 0; i < sk_X509_OBJECT_num(objs); i++) { - X509 *cert = X509_OBJECT_get0_X509(sk_X509_OBJECT_value(objs, i)); + if (store->objs_ht != NULL) { + ossl_ht_foreach_until(store->objs_ht, obj_ht_foreach_certs, &sk); + } else { + for (int i = 0; i < sk_X509_OBJECT_num(store->objs); i++) { + X509 *cert = X509_OBJECT_get0_X509(sk_X509_OBJECT_value(store->objs, i)); - if (cert != NULL - && !X509_add_cert(sk, cert, X509_ADD_FLAG_UP_REF)) - goto err; + if (cert != NULL + && !X509_add_cert(sk, cert, X509_ADD_FLAG_UP_REF)) + goto err; + } } X509_STORE_unlock(store); + return sk; err: @@ -653,21 +830,28 @@ STACK_OF(X509) *X509_STORE_get1_all_certs(X509_STORE *store) STACK_OF(X509) *X509_STORE_CTX_get1_certs(X509_STORE_CTX *ctx, const X509_NAME *nm) { - int i, idx, cnt; + int i, idx = -1, cnt = 0; STACK_OF(X509) *sk = NULL; X509 *x; X509_OBJECT *obj; X509_STORE *store = ctx->store; + STACK_OF(X509_OBJECT) *objs; if (store == NULL) return sk_X509_new_null(); - if (!X509_STORE_lock(store)) + if (!ossl_x509_store_read_lock(store)) return NULL; - sk_X509_OBJECT_sort(store->objs); - idx = x509_object_idx_cnt(store->objs, X509_LU_X509, nm, &cnt); - if (idx < 0) { + if (store->objs_ht != NULL) { + objs = ossl_x509_store_ht_get_by_name(store, nm); + if (objs != NULL) + idx = x509_object_idx_cnt(objs, X509_LU_X509, nm, &cnt); + } else { + objs = store->objs; + idx = x509_object_idx_cnt(objs, X509_LU_X509, nm, &cnt); + } + if (idx < 0 || objs == NULL) { /* * Nothing found in cache: do lookup to possibly add new objects to * cache @@ -676,24 +860,31 @@ STACK_OF(X509) *X509_STORE_CTX_get1_certs(X509_STORE_CTX *ctx, i = ossl_x509_store_ctx_get_by_subject(ctx, X509_LU_X509, nm, NULL); if (i <= 0) return i < 0 ? NULL : sk_X509_new_null(); - if (!X509_STORE_lock(store)) + if (!ossl_x509_store_read_lock(store)) return NULL; - sk_X509_OBJECT_sort(store->objs); - idx = x509_object_idx_cnt(store->objs, X509_LU_X509, nm, &cnt); + if (store->objs_ht != NULL) { + objs = ossl_x509_store_ht_get_by_name(store, nm); + if (objs == NULL) + goto end; + } + idx = x509_object_idx_cnt(objs, X509_LU_X509, nm, &cnt); } sk = sk_X509_new_null(); if (idx < 0 || sk == NULL) goto end; - for (i = 0; i < cnt; i++, idx++) { - obj = sk_X509_OBJECT_value(store->objs, idx); + for (i = idx; i < sk_X509_OBJECT_num(objs); i++) { + obj = sk_X509_OBJECT_value(objs, i); x = obj->data.x509; - if (!X509_add_cert(sk, x, X509_ADD_FLAG_UP_REF)) { + if (obj->type != X509_LU_X509) + continue; + if (X509_add_cert(sk, x, X509_ADD_FLAG_UP_REF) == 0) { X509_STORE_unlock(store); OSSL_STACK_OF_X509_free(sk); return NULL; } } + end: X509_STORE_unlock(store); return sk; @@ -703,11 +894,12 @@ STACK_OF(X509) *X509_STORE_CTX_get1_certs(X509_STORE_CTX *ctx, STACK_OF(X509_CRL) *X509_STORE_CTX_get1_crls(const X509_STORE_CTX *ctx, const X509_NAME *nm) { - int i = 1, idx, cnt; + int i = 1, idx, cnt = 0; STACK_OF(X509_CRL) *sk; X509_CRL *x; X509_OBJECT *obj; X509_STORE *store = ctx->store; + STACK_OF(X509_OBJECT) *objs; /* Always do lookup to possibly add new CRLs to cache */ i = ossl_x509_store_ctx_get_by_subject(ctx, X509_LU_CRL, nm, NULL); @@ -716,20 +908,26 @@ STACK_OF(X509_CRL) *X509_STORE_CTX_get1_crls(const X509_STORE_CTX *ctx, sk = sk_X509_CRL_new_null(); if (i == 0) return sk; - if (!X509_STORE_lock(store)) { + if (!ossl_x509_store_read_lock(store)) { sk_X509_CRL_free(sk); return NULL; } - sk_X509_OBJECT_sort(store->objs); - idx = x509_object_idx_cnt(store->objs, X509_LU_CRL, nm, &cnt); - if (idx < 0) { - X509_STORE_unlock(store); - return sk; + if (store->objs_ht) { + objs = ossl_x509_store_ht_get_by_name(store, nm); + if (objs == NULL) + goto end; + } else { + objs = store->objs; } + idx = x509_object_idx_cnt(objs, X509_LU_CRL, nm, &cnt); + if (idx < 0) + goto end; - for (i = 0; i < cnt; i++, idx++) { - obj = sk_X509_OBJECT_value(store->objs, idx); + for (i = idx; i < sk_X509_OBJECT_num(objs); i++) { + obj = sk_X509_OBJECT_value(objs, i); x = obj->data.crl; + if (obj->type != X509_LU_CRL) + continue; if (!X509_CRL_up_ref(x)) { X509_STORE_unlock(store); sk_X509_CRL_pop_free(sk, X509_CRL_free); @@ -742,6 +940,7 @@ STACK_OF(X509_CRL) *X509_STORE_CTX_get1_crls(const X509_STORE_CTX *ctx, return NULL; } } + end: X509_STORE_unlock(store); return sk; } diff --git a/test/cmp_protect_test.c b/test/cmp_protect_test.c index d54a5112072..a4f7cb9ac58 100644 --- a/test/cmp_protect_test.c +++ b/test/cmp_protect_test.c @@ -477,7 +477,7 @@ static int execute_X509_STORE_test(CMP_PROTECT_TEST_FIXTURE *fixture) fixture->callback_arg))) goto err; sk = X509_STORE_get1_all_certs(store); - if (!TEST_int_eq(0, STACK_OF_X509_cmp(sk, fixture->chain))) + if (!TEST_int_eq(0, STACK_OF_X509_cmp_deep(sk, fixture->chain))) goto err; res = 1; err: diff --git a/test/helpers/cmp_testlib.c b/test/helpers/cmp_testlib.c index e0fb1d3d345..bbe928f6440 100644 --- a/test/helpers/cmp_testlib.c +++ b/test/helpers/cmp_testlib.c @@ -55,6 +55,36 @@ int STACK_OF_X509_cmp(const STACK_OF(X509) *sk1, const STACK_OF(X509) *sk2) return 0; } +/* + * Compares two stacks of certificates in any order of their elements. + * Returns 0 if sk1 and sk2 are equal and another value otherwise + */ +int STACK_OF_X509_cmp_deep(const STACK_OF(X509) *sk1, const STACK_OF(X509) *sk2) +{ + int i, res, idx; + X509 *a, *b; + + if (sk1 == sk2) + return 0; + if (sk1 == NULL) + return -1; + if (sk2 == NULL) + return 1; + if ((res = sk_X509_num(sk1) - sk_X509_num(sk2))) + return res; + for (i = 0; i < sk_X509_num(sk1); i++) { + a = sk_X509_value(sk1, i); + idx = sk_X509_find((STACK_OF(X509) *)sk2, a); + if (idx < 0) + return 0; + b = sk_X509_value(sk2, idx); + if (a != b) + if ((res = X509_cmp(a, b)) != 0) + return res; + } + return 0; +} + /* * Up refs and push a cert onto sk. * Returns the number of certificates on the stack on success diff --git a/test/helpers/cmp_testlib.h b/test/helpers/cmp_testlib.h index 50b085beca0..82cfefbe05d 100644 --- a/test/helpers/cmp_testlib.h +++ b/test/helpers/cmp_testlib.h @@ -24,6 +24,7 @@ OSSL_CMP_MSG *load_pkimsg(const char *file, OSSL_LIB_CTX *libctx); int valid_asn1_encoding(const OSSL_CMP_MSG *msg); int STACK_OF_X509_cmp(const STACK_OF(X509) *sk1, const STACK_OF(X509) *sk2); +int STACK_OF_X509_cmp_deep(const STACK_OF(X509) *sk1, const STACK_OF(X509) *sk2); int STACK_OF_X509_push1(STACK_OF(X509) *sk, X509 *cert); int print_to_bio_out(const char *func, const char *file, int line, OSSL_CMP_severity level, const char *msg); diff --git a/test/x509_load_cert_file_test.c b/test/x509_load_cert_file_test.c index 16caf48fec7..2e6920b26b6 100644 --- a/test/x509_load_cert_file_test.c +++ b/test/x509_load_cert_file_test.c @@ -5,6 +5,8 @@ * https://www.openssl.org/source/license.html */ +#define OPENSSL_SUPPRESS_DEPRECATED + #include #include #include @@ -14,6 +16,64 @@ static const char *chain; static const char *crl; +static const char *cn_cert1[] = { + "-----BEGIN CERTIFICATE-----\n", + "MIIC9DCCAdygAwIBAgICEAIwDQYJKoZIhvcNAQELBQAwGzEZMBcGA1UEAwwQd3d3\n", + "LmV4YW1wbGUudGVzdDAeFw0yNTAxMDEwMDAwMDBaFw0yNTAxMTAwMDAwMDBaMBsx\n", + "GTAXBgNVBAMMEHd3dy5leGFtcGxlLnRlc3QwggEiMA0GCSqGSIb3DQEBAQUAA4IB\n", + "DwAwggEKAoIBAQDPwPv+vSWbbNI+EUMH162Srn88C2ywadFWgBdnVikBJ10LcwQK\n", + "6zrTC+TzVafEhWUJDujnhJlTCc0ASIIJti2adwJODgbf2lk4n4mXcT4BFYvJbWaW\n", + "BjSRPnfwKyQyHoXDyCtLZGtwB295mNcF1sGOS7SmpM4n4hbzTx/o3mxCEd/CGEzh\n", + "hiljQIr6VtwVxcGoaI5AIW/gbKZB4tYrCeiqVDITZFx5kp0BQ4aPIr+1ws4Y9qta\n", + "1ljZgf/E3uaQK2CFAWYv6Zz2QYfjkoTZ2wq3PBj1DVWT0nJs/rezKA6R1iyePiFa\n", + "nqlNyjNKFDi/oVVmgsDgllVXoWaDoTncXQGzAgMBAAGjQjBAMB0GA1UdDgQWBBTl\n", + "aFbHpg5r2p4B6MyRy3MG9cwBijAfBgNVHSMEGDAWgBQIA+YPQEiawtPX3vt3hpGo\n", + "Wo+/3zANBgkqhkiG9w0BAQsFAAOCAQEAmx1N72vdZLCaMXkZapz5j9NND0cvDjXu\n", + "YvUfGU+Y9dTTX4+RmrWuLWgLlk7Unq5DPAQahE4Kc7+JsyMtSXQnOyTZAEmy+1qN\n", + "DG42aPiRtfm2Og0mmTrIml3n/tZzmYPU+hC4f9M6qtpI9S0IaqAXFZdup45o6uZj\n", + "Wt7oiIfnbj3UIf7Bc3k/0jUvCZavppH9yKdd70cpkxRXBfqSpift7YqH6RCpKp/P\n", + "jiDYR27N7qUmfe60AHCBhPA1A1fTUXmhRfl8mRm2wiUelKUZ77wRkk5VpbsAA/JW\n", + "07vUu3WC2773pUT/68MED5h26Mr6xjKxG8v8SfeMpri5R74P1TnJpQ==\n", + "-----END CERTIFICATE-----\n", + NULL, +}; +static const char *cn_cert2[] = { + "-----BEGIN CERTIFICATE-----\n", + "MIIC9DCCAdygAwIBAgICEAMwDQYJKoZIhvcNAQELBQAwGzEZMBcGA1UEAwwQd3d3\n", + "LmV4YW1wbGUudGVzdDAeFw0yNTAxMTEwMDAwMDBaFw0yNTAxMjAwMDAwMDBaMBsx\n", + "GTAXBgNVBAMMEHd3dy5leGFtcGxlLnRlc3QwggEiMA0GCSqGSIb3DQEBAQUAA4IB\n", + "DwAwggEKAoIBAQDPwPv+vSWbbNI+EUMH162Srn88C2ywadFWgBdnVikBJ10LcwQK\n", + "6zrTC+TzVafEhWUJDujnhJlTCc0ASIIJti2adwJODgbf2lk4n4mXcT4BFYvJbWaW\n", + "BjSRPnfwKyQyHoXDyCtLZGtwB295mNcF1sGOS7SmpM4n4hbzTx/o3mxCEd/CGEzh\n", + "hiljQIr6VtwVxcGoaI5AIW/gbKZB4tYrCeiqVDITZFx5kp0BQ4aPIr+1ws4Y9qta\n", + "1ljZgf/E3uaQK2CFAWYv6Zz2QYfjkoTZ2wq3PBj1DVWT0nJs/rezKA6R1iyePiFa\n", + "nqlNyjNKFDi/oVVmgsDgllVXoWaDoTncXQGzAgMBAAGjQjBAMB0GA1UdDgQWBBTl\n", + "aFbHpg5r2p4B6MyRy3MG9cwBijAfBgNVHSMEGDAWgBQIA+YPQEiawtPX3vt3hpGo\n", + "Wo+/3zANBgkqhkiG9w0BAQsFAAOCAQEAH+hviFeMZc55/H5AZG4p3VdOQ5ZxsHJF\n", + "NTkxY/0wGcKJhelyNN39rnlhUFLKlEhbWE/LlTBCU9ILLiFeSAaFiNgufqFEBefL\n", + "cjqcfQUeB2vUHNnrWy6QPCc0SNSdholTelEgf8eXPBrJWS/iazcKTLfBrFbs6K++\n", + "F/Efn1p7+DutFpgaTdupuonsJOMM33Nw3MfAz6bw5Hjd17ritjRW0g58OsLfv9Ac\n", + "Dbw2bEzZDvkkH5yDlHOl/dyyhDdcMs6bRsoM+LC0PuZ3dAUQhlnQ8SlPojm5qUjA\n", + "0hyy4w4tmYFCzdUxR6vnQie4a/D0o5cLuecIIemozKONMZo+FVGjFQ==\n", + "-----END CERTIFICATE-----\n", + NULL, +}; +static const char *cn_crl2[] = { + "-----BEGIN X509 CRL-----\n", + "MIIBtjCBnwIBATANBgkqhkiG9w0BAQsFADAbMRkwFwYDVQQDDBB3d3cuZXhhbXBs\n", + "ZS50ZXN0Fw0yNTA5MTExMzA0MTZaFw0yNTEyMjAxMzA0MTZaMD8wEwICEAAXDTI1\n", + "MDkxMTExNTcxMFowEwICEAEXDTI1MDkxMTExNTgwNVowEwICEAIXDTI1MDkxMTEz\n", + "MDM1MVqgDzANMAsGA1UdFAQEAgIQATANBgkqhkiG9w0BAQsFAAOCAQEAZUUsIeXL\n", + "zTwpa2bwAg0xUIm/ARnoHlZSxMoWPsQnIkrQreCTvc9QpTo+IrS+SDaBJqu8V+DM\n", + "Z4rNqreQG1wNNgkSTEOAFMjQPOmvtePyYGxNgM4MdpKaejdZSk22rccaAQVhSjGp\n", + "AYprwbWqyfAq7JUYLUZ1EjsIoRQIKMQa79bzF7S4oNN6sMq6icJueFmd55ufmv27\n", + "1amhUpJh5YJ8BNpLZGicujYm8TVCZqSjVI0JsXAQYhm1u/pUrcydRfc7c5PMkpIj\n", + "uByPkdYLVgpKJH6Y5Q9Kx5MR+p85JM31ePlSobft+wZh/WMV0Id6PZ8/56oOhV3V\n", + "bVxU+BlQ5ZKmYw==\n" + "-----END X509 CRL-----\n", + NULL, +}; + static int test_load_cert_file(void) { int ret = 0, i; @@ -27,6 +87,8 @@ static int test_load_cert_file(void) || !TEST_true(X509_load_cert_file(lookup, chain, X509_FILETYPE_PEM)) || !TEST_ptr(certs = X509_STORE_get1_all_certs(store)) || !TEST_int_eq(sk_X509_num(certs), 4) + || !TEST_ptr(objs = X509_STORE_get0_objects(store)) + || !TEST_int_eq(sk_X509_OBJECT_num(objs), 4) || !TEST_ptr(objs = X509_STORE_get1_objects(store)) || !TEST_int_eq(sk_X509_OBJECT_num(objs), 4)) goto err; @@ -49,6 +111,61 @@ err: return ret; } +/* + * Test loading multiple certificates and a CRL with identical CN into + * X509_STORE. + */ +static int test_load_same_cn_certs(void) +{ + X509 *cert1 = NULL, *cert2 = NULL; + X509_CRL *crl2 = NULL; + int ret = 0; + X509_STORE *store = NULL; + X509_STORE_CTX *s_ctx = NULL; + STACK_OF(X509_OBJECT) *objs = NULL; + STACK_OF(X509_CRL) *sk_x509_crl = NULL; + STACK_OF(X509) *sk_x509 = NULL; + X509_NAME *nm = NULL; + + if (!TEST_ptr(nm = X509_NAME_new()) + || !TEST_true(X509_NAME_add_entry_by_txt(nm, "CN", MBSTRING_ASC, + (unsigned char *)"www.example.test", + -1, -1, 0))) + goto err; + + if (!TEST_ptr(cert1 = X509_from_strings(cn_cert1)) + || !TEST_ptr(cert2 = X509_from_strings(cn_cert2)) + || !TEST_ptr(crl2 = CRL_from_strings(cn_crl2)) + || !TEST_ptr(s_ctx = X509_STORE_CTX_new()) + || !TEST_ptr(store = X509_STORE_new()) + || !TEST_true(X509_STORE_CTX_init(s_ctx, store, NULL, NULL)) + || !TEST_true(X509_STORE_add_cert(store, cert1)) + || !TEST_true(X509_STORE_add_crl(store, crl2)) + || !TEST_true(X509_STORE_add_cert(store, cert2)) + /* deliberately not taking lock in a single thread */ + || !TEST_ptr(objs = X509_STORE_get0_objects(store)) + || !TEST_int_eq(sk_X509_OBJECT_num(objs), 3) + || !TEST_ptr(sk_x509 = X509_STORE_CTX_get1_certs(s_ctx, nm)) + || !TEST_int_eq(sk_X509_num(sk_x509), 2) + || !TEST_ptr(sk_x509_crl = X509_STORE_CTX_get1_crls(s_ctx, nm)) + || !TEST_int_eq(sk_X509_CRL_num(sk_x509_crl), 1)) + goto err; + + ret = 1; + +err: + X509_NAME_free(nm); + OSSL_STACK_OF_X509_free(sk_x509); + sk_X509_CRL_pop_free(sk_x509_crl, X509_CRL_free); + X509_STORE_free(store); + X509_STORE_CTX_free(s_ctx); + X509_free(cert1); + X509_free(cert2); + X509_CRL_free(crl2); + + return ret; +} + OPT_TEST_DECLARE_USAGE("cert.pem [crl.pem]\n") int setup_tests(void) @@ -65,5 +182,7 @@ int setup_tests(void) crl = test_get_argument(1); ADD_TEST(test_load_cert_file); + ADD_TEST(test_load_same_cn_certs); + return 1; }