Patch keygen by istepic · Pull Request #2 · istepic/libp11

istepic · 2022-11-17T16:10:23Z

No description provided.

garw

I commented on some of the locations. Generally, I feel that the error handling needs a cleanup. While these different error codes depending on where the code bails out may be helpful for debugging, it is not really what one usually does. In particular, when it gets to the (real) library user, error values need to have a customer-understandable meaning without reading the code.

garw · 2022-11-23T13:27:39Z

src/p11_key.c

-
 	if (pkcs11_get_session(slot, 1, &session))
-		return -1;
+		return -10;


why? What are the magical numbers? At least add a comment.

As you said, debugging, but I'm fine with -1 too.

garw · 2022-11-23T13:30:12Z

src/p11_key.c

+	int curve_nid = NID_undef;
+
+	if (pkcs11_get_session(slot, 1, &session)) {
+		return -20;


Why is it here -20 and above -10?

garw · 2022-11-23T13:32:07Z

src/p11_key.c

+	ecdsa_params = (unsigned char *)OPENSSL_malloc(ecdsa_params_len);
+	if (!ecdsa_params)
+		return -23;
+	tmp = ecdsa_params;


what is the purpose of tmp?

~~Some leftover. Done~~
Turns out this breaks a test. It's somehow changing ecdsa_params_len indirectly by changing curve_obj?

Doesn't make sense imho.
Both tmp and ecdsa_params are of type "unsigned char *".
Just for clarification: Did you also changed from tmp to ecdsa_params in the next line?

This is the code I took from the other unmerged EC keygen PR. I will take a deeper look in openssl functions to see what is this magic.

So, in line 350, what actually happens is memory space pointed by ecdsa_params gets filled in some way but this also changes ecdsa_params pointer (as a side effect?) which we for some reason don't want and that's why we use a tmp pointer. I don't really understand this cryptic code in openssl:

void ASN1_put_object(unsigned char **pp, int constructed, int length, int tag, int xclass) { unsigned char *p = *pp; int i, ttag; i = (constructed) ? V_ASN1_CONSTRUCTED : 0; i |= (xclass & V_ASN1_PRIVATE); if (tag < 31) *(p++) = i | (tag & V_ASN1_PRIMITIVE_TAG); else { *(p++) = i | V_ASN1_PRIMITIVE_TAG; for (i = 0, ttag = tag; ttag > 0; i++) ttag >>= 7; ttag = i; while (i-- > 0) { p[i] = tag & 0x7f; if (i != (ttag - 1)) p[i] |= 0x80; tag >>= 7; } p += ttag; } if (constructed == 2) *(p++) = 0x80; else asn1_put_length(&p, length); *pp = p; }

and I think spending time to understand what this is doing is not worth it given our current situation. Especially since this seems to work as evidenced by the test, and this behavior only depends on curve_obj which is quite straightforward. So I don't anticipate things going wrong wrong here.

garw · 2022-11-23T13:34:32Z

src/p11_key.c

+	if (label)
+		pkcs11_addattr_s(&pubtmpl, CKA_LABEL, label);
+	pkcs11_addattr_bool(&pubtmpl, CKA_TOKEN, TRUE);
+	pkcs11_addattr_bool(&pubtmpl, CKA_DERIVE, TRUE);


Do we really have DERIVE on the public key?

Done. Maybe we should revisit these options to see if they match our use cases. Or make them configurable?

garw · 2022-11-23T13:41:02Z

src/p11_front.c

+	size_t key_id_len = 0;
+	if (kg && kg->key_id) {
+		key_id_len = strlen(kg->key_id);
+		if (key_id_len > 127) {


While this is not a security problem, should hex to bin compress the output by factor 1/2?

Do you mean output length? For even lengths yes, for odd no.
0x10a (3 bytes) -> 0000 0001 0000 1010 (2 bytes)

Suggested change

if (key_id_len > 127) {

if (key_id_len == 127) {

RETURN VALUE
The strnlen() function returns strlen(s), if that is less than maxlen, or maxlen if there is no null termi‐
nating ('\0') among the first maxlen characters pointed to by s.

I wonder why something greater than 127 shall be returned in any case.

garw · 2022-11-23T13:52:39Z

src/libp11.h


+typedef struct PKCS11_ec_kgen_st {
+	const char *curve;
+} PKCS11_EC_KGEN;


Any particular reason we safe two bytes of memory here and not just write KEYGEN?

I like shorter type names if it's trivial to deduce what it is. I can switch if you insist.

garw · 2022-11-23T13:58:36Z

src/eng_back.c

+	}
+
+	// Take the first token that has a matching label
+	// TODO: make this more intelligent


Todo? Do we want to change this? I guess its okay for now as this is an intermediate solution anyways.

src/eng_back.c

garw · 2022-11-23T14:13:45Z

tests/keygen.c

+		fprintf(stderr, "Where are the keys?\n");
+		return -1;
+	}
+	if(!(mdctx = EVP_MD_CTX_create())) {


style: We dislike assignments in if () statements.

garw · 2022-11-23T14:15:55Z

tests/keygen.c

+		exit(1);
+	}
+
+	ENGINE_finish(engine);


Can you also add test cases that extract the respective keys and use those to verify the signature?

This is what the test does. After successful ENGINE_ctrl_cmd(engine, "KEYGEN", 0, &eckg, NULL, 1); the keypair is living on the HSM. Then we extract it with

EVP_PKEY *ecpb = ENGINE_load_public_key(engine, "1234", NULL, NULL);
EVP_PKEY *ecpr = ENGINE_load_private_key(engine, "1234", NULL, NULL);

and use it to sign and verify.

wusto

found some minor things.

Otherwise it looks good to me.

src/eng_back.c

wusto · 2022-11-29T17:48:56Z

src/p11_front.c

-	return pkcs11_generate_key(slot, algorithm, bits, label, id, id_len);
+	unsigned char out[128] = {0};
+	size_t key_id_len = 0;
+	if (kg && kg->key_id) {


Suggested change

if (kg && kg->key_id) {

if (!kg) {

return -1;

}

if (kg->key_id) {

otherwise kg->type will cause an SIGSEGV in the end.

wusto · 2022-11-29T17:51:55Z

src/p11_front.c

+	size_t key_id_len = 0;
+	if (kg && kg->key_id) {
+		key_id_len = strlen(kg->key_id);
+		if (key_id_len > 127) {


Suggested change

if (key_id_len > 127) {

if (key_id_len == 127) {

RETURN VALUE
The strnlen() function returns strlen(s), if that is less than maxlen, or maxlen if there is no null termi‐
nating ('\0') among the first maxlen characters pointed to by s.

I wonder why something greater than 127 shall be returned in any case.

wusto · 2022-11-29T17:54:41Z

src/p11_front.c

+	case EVP_PKEY_EC:
+		return pkcs11_ec_keygen(slot, kg->kgen.ec->curve,
+				kg->key_label, out, key_id_len);
+	default:


Please add pkcs11_dilithium_keygen here ;)

wusto · 2022-11-29T17:59:21Z

src/p11_key.c

+	ecdsa_params = (unsigned char *)OPENSSL_malloc(ecdsa_params_len);
+	if (!ecdsa_params)
+		return -23;
+	tmp = ecdsa_params;


Doesn't make sense imho.
Both tmp and ecdsa_params are of type "unsigned char *".
Just for clarification: Did you also changed from tmp to ecdsa_params in the next line?

wusto · 2022-11-29T18:06:21Z

tests/keygen.c

+		display_openssl_errors(__LINE__);
+		fprintf(stderr, "Verify fail\n");
+		retval = -10;
+		goto err;


superfluous.

or also add it to the if-branch (not just the else) if you want to avoid stupid failures on extending the test. :)

As discussed in OpenSC#379 and OpenSC#378 we need a generic interface that supports multiple algorithms for key generation. Attempt was made to create a new keygen method and register it in PKCS11_pkey_meths() in p11_pkey.c (so that it's possible to generate keys using OpenSSL's EVP_PKEY_* API) but multiple design issues appeared. How and where do you pass the key ID, token label and alike was the first question. As suggested by the maintainer here: OpenSC#379 (comment), app_data from EVP_PKEY_CTX was (mis)used and that worked well. The reason why this approach was abandoned is because a good (or bad) way to get a handle of the PKCS11_CTX_private, that is necessary for the Cryptoki call, was not found. The way other operations work is that they rely on the key being loaded *_first_* through ENGINE_load_public(private)_key because this is when the PKCS11_CTX gets initialized and a handle to PKCS11_OBJECT_private gets set to the ex_data of the underlying key. Key generation obviously cannot rely on that mechanism since key doesn't yet exist. Instead, a generic PKCS11_generate_key interface was made that takes a structure describing the key generation algorithm. For now it only contains simple options like curve name for ECC or number of bits for RSA key generation. This interface can then be used as any other PKCS11 wrapper interface or using the ENGINE control commands. Using it with ENGINE control commands is demonstrated in the new tests/keygen.c file. Code for ECC keygen was taken from: OpenSC#379 and reworked to compile and work with some new additions to libp11 i.e. templates.

for identifying operations and libp11 already uses these identifiers, we keep using these for consistency.

- added NULL ptr checks - fixed the usage of strnlen when handling key_id - added another goto in keygen test

garw suggested changes Nov 23, 2022

View reviewed changes

wusto suggested changes Nov 29, 2022

View reviewed changes

istepic added 6 commits December 5, 2022 22:44

Fix constness of key generation attributes

3dfa15a

address garw's comments

8cff0b4

fix change that broke the test

915cb1e

Add a comment that explains "type" member. Since openssl uses #defines

d9cb376

for identifying operations and libp11 already uses these identifiers, we keep using these for consistency.

address wusto's comments

02af669

- added NULL ptr checks - fixed the usage of strnlen when handling key_id - added another goto in keygen test

istepic force-pushed the patch-keygen branch from 87c9b29 to 02af669 Compare December 5, 2022 21:45

istepic merged commit fb885ab into rel0.4.12 Dec 5, 2022

Conversation

istepic commented Nov 17, 2022

Uh oh!

garw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

istepic Nov 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wusto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

istepic Nov 24, 2022 •

edited

Loading