PKCS#11 Ed25519 by varder · Pull Request #5559 · OP-TEE/optee_os

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

 	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

 	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

+	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

+		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

+		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

+		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

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


- out: +out:

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

+
+	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

 	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

+				  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

 	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

+		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

-						       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

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


preserve original indentation :)

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

+	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

 		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

+
+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

 };

+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 <etienne.carriere@linaro.org> (minor inline comment style issue)

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

 	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 <valerii_chubar@epam.com> Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>

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

Conversation

varder commented Sep 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

etienne-lms 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!

etienne-lms Sep 29, 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!

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!

etienne-lms 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!

jforissier commented Sep 30, 2022

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!

etienne-lms Oct 3, 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!

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