PKCS#11 Ed25519 #5559

varder · 2022-09-29T07:19:34Z

ta: pkcs11: Add Ed25519 support
Add functionality to generate, import keys, sign/verify for
Ed25519, Ed25519ctx and Ed25519ph.

etienne-lms

Thanks for the port to pkcs11 TA.
I've pointed the coding style issues, mostly indentation. See also few other comments.
That said, the overall looks good but i'll need another pass.

etienne-lms · 2022-09-29T07:29:53Z

ta/pkcs11/include/pkcs11_ta.h

@@ -1169,6 +1169,8 @@ enum pkcs11_key_type {
 	PKCS11_CKK_DSA				= 0x001,
 	PKCS11_CKK_DH				= 0x002,
 	PKCS11_CKK_EC				= 0x003,
+    PKCS11_CKK_EC_EDWARDS		= 0x040,


indentation + move below line 1180 following macro values ordering.

etienne-lms · 2022-09-29T07:30:22Z

ta/pkcs11/include/pkcs11_ta.h

@@ -1250,6 +1252,8 @@ enum pkcs11_mechanism_id {
 	PKCS11_CKM_ECDSA_SHA384			= 0x01045,
 	PKCS11_CKM_ECDSA_SHA512			= 0x01046,
 	PKCS11_CKM_ECDH1_DERIVE			= 0x01050,
+	PKCS11_CKM_EC_EDWARDS_KEY_PAIR_GEN	= 0x01055,
+	PKCS11_CKM_EDDSA				= 0x01057,


PKCS11_CKM_EDDSA and PKCS11_CKM_ECDSA can be easy to confuse.
Would you be ok the rename it? Suggestion PKCS11_CKM_EC_EDDSA.
(and for consistency s/PKCS11_CKK_EDDSA/PKCS11_CKK_EC_EDDSA/g)

(indentation)

Discard this comment. This naming is driven by the PKCS#11 specs.
Sorry for the noise.

etienne-lms · 2022-09-29T07:37:06Z

ta/pkcs11/src/pkcs11_attributes.c

+	case PKCS11_CKM_EC_EDWARDS_KEY_PAIR_GEN:
+		if ((get_class(temp) != PKCS11_CKO_PUBLIC_KEY &&
+			 get_class(temp) != PKCS11_CKO_PRIVATE_KEY) ||
+			get_key_type(temp) != PKCS11_CKK_EC_EDWARDS) {


indentation:

if ((get_class(temp) != PKCS11_CKO_PUBLIC_KEY && get_class(temp) != PKCS11_CKO_PRIVATE_KEY) || get_key_type(temp) != PKCS11_CKK_EC_EDWARDS) { rc = PKCS11_CKR_TEMPLATE_INCONSISTENT; goto out; }

etienne-lms · 2022-09-29T07:38:22Z

ta/pkcs11/src/pkcs11_attributes.c

+		if (key_type != PKCS11_CKK_EC_EDWARDS) {
+			EMSG("Invalid key %s for mechanism %s",
+				id2str_type(key_type, key_class),
+				id2str_proc(proc_id));


EMSG("Invalid key %s for mechanism %s", - id2str_type(key_type, key_class), - id2str_proc(proc_id)); + id2str_type(key_type, key_class), + id2str_proc(proc_id));

etienne-lms · 2022-09-29T07:38:53Z

ta/pkcs11/src/pkcs11_attributes.c

+		if (key_class != PKCS11_CKO_PUBLIC_KEY &&
+			key_class != PKCS11_CKO_PRIVATE_KEY) {
+			EMSG("Invalid key class for mechanism %s",
+				id2str_proc(proc_id));


EMSG("Invalid key class for mechanism %s", id2str_proc(proc_id));

etienne-lms · 2022-09-29T07:58:32Z

ta/pkcs11/src/processing_ec.c

+		return rc;
+
+	rc = tee2pkcs_add_attribute(pub_head, PKCS11_CKA_EC_POINT,
+								tee_obj, TEE_ATTR_ED25519_PUBLIC_VALUE);


indent for the 3 above

etienne-lms · 2022-09-29T07:58:51Z

ta/pkcs11/src/processing_ec.c

+	if (rc)
+		return rc;
+
+	out:


- out: +out:

etienne-lms · 2022-09-29T08:00:17Z

ta/pkcs11/src/processing_ec.c

+
+	proc->extra_ctx = TEE_Malloc(sizeof(struct eddsa_processing_ctx) +
+								 ctx_len,
+								 TEE_USER_MEM_HINT_NO_FILL_ZERO);


proc->extra_ctx = TEE_Malloc(sizeof(struct eddsa_processing_ctx) + - ctx_len, - TEE_USER_MEM_HINT_NO_FILL_ZERO); + ctx_len, TEE_USER_MEM_HINT_NO_FILL_ZERO);

Fixed indentation,
Is it Ok?
sizeof(*ctx) + ctx_len,

etienne-lms · 2022-09-29T08:01:03Z

ta/pkcs11/src/token_capabilities.c

@@ -390,9 +395,15 @@ void pkcs11_mechanism_supported_key_sizes(uint32_t proc_id,
 	case PKCS11_CKM_ECDSA_SHA384:
 	case PKCS11_CKM_ECDSA_SHA512:
 	case PKCS11_CKM_ECDH1_DERIVE:
+	case PKCS11_CKFM_GENERATE_KEY_PAIR:


discard this change

Indeed, thanks for pointing out

etienne-lms · 2022-09-29T08:12:50Z

ta/pkcs11/src/processing_ec.c

+				  struct pkcs11_attribute_head *proc_params)
+{
+	enum pkcs11_rc rc = PKCS11_CKR_GENERAL_ERROR;
+	uint32_t flag = 0, ctx_len = 0;


1 variable definition per line

etienne-lms

few remaining comments on the pkcs11 TA part.

etienne-lms · 2022-09-30T08:06:50Z

ta/pkcs11/include/pkcs11_ta.h

@@ -1250,6 +1252,8 @@ enum pkcs11_mechanism_id {
 	PKCS11_CKM_ECDSA_SHA384			= 0x01045,
 	PKCS11_CKM_ECDSA_SHA512			= 0x01046,
 	PKCS11_CKM_ECDH1_DERIVE			= 0x01050,
+	PKCS11_CKM_EC_EDWARDS_KEY_PAIR_GEN	= 0x01055,
+	PKCS11_CKM_EDDSA				= 0x01057,


Discard this comment. This naming is driven by the PKCS#11 specs.
Sorry for the noise.

etienne-lms · 2022-09-30T13:20:15Z

ta/pkcs11/src/processing_ec.c

+		return PKCS11_CKR_ARGUMENTS_BAD;
+
+	proc->extra_ctx = TEE_Malloc(sizeof(*ctx) + ctx_len,
+					     TEE_USER_MEM_HINT_NO_FILL_ZERO);


indentation

etienne-lms · 2022-09-30T13:23:16Z

ta/pkcs11/src/processing_asymm.c

-						       out_buf, &out_size);
+						tee_attrs,
+						tee_attrs_count,
+						hash_buf, hash_size,


discard this change. The original code was good:

res = TEE_AsymmetricSignDigest(proc->tee_op_handle, tee_attrs, tee_attrs_count, in_buf, in_size, out_buf, &out_size);

jforissier · 2022-09-30T13:29:10Z

@varder when you address Etienne's comments please also rebase onto master which now has the "proper" Ed25519 implementation, thanks!

etienne-lms · 2022-10-03T06:37:57Z

ta/pkcs11/src/processing_asymm.c

+						tee_attrs,
+						tee_attrs_count,
+						in_buf, in_size,
+						out_buf, &out_size);


preserve original indentation :)

etienne-lms · 2022-10-03T06:40:16Z

ta/pkcs11/src/processing_ec.c

+	rc = tee2pkcs_add_attribute(priv_head, PKCS11_CKA_VALUE,
+				    tee_obj, TEE_ATTR_ED25519_PRIVATE_VALUE);
+	if (rc)
+		return rc;


goto out;
ditto lines below

etienne-lms · 2022-10-03T07:38:56Z

ta/pkcs11/src/token_capabilities.c

@@ -393,6 +400,11 @@ void pkcs11_mechanism_supported_key_sizes(uint32_t proc_id,
 		*min_key_size = 160;	/* in bits */
 		*max_key_size = 521;	/* in bits */
 		break;
+	case PKCS11_CKM_EC_EDWARDS_KEY_PAIR_GEN:
+	case PKCS11_CKM_EDDSA:
+		*min_key_size = 256;	/* in bits */


Spec mentions 255 while key effective bit size is 256.
Maybe a word here? /* in bits, v3.1 falsly states 255 */

According to rfc8032
https://www.rfc-editor.org/rfc/rfc8032#section-5.1.5
Key Generation
The private key is 32 octets (256 bits, corresponding to b) of
cryptographically secure random data.

I guess it is a typo in Specs,

etienne-lms · 2022-10-03T10:04:16Z

ta/pkcs11/src/processing_ec.c

+
+static const uint8_t ed25519ph_oid_der[] = {
+	0x13, 0x0C, 'e', 'd', 'w', 'a', 'r', 'd', 's',
+	'2', '5', '5', '1', '9',


missing 'p', 'h' ?

etienne-lms · 2022-10-03T10:35:14Z

ta/pkcs11/src/processing_ec.c

@@ -41,6 +42,24 @@ static const uint8_t secp521r1_name_der[] = {
 	0x06, 0x05, 0x2b, 0x81, 0x04, 0x00, 0x23,
 };

+static const uint8_t ed25519_name_der[] = {
+	0x06, 0x03, 0x2B, 0x65, 0x70,


It seems 0x06, 0x03, 0x2B, 0x65, 0x70 is the encoding of EdDSA algo while the curve OID's DER encoding is 06 09 2B 06 01 04 01 DA 47 0F 01.
See script ta/pkcs11/scripts/dump_ec_curve_params.sh used to get IDs used by openssl, maybe it can help.

For the existing algos the struct values
struct supported_ecc_curve
can be generated using dump_ec_curve_params.sh
with the commands:
openssl ecparam -name ${EC_CURVE} -param_enc explicit
But it looks like for Ed25519 with Openssl impossible to get the parameters.
I would like to generate it automatically, but i could't find the way.

I have looked at some existing implementation,
and it looks like, 1.3.101.112 is used in this context
https://github.com/opendnssec/SoftHSMv2/blob/develop/src/lib/crypto/BotanEDPrivateKey.cpp#L54
https://github.com/OpenSC/OpenSC/blob/master/src/libopensc/pkcs15-algo.c#L451

varder · 2022-10-04T05:37:38Z

@etienne-lms
Can I squash the fixups?

etienne-lms

Reviewed-by: Etienne Carriere <[email protected]> (minor inline comment style issue)

etienne-lms · 2022-10-04T13:08:41Z

ta/pkcs11/src/processing_ec.c

@@ -244,6 +245,21 @@ static const uint8_t secp521r1_oid_der[] = {
 	0x91, 0x38, 0x64, 0x09, 0x02, 0x01, 0x01,
 };

+/* Edwards curves may be specified in two flavours: 


/* * Edwards curves may be specified in two flavours:

etienne-lms · 2022-10-04T13:31:36Z

@etienne-lms
Can I squash the fixups?

Ok

Add functionality to generate, import keys, sign/verify for ED25519, ED25519ctx and ED25519ph. The values for the object identifies originates from: https://www.rfc-editor.org/rfc/rfc8420.html A.1. ASN.1 Object for Ed25519 The PKCS#11 Specification: https://docs.oasis-open.org/pkcs11/pkcs11-spec/v3.1/cs01/ pkcs11-spec-v3.1-cs01.pdf Signed-off-by: Valerii Chubar <[email protected]> Reviewed-by: Etienne Carriere <[email protected]>

varder mentioned this pull request Sep 29, 2022

PKCS#11 Ed25519 tests OP-TEE/optee_test#618

Merged

etienne-lms reviewed Sep 29, 2022

View reviewed changes

varder mentioned this pull request Sep 29, 2022

libckteec: Add EDDSA attribute serialization OP-TEE/optee_client#324

Merged

etienne-lms reviewed Sep 30, 2022

View reviewed changes

varder force-pushed the pkcs11_ed25519 branch from eaeb9f9 to 058ade1 Compare September 30, 2022 15:11

etienne-lms reviewed Oct 3, 2022

View reviewed changes

varder force-pushed the pkcs11_ed25519 branch from 058ade1 to 22c3e71 Compare October 3, 2022 09:07

etienne-lms reviewed Oct 3, 2022

View reviewed changes

varder force-pushed the pkcs11_ed25519 branch 3 times, most recently from 824cea6 to c2cbbbe Compare October 3, 2022 18:50

etienne-lms reviewed Oct 4, 2022

View reviewed changes

varder force-pushed the pkcs11_ed25519 branch from c2cbbbe to 4402b52 Compare October 4, 2022 14:57

jforissier merged commit 03e0743 into OP-TEE:master Oct 6, 2022

PKCS#11 Ed25519 #5559

PKCS#11 Ed25519 #5559

Conversation

varder commented Sep 29, 2022 • edited Loading

etienne-lms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etienne-lms Sep 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etienne-lms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jforissier commented Sep 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etienne-lms Oct 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

varder Oct 3, 2022 • edited Loading

Choose a reason for hiding this comment

varder commented Oct 4, 2022

etienne-lms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etienne-lms commented Oct 4, 2022

varder commented Sep 29, 2022 •

edited

Loading

etienne-lms Sep 29, 2022 •

edited

Loading

etienne-lms Oct 3, 2022 •

edited

Loading

varder Oct 3, 2022 •

edited

Loading