HTTP client: Fix cleanup of TLS BIO via 'bio_update_fn' callback function

Make app_http_tls_cb() tidy up on disconnect the SSL BIO it pushes on connect.
Make OSSL_HTTP_close() respect this.

Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/17318)

diff --git a/apps/lib/apps.c b/apps/lib/apps.c
index 88c4f7b97ae23e58e12610d7942710e65b4321b4..034fd45c4b7b81f9e460da3e9373ddfccfe18a93 100644 (file)
--- a/apps/lib/apps.c
+++ b/apps/lib/apps.c
@@ -2462,7 +2462,7 @@ static const char *tls_error_hint(void)
 }
 
 /* HTTP callback function that supports TLS connection also via HTTPS proxy */
-BIO *app_http_tls_cb(BIO *hbio, void *arg, int connect, int detail)
+BIO *app_http_tls_cb(BIO *bio, void *arg, int connect, int detail)
 {
     if (connect && detail) { /* connecting with TLS */
         APP_HTTP_TLS_INFO *info = (APP_HTTP_TLS_INFO *)arg;
@@ -2471,7 +2471,7 @@ BIO *app_http_tls_cb(BIO *hbio, void *arg, int connect, int detail)
         BIO *sbio = NULL;
 
         if ((info->use_proxy
-             && !OSSL_HTTP_proxy_connect(hbio, info->server, info->port,
+             && !OSSL_HTTP_proxy_connect(bio, info->server, info->port,
                                          NULL, NULL, /* no proxy credentials */
                                          info->timeout, bio_err, opt_getprog()))
                 || (sbio = BIO_new(BIO_f_ssl())) == NULL) {
@@ -2487,18 +2487,25 @@ BIO *app_http_tls_cb(BIO *hbio, void *arg, int connect, int detail)
         SSL_set_connect_state(ssl);
         BIO_set_ssl(sbio, ssl, BIO_CLOSE);
 
-        hbio = BIO_push(sbio, hbio);
-    } else if (!connect && !detail) { /* disconnecting after error */
-        const char *hint = tls_error_hint();
-
-        if (hint != NULL)
-            ERR_add_error_data(2, " : ", hint);
-        /*
-         * If we pop sbio and BIO_free() it this may lead to libssl double free.
-         * Rely on BIO_free_all() done by OSSL_HTTP_transfer() in http_client.c
-         */
+        bio = BIO_push(sbio, bio);
     }
-    return hbio;
+    if (!connect) {
+        const char *hint;
+        BIO *cbio;
+
+        if (!detail) { /* disconnecting after error */
+            hint = tls_error_hint();
+            if (hint != NULL)
+                ERR_add_error_data(2, " : ", hint);
+        }
+        (void)ERR_set_mark();
+        BIO_ssl_shutdown(bio);
+        cbio = BIO_pop(bio); /* connect+HTTP BIO */
+        BIO_free(bio); /* SSL BIO */
+        (void)ERR_pop_to_mark(); /* hide SSL_R_READ_BIO_NOT_SET etc. */
+        bio = cbio;
+    }
+    return bio;
 }
 
 void APP_HTTP_TLS_INFO_free(APP_HTTP_TLS_INFO *info)

diff --git a/crypto/http/http_client.c b/crypto/http/http_client.c
index ef0114240b4677026b951b2b334e99a412a50d7e..f786f831bf86711bf7218246aeb2c88b43243e75 100644 (file)
--- a/crypto/http/http_client.c
+++ b/crypto/http/http_client.c
@@ -1196,11 +1196,17 @@ BIO *OSSL_HTTP_transfer(OSSL_HTTP_REQ_CTX **prctx,
 
 int OSSL_HTTP_close(OSSL_HTTP_REQ_CTX *rctx, int ok)
 {
+    BIO *wbio;
     int ret = 1;
 
-    /* callback can be used to clean up TLS session on disconnect */
-    if (rctx != NULL && rctx->upd_fn != NULL)
-        ret = (*rctx->upd_fn)(rctx->wbio, rctx->upd_arg, 0, ok) != NULL;
+    /* callback can be used to finish TLS session and free its BIO */
+    if (rctx != NULL && rctx->upd_fn != NULL) {
+        wbio = (*rctx->upd_fn)(rctx->wbio, rctx->upd_arg,
+                               0 /* disconnect */, ok);
+        ret = wbio != NULL;
+        if (ret)
+            rctx->wbio = wbio;
+    }
     OSSL_HTTP_REQ_CTX_free(rctx);
     return ret;
 }

diff --git a/doc/man3/OSSL_HTTP_transfer.pod b/doc/man3/OSSL_HTTP_transfer.pod
index 7fcd71dbe03b563c60389d33f0596d5c1eedc770..7e823db3eab52581d37402a229ece22c5446dff5 100644 (file)
--- a/doc/man3/OSSL_HTTP_transfer.pod
+++ b/doc/man3/OSSL_HTTP_transfer.pod
@@ -113,17 +113,25 @@ or NULL to indicate failure, in which case it should not modify the BIO.
 
 Here is a simple example that supports TLS connections (but not via a proxy):
 
- BIO *http_tls_cb(BIO *hbio, void *arg, int connect, int detail)
+ BIO *http_tls_cb(BIO *bio, void *arg, int connect, int detail)
  {
      if (connect && detail) { /* connecting with TLS */
          SSL_CTX *ctx = (SSL_CTX *)arg;
          BIO *sbio = BIO_new_ssl(ctx, 1);
 
-         hbio = sbio != NULL ? BIO_push(sbio, hbio) : NULL;
-     } else if (!connect && !detail) { /* disconnecting after error */
-         /* optionally add diagnostics here */
+         bio = sbio != NULL ? BIO_push(sbio, bio) : NULL;
+     } else if (!connect) { /* disconnecting */
+         BIO *hbio;
+
+         if (!detail) { /* an error has occurred */
+             /* optionally add diagnostics here */
+         }
+         BIO_ssl_shutdown(bio);
+         hbio = BIO_pop(bio);
+         BIO_free(bio); /* SSL BIO */
+         bio = hbio;
      }
-     return hbio;
+     return bio;
  }
 
 After disconnect the modified BIO will be deallocated using BIO_free_all().