Skip to content

Commit

Permalink
Removing attached easy handles from OpenSSL SSL instances. Refs #10150.
Browse files Browse the repository at this point in the history 
    - keeping the "current" easy handle registered at SSL* is no longer
      necessary, since the "calling" data object is already stored in the
      cfilter's context (and used by other SSL backends from there).
    - The "detach" of an easy handle that goes out of scope is then avoided.
    - This could be the cause of the crash reported in #10150.
    - using SSL_set0_wbio for clear reference counting where available.
  • Loading branch information
@icing
icing committed Dec 28, 2022
1 parent 7fa449c commit 60a2329
Show file tree
Hide file tree
Showing 2 changed files with 41 additions and 144 deletions.
7 changes: 7 additions & 0 deletions configure.ac
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4217,6 +4217,13 @@ if test "x$want_ech" != "xno"; then
fi
fi

dnl *************************************************************
dnl check whether OpenSSL (lookalikes) have SSL_set0_wbio
dnl
if test "x$OPENSSL_ENABLED" = "x1"; then
AC_CHECK_FUNCS([SSL_set0_wbio])
fi

dnl *************************************************************
dnl WebSockets
dnl
Expand Down
178 changes: 34 additions & 144 deletions lib/vtls/openssl.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -277,8 +277,6 @@
#endif /* !LIBRESSL_VERSION_NUMBER */

struct ssl_backend_data {
struct Curl_easy *logger; /* transfer handle to pass trace logs to, only
using sockindex 0 */
/* these ones requires specific SSL-types */
SSL_CTX* ctx;
SSL* handle;
Expand Down Expand Up @@ -788,9 +786,6 @@ static void bio_cf_free_methods(void)
#endif


static bool ossl_attach_data(struct Curl_cfilter *cf,
struct Curl_easy *data);

/*
* Number of bytes to read from the random number seed file. This must be
* a finite value (because some entropy "files" like /dev/urandom have
Expand Down Expand Up @@ -922,18 +917,6 @@ static char *ossl_strerror(unsigned long error, char *buf, size_t size)
return buf;
}

/* Return an extra data index for the transfer data.
* This index can be used with SSL_get_ex_data() and SSL_set_ex_data().
*/
static int ossl_get_ssl_data_index(void)
{
static int ssl_ex_data_data_index = -1;
if(ssl_ex_data_data_index < 0) {
ssl_ex_data_data_index = SSL_get_ex_new_index(0, NULL, NULL, NULL, NULL);
}
return ssl_ex_data_data_index;
}

/* Return an extra data index for the associated Curl_cfilter instance.
* This index can be used with SSL_get_ex_data() and SSL_set_ex_data().
*/
Expand All @@ -946,28 +929,20 @@ static int ossl_get_ssl_cf_index(void)
return ssl_ex_data_cf_index;
}

/* Return an extra data index for the sockindex.
* This index can be used with SSL_get_ex_data() and SSL_set_ex_data().
*/
static int ossl_get_ssl_sockindex_index(void)
static bool ossl_attach_cf(struct Curl_cfilter *cf)
{
static int sockindex_index = -1;
if(sockindex_index < 0) {
sockindex_index = SSL_get_ex_new_index(0, NULL, NULL, NULL, NULL);
}
return sockindex_index;
}
struct ssl_connect_data *connssl = cf->ctx;
struct ssl_backend_data *backend = connssl->backend;
int cf_idx = ossl_get_ssl_cf_index();

