From 4d843d3da8e33fb80c35fd7b34e7b13587638e9d Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Wed, 8 Jul 2026 22:21:17 +0200 Subject: [PATCH] pkcs11: Fix memory leak in the hasher_t implementation If state was stored (for incremental hashing), the allocated memory was leaked. Added some limits for the allocation and use chunk_t to simplify it. The state is now also wiped just to be safe. Also removed the useless mutex. If the goal was to protect access to the hasher from multiple threads, then no other hasher currently implements such protection. And if the idea was to serialize access to the token (i.e. only allow a single hasher to concurrently load its state into the token and update it), then a per-hasher mutex was not the right approach. If that was the reason, we'd need a token-level mutex that all hashers shared. Should `get_hash()` fail due to such an issue with a transient error (e.g. `CKR_DEVICE_MEMORY`), we now at least don't leak memory from `allocate_hash()`. Fixes: 6e4f4d2fdfca ("Save/Load state of PKCS#11 hasher") --- .../plugins/pkcs11/pkcs11_hasher.c | 96 ++++++++++--------- 1 file changed, 53 insertions(+), 43 deletions(-) diff --git a/src/libstrongswan/plugins/pkcs11/pkcs11_hasher.c b/src/libstrongswan/plugins/pkcs11/pkcs11_hasher.c index 409a05a2a..f50fb04ae 100644 --- a/src/libstrongswan/plugins/pkcs11/pkcs11_hasher.c +++ b/src/libstrongswan/plugins/pkcs11/pkcs11_hasher.c @@ -16,15 +16,18 @@ #include "pkcs11_hasher.h" -#include - #include -#include #include "pkcs11_manager.h" typedef struct private_pkcs11_hasher_t private_pkcs11_hasher_t; +/** + * Some limits for allocating state + */ +#define STATE_SIZE_MAX 1024 +#define STATE_RETRIES_MAX 3 + /** * Private data of an pkcs11_hasher_t object. */ @@ -51,29 +54,19 @@ struct private_pkcs11_hasher_t { CK_SESSION_HANDLE session; /** - * size of the hash + * Size of the hash */ size_t size; /** - * Mutex to lock the tokens hashing engine + * Data currently in the state buffer */ - mutex_t *mutex; + size_t have_state; /** - * do we have an initialized state? + * State buffer */ - bool have_state; - - /** - * state buffer - */ - CK_BYTE_PTR state; - - /** - * Length of the state buffer - */ - CK_ULONG state_len; + chunk_t state; }; METHOD(hasher_t, get_hash_size, size_t, @@ -82,35 +75,55 @@ METHOD(hasher_t, get_hash_size, size_t, return this->size; } +/** + * Clear the saved state + */ +static void clear_state(private_pkcs11_hasher_t *this) +{ + chunk_clear(&this->state); + this->have_state = 0; +} + /** * Save the Operation state to host memory */ static bool save_state(private_pkcs11_hasher_t *this) { CK_RV rv; + CK_ULONG state_len = this->state.len; + int retries = 0; while (TRUE) { - if (!this->state) + if (!this->state.ptr) { rv = this->lib->f->C_GetOperationState(this->session, NULL, - &this->state_len); + &state_len); if (rv != CKR_OK) { break; } - this->state = malloc(this->state_len); + if (state_len > STATE_SIZE_MAX) + { + rv = CKR_HOST_MEMORY; + break; + } + this->state = chunk_alloc(state_len); } - rv = this->lib->f->C_GetOperationState(this->session, this->state, - &this->state_len); + rv = this->lib->f->C_GetOperationState(this->session, this->state.ptr, + &state_len); switch (rv) { case CKR_BUFFER_TOO_SMALL: - free(this->state); - this->state = NULL; + if (++retries >= STATE_RETRIES_MAX || state_len > STATE_SIZE_MAX) + { + rv = CKR_HOST_MEMORY; + break; + } + clear_state(this); continue; case CKR_OK: - this->have_state = TRUE; + this->have_state = state_len; return TRUE; default: break; @@ -118,6 +131,7 @@ static bool save_state(private_pkcs11_hasher_t *this) break; } DBG1(DBG_CFG, "C_GetOperationState() failed: %N", ck_rv_names, rv); + clear_state(this); return FALSE; } @@ -128,21 +142,22 @@ static bool load_state(private_pkcs11_hasher_t *this) { CK_RV rv; - rv = this->lib->f->C_SetOperationState(this->session, this->state, - this->state_len, CK_INVALID_HANDLE, CK_INVALID_HANDLE); + rv = this->lib->f->C_SetOperationState(this->session, this->state.ptr, + this->have_state, CK_INVALID_HANDLE, CK_INVALID_HANDLE); if (rv != CKR_OK) { DBG1(DBG_CFG, "C_SetOperationState() failed: %N", ck_rv_names, rv); return FALSE; } - this->have_state = FALSE; + this->have_state = 0; return TRUE; } METHOD(hasher_t, reset, bool, private_pkcs11_hasher_t *this) { - this->have_state = FALSE; + /* just reuse the existing buffer if any and sized correctly */ + this->have_state = 0; return TRUE; } @@ -152,12 +167,10 @@ METHOD(hasher_t, get_hash, bool, CK_RV rv; CK_ULONG len; - this->mutex->lock(this->mutex); if (this->have_state) { if (!load_state(this)) { - this->mutex->unlock(this->mutex); return FALSE; } } @@ -167,7 +180,6 @@ METHOD(hasher_t, get_hash, bool, if (rv != CKR_OK) { DBG1(DBG_CFG, "C_DigestInit() failed: %N", ck_rv_names, rv); - this->mutex->unlock(this->mutex); return FALSE; } } @@ -177,19 +189,16 @@ METHOD(hasher_t, get_hash, bool, if (rv != CKR_OK) { DBG1(DBG_CFG, "C_DigestUpdate() failed: %N", ck_rv_names, rv); - this->mutex->unlock(this->mutex); return FALSE; } } if (hash) { len = this->size; - rv = this->lib->f->C_DigestFinal(this->session, - hash, &len); + rv = this->lib->f->C_DigestFinal(this->session, hash, &len); if (rv != CKR_OK) { DBG1(DBG_CFG, "C_DigestFinal() failed: %N", ck_rv_names, rv); - this->mutex->unlock(this->mutex); return FALSE; } } @@ -197,11 +206,9 @@ METHOD(hasher_t, get_hash, bool, { if (!save_state(this)) { - this->mutex->unlock(this->mutex); return FALSE; } } - this->mutex->unlock(this->mutex); return TRUE; } @@ -211,7 +218,12 @@ METHOD(hasher_t, allocate_hash, bool, if (hash) { *hash = chunk_alloc(this->size); - return get_hash(this, chunk, hash->ptr); + if (!get_hash(this, chunk, hash->ptr)) + { + chunk_free(hash); + return FALSE; + } + return TRUE; } return get_hash(this, chunk, NULL); } @@ -220,7 +232,7 @@ METHOD(hasher_t, destroy, void, private_pkcs11_hasher_t *this) { this->lib->f->C_CloseSession(this->session); - this->mutex->destroy(this->mutex); + clear_state(this); free(this); } @@ -320,13 +332,11 @@ pkcs11_hasher_t *pkcs11_hasher_create(hash_algorithm_t algo) .destroy = _destroy, }, }, - .mutex = mutex_create(MUTEX_TYPE_DEFAULT), ); this->lib = find_token(algo, &this->session, &this->mech, &this->size); if (!this->lib) { - this->mutex->destroy(this->mutex); free(this); return NULL; }