From 8e107a6826591f51ee6cfcd285542be80540a66d Mon Sep 17 00:00:00 2001 From: Neil Horman Date: Tue, 2 Jul 2024 14:27:42 -0400 Subject: [PATCH 1/2] read lock store on ossl_method_store_do_all MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Theres a data race between ossl_method_store_insert and ossl_method_store_do_all, as the latter doesn't take the property lock before iterating. However, we can't lock in do_all, as the call stack in several cases later attempts to take the write lock. The choices to fix it are I think: 1) add an argument to indicate to ossl_method_store_do_all weather to take the read or write lock when doing iterations, and add an is_locked api to the ossl_property_[read|write] lock family so that subsequent callers can determine if they need to take a lock or not 2) Clone the algs sparse array in ossl_method_store_do_all and use the clone to iterate with no lock held, ensuring that updates to the parent copy of the sparse array are left untoucheTheres a data race between ossl_method_store_insert and ossl_method_store_do_all, as the latter doesn't take the property lock before iterating. I think method (2), while being a bit more expensive, is probably the far less invasive way to go here Fixes #24672 Reviewed-by: Paul Dale Reviewed-by: Matt Caswell Reviewed-by: Tomas Mraz (Merged from https://github.com/openssl/openssl/pull/24782) (cherry picked from commit d8def79838cd0d5e7c21d217aa26edb5229f0ab4) Signed-off-by: 王静 --- crypto/property/property.c | 50 ++++++++++++++++++++++++-------------- 1 file changed, 32 insertions(+), 18 deletions(-) diff --git a/crypto/property/property.c b/crypto/property/property.c index b97861d486..abf2a5aab7 100644 --- a/crypto/property/property.c +++ b/crypto/property/property.c @@ -95,6 +95,8 @@ typedef struct { DEFINE_SPARSE_ARRAY_OF(ALGORITHM); +DEFINE_STACK_OF(ALGORITHM) + typedef struct ossl_global_properties_st { OSSL_PROPERTY_LIST *list; #ifndef FIPS_MODULE @@ -469,33 +471,45 @@ static void alg_do_one(ALGORITHM *alg, IMPLEMENTATION *impl, fn(alg->nid, impl->method.method, fnarg); } -struct alg_do_each_data_st { - void (*fn)(int id, void *method, void *fnarg); - void *fnarg; -}; - -static void alg_do_each(ossl_uintmax_t idx, ALGORITHM *alg, void *arg) +static void alg_copy(ossl_uintmax_t idx, ALGORITHM *alg, void *arg) { - struct alg_do_each_data_st *data = arg; - int i, end = sk_IMPLEMENTATION_num(alg->impls); - - for (i = 0; i < end; i++) { - IMPLEMENTATION *impl = sk_IMPLEMENTATION_value(alg->impls, i); + STACK_OF(ALGORITHM) *newalg = arg; - alg_do_one(alg, impl, data->fn, data->fnarg); - } + (void)sk_ALGORITHM_push(newalg, alg); } void ossl_method_store_do_all(OSSL_METHOD_STORE *store, void (*fn)(int id, void *method, void *fnarg), void *fnarg) { - struct alg_do_each_data_st data; + int i, j; + int numalgs, numimps; + STACK_OF(ALGORITHM) *tmpalgs; + ALGORITHM *alg; - data.fn = fn; - data.fnarg = fnarg; - if (store != NULL) - ossl_sa_ALGORITHM_doall_arg(store->algs, alg_do_each, &data); + if (store != NULL) { + + if (!ossl_property_read_lock(store)) + return; + + tmpalgs = sk_ALGORITHM_new_reserve(NULL, + ossl_sa_ALGORITHM_num(store->algs)); + if (tmpalgs == NULL) { + ossl_property_unlock(store); + return; + } + + ossl_sa_ALGORITHM_doall_arg(store->algs, alg_copy, tmpalgs); + ossl_property_unlock(store); + numalgs = sk_ALGORITHM_num(tmpalgs); + for (i = 0; i < numalgs; i++) { + alg = sk_ALGORITHM_value(tmpalgs, i); + numimps = sk_IMPLEMENTATION_num(alg->impls); + for (j = 0; j < numimps; j++) + alg_do_one(alg, sk_IMPLEMENTATION_value(alg->impls, j), fn, fnarg); + } + sk_ALGORITHM_free(tmpalgs); + } } int ossl_method_store_fetch(OSSL_METHOD_STORE *store, -- Gitee From ee03f0795ced2415faac428ca0940bc4f47f6c63 Mon Sep 17 00:00:00 2001 From: erbsland-dev Date: Fri, 30 Aug 2024 10:56:58 +0200 Subject: [PATCH 2/2] Fix Edge Cases in Password Callback Handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #8441: Modify the password callback handling to reserve one byte in the buffer for a null terminator, ensuring compatibility with legacy behavior that puts a terminating null byte at the end. Additionally, validate the length returned by the callback to ensure it does not exceed the given buffer size. If the returned length is too large, the process now stops gracefully with an appropriate error, enhancing robustness by preventing crashes from out-of-bounds access. Reviewed-by: Dmitry Belyavskiy Reviewed-by: Tomas Mraz (Merged from https://github.com/openssl/openssl/pull/25330) (cherry picked from commit 5387b71acb833f1f635ab4a20ced0863747ef5c1) Signed-off-by: 王静 --- crypto/pem/pem_pk8.c | 4 ++-- crypto/ui/ui_util.c | 12 ++++++++---- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/crypto/pem/pem_pk8.c b/crypto/pem/pem_pk8.c index 1592e351ed..6e84f0afd0 100644 --- a/crypto/pem/pem_pk8.c +++ b/crypto/pem/pem_pk8.c @@ -173,7 +173,7 @@ EVP_PKEY *d2i_PKCS8PrivateKey_bio(BIO *bp, EVP_PKEY **x, pem_password_cb *cb, X509_SIG *p8 = NULL; int klen; EVP_PKEY *ret; - char psbuf[PEM_BUFSIZE]; + char psbuf[PEM_BUFSIZE + 1]; /* reserve one byte at the end */ p8 = d2i_PKCS8_bio(bp, NULL); if (p8 == NULL) @@ -182,7 +182,7 @@ EVP_PKEY *d2i_PKCS8PrivateKey_bio(BIO *bp, EVP_PKEY **x, pem_password_cb *cb, klen = cb(psbuf, PEM_BUFSIZE, 0, u); else klen = PEM_def_callback(psbuf, PEM_BUFSIZE, 0, u); - if (klen < 0) { + if (klen < 0 || klen > PEM_BUFSIZE) { ERR_raise(ERR_LIB_PEM, PEM_R_BAD_PASSWORD_READ); X509_SIG_free(p8); return NULL; diff --git a/crypto/ui/ui_util.c b/crypto/ui/ui_util.c index 59b00b225a..554bf79856 100644 --- a/crypto/ui/ui_util.c +++ b/crypto/ui/ui_util.c @@ -105,14 +105,18 @@ static int ui_read(UI *ui, UI_STRING *uis) switch (UI_get_string_type(uis)) { case UIT_PROMPT: { - char result[PEM_BUFSIZE + 1]; + int len; + char result[PEM_BUFSIZE + 1]; /* reserve one byte at the end */ const struct pem_password_cb_data *data = UI_method_get_ex_data(UI_get_method(ui), ui_method_data_index); int maxsize = UI_get_result_maxsize(uis); - int len = data->cb(result, - maxsize > PEM_BUFSIZE ? PEM_BUFSIZE : maxsize, - data->rwflag, UI_get0_user_data(ui)); + if (maxsize > PEM_BUFSIZE) + maxsize = PEM_BUFSIZE; + len = data->cb(result, maxsize, data->rwflag, + UI_get0_user_data(ui)); + if (len > maxsize) + return -1; if (len >= 0) result[len] = '\0'; if (len < 0) -- Gitee