/* Return an extra data index for proxy boolean.
* This index can be used with SSL_get_ex_data() and SSL_set_ex_data().
*/
static int ossl_get_proxy_index(void)
{
static int proxy_index = -1;
if(proxy_index < 0) {
proxy_index = SSL_get_ex_new_index(0, NULL, NULL, NULL, NULL);
DEBUGASSERT(backend);

/* If we don't have SSL context, do nothing. */
if(backend->handle && cf_idx >= 0) {
if(SSL_set_ex_data(backend->handle, cf_idx, cf))
return TRUE;
}
return proxy_index;
return FALSE;
}

static int passwd_callback(char *buf, int num, int encrypting,
Expand Down Expand Up @@ -1772,8 +1747,7 @@ static int ossl_init(void)
Curl_tls_keylog_open();

/* Initialize the extra data indexes */
if(ossl_get_ssl_data_index() < 0 || ossl_get_ssl_cf_index() < 0 ||
ossl_get_ssl_sockindex_index() < 0 || ossl_get_proxy_index() < 0)
if(ossl_get_ssl_cf_index() < 0)
return 0;

return 1;
Expand Down Expand Up @@ -1960,19 +1934,15 @@ static struct curl_slist *ossl_engines_list(struct Curl_easy *data)
return list;
}

#define set_logger(connssl, data) \
connssl->backend->logger = data

static void ossl_close(struct Curl_cfilter *cf, struct Curl_easy *data)
{
struct ssl_connect_data *connssl = cf->ctx;
struct ssl_backend_data *backend = connssl->backend;

(void)data;
DEBUGASSERT(backend);

if(backend->handle) {
set_logger(connssl, data);

if(cf->next && cf->next->connected) {
char buf[32];
/* Maybe the server has already sent a close notify alert.
Expand Down Expand Up @@ -2674,7 +2644,7 @@ static void ossl_trace(int direction, int ssl_ver, int content_type,
connssl = cf->ctx;
DEBUGASSERT(connssl);
DEBUGASSERT(connssl->backend);
data = connssl->backend->logger;
data = connssl->call_data;

if(!conn || !data || !data->set.fdebug ||
(direction != 0 && direction != 1))
Expand Down Expand Up @@ -2967,21 +2937,17 @@ static int ossl_new_session_cb(SSL *ssl, SSL_SESSION *ssl_sessionid)
struct Curl_easy *data;
struct Curl_cfilter *cf;
const struct ssl_config_data *config;
curl_socket_t *sockindex_ptr;
int data_idx = ossl_get_ssl_data_index();
int cf_idx = ossl_get_ssl_cf_index();
int sockindex_idx = ossl_get_ssl_sockindex_index();
int proxy_idx = ossl_get_proxy_index();
struct ssl_connect_data *connssl;
bool isproxy;

if(data_idx < 0 || cf_idx < 0 || sockindex_idx < 0 || proxy_idx < 0)
if(cf_idx < 0)
return 0;

cf = (struct Curl_cfilter*) SSL_get_ex_data(ssl, cf_idx);
data = (struct Curl_easy *) SSL_get_ex_data(ssl, data_idx);
connssl = cf? cf->ctx : NULL;
data = connssl? connssl->call_data : NULL;
/* The sockindex has been stored as a pointer to an array element */
sockindex_ptr = (curl_socket_t*) SSL_get_ex_data(ssl, sockindex_idx);
if(!cf || !data || !sockindex_ptr)
if(!cf || !data)
return 0;

isproxy = Curl_ssl_cf_is_proxy(cf);
Expand Down Expand Up @@ -3565,7 +3531,6 @@ static CURLcode ossl_connect_step1(struct Curl_cfilter *cf,
/* the SSL trace callback is only used for verbose logging */
SSL_CTX_set_msg_callback(backend->ctx, ossl_trace);
SSL_CTX_set_msg_callback_arg(backend->ctx, cf->conn);
set_logger(connssl, data);
}
#endif

Expand Down Expand Up @@ -3847,9 +3812,9 @@ static CURLcode ossl_connect_step1(struct Curl_cfilter *cf,
}
#endif

if(!ossl_attach_data(cf, data)) {
if(!ossl_attach_cf(cf)) {
/* Maybe the internal errors of SSL_get_ex_new_index or SSL_set_ex_data */
failf(data, "SSL: ossl_attach_data failed: %s",
failf(data, "SSL: ossl_attach_cf failed: %s",
ossl_strerror(ERR_get_error(), error_buffer,
sizeof(error_buffer)));
return CURLE_SSL_CONNECT_ERROR;
Expand Down Expand Up @@ -3877,8 +3842,18 @@ static CURLcode ossl_connect_step1(struct Curl_cfilter *cf,
return CURLE_OUT_OF_MEMORY;

BIO_set_data(bio, cf);
#ifdef HAVE_SSL_SET0_WBIO
/* with OpenSSL v1.1.1 we get an alternative to SSL_set_bio() that works
* without backward compat quirks. Every call takes one reference, so we
* up it and pass. SSL* then owns it and will free.
* We check on the function in configure, since libressl and friends
* each have their own versions to add support for this. */
BIO_up_ref(bio);
SSL_set0_rbio(backend->handle, bio);
SSL_set0_wbio(backend->handle, bio);
#else
SSL_set_bio(backend->handle, bio, bio);

#endif
connssl->connecting_state = ssl_connect_2;

return CURLE_OK;
Expand Down Expand Up @@ -4523,7 +4498,6 @@ static ssize_t ossl_send(struct Curl_cfilter *cf,
ERR_clear_error();

memlen = (len > (size_t)INT_MAX) ? INT_MAX : (int)len;
set_logger(connssl, data);
rc = SSL_write(backend->handle, mem, memlen);

if(rc <= 0) {
Expand Down Expand Up @@ -4620,7 +4594,6 @@ static ssize_t ossl_recv(struct Curl_cfilter *cf,
ERR_clear_error();

buffsize = (buffersize > (size_t)INT_MAX) ? INT_MAX : (int)buffersize;
set_logger(connssl, data);
nread = (ssize_t)SSL_read(backend->handle, buf, buffsize);

if(nread <= 0) {
Expand Down Expand Up @@ -4838,89 +4811,6 @@ static void *ossl_get_internals(struct ssl_connect_data *connssl,
(void *)backend->ctx : (void *)backend->handle;
}

static bool ossl_attach_data(struct Curl_cfilter *cf,
struct Curl_easy *data)
{
struct ssl_connect_data *connssl = cf->ctx;
struct ssl_backend_data *backend = connssl->backend;
const struct ssl_config_data *config;

DEBUGASSERT(backend);

/* If we don't have SSL context, do nothing. */
if(!backend->handle)
return FALSE;

config = Curl_ssl_cf_get_config(cf, data);
if(config->primary.sessionid) {
int data_idx = ossl_get_ssl_data_index();
int cf_idx = ossl_get_ssl_cf_index();
int sockindex_idx = ossl_get_ssl_sockindex_index();
int proxy_idx = ossl_get_proxy_index();

if(data_idx >= 0 && cf_idx >= 0 && sockindex_idx >= 0 &&
proxy_idx >= 0) {
int data_status, cf_status, sockindex_status, proxy_status;

/* Store the data needed for the "new session" callback.
* The sockindex is stored as a pointer to an array element. */
data_status = SSL_set_ex_data(backend->handle, data_idx, data);
cf_status = SSL_set_ex_data(backend->handle, cf_idx, cf);
sockindex_status = SSL_set_ex_data(backend->handle, sockindex_idx,
cf->conn->sock + cf->sockindex);
#ifndef CURL_DISABLE_PROXY
proxy_status = SSL_set_ex_data(backend->handle, proxy_idx,
Curl_ssl_cf_is_proxy(cf)?
(void *) 1 : NULL);
#else
proxy_status = SSL_set_ex_data(backend->handle, proxy_idx, NULL);
#endif
if(data_status && cf_status && sockindex_status && proxy_status)
return TRUE;
}
return FALSE;
}
return TRUE;
}

/*
* Starting with TLS 1.3, the ossl_new_session_cb callback gets called after
* the handshake. If the transfer that sets up the callback gets killed before
* this callback arrives, we must make sure to properly clear the data to
* avoid UAF problems. A future optimization could be to instead store another
* transfer that might still be using the same connection.
*/

static void ossl_detach_data(struct Curl_cfilter *cf,
struct Curl_easy *data)
{
struct ssl_config_data *ssl_config = Curl_ssl_cf_get_config(cf, data);
struct ssl_connect_data *connssl = cf->ctx;
struct ssl_backend_data *backend = connssl->backend;
DEBUGASSERT(backend);

/* If we don't have SSL context, do nothing. */
if(!backend->handle)
return;

if(ssl_config->primary.sessionid) {
int data_idx = ossl_get_ssl_data_index();
int cf_idx = ossl_get_ssl_cf_index();
int sockindex_idx = ossl_get_ssl_sockindex_index();
int proxy_idx = ossl_get_proxy_index();

if(data_idx >= 0 && cf_idx >= 0 && sockindex_idx >= 0 &&
proxy_idx >= 0) {
/* Disable references to data in "new session" callback to avoid
* accessing a stale pointer. */
SSL_set_ex_data(backend->handle, data_idx, NULL);
SSL_set_ex_data(backend->handle, cf_idx, NULL);
SSL_set_ex_data(backend->handle, sockindex_idx, NULL);
SSL_set_ex_data(backend->handle, proxy_idx, NULL);
}
}
}

static void ossl_free_multi_ssl_backend_data(
struct multi_ssl_backend_data *mbackend)
{
Expand Down Expand Up @@ -4974,8 +4864,8 @@ const struct Curl_ssl Curl_ssl_openssl = {
#else
NULL, /* sha256sum */
#endif
ossl_attach_data, /* use of data in this connection */
ossl_detach_data, /* remote of data from this connection */
NULL, /* use of data in this connection */
NULL, /* remote of data from this connection */
ossl_free_multi_ssl_backend_data, /* free_multi_ssl_backend_data */
ossl_recv, /* recv decrypted data */
ossl_send, /* send data to encrypt */
Expand Down

0 comments on commit 60a2329

Please sign in to comment.