-
Notifications
You must be signed in to change notification settings - Fork 74
Destroy stored imported public keys on free #751
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking at this code TBH I feel like I want to do away with p11prov_obj_copy_key_data() but the incoming key comes from openssl so it is not really possible, sigh ... That said, "handle_owner" is really not very clear, because what matters here is that you want to keep a reference to the original object so that it is not deleted. So I think this field is probably better called "ref_obj" or maybe "ref_orig_obj" to make it clear this is a reference being hold.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah your name makes more sense. Will change it. |
||
| /* 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); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. bring this out as a proper if/else, please |
||
| } | ||
| 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); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,208 @@ | ||
| /* Copyright (C) 2026 Jakub Zelenka <jakub.openssl@gmail.com> | ||
| 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 <stdlib.h> | ||
| #include <unistd.h> | ||
| #include <string.h> | ||
| #include <openssl/err.h> | ||
| #include <openssl/evp.h> | ||
| #include <openssl/store.h> | ||
| #include <stdio.h> | ||
| #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 <privkey> <pubkey> <matchuri>\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); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the proper name for this should be either "ephemeral" or "session_obj", "imported" is ambiguous because I can import and store in the token database.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually now that I think of it, what is the difference between this "imported" bool set to true and cka_token below set to false?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, couldn't this result in attempt to destroy objects that are not imported potentially causing some mix up. In other word aren't there cases where cka_token is false but imported is also false. It looked to me there are few suspicious places where this could happen but will need to verify it properly if you are not sure?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well any object that has cka_token = false, is a session object and therefore ephemeral in nature. What I am trying to understand here is if there is a case where the distinction between:
vs
is an important question for this functionality.
If you do not have any reference anymore to an object do we still need to retain it in the session at all if we generated it?
Generally these would be ephemeral symmetric keys that are not "recalled" once the EVP_PKEY object is destroyed, so I can't think of a case where the distinction matters for the purpose of retaining versus destroying the object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will check this out when I have more time for this and verify it.