From 9e93bd47c29b647171fefbe1fb4f5e01da91c488 Mon Sep 17 00:00:00 2001 From: Stefan Eissing Date: Mon, 23 Jan 2023 11:41:23 +0100 Subject: [PATCH] vtls: Manage current easy handle in nested cfilter calls The previous implementation cleared `data` so the outer invocation lost its data, which could lead to a crash. Bug: https://github.com/curl/curl/issues/10336 Reported-by: Fujii Hironori Closes https://github.com/curl/curl/pull/10340 --- lib/cfilters.h | 69 ++++++++++++++++++++++++++++++++++++++++++++ lib/vtls/gtls.c | 4 +-- lib/vtls/mbedtls.c | 6 ++-- lib/vtls/openssl.c | 12 +++----- lib/vtls/sectransp.c | 4 +-- lib/vtls/vtls.c | 65 ++++++++++++++++++++++++----------------- lib/vtls/vtls_int.h | 10 +++---- lib/vtls/wolfssl.c | 6 ++-- 8 files changed, 124 insertions(+), 52 deletions(-) diff --git a/lib/cfilters.h b/lib/cfilters.h index e101cd37e9..149d1ee0e3 100644 --- a/lib/cfilters.h +++ b/lib/cfilters.h @@ -433,4 +433,73 @@ size_t Curl_conn_get_max_concurrent(struct Curl_easy *data, struct connectdata *conn, int sockindex); + +/** + * Types and macros used to keep the current easy handle in filter calls, + * allowing for nested invocations. See #10336. + * + * `cf_call_data` is intended to be a member of the cfilter's `ctx` type. + * A filter defines the macro `CF_CTX_CALL_DATA` to give access to that. + * + * With all values 0, the default, this indicates that there is no cfilter + * call with `data` ongoing. + * Macro `CF_DATA_SAVE` preserves the current `cf_call_data` in a local + * variable and sets the `data` given, incrementing the `depth` counter. + * + * Macro `CF_DATA_RESTORE` restores the old values from the local variable, + * while checking that `depth` values are as expected (debug build), catching + * cases where a "lower" RESTORE was not called. + * + * Finally, macro `CF_DATA_CURRENT` gives the easy handle of the current + * invocation. + */ +struct cf_call_data { + struct Curl_easy *data; +#ifdef DEBUGBUILD + int depth; +#endif +}; + +/** + * define to access the `struct cf_call_data for a cfilter. Normally + * a member in the cfilter's `ctx`. + * + * #define CF_CTX_CALL_DATA(cf) -> struct cf_call_data instance +*/ + +#ifdef DEBUGBUILD + +#define CF_DATA_SAVE(save, cf, data) \ + do { \ + (save) = CF_CTX_CALL_DATA(cf); \ + DEBUGASSERT((save).data == NULL || (save).depth > 0); \ + CF_CTX_CALL_DATA(cf).depth++; \ + CF_CTX_CALL_DATA(cf).data = (struct Curl_easy *)data; \ + } while(0) + +#define CF_DATA_RESTORE(cf, save) \ + do { \ + DEBUGASSERT(CF_CTX_CALL_DATA(cf).depth == (save).depth + 1); \ + DEBUGASSERT((save).data == NULL || (save).depth > 0); \ + CF_CTX_CALL_DATA(cf) = (save); \ + } while(0) + +#else /* DEBUGBUILD */ + +#define CF_DATA_SAVE(save, cf, data) \ + do { \ + (save) = CF_CTX_CALL_DATA(cf); \ + CF_CTX_CALL_DATA(cf).data = (struct Curl_easy *)data; \ + } while(0) + +#define CF_DATA_RESTORE(cf, save) \ + do { \ + CF_CTX_CALL_DATA(cf) = (save); \ + } while(0) + +#endif /* !DEBUGBUILD */ + +#define CF_DATA_CURRENT(cf) \ + ((cf)? (CF_CTX_CALL_DATA(cf).data) : NULL) + #endif /* HEADER_CURL_CFILTERS_H */ diff --git a/lib/vtls/gtls.c b/lib/vtls/gtls.c index aece7bec83..286bce3b36 100644 --- a/lib/vtls/gtls.c +++ b/lib/vtls/gtls.c @@ -84,7 +84,7 @@ static ssize_t gtls_push(void *s, const void *buf, size_t blen) { struct Curl_cfilter *cf = s; struct ssl_connect_data *connssl = cf->ctx; - struct Curl_easy *data = connssl->call_data; + struct Curl_easy *data = CF_DATA_CURRENT(cf); ssize_t nwritten; CURLcode result; @@ -102,7 +102,7 @@ static ssize_t gtls_pull(void *s, void *buf, size_t blen) { struct Curl_cfilter *cf = s; struct ssl_connect_data *connssl = cf->ctx; - struct Curl_easy *data = connssl->call_data; + struct Curl_easy *data = CF_DATA_CURRENT(cf); ssize_t nread; CURLcode result; diff --git a/lib/vtls/mbedtls.c b/lib/vtls/mbedtls.c index acb9488b6a..32d0fa981b 100644 --- a/lib/vtls/mbedtls.c +++ b/lib/vtls/mbedtls.c @@ -159,8 +159,7 @@ static void mbed_debug(void *context, int level, const char *f_name, static int bio_cf_write(void *bio, const unsigned char *buf, size_t blen) { struct Curl_cfilter *cf = bio; - struct ssl_connect_data *connssl = cf->ctx; - struct Curl_easy *data = connssl->call_data; + struct Curl_easy *data = CF_DATA_CURRENT(cf); ssize_t nwritten; CURLcode result; @@ -177,8 +176,7 @@ static int bio_cf_write(void *bio, const unsigned char *buf, size_t blen) static int bio_cf_read(void *bio, unsigned char *buf, size_t blen) { struct Curl_cfilter *cf = bio; - struct ssl_connect_data *connssl = cf->ctx; - struct Curl_easy *data = connssl->call_data; + struct Curl_easy *data = CF_DATA_CURRENT(cf); ssize_t nread; CURLcode result; diff --git a/lib/vtls/openssl.c b/lib/vtls/openssl.c index 9dbd4a4e12..31a8bfc4f8 100644 --- a/lib/vtls/openssl.c +++ b/lib/vtls/openssl.c @@ -702,7 +702,7 @@ static int bio_cf_out_write(BIO *bio, const char *buf, int blen) { struct Curl_cfilter *cf = BIO_get_data(bio); struct ssl_connect_data *connssl = cf->ctx; - struct Curl_easy *data = connssl->call_data; + struct Curl_easy *data = CF_DATA_CURRENT(cf); ssize_t nwritten; CURLcode result = CURLE_SEND_ERROR; @@ -723,7 +723,7 @@ static int bio_cf_in_read(BIO *bio, char *buf, int blen) { struct Curl_cfilter *cf = BIO_get_data(bio); struct ssl_connect_data *connssl = cf->ctx; - struct Curl_easy *data = connssl->call_data; + struct Curl_easy *data = CF_DATA_CURRENT(cf); ssize_t nread; CURLcode result = CURLE_RECV_ERROR; @@ -2639,7 +2639,6 @@ static void ossl_trace(int direction, int ssl_ver, int content_type, const char *verstr = "???"; struct connectdata *conn = userp; int cf_idx = ossl_get_ssl_cf_index(); - struct ssl_connect_data *connssl; struct Curl_easy *data = NULL; struct Curl_cfilter *cf; char unknown[32]; @@ -2647,10 +2646,7 @@ static void ossl_trace(int direction, int ssl_ver, int content_type, DEBUGASSERT(cf_idx >= 0); cf = (struct Curl_cfilter*) SSL_get_ex_data(ssl, cf_idx); DEBUGASSERT(cf); - connssl = cf->ctx; - DEBUGASSERT(connssl); - DEBUGASSERT(connssl->backend); - data = connssl->call_data; + data = CF_DATA_CURRENT(cf); if(!conn || !data || !data->set.fdebug || (direction != 0 && direction != 1)) @@ -2954,7 +2950,7 @@ static int ossl_new_session_cb(SSL *ssl, SSL_SESSION *ssl_sessionid) return 0; cf = (struct Curl_cfilter*) SSL_get_ex_data(ssl, cf_idx); connssl = cf? cf->ctx : NULL; - data = connssl? connssl->call_data : NULL; + data = connssl? CF_DATA_CURRENT(cf) : NULL; /* The sockindex has been stored as a pointer to an array element */ if(!cf || !data) return 0; diff --git a/lib/vtls/sectransp.c b/lib/vtls/sectransp.c index 4897abcbd8..7f311c643c 100644 --- a/lib/vtls/sectransp.c +++ b/lib/vtls/sectransp.c @@ -832,7 +832,7 @@ static OSStatus bio_cf_in_read(SSLConnectionRef connection, struct Curl_cfilter *cf = (struct Curl_cfilter *)connection; struct ssl_connect_data *connssl = cf->ctx; struct ssl_backend_data *backend = connssl->backend; - struct Curl_easy *data = connssl->call_data; + struct Curl_easy *data = CF_DATA_CURRENT(cf); ssize_t nread; CURLcode result; OSStatus rtn = noErr; @@ -868,7 +868,7 @@ static OSStatus bio_cf_out_write(SSLConnectionRef connection, struct Curl_cfilter *cf = (struct Curl_cfilter *)connection; struct ssl_connect_data *connssl = cf->ctx; struct ssl_backend_data *backend = connssl->backend; - struct Curl_easy *data = connssl->call_data; + struct Curl_easy *data = CF_DATA_CURRENT(cf); ssize_t nwritten; CURLcode result; OSStatus rtn = noErr; diff --git a/lib/vtls/vtls.c b/lib/vtls/vtls.c index dd0414ce7c..f9eb4f4335 100644 --- a/lib/vtls/vtls.c +++ b/lib/vtls/vtls.c @@ -315,13 +315,6 @@ static void cf_ctx_free(struct ssl_connect_data *ctx) } } -static void cf_ctx_set_data(struct Curl_cfilter *cf, - struct Curl_easy *data) -{ - if(cf->ctx) - ((struct ssl_connect_data *)cf->ctx)->call_data = data; -} - static CURLcode ssl_connect(struct Curl_cfilter *cf, struct Curl_easy *data) { struct ssl_connect_data *connssl = cf->ctx; @@ -1482,8 +1475,11 @@ static CURLcode reinit_hostname(struct Curl_cfilter *cf) static void ssl_cf_destroy(struct Curl_cfilter *cf, struct Curl_easy *data) { - cf_ctx_set_data(cf, data); + struct cf_call_data save; + + CF_DATA_SAVE(save, cf, data); cf_close(cf, data); + CF_DATA_RESTORE(cf, save); cf_ctx_free(cf->ctx); cf->ctx = NULL; } @@ -1491,10 +1487,12 @@ static void ssl_cf_destroy(struct Curl_cfilter *cf, struct Curl_easy *data) static void ssl_cf_close(struct Curl_cfilter *cf, struct Curl_easy *data) { - cf_ctx_set_data(cf, data); + struct cf_call_data save; + + CF_DATA_SAVE(save, cf, data); cf_close(cf, data); cf->next->cft->close(cf->next, data); - cf_ctx_set_data(cf, NULL); + CF_DATA_RESTORE(cf, save); } static CURLcode ssl_cf_connect(struct Curl_cfilter *cf, @@ -1502,6 +1500,7 @@ static CURLcode ssl_cf_connect(struct Curl_cfilter *cf, bool blocking, bool *done) { struct ssl_connect_data *connssl = cf->ctx; + struct cf_call_data save; CURLcode result; if(cf->connected) { @@ -1509,7 +1508,7 @@ static CURLcode ssl_cf_connect(struct Curl_cfilter *cf, return CURLE_OK; } - cf_ctx_set_data(cf, data); + CF_DATA_SAVE(save, cf, data); (void)connssl; DEBUGASSERT(data->conn); DEBUGASSERT(data->conn == cf->conn); @@ -1540,21 +1539,22 @@ static CURLcode ssl_cf_connect(struct Curl_cfilter *cf, DEBUGASSERT(connssl->state == ssl_connection_complete); } out: - cf_ctx_set_data(cf, NULL); + CF_DATA_RESTORE(cf, save); return result; } static bool ssl_cf_data_pending(struct Curl_cfilter *cf, const struct Curl_easy *data) { + struct cf_call_data save; bool result; - cf_ctx_set_data(cf, (struct Curl_easy *)data); + CF_DATA_SAVE(save, cf, data); if(cf->ctx && Curl_ssl->data_pending(cf, data)) result = TRUE; else result = cf->next->cft->has_data_pending(cf->next, data); - cf_ctx_set_data(cf, NULL); + CF_DATA_RESTORE(cf, save); return result; } @@ -1562,12 +1562,13 @@ static ssize_t ssl_cf_send(struct Curl_cfilter *cf, struct Curl_easy *data, const void *buf, size_t len, CURLcode *err) { + struct cf_call_data save; ssize_t nwritten; + CF_DATA_SAVE(save, cf, data); *err = CURLE_OK; - cf_ctx_set_data(cf, data); nwritten = Curl_ssl->send_plain(cf, data, buf, len, err); - cf_ctx_set_data(cf, NULL); + CF_DATA_RESTORE(cf, save); return nwritten; } @@ -1575,12 +1576,13 @@ static ssize_t ssl_cf_recv(struct Curl_cfilter *cf, struct Curl_easy *data, char *buf, size_t len, CURLcode *err) { + struct cf_call_data save; ssize_t nread; + CF_DATA_SAVE(save, cf, data); *err = CURLE_OK; - cf_ctx_set_data(cf, data); nread = Curl_ssl->recv_plain(cf, data, buf, len, err); - cf_ctx_set_data(cf, NULL); + CF_DATA_RESTORE(cf, save); return nread; } @@ -1588,11 +1590,12 @@ static int ssl_cf_get_select_socks(struct Curl_cfilter *cf, struct Curl_easy *data, curl_socket_t *socks) { + struct cf_call_data save; int result; - cf_ctx_set_data(cf, data); + CF_DATA_SAVE(save, cf, data); result = Curl_ssl->get_select_socks(cf, data, socks); - cf_ctx_set_data(cf, NULL); + CF_DATA_RESTORE(cf, save); return result; } @@ -1600,21 +1603,23 @@ static CURLcode ssl_cf_cntrl(struct Curl_cfilter *cf, struct Curl_easy *data, int event, int arg1, void *arg2) { + struct cf_call_data save; + (void)arg1; (void)arg2; switch(event) { case CF_CTRL_DATA_ATTACH: if(Curl_ssl->attach_data) { - cf_ctx_set_data(cf, data); + CF_DATA_SAVE(save, cf, data); Curl_ssl->attach_data(cf, data); - cf_ctx_set_data(cf, NULL); + CF_DATA_RESTORE(cf, save); } break; case CF_CTRL_DATA_DETACH: if(Curl_ssl->detach_data) { - cf_ctx_set_data(cf, data); + CF_DATA_SAVE(save, cf, data); Curl_ssl->detach_data(cf, data); - cf_ctx_set_data(cf, NULL); + CF_DATA_RESTORE(cf, save); } break; default: @@ -1625,6 +1630,8 @@ static CURLcode ssl_cf_cntrl(struct Curl_cfilter *cf, static bool cf_ssl_is_alive(struct Curl_cfilter *cf, struct Curl_easy *data) { + struct cf_call_data save; + bool result; /* * This function tries to determine connection status. * @@ -1633,7 +1640,10 @@ static bool cf_ssl_is_alive(struct Curl_cfilter *cf, struct Curl_easy *data) * 0 means the connection has been closed * -1 means the connection status is unknown */ - return Curl_ssl->check_cxn(cf, data) != 0; + CF_DATA_SAVE(save, cf, data); + result = Curl_ssl->check_cxn(cf, data) != 0; + CF_DATA_RESTORE(cf, save); + return result; } struct Curl_cftype Curl_cft_ssl = { @@ -1786,9 +1796,10 @@ void *Curl_ssl_get_internals(struct Curl_easy *data, int sockindex, /* get first filter in chain, if any is present */ cf = Curl_ssl_cf_get_ssl(data->conn->cfilter[sockindex]); if(cf) { - cf_ctx_set_data(cf, data); + struct cf_call_data save; + CF_DATA_SAVE(save, cf, data); result = Curl_ssl->get_internals(cf->ctx, info); - cf_ctx_set_data(cf, NULL); + CF_DATA_RESTORE(cf, save); } } return result; diff --git a/lib/vtls/vtls_int.h b/lib/vtls/vtls_int.h index 1d5f415d46..9403173af9 100644 --- a/lib/vtls/vtls_int.h +++ b/lib/vtls/vtls_int.h @@ -37,14 +37,14 @@ struct ssl_connect_data { char *dispname; /* display version of hostname */ int port; /* remote port at origin */ struct ssl_backend_data *backend; /* vtls backend specific props */ - struct Curl_easy *call_data; /* data handle used in current call, - * same as parameter passed, but available - * here for backend internal callbacks - * that need it. NULLed after at the - * end of each vtls filter invcocation. */ + struct cf_call_data call_data; /* data handle used in current call */ }; +#define CF_CTX_CALL_DATA(cf) \ + ((struct ssl_connect_data *)(cf)->ctx)->call_data + + /* Definitions for SSL Implementations */ struct Curl_ssl { diff --git a/lib/vtls/wolfssl.c b/lib/vtls/wolfssl.c index 2d78eafa08..93f3e2c940 100644 --- a/lib/vtls/wolfssl.c +++ b/lib/vtls/wolfssl.c @@ -299,8 +299,7 @@ static long bio_cf_ctrl(WOLFSSL_BIO *bio, int cmd, long num, void *ptr) static int bio_cf_out_write(WOLFSSL_BIO *bio, const char *buf, int blen) { struct Curl_cfilter *cf = wolfSSL_BIO_get_data(bio); - struct ssl_connect_data *connssl = cf->ctx; - struct Curl_easy *data = connssl->call_data; + struct Curl_easy *data = CF_DATA_CURRENT(cf); ssize_t nwritten; CURLcode result = CURLE_OK; @@ -315,8 +314,7 @@ static int bio_cf_out_write(WOLFSSL_BIO *bio, const char *buf, int blen) static int bio_cf_in_read(WOLFSSL_BIO *bio, char *buf, int blen) { struct Curl_cfilter *cf = wolfSSL_BIO_get_data(bio); - struct ssl_connect_data *connssl = cf->ctx; - struct Curl_easy *data = connssl->call_data; + struct Curl_easy *data = CF_DATA_CURRENT(cf); ssize_t nread; CURLcode result = CURLE_OK; -- 2.47.3