Skip to content
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.

Sign up for GitHub

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

Allow generated EVP_PKEY to be used to verify operation #479

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Allow generated EVP_PKEY to be used to verify operation #479

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2b9df2d
Add public key object pointer to P11PROV_OBJ
Dec 5, 2024
6f1a787
Use public key object from P11PROV_OBJ during generate and verify
Dec 5, 2024
57fb91a
Remove reinitialization of P11PROV_OBJ some fields
Dec 5, 2024
9fd45c3
Reformat some files with clang-format
Dec 5, 2024
f5f049d
Add test on sign and verify with generated keys
Dec 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions src/keymgmt.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -482,14 +482,19 @@ static int p11prov_common_gen(struct key_generator *ctx,
}

ret = p11prov_merge_pub_attrs_into_priv(pub_key, priv_key);
simo5 marked this conversation as resolved.
Show resolved Hide resolved
if (ret != CKR_OK) {
goto done;
}

ret = p11prov_set_pub(priv_key, pub_key);

done:
if (ret != CKR_OK) {
p11prov_obj_free(pub_key);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is wrong now because the next line will already free the pub_key you set on it.
After you set the pub key on the priv key you need to set pub_key = NULL;

This is may be one of the segfault issue, because you are causing a double-free here.

p11prov_obj_free(priv_key);
priv_key = NULL;
pub_key = priv_key = NULL;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This become useless once you correctly zero the pub_key right after you pass ownership of it to the priv_key object.

}
p11prov_return_session(session);
p11prov_obj_free(pub_key);
*key = priv_key;
return ret;
}
Expand Down
28 changes: 24 additions & 4 deletions src/objects.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ struct p11prov_obj {
CK_BBOOL cka_copyable;
CK_BBOOL cka_token;

P11PROV_OBJ *pub_key_obj;

P11PROV_URI *refresh_uri;

union {
Expand Down Expand Up @@ -108,8 +110,8 @@ void p11prov_obj_pool_free(P11PROV_OBJ_POOL *pool)
}
OPENSSL_free(pool->objects);
(void)MUTEX_UNLOCK(pool);
/* ------------- LOCKED SECTION */ }
else {
/* ------------- LOCKED SECTION */
} else {
P11PROV_debug("Failed to lock object pool, leaking it!");
return;
}
Expand Down Expand Up @@ -438,6 +440,9 @@ void p11prov_obj_free(P11PROV_OBJ *obj)
if (obj == NULL) {
return;
}

p11prov_obj_free(obj->pub_key_obj);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is wrong, all freeing MUST happen after the refcounting reaches 0.
Move it after the following block and you'll solve most of your segfaults.

