Update sslkeylog in response to comments

* instead of keeping an external reference count, just use the
  BIO_up_ref call, and the BIO's callback mechanism to detect the
  final free, for which we set keylog_bio to NULL

* Return an error from SSL_CTX_new_ex if the setup of the keylog file
  fails

Reviewed-by: Tim Hudson <tjh@openssl.org>
Reviewed-by: Saša Nedvědický <sashan@openssl.org>
Reviewed-by: Hugo Landau <hlandau@devever.net>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/25297)

diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index 7989f29a5d1b8266a91b4d9f69d9a7df2db45bc8..27150db61c84a0e435128d7aeda84beffe2fe7c8 100644 (file)
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -3866,11 +3866,6 @@ static CRYPTO_RWLOCK *keylog_lock = NULL;
  */
 static BIO *keylog_bio = NULL;
 
-/**
- * @brief Ref counter tracking the number of keylog_bio users.
- */
-static unsigned int keylog_count = 0;
-
 /**
  * @brief Initializes the SSLKEYLOGFILE lock.
  *
@@ -3884,6 +3879,32 @@ DEFINE_RUN_ONCE_STATIC(ssl_keylog_init)
     return 1;
 }
 
+/**
+ * @brief checks when a BIO refcount has reached zero, and sets
+ * keylog_cb to NULL if it has
+ *
+ * @returns 1 always
+ */
+static long check_keylog_bio_free(BIO *b, int oper, const char *argp,
+                                  size_t len, int argi, long argl, int ret,
+                                  size_t *processed)
+{
+
+    /*
+     * Note we _dont_ take the keylog_lock here
+     * This is intentional, because we only free the keylog lock
+     * During SSL_CTX_free, in which we already posess the lock, so 
+     * Theres no need to grab it again here
+     */
+    if (oper == BIO_CB_FREE)
+        keylog_bio = NULL;
+out:
+    return ret;
+}
+
+/**
+ * @brief records ssl secrets to a file
+ */
 static void sslkeylogfile_cb(const SSL *ssl, const char *line)
 {
     if (keylog_lock == NULL)
@@ -4185,6 +4206,7 @@ SSL_CTX *SSL_CTX_new_ex(OSSL_LIB_CTX *libctx, const char *propq,
         /* Grab out global lock */
         if (!CRYPTO_THREAD_write_lock(keylog_lock)) {
             ERR_raise(ERR_LIB_SSL, ERR_R_SSL_LIB);
+            goto err;
         } else {
             /*
              * If the bio for the requested keylog file hasn't been
@@ -4193,14 +4215,14 @@ SSL_CTX *SSL_CTX_new_ex(OSSL_LIB_CTX *libctx, const char *propq,
              */
             if (keylog_bio == NULL) {
                 keylog_bio = BIO_new_file(keylogfile, "a");
-                if (keylog_bio == NULL)
+                if (keylog_bio == NULL) {
                     ERR_raise(ERR_LIB_SSL, ERR_R_SSL_LIB);
-                else
-                    /* up our ref count for the newly created case */
-                    keylog_count++;
+                    goto err;
+                }
+                BIO_set_callback_ex(keylog_bio, check_keylog_bio_free);
             } else {
                 /* up our refcount for the already-created case */
-                keylog_count++;
+                BIO_up_ref(keylog_bio);
             }
             /* If we have a bio now, assign the callback handler */
             if (keylog_bio != NULL)
@@ -4213,6 +4235,7 @@ SSL_CTX *SSL_CTX_new_ex(OSSL_LIB_CTX *libctx, const char *propq,
     return ret;
  err:
     SSL_CTX_free(ret);
+    BIO_free(keylog_bio);
     return NULL;
 }
 
@@ -4250,13 +4273,8 @@ void SSL_CTX_free(SSL_CTX *a)
 #ifndef OPENSSL_NO_SSLKEYLOG
     if (keylog_lock != NULL && CRYPTO_THREAD_write_lock(keylog_lock)) {
         if (a->sslkeylog_callback != NULL)
-            keylog_count--;
-        a->sslkeylog_callback = NULL;
-        /* If we're the last user, close the bio */
-        if (keylog_count == 0) {
             BIO_free(keylog_bio);
-            keylog_bio = NULL;
-        }
+        a->sslkeylog_callback = NULL;
         CRYPTO_THREAD_unlock(keylog_lock);
     }
 #endif