From 7b07bc4c99466b871de7c35312ada18fd5ec4af2 Mon Sep 17 00:00:00 2001 From: Neil Horman Date: Wed, 16 Oct 2024 14:34:08 -0400 Subject: [PATCH] Fix potential use-after-free in REF_PRINT_COUNT We use REF_PRINT_COUNT to dump out the value of various reference counters in our code However, we commonly use this macro after an increment or decrement. On increment its fine, but on decrement its not, because the macro dereferences the object holding the counter value, which may be freed by another thread, as we've given up our ref count to it prior to using the macro. The rule is that we can't reference memory for an object once we've released our reference, so lets fix this by altering REF_PRINT_COUNT to accept the value returned by CRYPTO_[UP|DOWN]_REF instead. The eliminates the need to dereference the memory the object points to an allows us to use the call after we release our reference count Reviewed-by: Richard Levitte Reviewed-by: Tomas Mraz (Merged from https://github.com/openssl/openssl/pull/25664) (cherry picked from commit dc10ffc2834e0d2f5ebc1c3e29bd97f1f43a0ead) --- crypto/bio/bio_lib.c | 4 ++-- crypto/dh/dh_lib.c | 4 ++-- crypto/dsa/dsa_lib.c | 4 ++-- crypto/dso/dso_lib.c | 4 ++-- crypto/ec/ec_key.c | 4 ++-- crypto/ec/ec_mult.c | 2 +- crypto/ec/ecp_nistp224.c | 2 +- crypto/ec/ecp_nistp256.c | 2 +- crypto/ec/ecp_nistp384.c | 2 +- crypto/ec/ecp_nistp521.c | 2 +- crypto/ec/ecp_nistz256.c | 2 +- crypto/ec/ecx_key.c | 4 ++-- crypto/evp/p_lib.c | 4 ++-- crypto/rsa/rsa_lib.c | 4 ++-- crypto/x509/x509_lu.c | 4 ++-- crypto/x509/x509_set.c | 2 +- crypto/x509/x509cset.c | 2 +- include/internal/refcount.h | 4 ++-- ssl/ssl_cert.c | 2 +- ssl/ssl_cert_comp.c | 4 ++-- ssl/ssl_lib.c | 8 ++++---- ssl/ssl_sess.c | 4 ++-- 22 files changed, 37 insertions(+), 37 deletions(-) diff --git a/crypto/bio/bio_lib.c b/crypto/bio/bio_lib.c index 272189a9a64..85ab4afe182 100644 --- a/crypto/bio/bio_lib.c +++ b/crypto/bio/bio_lib.c @@ -126,7 +126,7 @@ int BIO_free(BIO *a) if (CRYPTO_DOWN_REF(&a->references, &ret) <= 0) return 0; - REF_PRINT_COUNT("BIO", a); + REF_PRINT_COUNT("BIO", ret, a); if (ret > 0) return 1; REF_ASSERT_ISNT(ret < 0); @@ -191,7 +191,7 @@ int BIO_up_ref(BIO *a) if (CRYPTO_UP_REF(&a->references, &i) <= 0) return 0; - REF_PRINT_COUNT("BIO", a); + REF_PRINT_COUNT("BIO", i, a); REF_ASSERT_ISNT(i < 2); return i > 1; } diff --git a/crypto/dh/dh_lib.c b/crypto/dh/dh_lib.c index 9d5a6b0b6c2..93e08b3f8c7 100644 --- a/crypto/dh/dh_lib.c +++ b/crypto/dh/dh_lib.c @@ -141,7 +141,7 @@ void DH_free(DH *r) return; CRYPTO_DOWN_REF(&r->references, &i); - REF_PRINT_COUNT("DH", r); + REF_PRINT_COUNT("DH", i, r); if (i > 0) return; REF_ASSERT_ISNT(i < 0); @@ -171,7 +171,7 @@ int DH_up_ref(DH *r) if (CRYPTO_UP_REF(&r->references, &i) <= 0) return 0; - REF_PRINT_COUNT("DH", r); + REF_PRINT_COUNT("DH", i, r); REF_ASSERT_ISNT(i < 2); return ((i > 1) ? 1 : 0); } diff --git a/crypto/dsa/dsa_lib.c b/crypto/dsa/dsa_lib.c index 7997c2ac25e..db6e3b059b4 100644 --- a/crypto/dsa/dsa_lib.c +++ b/crypto/dsa/dsa_lib.c @@ -218,7 +218,7 @@ void DSA_free(DSA *r) return; CRYPTO_DOWN_REF(&r->references, &i); - REF_PRINT_COUNT("DSA", r); + REF_PRINT_COUNT("DSA", i, r); if (i > 0) return; REF_ASSERT_ISNT(i < 0); @@ -249,7 +249,7 @@ int DSA_up_ref(DSA *r) if (CRYPTO_UP_REF(&r->references, &i) <= 0) return 0; - REF_PRINT_COUNT("DSA", r); + REF_PRINT_COUNT("DSA", i, r); REF_ASSERT_ISNT(i < 2); return ((i > 1) ? 1 : 0); } diff --git a/crypto/dso/dso_lib.c b/crypto/dso/dso_lib.c index 8f3387e9b80..65579cb8b36 100644 --- a/crypto/dso/dso_lib.c +++ b/crypto/dso/dso_lib.c @@ -54,7 +54,7 @@ int DSO_free(DSO *dso) if (CRYPTO_DOWN_REF(&dso->references, &i) <= 0) return 0; - REF_PRINT_COUNT("DSO", dso); + REF_PRINT_COUNT("DSO", i, dso); if (i > 0) return 1; REF_ASSERT_ISNT(i < 0); @@ -96,7 +96,7 @@ int DSO_up_ref(DSO *dso) if (CRYPTO_UP_REF(&dso->references, &i) <= 0) return 0; - REF_PRINT_COUNT("DSO", dso); + REF_PRINT_COUNT("DSO", i, dso); REF_ASSERT_ISNT(i < 2); return ((i > 1) ? 1 : 0); } diff --git a/crypto/ec/ec_key.c b/crypto/ec/ec_key.c index 05224b31a4f..681488e3f30 100644 --- a/crypto/ec/ec_key.c +++ b/crypto/ec/ec_key.c @@ -76,7 +76,7 @@ void EC_KEY_free(EC_KEY *r) return; CRYPTO_DOWN_REF(&r->references, &i); - REF_PRINT_COUNT("EC_KEY", r); + REF_PRINT_COUNT("EC_KEY", i, r); if (i > 0) return; REF_ASSERT_ISNT(i < 0); @@ -197,7 +197,7 @@ int EC_KEY_up_ref(EC_KEY *r) if (CRYPTO_UP_REF(&r->references, &i) <= 0) return 0; - REF_PRINT_COUNT("EC_KEY", r); + REF_PRINT_COUNT("EC_KEY", i, r); REF_ASSERT_ISNT(i < 2); return ((i > 1) ? 1 : 0); } diff --git a/crypto/ec/ec_mult.c b/crypto/ec/ec_mult.c index 9eb007cdf90..e9092a6c9db 100644 --- a/crypto/ec/ec_mult.c +++ b/crypto/ec/ec_mult.c @@ -85,7 +85,7 @@ void EC_ec_pre_comp_free(EC_PRE_COMP *pre) return; CRYPTO_DOWN_REF(&pre->references, &i); - REF_PRINT_COUNT("EC_ec", pre); + REF_PRINT_COUNT("EC_ec", i, pre); if (i > 0) return; REF_ASSERT_ISNT(i < 0); diff --git a/crypto/ec/ecp_nistp224.c b/crypto/ec/ecp_nistp224.c index debfdb3dc94..6485f46f717 100644 --- a/crypto/ec/ecp_nistp224.c +++ b/crypto/ec/ecp_nistp224.c @@ -1264,7 +1264,7 @@ void EC_nistp224_pre_comp_free(NISTP224_PRE_COMP *p) return; CRYPTO_DOWN_REF(&p->references, &i); - REF_PRINT_COUNT("EC_nistp224", p); + REF_PRINT_COUNT("EC_nistp224", i, p); if (i > 0) return; REF_ASSERT_ISNT(i < 0); diff --git a/crypto/ec/ecp_nistp256.c b/crypto/ec/ecp_nistp256.c index 325ace67bc1..eaf9dddbc8e 100644 --- a/crypto/ec/ecp_nistp256.c +++ b/crypto/ec/ecp_nistp256.c @@ -1876,7 +1876,7 @@ void EC_nistp256_pre_comp_free(NISTP256_PRE_COMP *pre) return; CRYPTO_DOWN_REF(&pre->references, &i); - REF_PRINT_COUNT("EC_nistp256", pre); + REF_PRINT_COUNT("EC_nistp256", i, pre); if (i > 0) return; REF_ASSERT_ISNT(i < 0); diff --git a/crypto/ec/ecp_nistp384.c b/crypto/ec/ecp_nistp384.c index e2a0e36488f..44ac1cea3d0 100644 --- a/crypto/ec/ecp_nistp384.c +++ b/crypto/ec/ecp_nistp384.c @@ -1560,7 +1560,7 @@ void ossl_ec_nistp384_pre_comp_free(NISTP384_PRE_COMP *p) return; CRYPTO_DOWN_REF(&p->references, &i); - REF_PRINT_COUNT("ossl_ec_nistp384", p); + REF_PRINT_COUNT("ossl_ec_nistp384", i, p); if (i > 0) return; REF_ASSERT_ISNT(i < 0); diff --git a/crypto/ec/ecp_nistp521.c b/crypto/ec/ecp_nistp521.c index db5a9dd5def..fe6836a1471 100644 --- a/crypto/ec/ecp_nistp521.c +++ b/crypto/ec/ecp_nistp521.c @@ -1766,7 +1766,7 @@ void EC_nistp521_pre_comp_free(NISTP521_PRE_COMP *p) return; CRYPTO_DOWN_REF(&p->references, &i); - REF_PRINT_COUNT("EC_nistp521", p); + REF_PRINT_COUNT("EC_nistp521", i, p); if (i > 0) return; REF_ASSERT_ISNT(i < 0); diff --git a/crypto/ec/ecp_nistz256.c b/crypto/ec/ecp_nistz256.c index 55ae2651acc..36b1d164f4b 100644 --- a/crypto/ec/ecp_nistz256.c +++ b/crypto/ec/ecp_nistz256.c @@ -1238,7 +1238,7 @@ void EC_nistz256_pre_comp_free(NISTZ256_PRE_COMP *pre) return; CRYPTO_DOWN_REF(&pre->references, &i); - REF_PRINT_COUNT("EC_nistz256", pre); + REF_PRINT_COUNT("EC_nistz256", i, pre); if (i > 0) return; REF_ASSERT_ISNT(i < 0); diff --git a/crypto/ec/ecx_key.c b/crypto/ec/ecx_key.c index ba725eb573c..aeaf5783d10 100644 --- a/crypto/ec/ecx_key.c +++ b/crypto/ec/ecx_key.c @@ -69,7 +69,7 @@ void ossl_ecx_key_free(ECX_KEY *key) return; CRYPTO_DOWN_REF(&key->references, &i); - REF_PRINT_COUNT("ECX_KEY", key); + REF_PRINT_COUNT("ECX_KEY", i, key); if (i > 0) return; REF_ASSERT_ISNT(i < 0); @@ -92,7 +92,7 @@ int ossl_ecx_key_up_ref(ECX_KEY *key) if (CRYPTO_UP_REF(&key->references, &i) <= 0) return 0; - REF_PRINT_COUNT("ECX_KEY", key); + REF_PRINT_COUNT("ECX_KEY", i, key); REF_ASSERT_ISNT(i < 2); return ((i > 1) ? 1 : 0); } diff --git a/crypto/evp/p_lib.c b/crypto/evp/p_lib.c index 09bd185a25b..2eb142fa766 100644 --- a/crypto/evp/p_lib.c +++ b/crypto/evp/p_lib.c @@ -1671,7 +1671,7 @@ int EVP_PKEY_up_ref(EVP_PKEY *pkey) if (CRYPTO_UP_REF(&pkey->references, &i) <= 0) return 0; - REF_PRINT_COUNT("EVP_PKEY", pkey); + REF_PRINT_COUNT("EVP_PKEY", i, pkey); REF_ASSERT_ISNT(i < 2); return ((i > 1) ? 1 : 0); } @@ -1792,7 +1792,7 @@ void EVP_PKEY_free(EVP_PKEY *x) return; CRYPTO_DOWN_REF(&x->references, &i); - REF_PRINT_COUNT("EVP_PKEY", x); + REF_PRINT_COUNT("EVP_PKEY", i, x); if (i > 0) return; REF_ASSERT_ISNT(i < 0); diff --git a/crypto/rsa/rsa_lib.c b/crypto/rsa/rsa_lib.c index 071c6245f6c..eece0a4f35d 100644 --- a/crypto/rsa/rsa_lib.c +++ b/crypto/rsa/rsa_lib.c @@ -141,7 +141,7 @@ void RSA_free(RSA *r) return; CRYPTO_DOWN_REF(&r->references, &i); - REF_PRINT_COUNT("RSA", r); + REF_PRINT_COUNT("RSA", i, r); if (i > 0) return; REF_ASSERT_ISNT(i < 0); @@ -193,7 +193,7 @@ int RSA_up_ref(RSA *r) if (CRYPTO_UP_REF(&r->references, &i) <= 0) return 0; - REF_PRINT_COUNT("RSA", r); + REF_PRINT_COUNT("RSA", i, r); REF_ASSERT_ISNT(i < 2); return i > 1 ? 1 : 0; } diff --git a/crypto/x509/x509_lu.c b/crypto/x509/x509_lu.c index e7fdf3d6ab6..09fa2ee1f74 100644 --- a/crypto/x509/x509_lu.c +++ b/crypto/x509/x509_lu.c @@ -232,7 +232,7 @@ void X509_STORE_free(X509_STORE *xs) if (xs == NULL) return; CRYPTO_DOWN_REF(&xs->references, &i); - REF_PRINT_COUNT("X509_STORE", xs); + REF_PRINT_COUNT("X509_STORE", i, xs); if (i > 0) return; REF_ASSERT_ISNT(i < 0); @@ -260,7 +260,7 @@ int X509_STORE_up_ref(X509_STORE *xs) if (CRYPTO_UP_REF(&xs->references, &i) <= 0) return 0; - REF_PRINT_COUNT("X509_STORE", xs); + REF_PRINT_COUNT("X509_STORE", i, xs); REF_ASSERT_ISNT(i < 2); return i > 1 ? 1 : 0; } diff --git a/crypto/x509/x509_set.c b/crypto/x509/x509_set.c index 2aba0e8c142..0c9df51b3c7 100644 --- a/crypto/x509/x509_set.c +++ b/crypto/x509/x509_set.c @@ -119,7 +119,7 @@ int X509_up_ref(X509 *x) if (CRYPTO_UP_REF(&x->references, &i) <= 0) return 0; - REF_PRINT_COUNT("X509", x); + REF_PRINT_COUNT("X509", i, x); REF_ASSERT_ISNT(i < 2); return i > 1; } diff --git a/crypto/x509/x509cset.c b/crypto/x509/x509cset.c index 205fe3d6e5a..e5dd4d5c3a3 100644 --- a/crypto/x509/x509cset.c +++ b/crypto/x509/x509cset.c @@ -78,7 +78,7 @@ int X509_CRL_up_ref(X509_CRL *crl) if (CRYPTO_UP_REF(&crl->references, &i) <= 0) return 0; - REF_PRINT_COUNT("X509_CRL", crl); + REF_PRINT_COUNT("X509_CRL", i, crl); REF_ASSERT_ISNT(i < 2); return i > 1; } diff --git a/include/internal/refcount.h b/include/internal/refcount.h index a90fa2453ae..8de230f343a 100644 --- a/include/internal/refcount.h +++ b/include/internal/refcount.h @@ -297,7 +297,7 @@ static ossl_unused ossl_inline void CRYPTO_FREE_REF(CRYPTO_REF_COUNT *refcnt) # define REF_PRINT_EX(text, count, object) \ OSSL_TRACE3(REF_COUNT, "%p:%4d:%s\n", (object), (count), (text)); -# define REF_PRINT_COUNT(text, object) \ - REF_PRINT_EX(text, object->references.val, (void *)object) +# define REF_PRINT_COUNT(text, val, object) \ + REF_PRINT_EX(text, val, (void *)object) #endif diff --git a/ssl/ssl_cert.c b/ssl/ssl_cert.c index e466efc5377..24ff2f18103 100644 --- a/ssl/ssl_cert.c +++ b/ssl/ssl_cert.c @@ -267,7 +267,7 @@ void ssl_cert_free(CERT *c) if (c == NULL) return; CRYPTO_DOWN_REF(&c->references, &i); - REF_PRINT_COUNT("CERT", c); + REF_PRINT_COUNT("CERT", i, c); if (i > 0) return; REF_ASSERT_ISNT(i < 0); diff --git a/ssl/ssl_cert_comp.c b/ssl/ssl_cert_comp.c index ba9bfb480c0..7bf49d435f5 100644 --- a/ssl/ssl_cert_comp.c +++ b/ssl/ssl_cert_comp.c @@ -136,7 +136,7 @@ void OSSL_COMP_CERT_free(OSSL_COMP_CERT *cc) return; CRYPTO_DOWN_REF(&cc->references, &i); - REF_PRINT_COUNT("OSSL_COMP_CERT", cc); + REF_PRINT_COUNT("OSSL_COMP_CERT", i, cc); if (i > 0) return; REF_ASSERT_ISNT(i < 0); @@ -152,7 +152,7 @@ int OSSL_COMP_CERT_up_ref(OSSL_COMP_CERT *cc) if (CRYPTO_UP_REF(&cc->references, &i) <= 0) return 0; - REF_PRINT_COUNT("OSSL_COMP_CERT", cc); + REF_PRINT_COUNT("OSSL_COMP_CERT", i, cc); REF_ASSERT_ISNT(i < 2); return ((i > 1) ? 1 : 0); } diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c index c1278d854b2..295b719ff2c 100644 --- a/ssl/ssl_lib.c +++ b/ssl/ssl_lib.c @@ -982,7 +982,7 @@ int SSL_up_ref(SSL *s) if (CRYPTO_UP_REF(&s->references, &i) <= 0) return 0; - REF_PRINT_COUNT("SSL", s); + REF_PRINT_COUNT("SSL", i, s); REF_ASSERT_ISNT(i < 2); return ((i > 1) ? 1 : 0); } @@ -1383,7 +1383,7 @@ void SSL_free(SSL *s) if (s == NULL) return; CRYPTO_DOWN_REF(&s->references, &i); - REF_PRINT_COUNT("SSL", s); + REF_PRINT_COUNT("SSL", i, s); if (i > 0) return; REF_ASSERT_ISNT(i < 0); @@ -4141,7 +4141,7 @@ int SSL_CTX_up_ref(SSL_CTX *ctx) if (CRYPTO_UP_REF(&ctx->references, &i) <= 0) return 0; - REF_PRINT_COUNT("SSL_CTX", ctx); + REF_PRINT_COUNT("SSL_CTX", i, ctx); REF_ASSERT_ISNT(i < 2); return ((i > 1) ? 1 : 0); } @@ -4155,7 +4155,7 @@ void SSL_CTX_free(SSL_CTX *a) return; CRYPTO_DOWN_REF(&a->references, &i); - REF_PRINT_COUNT("SSL_CTX", a); + REF_PRINT_COUNT("SSL_CTX", i, a); if (i > 0) return; REF_ASSERT_ISNT(i < 0); diff --git a/ssl/ssl_sess.c b/ssl/ssl_sess.c index f35fcc77481..69149de0507 100644 --- a/ssl/ssl_sess.c +++ b/ssl/ssl_sess.c @@ -844,7 +844,7 @@ void SSL_SESSION_free(SSL_SESSION *ss) if (ss == NULL) return; CRYPTO_DOWN_REF(&ss->references, &i); - REF_PRINT_COUNT("SSL_SESSION", ss); + REF_PRINT_COUNT("SSL_SESSION", i, ss); if (i > 0) return; REF_ASSERT_ISNT(i < 0); @@ -878,7 +878,7 @@ int SSL_SESSION_up_ref(SSL_SESSION *ss) if (CRYPTO_UP_REF(&ss->references, &i) <= 0) return 0; - REF_PRINT_COUNT("SSL_SESSION", ss); + REF_PRINT_COUNT("SSL_SESSION", i, ss); REF_ASSERT_ISNT(i < 2); return ((i > 1) ? 1 : 0); } -- 2.47.2