if (__atomic_sub_fetch(&obj->refcnt, 1, __ATOMIC_SEQ_CST) != 0) {
P11PROV_debug("object free: reference held");
return;
Expand Down Expand Up @@ -941,8 +946,6 @@ CK_RV p11prov_obj_from_handle(P11PROV_CTX *ctx, P11PROV_SESSION *session,
if (obj == NULL) {
return CKR_HOST_MEMORY;
}
obj->handle = handle;
obj->slotid = p11prov_session_slotid(session);
simo5 marked this conversation as resolved.
Show resolved Hide resolved
obj->data.key.type = CK_UNAVAILABLE_INFORMATION;

num = 0;
Expand Down Expand Up @@ -4027,6 +4030,7 @@ CK_RV p11prov_obj_copy_specific_attr(P11PROV_OBJ *pub_key,
#define RSA_PRIV_ATTRS_NUM 2

#define EC_PRIV_ATTRS_NUM 3
// TODO: maybe remove after public key object was added in P11PROV_OBJ
CK_RV p11prov_merge_pub_attrs_into_priv(P11PROV_OBJ *pub_key,
P11PROV_OBJ *priv_key)
{
Expand Down Expand Up @@ -4149,3 +4153,19 @@ P11PROV_OBJ *mock_pub_ec_key(P11PROV_CTX *ctx, CK_ATTRIBUTE_TYPE type,

return key;
}

CK_RV p11prov_set_pub(P11PROV_OBJ *priv, P11PROV_OBJ *pub)
{
if (!priv || !pub) return CKR_ARGUMENTS_BAD;
if (priv->class != CKO_PRIVATE_KEY || pub->class != CKO_PUBLIC_KEY)
return CKR_ARGUMENTS_BAD;
if (priv->pub_key_obj) return CKR_ARGUMENTS_BAD;
priv->pub_key_obj = pub;
return CKR_OK;
}

P11PROV_OBJ *p11prov_get_pub(P11PROV_OBJ *priv)
{
if (!priv || priv->class != CKO_PRIVATE_KEY) return NULL;
return priv->pub_key_obj;
}
2 changes: 2 additions & 0 deletions src/objects.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ int p11prov_obj_get_ed_pub_key(P11PROV_OBJ *obj, CK_ATTRIBUTE **pub);
CK_ATTRIBUTE *p11prov_obj_get_ec_public_raw(P11PROV_OBJ *key);
P11PROV_OBJ *mock_pub_ec_key(P11PROV_CTX *ctx, CK_ATTRIBUTE_TYPE type,
CK_ATTRIBUTE *ec_params);
CK_RV p11prov_set_pub(P11PROV_OBJ *priv, P11PROV_OBJ *pub);
P11PROV_OBJ *p11prov_get_pub(P11PROV_OBJ *priv);

#define OBJ_CMP_KEY_TYPE 0x00
#define OBJ_CMP_KEY_PUBLIC 0x01
Expand Down
30 changes: 16 additions & 14 deletions src/signature.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -686,11 +686,7 @@ static CK_RV p11prov_sig_op_init(void *ctx, void *provkey, CK_FLAGS operation,
return ret;
}

sigctx->key = p11prov_obj_ref(key);
if (sigctx->key == NULL) {
return CKR_KEY_NEEDED;
}
class = p11prov_obj_get_class(sigctx->key);
class = p11prov_obj_get_class(key);
switch (operation) {
case CKF_SIGN:
if (class != CKO_PRIVATE_KEY) {
Expand All @@ -699,17 +695,25 @@ static CK_RV p11prov_sig_op_init(void *ctx, void *provkey, CK_FLAGS operation,
break;
case CKF_VERIFY:
if (class != CKO_PUBLIC_KEY) {
return CKR_KEY_TYPE_INCONSISTENT;
key = p11prov_get_pub(key);
if (key == NULL) {
return CKR_KEY_TYPE_INCONSISTENT;
}
}
break;
default:
return CKR_GENERAL_ERROR;
}
sigctx->key = p11prov_obj_ref(key);
if (sigctx->key == NULL) {
return CKR_KEY_NEEDED;
}
sigctx->operation = operation;

if (digest) {
ret = p11prov_digest_get_by_name(digest, &sigctx->digest);
if (ret != CKR_OK) {
p11prov_obj_free(sigctx->key);
Copy link
Member

@simo5 simo5 Dec 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is wrong, if you free the key here you also need to NULL the pointer or we will get a double free in p11prov_sig_freectx(), but you do not have to null anything here exactly because p11prov_sig_freectx() already takes care of this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are concerned about someone potentially calling the init operation multiple times, just check if the sigctx->key is not NULL at the start of the function and immediately error if that is the case. But there is no need to add that, it is an application error to try to initialize multiple times and the worst case is some memory leakage.

ERR_raise(ERR_LIB_PROV, PROV_R_INVALID_DIGEST);
return ret;
}
Expand Down Expand Up @@ -1252,10 +1256,9 @@ static int p11prov_rsasig_digest_sign_final(void *ctx, unsigned char *sig,
/* the siglen might be uninitialized when called from openssl */
*siglen = 0;

P11PROV_debug(
"rsa digest sign final (ctx=%p, sig=%p, siglen=%zu, "
"sigsize=%zu)",
ctx, sig, *siglen, sigsize);
P11PROV_debug("rsa digest sign final (ctx=%p, sig=%p, siglen=%zu, "
"sigsize=%zu)",
ctx, sig, *siglen, sigsize);

if (sigctx == NULL) {
return RET_OSSL_ERR;
Expand Down Expand Up @@ -1903,10 +1906,9 @@ static int p11prov_ecdsa_digest_sign_final(void *ctx, unsigned char *sig,
/* the siglen might be uninitialized when called from openssl */
*siglen = 0;

P11PROV_debug(
"ecdsa digest sign final (ctx=%p, sig=%p, siglen=%zu, "
"sigsize=%zu)",
ctx, sig, *siglen, sigsize);
P11PROV_debug("ecdsa digest sign final (ctx=%p, sig=%p, siglen=%zu, "
"sigsize=%zu)",
ctx, sig, *siglen, sigsize);

if (sigctx == NULL) {
return RET_OSSL_ERR;
Expand Down
1 change: 1 addition & 0 deletions tests/meson.build
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ test_programs = {
'tcmpkeys': ['tcmpkeys.c', 'util.c'],
'tfork': ['tfork.c'],
'pincache': ['pincache.c'],
'tsigver': ['tsigver.c'],
}

test_executables = []
Expand Down
4 changes: 4 additions & 0 deletions tests/tbasic
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,5 +296,9 @@ if [ $FAIL -ne 0 ]; then
echo
exit 1
fi
OPENSSL_CONF=${ORIG_OPENSSL_CONF}

title PARA "Test sign and verify on generated keys"
$CHECKER "${TESTBLDDIR}/tsigver"

exit 0
153 changes: 153 additions & 0 deletions tests/tsigver.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,153 @@
#define _GNU_SOURCE
#include <stdio.h>
#include <stdlib.h>
#include <openssl/core_names.h>
#include <openssl/evp.h>
#include <openssl/rand.h>

static void hexify(char *out, unsigned char *byte, size_t len)
{
char c[2], s;

for (size_t i = 0; i < len; i++) {
out[i * 3] = '%';
c[0] = byte[i] >> 4;
c[1] = byte[i] & 0x0f;
for (int j = 0; j < 2; j++) {
if (c[j] < 0x0A) {
s = '0';
} else {
s = 'a' - 10;
}
out[i * 3 + 1 + j] = c[j] + s;
}
}
out[len * 3] = '\0';
}

// TODO: add paddings
int main(int argc, char *argv[])
{
char *label;
unsigned char id[16];
char idhex[16 * 3 + 1];
char *uri;
size_t rsa_bits = 1024;
const char *key_usage = "digitalSignature";
OSSL_PARAM params[4];
int miniid;
EVP_PKEY_CTX *ctx;
EVP_PKEY *key = NULL;
// SHA-256 hash of "Plaintext Data"
const unsigned char md[] = {
0xac, 0x26, 0x5c, 0x10, 0x09, 0xf8, 0xf4, 0xdf, 0x05, 0xf4, 0x25,
0x18, 0x86, 0x92, 0x33, 0x5c, 0x2f, 0x9e, 0x9a, 0xe3, 0xdb, 0x44,
0xc4, 0xa9, 0x12, 0xfd, 0xa5, 0x07, 0x7e, 0xd4, 0xe1, 0x76
};
unsigned char *sig;
size_t siglen;
int ret;

ctx = EVP_PKEY_CTX_new_from_name(NULL, "RSA", "provider=pkcs11");
if (ctx == NULL) {
fprintf(stderr, "Failed to init PKEY context for generate\n");
exit(EXIT_FAILURE);
}

ret = EVP_PKEY_keygen_init(ctx);
if (ret != 1) {
fprintf(stderr, "Failed to init keygen\n");
exit(EXIT_FAILURE);
}

ret = RAND_bytes(id, 16);
if (ret != 1) {
fprintf(stderr, "Failed to generate key id\n");
exit(EXIT_FAILURE);
}
miniid = (id[0] << 24) + (id[1] << 16) + (id[2] << 8) + id[3];
ret = asprintf(&label, "Test-RSA-gen-%08x", miniid);
if (ret == -1) {
fprintf(stderr, "Failed to make label\n");
exit(EXIT_FAILURE);
}
hexify(idhex, id, 16);
ret = asprintf(&uri, "pkcs11:object=%s;id=%s", label, idhex);
if (ret == -1) {
fprintf(stderr, "Failed to compose PKCS#11 URI\n");
exit(EXIT_FAILURE);
}
params[0] = OSSL_PARAM_construct_utf8_string("pkcs11_uri", uri, 0);
params[1] = OSSL_PARAM_construct_utf8_string("pkcs11_key_usage",
(char *)key_usage, 0);
params[2] =
OSSL_PARAM_construct_size_t(OSSL_PKEY_PARAM_RSA_BITS, &rsa_bits);
params[3] = OSSL_PARAM_construct_end();
ret = EVP_PKEY_CTX_set_params(ctx, params);
if (ret != 1) {
fprintf(stderr, "Failed to set params\n");
exit(EXIT_FAILURE);
}

ret = EVP_PKEY_generate(ctx, &key);
if (ret != 1) {
fprintf(stderr, "Failed to generate key\n");
exit(EXIT_FAILURE);
}

EVP_PKEY_CTX_free(ctx);

ctx = EVP_PKEY_CTX_new_from_pkey(NULL, key, "provider=pkcs11");
if (ctx == NULL) {
fprintf(stderr, "Failed to init PKEY context for sign\n");
exit(EXIT_FAILURE);
}

ret = EVP_PKEY_sign_init(ctx);
if (ret != 1) {
fprintf(stderr, "Failed to init sign\n");
exit(EXIT_FAILURE);
}

ret = EVP_PKEY_sign(ctx, NULL, &siglen, md, sizeof(md) / sizeof(*md));
if (ret != 1) {
fprintf(stderr, "Failed to determine buffer length\n");
exit(EXIT_FAILURE);
}
sig = OPENSSL_malloc(siglen);
if (sig == NULL) {
fprintf(stderr, "Failed to malloc memory for buffer\n");
exit(EXIT_FAILURE);
}

ret = EVP_PKEY_sign(ctx, sig, &siglen, md, sizeof(md) / sizeof(*md));
if (ret != 1) {
fprintf(stderr, "Failed to sign\n");
exit(EXIT_FAILURE);
}

EVP_PKEY_CTX_free(ctx);

ctx = EVP_PKEY_CTX_new_from_pkey(NULL, key, "provider=pkcs11");
if (ctx == NULL) {
fprintf(stderr, "Failed to init PKEY context for verify\n");
exit(EXIT_FAILURE);
}

ret = EVP_PKEY_verify_init(ctx);
if (ret != 1) {
fprintf(stderr, "Failed to init verify\n");
exit(EXIT_FAILURE);
}

ret = EVP_PKEY_verify(ctx, sig, siglen, md, sizeof(md) / sizeof(*md));
if (ret != 1) {
fprintf(stderr, "Failed to verify\n");
exit(EXIT_FAILURE);
}

OPENSSL_free(sig);
EVP_PKEY_CTX_free(ctx);
EVP_PKEY_free(key);
exit(EXIT_SUCCESS);
}
Loading