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

Fix serialization pub key #491

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

Fix serialization pub key #491

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d0a3393
Fix serialization pub key
Dec 27, 2024
90a0b8a
Fix a bug with freeing up the memory of an object for which memory is…
Dec 27, 2024
70f5d83
Add a test with public information about the generated key
Dec 27, 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
10 changes: 5 additions & 5 deletions src/encoder.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,10 +140,10 @@ static int p11prov_rsa_encoder_encode_text(void *inctx, OSSL_CORE_BIO *cbio,
}
}

uri = p11prov_key_to_uri(ctx->provctx, key);
uri = p11prov_key_to_uri(ctx->provctx, key, selection);
Copy link
Member

Choose a reason for hiding this comment

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

In this place selection may contain both flags, which means you will always print a URI that strictly refers to the public key.

You need to pass the key class not the selection to get the correct type.

if (uri) {
BIO_printf(out, "URI %s\n", uri);
free(uri);
OPENSSL_free(uri);
}

BIO_free(out);
Expand Down Expand Up @@ -474,7 +474,7 @@ static P11PROV_PK11_URI *p11prov_encoder_private_key_to_asn1(P11PROV_CTX *pctx,
size_t uri_len;
int ret = RET_OSSL_ERR;

uri = p11prov_key_to_uri(pctx, key);
uri = p11prov_key_to_uri(pctx, key, OSSL_KEYMGMT_SELECT_PRIVATE_KEY);
if (!uri) {
goto done;
}
Expand Down Expand Up @@ -896,7 +896,7 @@ static int p11prov_ec_encoder_encode_text(void *inctx, OSSL_CORE_BIO *cbio,
}
}

uri = p11prov_key_to_uri(ctx->provctx, key);
uri = p11prov_key_to_uri(ctx->provctx, key, selection);
if (uri) {
BIO_printf(out, "URI %s\n", uri);
}
Expand Down Expand Up @@ -1014,7 +1014,7 @@ static int p11prov_ec_edwards_encoder_encode_text(
}
}

uri = p11prov_key_to_uri(ctx->provctx, key);
uri = p11prov_key_to_uri(ctx->provctx, key, selection);
if (uri) {
BIO_printf(out, "URI %s\n", uri);
}
Expand Down
8 changes: 6 additions & 2 deletions src/util.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -670,7 +670,7 @@ static char *uri_component(const char *name, const char *val, size_t vlen,
return c;
}

char *p11prov_key_to_uri(P11PROV_CTX *ctx, P11PROV_OBJ *key)
char *p11prov_key_to_uri(P11PROV_CTX *ctx, P11PROV_OBJ *key, int selection)
Copy link
Member

Choose a reason for hiding this comment

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

Using selection here makes little sense and will lead to incorrect results.
In PKCS#11 objects are either private or public keys, never both.

I do not see how using selection here improves anything, all you get is a lie.
If you need to "force" the type for some reason, then you should straight on pass the requested class as an attribute and let the caller make the call.

An alternative is to pass a boolean called use_class, and when set to false the URI will not include the keytype. This may be better than trying to lie about the object type.

Copy link
Member

Choose a reason for hiding this comment

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

Note that this comment makes sense only if we determine that this change is needed at all, which I do not think it is, see the main review comment about this change.

Copy link
Author

Choose a reason for hiding this comment

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

In this case, we pointed out an error when requesting information about the public key. It's acceptable if you fix it in any way you can. As users of the pkcs11 library, it would be important for us to fix this error.

Also important in this pr is the correction of UB when calling free from an object for which memory is allocated using openssl.

{
P11PROV_SLOTS_CTX *slots;
P11PROV_SLOT *slot;
Expand All @@ -691,7 +691,11 @@ char *p11prov_key_to_uri(P11PROV_CTX *ctx, P11PROV_OBJ *key)
size_t size_hint = 0;
CK_RV ret;

class = p11prov_obj_get_class(key);
if (selection & OSSL_KEYMGMT_SELECT_PUBLIC_KEY) {
class = CKO_PUBLIC_KEY;
} else {
class = p11prov_obj_get_class(key);
}
slot_id = p11prov_obj_get_slotid(key);
cka_id = p11prov_obj_get_attr(key, CKA_ID);
cka_label = p11prov_obj_get_attr(key, CKA_LABEL);
Expand Down
2 changes: 1 addition & 1 deletion src/util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ void p11prov_fetch_attrs_free(struct fetch_attrs *attrs, int num);
#define MAX_PIN_LENGTH 32
int parse_ulong(P11PROV_CTX *ctx, const char *str, size_t len, void **output);
P11PROV_URI *p11prov_parse_uri(P11PROV_CTX *ctx, const char *uri);
char *p11prov_key_to_uri(P11PROV_CTX *ctx, P11PROV_OBJ *key);
char *p11prov_key_to_uri(P11PROV_CTX *ctx, P11PROV_OBJ *key, int selection);
void p11prov_uri_free(P11PROV_URI *parsed_uri);
CK_OBJECT_CLASS p11prov_uri_get_class(P11PROV_URI *uri);
void p11prov_uri_set_class(P11PROV_URI *uri, CK_OBJECT_CLASS class);
Expand Down
2 changes: 2 additions & 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'],
'tgetinfopkey': ['tgetinfopkey.c']
}

test_executables = []
Expand Down Expand Up @@ -133,6 +134,7 @@ tests = {
'rsapss': {'suites': ['softokn', 'softhsm', 'kryoptic', 'kryoptic.nss']},
'rsapssam': {'suites': ['softhsm']},
'genkey': {'suites': ['softokn', 'softhsm', 'kryoptic', 'kryoptic.nss']},
'getinfopkey': {'suites': ['softokn', 'softhsm']},
'session': {'suites': ['softokn', 'softhsm', 'kryoptic', 'kryoptic.nss']},
'rand': {'suites': ['softokn', 'softhsm', 'kryoptic', 'kryoptic.nss']},
'readkeys': {'suites': ['softokn', 'softhsm', 'kryoptic', 'kryoptic.nss']},
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 Get generated key info"
$CHECKER "${TESTBLDDIR}/tgetinfopkey"

exit 0
127 changes: 127 additions & 0 deletions tests/tgetinfopkey.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,127 @@
#define _GNU_SOURCE
#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <openssl/core_names.h>
#include <openssl/evp.h>
#include <openssl/store.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';
}

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;
BIO *mem;
int maxlen = 4000;
char buf[maxlen];
const char pub_part[] = "type=public";
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);
}

mem = BIO_new(BIO_s_mem());
if (mem == NULL) {
fprintf(stderr, "Failed to init BIO\n");
exit(EXIT_FAILURE);
}

ret = EVP_PKEY_print_public(mem, key, 0, NULL);
if (ret != 1) {
fprintf(stderr, "Failed to print public key\n");
exit(EXIT_FAILURE);
}

memset(buf, 0x00, maxlen);
BIO_read(mem, buf, maxlen);
if (strstr(buf, pub_part) == NULL) {
fprintf(stderr, "Incorrect information about the public key\n");
exit(EXIT_FAILURE);
}

BIO_free(mem);
EVP_PKEY_CTX_free(ctx);
EVP_PKEY_free(key);
exit(EXIT_SUCCESS);
}
Loading