From 32567c9f3be1ba5c47079da5e881ded20fa7a078 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Bodo=20M=C3=B6ller?= Date: Fri, 19 Feb 2010 18:26:23 +0000 Subject: [PATCH] Fix X509_STORE locking --- CHANGES | 6 +++- crypto/x509/by_dir.c | 5 ++- crypto/x509/x509_lu.c | 83 +++++++++++++++++++++++-------------------- 3 files changed, 51 insertions(+), 43 deletions(-) diff --git a/CHANGES b/CHANGES index 947962fffd..b1c4ac218d 100644 --- a/CHANGES +++ b/CHANGES @@ -843,7 +843,11 @@ *) Change 'Configure' script to enable Camellia by default. [NTT] - Changes between 0.9.8l (?) and 0.9.8m (?) [xx XXX xxxx] + Changes between 0.9.8l and 0.9.8m [xx XXX xxxx] + + *) Fix X509_STORE locking: Every 'objs' access requires a lock (to + accommodate for stack sorting, always a write lock!). + [Bodo Moeller] *) On some versions of WIN32 Heap32Next is very slow. This can cause excessive delays in the RAND_poll(): over a minute. As a workaround diff --git a/crypto/x509/by_dir.c b/crypto/x509/by_dir.c index 170cd89608..27ca5150c1 100644 --- a/crypto/x509/by_dir.c +++ b/crypto/x509/by_dir.c @@ -420,11 +420,11 @@ static int get_cert_by_subject(X509_LOOKUP *xl, int type, X509_NAME *name, /* we have added it to the cache so now pull * it out again */ - CRYPTO_r_lock(CRYPTO_LOCK_X509_STORE); + CRYPTO_w_lock(CRYPTO_LOCK_X509_STORE); j = sk_X509_OBJECT_find(xl->store_ctx->objs,&stmp); if(j != -1) tmp=sk_X509_OBJECT_value(xl->store_ctx->objs,j); else tmp = NULL; - CRYPTO_r_unlock(CRYPTO_LOCK_X509_STORE); + CRYPTO_w_unlock(CRYPTO_LOCK_X509_STORE); /* If a CRL, update the last file suffix added for this */ @@ -480,4 +480,3 @@ finish: if (b != NULL) BUF_MEM_free(b); return(ok); } - diff --git a/crypto/x509/x509_lu.c b/crypto/x509/x509_lu.c index 7e38544e5f..3a6e04a1de 100644 --- a/crypto/x509/x509_lu.c +++ b/crypto/x509/x509_lu.c @@ -294,7 +294,9 @@ int X509_STORE_get_by_subject(X509_STORE_CTX *vs, int type, X509_NAME *name, X509_OBJECT stmp,*tmp; int i,j; + CRYPTO_w_lock(CRYPTO_LOCK_X509_STORE); tmp=X509_OBJECT_retrieve_by_subject(ctx->objs,type,name); + CRYPTO_w_unlock(CRYPTO_LOCK_X509_STORE); if (tmp == NULL || type == X509_LU_CRL) { @@ -348,7 +350,6 @@ int X509_STORE_add_cert(X509_STORE *ctx, X509 *x) X509_OBJECT_up_ref_count(obj); - if (X509_OBJECT_retrieve_match(ctx->objs, obj)) { X509_OBJECT_free_contents(obj); @@ -477,12 +478,12 @@ int X509_OBJECT_idx_by_subject(STACK_OF(X509_OBJECT) *h, int type, X509_OBJECT *X509_OBJECT_retrieve_by_subject(STACK_OF(X509_OBJECT) *h, int type, X509_NAME *name) -{ + { int idx; idx = X509_OBJECT_idx_by_subject(h, type, name); if (idx==-1) return NULL; return sk_X509_OBJECT_value(h, idx); -} + } STACK_OF(X509)* X509_STORE_get1_certs(X509_STORE_CTX *ctx, X509_NAME *nm) { @@ -491,7 +492,7 @@ STACK_OF(X509)* X509_STORE_get1_certs(X509_STORE_CTX *ctx, X509_NAME *nm) X509 *x; X509_OBJECT *obj; sk = sk_X509_new_null(); - CRYPTO_r_lock(CRYPTO_LOCK_X509_STORE); + CRYPTO_w_lock(CRYPTO_LOCK_X509_STORE); idx = x509_object_idx_cnt(ctx->ctx->objs, X509_LU_X509, nm, &cnt); if (idx < 0) { @@ -499,18 +500,18 @@ STACK_OF(X509)* X509_STORE_get1_certs(X509_STORE_CTX *ctx, X509_NAME *nm) * objects to cache */ X509_OBJECT xobj; - CRYPTO_r_unlock(CRYPTO_LOCK_X509_STORE); + CRYPTO_w_unlock(CRYPTO_LOCK_X509_STORE); if (!X509_STORE_get_by_subject(ctx, X509_LU_X509, nm, &xobj)) { sk_X509_free(sk); return NULL; } X509_OBJECT_free_contents(&xobj); - CRYPTO_r_lock(CRYPTO_LOCK_X509_STORE); + CRYPTO_w_lock(CRYPTO_LOCK_X509_STORE); idx = x509_object_idx_cnt(ctx->ctx->objs,X509_LU_X509,nm, &cnt); if (idx < 0) { - CRYPTO_r_unlock(CRYPTO_LOCK_X509_STORE); + CRYPTO_w_unlock(CRYPTO_LOCK_X509_STORE); sk_X509_free(sk); return NULL; } @@ -522,13 +523,13 @@ STACK_OF(X509)* X509_STORE_get1_certs(X509_STORE_CTX *ctx, X509_NAME *nm) CRYPTO_add(&x->references, 1, CRYPTO_LOCK_X509); if (!sk_X509_push(sk, x)) { - CRYPTO_r_unlock(CRYPTO_LOCK_X509_STORE); + CRYPTO_w_unlock(CRYPTO_LOCK_X509_STORE); X509_free(x); sk_X509_pop_free(sk, X509_free); return NULL; } } - CRYPTO_r_unlock(CRYPTO_LOCK_X509_STORE); + CRYPTO_w_unlock(CRYPTO_LOCK_X509_STORE); return sk; } @@ -540,24 +541,24 @@ STACK_OF(X509_CRL)* X509_STORE_get1_crls(X509_STORE_CTX *ctx, X509_NAME *nm) X509_CRL *x; X509_OBJECT *obj, xobj; sk = sk_X509_CRL_new_null(); - CRYPTO_r_lock(CRYPTO_LOCK_X509_STORE); + CRYPTO_w_lock(CRYPTO_LOCK_X509_STORE); /* Check cache first */ idx = x509_object_idx_cnt(ctx->ctx->objs, X509_LU_CRL, nm, &cnt); /* Always do lookup to possibly add new CRLs to cache */ - CRYPTO_r_unlock(CRYPTO_LOCK_X509_STORE); + CRYPTO_w_unlock(CRYPTO_LOCK_X509_STORE); if (!X509_STORE_get_by_subject(ctx, X509_LU_CRL, nm, &xobj)) { sk_X509_CRL_free(sk); return NULL; } X509_OBJECT_free_contents(&xobj); - CRYPTO_r_lock(CRYPTO_LOCK_X509_STORE); + CRYPTO_w_lock(CRYPTO_LOCK_X509_STORE); idx = x509_object_idx_cnt(ctx->ctx->objs,X509_LU_CRL, nm, &cnt); if (idx < 0) { - CRYPTO_r_unlock(CRYPTO_LOCK_X509_STORE); + CRYPTO_w_unlock(CRYPTO_LOCK_X509_STORE); sk_X509_CRL_free(sk); return NULL; } @@ -569,19 +570,18 @@ STACK_OF(X509_CRL)* X509_STORE_get1_crls(X509_STORE_CTX *ctx, X509_NAME *nm) CRYPTO_add(&x->references, 1, CRYPTO_LOCK_X509_CRL); if (!sk_X509_CRL_push(sk, x)) { - CRYPTO_r_unlock(CRYPTO_LOCK_X509_STORE); + CRYPTO_w_unlock(CRYPTO_LOCK_X509_STORE); X509_CRL_free(x); sk_X509_CRL_pop_free(sk, X509_CRL_free); return NULL; } } - CRYPTO_r_unlock(CRYPTO_LOCK_X509_STORE); + CRYPTO_w_unlock(CRYPTO_LOCK_X509_STORE); return sk; - } X509_OBJECT *X509_OBJECT_retrieve_match(STACK_OF(X509_OBJECT) *h, X509_OBJECT *x) -{ + { int idx, i; X509_OBJECT *obj; idx = sk_X509_OBJECT_find(h, x); @@ -607,13 +607,13 @@ X509_OBJECT *X509_OBJECT_retrieve_match(STACK_OF(X509_OBJECT) *h, X509_OBJECT *x return obj; } return NULL; -} + } /* Try to get issuer certificate from store. Due to limitations * of the API this can only retrieve a single certificate matching * a given subject name. However it will fill the cache with all - * matching certificates, so we can examine the cache for all + * matching certificates, so we can examine the cache for all * matches. * * Return values are: @@ -621,13 +621,11 @@ X509_OBJECT *X509_OBJECT_retrieve_match(STACK_OF(X509_OBJECT) *h, X509_OBJECT *x * 0 certificate not found. * -1 some other error. */ - - int X509_STORE_CTX_get1_issuer(X509 **issuer, X509_STORE_CTX *ctx, X509 *x) -{ + { X509_NAME *xn; X509_OBJECT obj, *pobj; - int i, ok, idx; + int i, ok, idx, ret; xn=X509_get_issuer_name(x); ok=X509_STORE_get_by_subject(ctx,X509_LU_X509,xn,&obj); if (ok != X509_LU_X509) @@ -653,27 +651,34 @@ int X509_STORE_CTX_get1_issuer(X509 **issuer, X509_STORE_CTX *ctx, X509 *x) return 1; } X509_OBJECT_free_contents(&obj); - /* Else find index of first matching cert */ - idx = X509_OBJECT_idx_by_subject(ctx->ctx->objs, X509_LU_X509, xn); - /* This shouldn't normally happen since we already have one match */ - if (idx == -1) return 0; - /* Look through all matching certificates for a suitable issuer */ - for (i = idx; i < sk_X509_OBJECT_num(ctx->ctx->objs); i++) + /* Else find index of first cert accepted by 'check_issued' */ + ret = 0; + CRYPTO_w_lock(CRYPTO_LOCK_X509_STORE); + idx = X509_OBJECT_idx_by_subject(ctx->ctx->objs, X509_LU_X509, xn); + if (idx != -1) /* should be true as we've had at least one match */ { - pobj = sk_X509_OBJECT_value(ctx->ctx->objs, i); - /* See if we've ran out of matches */ - if (pobj->type != X509_LU_X509) return 0; - if (X509_NAME_cmp(xn, X509_get_subject_name(pobj->data.x509))) return 0; - if (ctx->check_issued(ctx, x, pobj->data.x509)) + /* Look through all matching certs for suitable issuer */ + for (i = idx; i < sk_X509_OBJECT_num(ctx->ctx->objs); i++) { - *issuer = pobj->data.x509; - X509_OBJECT_up_ref_count(pobj); - return 1; + pobj = sk_X509_OBJECT_value(ctx->ctx->objs, i); + /* See if we've run past the matches */ + if (pobj->type != X509_LU_X509) + break; + if (X509_NAME_cmp(xn, X509_get_subject_name(pobj->data.x509))) + break; + if (ctx->check_issued(ctx, x, pobj->data.x509)) + { + *issuer = pobj->data.x509; + X509_OBJECT_up_ref_count(pobj); + ret = 1; + break; + } } } - return 0; -} + CRYPTO_w_unlock(CRYPTO_LOCK_X509_STORE); + return ret; + } int X509_STORE_set_flags(X509_STORE *ctx, unsigned long flags) { -- 2.34.1