From f607cad47a86c1e180513aee0ec95b6c346b3b13 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Mon, 13 Jun 2022 23:29:07 -0400 Subject: [PATCH 1/2] Simplify BN_GENCB handling When callback was NULL, bind9 would use BN_GENCB_set_old to set a NULL callback because OpenSSL happened to allow a NULL "old" callback, but not a NULL "new" callback. Instead, the way to turn off the callback is to pass a NULL BN_GENCB itself. Switch to doing that. --- lib/dns/openssldh_link.c | 12 +++++------- lib/dns/opensslrsa_link.c | 15 ++++++++++----- 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/lib/dns/openssldh_link.c b/lib/dns/openssldh_link.c index d5dbc2e889..0a2e11f51f 100644 --- a/lib/dns/openssldh_link.c +++ b/lib/dns/openssldh_link.c @@ -444,16 +444,14 @@ openssldh_generate(dst_key_t *key, int generator, void (*callback)(int)) { if (generator != 0) { #if OPENSSL_VERSION_NUMBER < 0x30000000L - cb = BN_GENCB_new(); + if (callback != NULL) { + cb = BN_GENCB_new(); #if OPENSSL_VERSION_NUMBER >= 0x10100000L && !defined(LIBRESSL_VERSION_NUMBER) - if (cb == NULL) { - DST_RET(dst__openssl_toresult(ISC_R_NOMEMORY)); - } + if (cb == NULL) { + DST_RET(dst__openssl_toresult(ISC_R_NOMEMORY)); + } #endif /* if OPENSSL_VERSION_NUMBER >= 0x10100000L && \ * !defined(LIBRESSL_VERSION_NUMBER) */ - if (callback == NULL) { - BN_GENCB_set_old(cb, NULL, NULL); - } else { u.fptr = callback; BN_GENCB_set(cb, progress_cb, u.dptr); } diff --git a/lib/dns/opensslrsa_link.c b/lib/dns/opensslrsa_link.c index be1998e381..26c1182805 100644 --- a/lib/dns/opensslrsa_link.c +++ b/lib/dns/opensslrsa_link.c @@ -384,14 +384,14 @@ opensslrsa_generate(dst_key_t *key, int exp, void (*callback)(int)) { #if !HAVE_BN_GENCB_NEW BN_GENCB _cb; #endif /* !HAVE_BN_GENCB_NEW */ - BN_GENCB *cb = BN_GENCB_new(); + BN_GENCB *cb = NULL; #else EVP_PKEY_CTX *ctx = EVP_PKEY_CTX_new_from_name(NULL, "RSA", NULL); EVP_PKEY *pkey = NULL; #endif /* OPENSSL_VERSION_NUMBER < 0x30000000L */ #if OPENSSL_VERSION_NUMBER < 0x30000000L - if (e == NULL || rsa == NULL || pkey == NULL || cb == NULL) { + if (e == NULL || rsa == NULL || pkey == NULL) { DST_RET(dst__openssl_toresult(DST_R_OPENSSLFAILURE)); } #else @@ -442,9 +442,14 @@ opensslrsa_generate(dst_key_t *key, int exp, void (*callback)(int)) { DST_RET(dst__openssl_toresult(DST_R_OPENSSLFAILURE)); } - if (callback == NULL) { - BN_GENCB_set_old(cb, NULL, NULL); - } else { + if (callback != NULL) { + cb = BN_GENCB_new(); +#if OPENSSL_VERSION_NUMBER >= 0x10100000L && !defined(LIBRESSL_VERSION_NUMBER) + if (cb == NULL) { + DST_RET(dst__openssl_toresult(ISC_R_NOMEMORY)); + } +#endif /* if OPENSSL_VERSION_NUMBER >= 0x10100000L && \ + * !defined(LIBRESSL_VERSION_NUMBER) */ u.fptr = callback; BN_GENCB_set(cb, progress_cb, u.dptr); } From e819866a2df5a0ce820a49eb01684e83d2c12a83 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Mon, 13 Jun 2022 23:37:55 -0400 Subject: [PATCH 2/2] Remove DH_clear_flags call These calls have not been needed since OpenSSL 0.9.7h. This dates to commit 704d6eeab1d8d6a2aeb99c37fa5a97322d9340fc, "Work around non-reentrancy in openssl by disabling precomputation in keys". This was in the bundled OpenSSL 0.9.3a era and made two changes. First, it registered a locking callback because, in those days, OpenSSL needed a callback to support locks. Second, it set flags to disable various bits of cached state on DH, DSA, and RSA objects. Looking back in OpenSSL 0.9.3a, that cached state was not protected by a lock: https://github.com/openssl/openssl/blob/OpenSSL_0_9_3a/crypto/rsa/rsa_eay.c#L137-L142 However, this was fixed in OpenSSL 0.9.7h: https://github.com/openssl/openssl/commit/6ec8e63af6c1835a8b222350dbabf7bb2ace094f The other flags (DSA and RSA) have since fallen away, DSA with the removal of DSA altogether (3994b1f9c2bd4438586523fb2e49b0fb847b487b) and RSA with 3a8d4a316eae09966c85e7e5befc682bd4744b34, "openssl 0.9.6a and higher don't have the RSA locking bug [...] other algorithms still don't do locking when performing precomputation [...]". That seems to be referring to this OpenSSL change, which indeed fixed it for RSA but not others: https://github.com/openssl/openssl/commit/bb617a9646d95d0454edda995518f370172390e9 The 0.9.7h change above fixed it across the board, but there was never a similar update to the workaround for DSA and DH. With such OpenSSL versions long since out of support, the last remains of this workaround can finally be removed. --- lib/dns/openssl_shim.h | 2 -- lib/dns/openssldh_link.c | 3 --- 2 files changed, 5 deletions(-) diff --git a/lib/dns/openssl_shim.h b/lib/dns/openssl_shim.h index 439d9f999a..e716b3a7a3 100644 --- a/lib/dns/openssl_shim.h +++ b/lib/dns/openssl_shim.h @@ -72,8 +72,6 @@ DH_get0_pqg(const DH *dh, const BIGNUM **p, const BIGNUM **q, const BIGNUM **g); int DH_set0_pqg(DH *dh, BIGNUM *p, BIGNUM *q, BIGNUM *g); - -#define DH_clear_flags(d, f) ((d)->flags &= ~(f)) #endif /* !HAVE_DH_GET0_KEY */ #if !HAVE_ERR_GET_ERROR_ALL diff --git a/lib/dns/openssldh_link.c b/lib/dns/openssldh_link.c index 0a2e11f51f..e76cfbe310 100644 --- a/lib/dns/openssldh_link.c +++ b/lib/dns/openssldh_link.c @@ -492,7 +492,6 @@ openssldh_generate(dst_key_t *key, int generator, void (*callback)(int)) { DST_RET(dst__openssl_toresult2("DH_generate_key", DST_R_OPENSSLFAILURE)); } - DH_clear_flags(dh, DH_FLAG_CACHE_MONT_P); key->keydata.dh = dh; dh = NULL; #else @@ -785,7 +784,6 @@ openssldh_fromdns(dst_key_t *key, isc_buffer_t *data) { if (dh == NULL) { DST_RET(dst__openssl_toresult(ISC_R_NOMEMORY)); } - DH_clear_flags(dh, DH_FLAG_CACHE_MONT_P); #else bld = OSSL_PARAM_BLD_new(); if (bld == NULL) { @@ -1116,7 +1114,6 @@ openssldh_parse(dst_key_t *key, isc_lex_t *lexer, dst_key_t *pub) { if (dh == NULL) { DST_RET(ISC_R_NOMEMORY); } - DH_clear_flags(dh, DH_FLAG_CACHE_MONT_P); #else bld = OSSL_PARAM_BLD_new(); if (bld == NULL) {