From 1c5418bf8d05e82576677e7f3d559f7aac75698c Mon Sep 17 00:00:00 2001 From: Jakub Zelenka Date: Thu, 16 Jul 2026 19:32:45 +0200 Subject: [PATCH] Destroy stored imported public keys on free When an imported public key is used in a token operation, the provider stores it on the token as a session object. The object was never destroyed, so every unique imported public key leaked one object on the long lived login session. Mark objects stored by the provider and destroy their handle when the object is freed. Objects created from a pool copy borrow the handle and keep a reference on the owning object, so the stored object is only destroyed when the last object using it goes away. Refreshing an imported object now stores the key again instead of searching the token for an object that cannot be found there. Fixes #739 Signed-off-by: Jakub Zelenka --- src/obj/internal.h | 2 + src/obj/object.c | 58 +++++++++++ src/obj/pool.c | 5 + src/obj/store.c | 9 +- tests/meson.build | 1 + tests/setup.sh | 1 + tests/timported | 4 + tests/tstorepubkeyfree.c | 208 +++++++++++++++++++++++++++++++++++++++ 8 files changed, 287 insertions(+), 1 deletion(-) create mode 100644 tests/tstorepubkeyfree.c diff --git a/src/obj/internal.h b/src/obj/internal.h index 909661a6..f10c2d53 100644 --- a/src/obj/internal.h +++ b/src/obj/internal.h @@ -23,6 +23,7 @@ struct p11prov_crt { struct p11prov_obj { P11PROV_CTX *ctx; bool raf; /* re-init after fork */ + bool imported; CK_SLOT_ID slotid; CK_OBJECT_HANDLE handle; @@ -46,6 +47,7 @@ struct p11prov_obj { int poolid; P11PROV_OBJ *assoc_obj; + P11PROV_OBJ *handle_owner; P11PROV_SESSION *ref_session; }; diff --git a/src/obj/object.c b/src/obj/object.c index cfcacc3c..477e0dbb 100644 --- a/src/obj/object.c +++ b/src/obj/object.c @@ -55,6 +55,39 @@ static void destroy_key_cache(P11PROV_OBJ *obj, P11PROV_SESSION *session) } } +static void destroy_imported_key(P11PROV_OBJ *obj) +{ + P11PROV_SESSION *session = NULL; + CK_RV ret; + + if (!obj->imported) { + return; + } + /* nothing to destroy if the key was never stored or was reset */ + if (obj->handle == CK_INVALID_HANDLE + || obj->handle == CK_P11PROV_IMPORTED_HANDLE) { + return; + } + + ret = p11prov_try_session_ref(obj, CK_UNAVAILABLE_INFORMATION, false, false, + &session); + if (ret != CKR_OK) { + P11PROV_debug("Failed to get session to destroy imported key. " + "Error %lx", + ret); + return; + } + + ret = p11prov_DestroyObject(obj->ctx, p11prov_session_handle(session), + obj->handle); + if (ret != CKR_OK) { + P11PROV_debug("Failed to destroy imported key. Error %lx", ret); + } + obj->handle = CK_INVALID_HANDLE; + + p11prov_return_session(session); +} + static void cache_key(P11PROV_OBJ *obj) { P11PROV_SESSION *session = NULL; @@ -147,6 +180,25 @@ static void p11prov_obj_refresh(P11PROV_OBJ *obj) P11PROV_debug("Refresh object %p", obj); + if (obj->handle_owner) { + /* re-borrow the handle from the owner */ + obj->handle = p11prov_obj_get_handle(obj->handle_owner); + obj->raf = false; + return; + } + + if (obj->imported) { + /* the key is not on the token so it needs to be stored again */ + ret = p11prov_obj_store_public_key(obj); + if (ret != CKR_OK) { + P11PROV_raise(obj->ctx, ret, "Failed to refresh imported object %p", + obj); + return; + } + obj->raf = false; + return; + } + if (obj->class == CKO_PRIVATE_KEY || obj->class == CKO_SECRET_KEY) { login = true; } @@ -291,6 +343,8 @@ void p11prov_obj_free(P11PROV_OBJ *obj) return; } + destroy_imported_key(obj); + if (obj->ref_session) { p11prov_session_deref(obj->ref_session); obj->ref_session = NULL; @@ -309,6 +363,7 @@ void p11prov_obj_free(P11PROV_OBJ *obj) p11prov_uri_free(obj->refresh_uri); p11prov_obj_free(obj->assoc_obj); + p11prov_obj_free(obj->handle_owner); OPENSSL_clear_free(obj, sizeof(P11PROV_OBJ)); } @@ -637,6 +692,9 @@ void p11prov_obj_set_associated(P11PROV_OBJ *obj, P11PROV_OBJ *assoc) void p11prov_obj_set_session_ref(P11PROV_OBJ *obj, P11PROV_SESSION *session) { p11prov_session_ref(session); + if (obj->ref_session) { + p11prov_session_deref(obj->ref_session); + } obj->ref_session = session; } diff --git a/src/obj/pool.c b/src/obj/pool.c index b645d317..51cb905b 100644 --- a/src/obj/pool.c +++ b/src/obj/pool.c @@ -102,6 +102,11 @@ CK_RV obj_add_to_pool(P11PROV_OBJ *obj) P11PROV_OBJ_POOL *pool; CK_RV ret; + if (obj->poolid != -1) { + /* already in the pool */ + return CKR_OK; + } + ret = p11prov_slot_get_obj_pool(obj->ctx, obj->slotid, &pool); if (ret != CKR_OK) { return ret; diff --git a/src/obj/store.c b/src/obj/store.c index 1b9feb55..acc41ed3 100644 --- a/src/obj/store.c +++ b/src/obj/store.c @@ -659,7 +659,12 @@ CK_RV p11prov_obj_copy_key_data(P11PROV_OBJ *dst, P11PROV_OBJ *src) } dst->slotid = src->slotid; - dst->handle = src->handle; + dst->handle = p11prov_obj_get_handle(src); + if (src->handle_owner || src->imported) { + /* keep the owner alive as the handle is destroyed on its free */ + dst->handle_owner = p11prov_obj_ref_no_cache( + src->handle_owner ? src->handle_owner : src); + } dst->class = src->class; dst->cka_copyable = src->cka_copyable; dst->cka_token = src->cka_token; @@ -1265,6 +1270,8 @@ CK_RV p11prov_obj_store_public_key(P11PROV_OBJ *key) } if (rv == CKR_OK) { + /* destroy the handle when the object is freed */ + key->imported = true; /* this is a real object now, add it to the pool, but do not * fail if the operation goes haywire for some reason */ (void)obj_add_to_pool(key); diff --git a/tests/meson.build b/tests/meson.build index cb824718..b64235cd 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -106,6 +106,7 @@ test_programs = { 'tcachekeys': ['tcachekeys.c', 'util.c'], 'tfork': ['tfork.c', 'util.c'], 'tstorepubkey': ['tstorepubkey.c', 'util.c'], + 'tstorepubkeyfree': ['tstorepubkeyfree.c', 'util.c'], 'tpkey': ['tpkey.c', 'util.c'], 'pincache': ['pincache.c'], 'ccerts': ['ccerts.c', 'util.c'], diff --git a/tests/setup.sh b/tests/setup.sh index 389ea33b..dbea492d 100755 --- a/tests/setup.sh +++ b/tests/setup.sh @@ -653,6 +653,7 @@ ${TOKENCONFIGVARS} export P11LIB="${P11LIB}" export TOKENTYPE="${TOKENTYPE}" export TOKENLABEL="${TOKENLABEL}" +export TOKENLABELURI="${TOKENLABELURI}" export PKCS11_PROVIDER_MODULE=${P11LIB} export PPDBGFILE=${TMPPDIR}/p11prov-debug.log export PKCS11_PROVIDER_DEBUG="file:${TMPPDIR}/p11prov-debug.log" diff --git a/tests/timported b/tests/timported index 53597efb..aefbfdb3 100755 --- a/tests/timported +++ b/tests/timported @@ -185,4 +185,8 @@ fi OPENSSL_CONF=${ORIG_OPENSSL_CONF} +title PARA "Test stored pub key object is freed" +"${TESTBLDDIR}/tstorepubkeyfree" "${PRIURI}" "${PUBURI}" \ + "pkcs11:token=${TOKENLABELURI};type=public" + exit 0 diff --git a/tests/tstorepubkeyfree.c b/tests/tstorepubkeyfree.c new file mode 100644 index 00000000..6222281b --- /dev/null +++ b/tests/tstorepubkeyfree.c @@ -0,0 +1,208 @@ +/* Copyright (C) 2026 Jakub Zelenka + SPDX-License-Identifier: Apache-2.0 */ + +/* Regression test for imported public keys leaking token session objects. + * + * When an imported public key is used in a token operation, the provider + * stores it on the token as a session object (C_CreateObject). That object + * must be destroyed again when the last key using it is freed, otherwise + * every unique imported public key leaks one object on the long lived login + * session. + * + * The test counts the public key objects matching a token URI, imports a + * public key twice and uses it to force the provider to store it on the + * token (count goes up by one and the copy shares the stored object), then + * frees the keys and verifies the stored object is gone (count drops back). */ + +#define _GNU_SOURCE +#include +#include +#include +#include +#include +#include +#include +#include "util.h" + +static unsigned char tbs[32] = { 0 }; +static unsigned char sig[1024]; +static size_t siglen; + +/* Count the number of objects matching the URI. */ +static int count_matches(const char *uri) +{ + OSSL_STORE_CTX *store; + OSSL_STORE_INFO *info; + int count = 0; + + store = OSSL_STORE_open(uri, NULL, NULL, NULL, NULL); + if (store == NULL) { + fprintf(stderr, "Failed to open store: %s\n", uri); + ossl_err_print(); + exit(EXIT_FAILURE); + } + + for (info = OSSL_STORE_load(store); info != NULL; + info = OSSL_STORE_load(store)) { + count++; + OSSL_STORE_INFO_free(info); + } + + OSSL_STORE_close(store); + return count; +} + +static void check_count(const char *uri, int expected, const char *when) +{ + int count = count_matches(uri); + + if (count != expected) { + fprintf(stderr, "Expected %d matches %s, got %d for %s\n", expected, + when, count, uri); + exit(EXIT_FAILURE); + } +} + +static void sign_data(const char *uri) +{ + EVP_PKEY *key; + EVP_PKEY_CTX *ctx; + + key = load_key_ex(uri, "provider=pkcs11"); + if (!key) { + exit(EXIT_FAILURE); + } + + ctx = EVP_PKEY_CTX_new_from_pkey(NULL, key, "provider=pkcs11"); + if (ctx == NULL) { + PRINTERROSSL("Failed to create signature context\n"); + exit(EXIT_FAILURE); + } + if (EVP_PKEY_sign_init(ctx) != 1) { + PRINTERROSSL("Failed to init signature\n"); + exit(EXIT_FAILURE); + } + siglen = sizeof(sig); + if (EVP_PKEY_sign(ctx, sig, &siglen, tbs, sizeof(tbs)) != 1) { + PRINTERROSSL("Failed to sign\n"); + exit(EXIT_FAILURE); + } + EVP_PKEY_CTX_free(ctx); + EVP_PKEY_free(key); +} + +/* The verification forces the imported key to be stored on the token. */ +static void verify_data(EVP_PKEY *key) +{ + EVP_PKEY_CTX *ctx; + + ctx = EVP_PKEY_CTX_new_from_pkey(NULL, key, "provider=pkcs11"); + if (ctx == NULL) { + PRINTERROSSL("Failed to create verification context\n"); + exit(EXIT_FAILURE); + } + if (EVP_PKEY_verify_init(ctx) != 1) { + PRINTERROSSL("Failed to init verification\n"); + exit(EXIT_FAILURE); + } + if (EVP_PKEY_verify(ctx, sig, siglen, tbs, sizeof(tbs)) != 1) { + PRINTERROSSL("Failed to verify\n"); + exit(EXIT_FAILURE); + } + EVP_PKEY_CTX_free(ctx); +} + +static EVP_PKEY *import_pubkey(const char *type_name, OSSL_PARAM *params) +{ + EVP_PKEY_CTX *pctx; + EVP_PKEY *pubkey = NULL; + int ret; + + pctx = EVP_PKEY_CTX_new_from_name(NULL, type_name, "provider=pkcs11"); + if (!pctx) { + PRINTERROSSL("Failed to create fromdata ctx\n"); + exit(EXIT_FAILURE); + } + + ret = EVP_PKEY_fromdata_init(pctx); + if (ret != 1) { + PRINTERROSSL("Failed to init fromdata\n"); + exit(EXIT_FAILURE); + } + + ret = EVP_PKEY_fromdata(pctx, &pubkey, EVP_PKEY_PUBLIC_KEY, params); + if (ret != 1) { + PRINTERROSSL("Failed to import key via fromdata\n"); + exit(EXIT_FAILURE); + } + + EVP_PKEY_CTX_free(pctx); + return pubkey; +} + +int main(int argc, char *argv[]) +{ + EVP_PKEY *pubkey_main, *pubkey1, *pubkey2; + OSSL_PARAM *params = NULL; + char *type_name; + int before; + int ret; + + if (argc != 4) { + fprintf(stderr, "Usage: %s \n", argv[0]); + exit(EXIT_FAILURE); + } + + const char *privkey_uri = argv[1]; + const char *pubkey_uri = argv[2]; + const char *match_uri = argv[3]; + + OPENSSL_init_crypto(OPENSSL_INIT_LOAD_CONFIG, NULL); + + sign_data(privkey_uri); + + pubkey_main = load_key_ex(pubkey_uri, "provider=pkcs11"); + if (!pubkey_main) { + exit(EXIT_FAILURE); + } + + ret = EVP_PKEY_todata(pubkey_main, EVP_PKEY_PUBLIC_KEY, ¶ms); + if (ret != 1) { + PRINTERROSSL("Failed to export key params\n"); + exit(EXIT_FAILURE); + } + + type_name = OPENSSL_strdup(EVP_PKEY_get0_type_name(pubkey_main)); + if (!type_name) { + fprintf(stderr, "Failed to copy key type name\n"); + exit(EXIT_FAILURE); + } + /* free the token key so the import below cannot just reuse it */ + EVP_PKEY_free(pubkey_main); + + before = count_matches(match_uri); + + pubkey1 = import_pubkey(type_name, params); + verify_data(pubkey1); + check_count(match_uri, before + 1, "after the first imported key is used"); + + /* the second import shares the object stored by the first one */ + pubkey2 = import_pubkey(type_name, params); + verify_data(pubkey2); + check_count(match_uri, before + 1, "after the second imported key is used"); + + /* the stored object is kept until all keys using it are freed */ + EVP_PKEY_free(pubkey1); + verify_data(pubkey2); + check_count(match_uri, before + 1, "after the first imported key is freed"); + + EVP_PKEY_free(pubkey2); + check_count(match_uri, before, "after all imported keys are freed"); + + OSSL_PARAM_free(params); + OPENSSL_free(type_name); + + fprintf(stderr, "ALL A-OK\n"); + + exit(EXIT_SUCCESS); +}