From 2f1f1219f82fb92e9e4eff9fc7ef14176bc69b41 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Kuzn=C3=ADk?= Date: Mon, 22 Jul 2024 16:16:25 +0100 Subject: [PATCH] Fix races in sasl_{client,server}_init() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Ondřej Kuzník --- lib/client.c | 23 +++++++++++++++-------- lib/common.c | 10 +++++++--- lib/saslint.h | 3 ++- lib/server.c | 30 ++++++++++++++++-------------- 4 files changed, 40 insertions(+), 26 deletions(-) diff --git a/lib/client.c b/lib/client.c index 03a2efcc..3035fb2c 100644 --- a/lib/client.c +++ b/lib/client.c @@ -279,6 +279,7 @@ client_idle(sasl_conn_t *conn) int sasl_client_init(const sasl_callback_t *callbacks) { int ret; + void *mutex = NULL; const add_plugin_list_t ep_list[] = { { "sasl_client_plug_init", (add_plugin_t *)sasl_client_add_plugin }, { "sasl_canonuser_init", (add_plugin_t *)sasl_canonuser_add_plugin }, @@ -287,22 +288,30 @@ int sasl_client_init(const sasl_callback_t *callbacks) /* lock allocation type */ _sasl_allocation_locked++; - - if(_sasl_client_active) { + + global_callbacks_client.callbacks = callbacks; + global_callbacks_client.appname = NULL; + + ret = _sasl_common_init(&global_callbacks_client, &mutex); + if (ret != SASL_OK) return ret; + + if (_sasl_client_active) { /* We're already active, just increase our refcount */ /* xxx do something with the callback structure? */ _sasl_client_active++; + sasl_MUTEX_UNLOCK(mutex); return SASL_OK; } - global_callbacks_client.callbacks = callbacks; - global_callbacks_client.appname = NULL; - cmechlist=sasl_ALLOC(sizeof(cmech_list_t)); - if (cmechlist==NULL) return SASL_NOMEM; + if (cmechlist==NULL) { + sasl_MUTEX_UNLOCK(mutex); + return SASL_NOMEM; + } /* We need to call client_done if we fail now */ _sasl_client_active = 1; + sasl_MUTEX_UNLOCK(mutex); /* load plugins */ ret=init_mechlist(); @@ -313,8 +322,6 @@ int sasl_client_init(const sasl_callback_t *callbacks) sasl_client_add_plugin("EXTERNAL", &external_client_plug_init); - ret = _sasl_common_init(&global_callbacks_client); - if (ret == SASL_OK) ret = _sasl_load_plugins(ep_list, _sasl_find_getpath_callback(callbacks), diff --git a/lib/common.c b/lib/common.c index b9c8bf50..c4b21a67 100644 --- a/lib/common.c +++ b/lib/common.c @@ -848,7 +848,7 @@ int _sasl_conn_init(sasl_conn_t *conn, RETURN(conn, SASL_OK); } -int _sasl_common_init(sasl_global_callbacks_t *global_callbacks) +int _sasl_common_init(sasl_global_callbacks_t *global_callbacks, void **mutex) { int result; @@ -868,7 +868,7 @@ int _sasl_common_init(sasl_global_callbacks_t *global_callbacks) global_utils->getopt = &_sasl_global_getopt; global_utils->getopt_context = global_callbacks; - sasl_MUTEX_UNLOCK(free_mutex); + *mutex = free_mutex; return SASL_OK; } else { sasl_global_utils = _sasl_alloc_utils(NULL, global_callbacks); @@ -884,7 +884,11 @@ int _sasl_common_init(sasl_global_callbacks_t *global_callbacks) result = SASL_FAIL; } - sasl_MUTEX_UNLOCK(free_mutex); + if (result == SASL_OK) { + *mutex = free_mutex; + } else { + sasl_MUTEX_UNLOCK(free_mutex); + } return result; } diff --git a/lib/saslint.h b/lib/saslint.h index ebade78f..9ddf4c21 100644 --- a/lib/saslint.h +++ b/lib/saslint.h @@ -384,7 +384,8 @@ _sasl_find_getconfpath_callback(const sasl_callback_t *callbacks); extern const sasl_callback_t * _sasl_find_verifyfile_callback(const sasl_callback_t *callbacks); -extern int _sasl_common_init(sasl_global_callbacks_t *global_callbacks); +extern int _sasl_common_init(sasl_global_callbacks_t *global_callbacks, + void **mutex); extern int _sasl_conn_init(sasl_conn_t *conn, const char *service, diff --git a/lib/server.c b/lib/server.c index c69e58b8..927cb708 100644 --- a/lib/server.c +++ b/lib/server.c @@ -826,6 +826,7 @@ int sasl_server_init(const sasl_callback_t *callbacks, int ret; const sasl_callback_t *vf; const char *pluginfile = NULL; + void *mutex = NULL; #ifdef PIC sasl_getopt_t *getopt; void *context; @@ -845,32 +846,33 @@ int sasl_server_init(const sasl_callback_t *callbacks, if (appname != NULL && strlen(appname) >= PATH_MAX) return SASL_BADPARAM; - if (_sasl_server_active) { - /* We're already active, just increase our refcount */ - /* xxx do something with the callback structure? */ - _sasl_server_active++; - return SASL_OK; - } - - ret = _sasl_common_init(&global_callbacks); - if (ret != SASL_OK) - return ret; - /* verify that the callbacks look ok */ ret = verify_server_callbacks(callbacks); - if (ret != SASL_OK) + if (ret != SASL_OK) { + sasl_MUTEX_UNLOCK(mutex); return ret; + } global_callbacks.callbacks = callbacks; - + /* A shared library calling sasl_server_init will pass NULL as appname. This should retain the original appname. */ if (appname != NULL) { global_callbacks.appname = appname; } + ret = _sasl_common_init(&global_callbacks, &mutex); + if (ret != SASL_OK) return ret; + + if (_sasl_server_active++) { + /* We're already active */ + /* xxx do something with the callback structure? */ + sasl_MUTEX_UNLOCK(mutex); + return SASL_OK; + } + /* If we fail now, we have to call server_done */ - _sasl_server_active = 1; + sasl_MUTEX_UNLOCK(mutex); /* allocate mechlist and set it to empty */ mechlist = sasl_ALLOC(sizeof(mech_list_t));