Don't pass `parent` or `ad` to CRYPTO_EX_free We used to pass the parent object to callbacks, but this isn't safe. The object is in the middle of being destroyed, so the object's invariants won't hold. We can't remove the parameters, but pass in NULL instead. This also has the side effect of making it possible to manage CRYPTO_EX_DATA lifetime in its destructor, because we won't need to find some handle back to the parent object. It also means that defensively clearing fields in BIO destroy callbacks, while harmless, is more obviously unnecessary. Update-Note: CRYPTO_EX_free is no longer passed the parent object. We do not expect any callers to be impacted. Callers that were impacted probably had some object lifetime bug already. Fixed: 412707574 Change-Id: Idda889898a9eca3b59e3200ba1d5b4f2e62f2d37 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/79607 Reviewed-by: Adam Langley <agl@google.com> Auto-Submit: David Benjamin <davidben@google.com> Commit-Queue: David Benjamin <davidben@google.com>
diff --git a/crypto/bio/bio.cc b/crypto/bio/bio.cc index 8de2fa0..f585d68 100644 --- a/crypto/bio/bio.cc +++ b/crypto/bio/bio.cc
@@ -63,7 +63,7 @@ bio->method->destroy(bio); } - CRYPTO_free_ex_data(&g_ex_data_class, bio, &bio->ex_data); + CRYPTO_free_ex_data(&g_ex_data_class, &bio->ex_data); OPENSSL_free(bio); } return 1;
diff --git a/crypto/dsa/dsa.cc b/crypto/dsa/dsa.cc index 3058cf4..0380525 100644 --- a/crypto/dsa/dsa.cc +++ b/crypto/dsa/dsa.cc
@@ -66,7 +66,7 @@ return; } - CRYPTO_free_ex_data(&g_ex_data_class, dsa, &dsa->ex_data); + CRYPTO_free_ex_data(&g_ex_data_class, &dsa->ex_data); BN_clear_free(dsa->p); BN_clear_free(dsa->q);
diff --git a/crypto/ex_data.cc b/crypto/ex_data.cc index 36fc34b..86a3d58 100644 --- a/crypto/ex_data.cc +++ b/crypto/ex_data.cc
@@ -108,7 +108,7 @@ void CRYPTO_new_ex_data(CRYPTO_EX_DATA *ad) { ad->sk = NULL; } -void CRYPTO_free_ex_data(CRYPTO_EX_DATA_CLASS *ex_data_class, void *obj, +void CRYPTO_free_ex_data(CRYPTO_EX_DATA_CLASS *ex_data_class, CRYPTO_EX_DATA *ad) { if (ad->sk == NULL) { // Nothing to do. @@ -126,7 +126,8 @@ if ((*funcs)->free_func != NULL) { int index = (int)i + ex_data_class->num_reserved; void *ptr = CRYPTO_get_ex_data(ad, index); - (*funcs)->free_func(obj, ptr, ad, index, (*funcs)->argl, (*funcs)->argp); + (*funcs)->free_func(/*parent=*/nullptr, ptr, /*ad*/ nullptr, index, + (*funcs)->argl, (*funcs)->argp); } funcs = &(*funcs)->next; }
diff --git a/crypto/fipsmodule/ec/ec_key.cc.inc b/crypto/fipsmodule/ec/ec_key.cc.inc index 3af5d9a..d471e84 100644 --- a/crypto/fipsmodule/ec/ec_key.cc.inc +++ b/crypto/fipsmodule/ec/ec_key.cc.inc
@@ -73,7 +73,7 @@ CRYPTO_new_ex_data(&ret->ex_data); if (ret->ecdsa_meth && ret->ecdsa_meth->init && !ret->ecdsa_meth->init(ret)) { - CRYPTO_free_ex_data(g_ec_ex_data_class_bss_get(), ret, &ret->ex_data); + CRYPTO_free_ex_data(g_ec_ex_data_class_bss_get(), &ret->ex_data); if (ret->ecdsa_meth) { METHOD_unref(ret->ecdsa_meth); } @@ -113,7 +113,7 @@ METHOD_unref(r->ecdsa_meth); } - CRYPTO_free_ex_data(g_ec_ex_data_class_bss_get(), r, &r->ex_data); + CRYPTO_free_ex_data(g_ec_ex_data_class_bss_get(), &r->ex_data); EC_GROUP_free(r->group); EC_POINT_free(r->pub_key);
diff --git a/crypto/fipsmodule/rsa/rsa.cc.inc b/crypto/fipsmodule/rsa/rsa.cc.inc index 771b6df..f1ba37b 100644 --- a/crypto/fipsmodule/rsa/rsa.cc.inc +++ b/crypto/fipsmodule/rsa/rsa.cc.inc
@@ -183,10 +183,8 @@ CRYPTO_new_ex_data(&rsa->ex_data); if (rsa->meth->init && !rsa->meth->init(rsa)) { - CRYPTO_free_ex_data(g_rsa_ex_data_class_bss_get(), rsa, &rsa->ex_data); - CRYPTO_MUTEX_cleanup(&rsa->lock); - METHOD_unref(rsa->meth); - OPENSSL_free(rsa); + rsa->meth = nullptr; + RSA_free(rsa); return NULL; } @@ -212,12 +210,12 @@ return; } - if (rsa->meth->finish) { + if (rsa->meth != nullptr && rsa->meth->finish != nullptr) { rsa->meth->finish(rsa); } METHOD_unref(rsa->meth); - CRYPTO_free_ex_data(g_rsa_ex_data_class_bss_get(), rsa, &rsa->ex_data); + CRYPTO_free_ex_data(g_rsa_ex_data_class_bss_get(), &rsa->ex_data); BN_free(rsa->n); BN_free(rsa->e);
diff --git a/crypto/internal.h b/crypto/internal.h index a78338c..85e6d0b 100644 --- a/crypto/internal.h +++ b/crypto/internal.h
@@ -760,10 +760,9 @@ // CRYPTO_new_ex_data initialises a newly allocated |CRYPTO_EX_DATA|. OPENSSL_EXPORT void CRYPTO_new_ex_data(CRYPTO_EX_DATA *ad); -// CRYPTO_free_ex_data frees |ad|, which is embedded inside |obj|, which is an -// object of the given class. +// CRYPTO_free_ex_data frees |ad|, which is an object of the given class. OPENSSL_EXPORT void CRYPTO_free_ex_data(CRYPTO_EX_DATA_CLASS *ex_data_class, - void *obj, CRYPTO_EX_DATA *ad); + CRYPTO_EX_DATA *ad); // Endianness conversions.
diff --git a/crypto/x509/x509_vfy.cc b/crypto/x509/x509_vfy.cc index a1ad59f..87a714a 100644 --- a/crypto/x509/x509_vfy.cc +++ b/crypto/x509/x509_vfy.cc
@@ -1533,12 +1533,7 @@ return 1; err: - CRYPTO_free_ex_data(&g_ex_data_class, ctx, &ctx->ex_data); - if (ctx->param != NULL) { - X509_VERIFY_PARAM_free(ctx->param); - } - - OPENSSL_memset(ctx, 0, sizeof(X509_STORE_CTX)); + X509_STORE_CTX_cleanup(ctx); return 0; } @@ -1555,7 +1550,7 @@ } void X509_STORE_CTX_cleanup(X509_STORE_CTX *ctx) { - CRYPTO_free_ex_data(&g_ex_data_class, ctx, &(ctx->ex_data)); + CRYPTO_free_ex_data(&g_ex_data_class, &ctx->ex_data); X509_VERIFY_PARAM_free(ctx->param); sk_X509_pop_free(ctx->chain, X509_free); OPENSSL_memset(ctx, 0, sizeof(X509_STORE_CTX));
diff --git a/crypto/x509/x_x509.cc b/crypto/x509/x_x509.cc index 465c53f..3e6c2a0 100644 --- a/crypto/x509/x_x509.cc +++ b/crypto/x509/x_x509.cc
@@ -85,7 +85,7 @@ return; } - CRYPTO_free_ex_data(&g_ex_data_class, x509, &x509->ex_data); + CRYPTO_free_ex_data(&g_ex_data_class, &x509->ex_data); X509_CINF_free(x509->cert_info); X509_ALGOR_free(x509->sig_alg);
diff --git a/include/openssl/bio.h b/include/openssl/bio.h index 794d7a7..b19607a 100644 --- a/include/openssl/bio.h +++ b/include/openssl/bio.h
@@ -657,6 +657,11 @@ // BIO_meth_set_destroy sets a function to release data associated with a |BIO| // and returns one. The function's return value is ignored. +// +// As the |BIO| is about to be destroyed, it is not necessary for |destroy_func| +// to clear the BIO's state with |BIO_set_data| or |BIO_set_init|. There is no +// harm in clearing them, but the |BIO| will not be passed to |BIO| operations, +// unless |destroy_func| itself does so. OPENSSL_EXPORT int BIO_meth_set_destroy(BIO_METHOD *method, int (*destroy_func)(BIO *));
diff --git a/include/openssl/ex_data.h b/include/openssl/ex_data.h index 55899df..a1679c8 100644 --- a/include/openssl/ex_data.h +++ b/include/openssl/ex_data.h
@@ -71,9 +71,8 @@ // Callback types. -// TODO(davidben): This is only declared in public headers to be a parameter of -// |CRYPTO_EX_*| callbacks. Callers cannot do anything useful with it, so we may -// as well pass NULL and use an internal type for the actual storage. +// CRYPTO_EX_DATA, in the public API, is an opaque struct that is never returned +// from the library. typedef struct crypto_ex_data_st CRYPTO_EX_DATA; // CRYPTO_EX_free is a callback function that is called when an object of the @@ -81,15 +80,18 @@ // callback has been passed to |SSL_get_ex_new_index| then it may be called each // time an |SSL*| is destroyed. // -// The callback is passed the to-be-destroyed object (i.e. the |SSL*|) in -// |parent|. As |parent| will shortly be destroyed, callers must not perform -// operations that would increment its reference count, pass ownership, or -// assume the object outlives the function call. The arguments |argl| and |argp| -// contain opaque values that were given to |CRYPTO_get_ex_new_index_ex|. +// |parent| and |ad| will be NULL. Historically, the parent object was passed in +// |parent|, but accessing the pointer was not safe because |parent| was in the +// process of being destroyed. If the callback has access to some other pointer +// to the parent object, it must not pass the pointer to any BoringSSL APIs. +// Mid-destruction, invariants on the parent object no longer hold. // -// This callback may be called with a NULL value for |ptr| if |parent| has no +// The arguments |argl| and |argp| contain opaque values that were given to +// |CRYPTO_get_ex_new_index_ex|. +// +// This callback may be called with a NULL value for |ptr| if the object has no // value set for this index. However, the callbacks may also be skipped entirely -// if no extra data pointers are set on |parent| at all. +// if no extra data pointers are set on the object at all. typedef void CRYPTO_EX_free(void *parent, void *ptr, CRYPTO_EX_DATA *ad, int index, long argl, void *argp);
diff --git a/ssl/ssl_credential.cc b/ssl/ssl_credential.cc index 59e2323..95c1a76 100644 --- a/ssl/ssl_credential.cc +++ b/ssl/ssl_credential.cc
@@ -113,7 +113,7 @@ } ssl_credential_st::~ssl_credential_st() { - CRYPTO_free_ex_data(&g_ex_data_class, this, &ex_data); + CRYPTO_free_ex_data(&g_ex_data_class, &ex_data); } static CRYPTO_BUFFER *buffer_up_ref(const CRYPTO_BUFFER *buffer) {
diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc index 69f3fe2..c09c071 100644 --- a/ssl/ssl_lib.cc +++ b/ssl/ssl_lib.cc
@@ -411,7 +411,7 @@ // [openssl.org #212].) SSL_CTX_flush_sessions(this, 0); - CRYPTO_free_ex_data(&g_ex_data_class_ssl_ctx, this, &ex_data); + CRYPTO_free_ex_data(&g_ex_data_class_ssl_ctx, &ex_data); CRYPTO_MUTEX_cleanup(&lock); lh_SSL_SESSION_free(sessions); @@ -483,7 +483,7 @@ } ssl_st::~ssl_st() { - CRYPTO_free_ex_data(&g_ex_data_class_ssl, this, &ex_data); + CRYPTO_free_ex_data(&g_ex_data_class_ssl, &ex_data); // |config| refers to |this|, so we must release it earlier. config.reset(); if (method != NULL) {
diff --git a/ssl/ssl_session.cc b/ssl/ssl_session.cc index 87d263b..01a1211 100644 --- a/ssl/ssl_session.cc +++ b/ssl/ssl_session.cc
@@ -821,7 +821,7 @@ } ssl_session_st::~ssl_session_st() { - CRYPTO_free_ex_data(&g_ex_data_class, this, &ex_data); + CRYPTO_free_ex_data(&g_ex_data_class, &ex_data); x509_method->session_clear(this); }