WIP to accept pkcs11 ec_point encoded in bit string or byte string

See opensc issue OpenSC#3000

 On branch X25519-improvements-2
 Changes to be committed:
	modified:   libopensc/card-openpgp.c
	modified:   libopensc/pkcs15-pubkey.c
	modified:   tools/pkcs11-tool.c
	modified:   tools/pkcs15-init.c

src/libopensc/card-openpgp.c

@@ -2874,7 +2874,7 @@ pgp_update_pubkey_blob(sc_card_t *card, sc_cardctl_openpgp_keygen_info_t *key_in
 		memset(&p15pubkey, 0, sizeof(p15pubkey));
 		p15pubkey.algorithm = SC_ALGORITHM_EC;
 		p15pubkey.u.ec.ecpointQ.value = key_info->u.ec.ecpoint;
-		p15pubkey.u.ec.ecpointQ.len = key_info->u.ec.ecpoint_len;
+		p15pubkey.u.ec.ecpointQ.len = BYTES4BITS(key_info->u.ec.ecpoint_len);
 	}
 	else
 		LOG_FUNC_RETURN(card->ctx, SC_ERROR_INVALID_ARGUMENTS);

src/libopensc/pkcs15-pubkey.c

@@ -536,9 +536,13 @@ static struct sc_asn1_entry c_asn1_gostr3410_pub_coefficients[C_ASN1_GOSTR3410_P
 		{ NULL, 0, 0, 0, NULL, NULL }
 };
 
-#define C_ASN1_EC_POINTQ_SIZE 2
+
+/* older incorrect implementation may have encoded as OCTET STRING */
+/* accept either */
+#define C_ASN1_EC_POINTQ_SIZE 3
 static struct sc_asn1_entry c_asn1_ec_pointQ[C_ASN1_EC_POINTQ_SIZE] = {
-		{ "ecpointQ", SC_ASN1_OCTET_STRING, SC_ASN1_TAG_OCTET_STRING, SC_ASN1_ALLOC, NULL, NULL },
+		{ "ecpointQ", SC_ASN1_BIT_STRING_NI, SC_ASN1_TAG_BIT_STRING, SC_ASN1_OPTIONAL | SC_ASN1_ALLOC, NULL, NULL },
+		{ "ecpointQ-OS",SC_ASN1_OCTET_STRING, SC_ASN1_TAG_OCTET_STRING, SC_ASN1_OPTIONAL | SC_ASN1_ALLOC, NULL, NULL },
 		{ NULL, 0, 0, 0, NULL, NULL }
 };
 
@@ -629,7 +633,8 @@ sc_pkcs15_encode_pubkey_gostr3410(sc_context_t *ctx,
 }
 
 /*
- * We are storing the ec_pointQ as u8 string. not as DER
+ * We are storing the ec_pointQ as u8 string not as DER
+ * Will accept either BIT STRING or OCTET STRING
  */
 int
 sc_pkcs15_decode_pubkey_ec(sc_context_t *ctx,
@@ -644,25 +649,22 @@ sc_pkcs15_decode_pubkey_ec(sc_context_t *ctx,
 	LOG_FUNC_CALLED(ctx);
 	sc_copy_asn1_entry(c_asn1_ec_pointQ, asn1_ec_pointQ);
 	sc_format_asn1_entry(asn1_ec_pointQ + 0, &ecpoint_data, &ecpoint_len, 1);
-	r = sc_asn1_decode(ctx, asn1_ec_pointQ, buf, buflen, NULL, NULL);
-	if (r < 0) {
+	sc_format_asn1_entry(asn1_ec_pointQ + 1, &ecpoint_data, &ecpoint_len, 1);
+	r = sc_asn1_decode_choice(ctx, asn1_ec_pointQ, buf, buflen, NULL, NULL);
+	if (r < 0 || ecpoint_len == 0) {
 		free(ecpoint_data);
+		/* TODO DEE fix if r == 0 */
 		LOG_TEST_RET(ctx, r, "ASN.1 decoding failed");
 	}
 
-	if (ecpoint_len == 0 || *ecpoint_data != 0x04) {
+	if (*ecpoint_data != 0x04) {
 		free(ecpoint_data);
 		LOG_TEST_RET(ctx, SC_ERROR_NOT_SUPPORTED, "Supported only uncompressed EC pointQ value");
 	}
 
 	key->ecpointQ.len = ecpoint_len;
 	key->ecpointQ.value = ecpoint_data;
 
-	/*
-	 * Only get here if raw point is stored in pkcs15 without curve name
-	 * spki has the curvename, so we can get the field_length
-	 * Following only true for curves that are multiple of 8
-	 */
 	key->params.field_length = (ecpoint_len - 1)/2 * 8;
 	LOG_FUNC_RETURN(ctx, SC_SUCCESS);
 }
@@ -673,10 +675,19 @@ sc_pkcs15_encode_pubkey_ec(sc_context_t *ctx, struct sc_pkcs15_pubkey_ec *key,
 		u8 **buf, size_t *buflen)
 {
 	struct sc_asn1_entry asn1_ec_pointQ[C_ASN1_EC_POINTQ_SIZE];
+	size_t  key_len;
+	volatile int gdb_test = 0; /* so can reset via gdb for testing old way OS*/
 
 	LOG_FUNC_CALLED(ctx);
 	sc_copy_asn1_entry(c_asn1_ec_pointQ, asn1_ec_pointQ);
-	sc_format_asn1_entry(asn1_ec_pointQ + 0, key->ecpointQ.value, &key->ecpointQ.len, 1);
+
+	if (gdb_test == 0) {
+		key_len  = key->ecpointQ.len * 8; /* encode in bit string */
+		sc_format_asn1_entry(asn1_ec_pointQ + 0, key->ecpointQ.value, &key_len, 1);
+	} else {
+		key_len  = key->ecpointQ.len;
+		sc_format_asn1_entry(asn1_ec_pointQ + 1, key->ecpointQ.value, &key_len, 1);
+	}
 
 	LOG_FUNC_RETURN(ctx,
 			sc_asn1_encode(ctx, asn1_ec_pointQ, buf, buflen));
@@ -685,13 +696,34 @@ sc_pkcs15_encode_pubkey_ec(sc_context_t *ctx, struct sc_pkcs15_pubkey_ec *key,
 /*
  * all "ec" keys uses same pubkey format, keep this external entrypoint
  * keys are just byte strings.
+ *  will accept in either BIT STRING or OCTET STRING
  */
 int
 sc_pkcs15_decode_pubkey_eddsa(sc_context_t *ctx,
 		struct sc_pkcs15_pubkey_ec *key,
 		const u8 *buf, size_t buflen)
 {
-	return sc_pkcs15_decode_pubkey_ec(ctx, key, buf, buflen);
+	int r;
+	u8 * ecpoint_data = NULL;
+	size_t ecpoint_len;
+	struct sc_asn1_entry asn1_ec_pointQ[C_ASN1_EC_POINTQ_SIZE];
+
+	LOG_FUNC_CALLED(ctx);
+	sc_copy_asn1_entry(c_asn1_ec_pointQ, asn1_ec_pointQ);
+	sc_format_asn1_entry(asn1_ec_pointQ + 0, &ecpoint_data, &ecpoint_len, 1);
+	sc_format_asn1_entry(asn1_ec_pointQ + 1, &ecpoint_data, &ecpoint_len, 1);
+	r = sc_asn1_decode_choice(ctx, asn1_ec_pointQ, buf, buflen, NULL, NULL);
+	if (r < 0 || ecpoint_data == NULL) {
+		free(ecpoint_data);
+		/* TODO DEE fix if r == 0 */
+		LOG_TEST_RET(ctx, r, "ASN.1 decoding failed");
+	}
+
+	key->ecpointQ.len = ecpoint_len;
+	key->ecpointQ.value = ecpoint_data;
+	key->params.field_length = ecpoint_len * 8;
+
+	LOG_FUNC_RETURN(ctx, SC_SUCCESS);
 }
 
 /*
@@ -701,6 +733,7 @@ int
 sc_pkcs15_encode_pubkey_eddsa(sc_context_t *ctx, struct sc_pkcs15_pubkey_ec *key,
 		u8 **buf, size_t *buflen)
 {
+	/* same format */
 	return sc_pkcs15_encode_pubkey_ec(ctx, key, buf, buflen);
 }
 
@@ -712,8 +745,10 @@ sc_pkcs15_encode_pubkey(sc_context_t *ctx, struct sc_pkcs15_pubkey *key,
 		return sc_pkcs15_encode_pubkey_rsa(ctx, &key->u.rsa, buf, len);
 	if (key->algorithm == SC_ALGORITHM_GOSTR3410)
 		return sc_pkcs15_encode_pubkey_gostr3410(ctx, &key->u.gostr3410, buf, len);
-	if (key->algorithm == SC_ALGORITHM_EC || key->algorithm == SC_ALGORITHM_EDDSA || key->algorithm == SC_ALGORITHM_XEDDSA)
+	if (key->algorithm == SC_ALGORITHM_EC)
 		return sc_pkcs15_encode_pubkey_ec(ctx, &key->u.ec, buf, len);
+	if (key->algorithm == SC_ALGORITHM_EDDSA || key->algorithm == SC_ALGORITHM_XEDDSA)
+		return sc_pkcs15_encode_pubkey_eddsa(ctx, &key->u.ec, buf, len);
 
 	sc_log(ctx, "Encoding of public key type %lu not supported", key->algorithm);
 	LOG_FUNC_RETURN(ctx, SC_ERROR_NOT_SUPPORTED);
@@ -829,8 +864,10 @@ sc_pkcs15_decode_pubkey(sc_context_t *ctx, struct sc_pkcs15_pubkey *key,
 		return sc_pkcs15_decode_pubkey_rsa(ctx, &key->u.rsa, buf, len);
 	if (key->algorithm == SC_ALGORITHM_GOSTR3410)
 		return sc_pkcs15_decode_pubkey_gostr3410(ctx, &key->u.gostr3410, buf, len);
-	if (key->algorithm == SC_ALGORITHM_EC  || key->algorithm == SC_ALGORITHM_EDDSA || key->algorithm == SC_ALGORITHM_XEDDSA)
+	if (key->algorithm == SC_ALGORITHM_EC)
 		return sc_pkcs15_decode_pubkey_ec(ctx, &key->u.ec, buf, len);
+	if (key->algorithm == SC_ALGORITHM_EDDSA || key->algorithm == SC_ALGORITHM_XEDDSA)
+		return sc_pkcs15_decode_pubkey_eddsa(ctx, &key->u.ec, buf, len);
 
 	sc_log(ctx, "Decoding of public key type %lu not supported", key->algorithm);
 	return SC_ERROR_NOT_SUPPORTED;

src/tools/pkcs11-tool.c

@@ -2441,6 +2441,7 @@ static void verify_signature(CK_SLOT_ID slot, CK_SESSION_HANDLE session,
 			unsigned char rs_buffer[512];
 			bytes = getEC_POINT(session, key, &len);
 			free(bytes);
+			/* TODO DEE EDDSA and EC_POINT returned in BIT STRING needs some work */
 			/*
 			 * (We only support uncompressed for now)
 			 * Uncompressed EC_POINT is DER OCTET STRING of "04||x||y"
@@ -5190,24 +5191,24 @@ show_key(CK_SESSION_HANDLE sess, CK_OBJECT_HANDLE obj)
 		}
 		if (pub) {
 			unsigned char *bytes = NULL;
-			unsigned long ksize;
+			unsigned long ksize = 0;
 			unsigned int n;
-			int body_len = 0;
+			unsigned long body_len = 0;
 
-			bytes = getEC_POINT(sess, obj, &size);
+			bytes = getEC_POINT(sess, obj, &ksize);
 			/*
 			 * simple parse of DER BIT STRING 0x03 or OCTET STRING 0x04
 			 * good to 65K bytes  
 			 */
-			if (size > 3 && (bytes[0] == 0x03 || bytes[0] == 0x04)) {
-				if (bytes[1] <= 127 && size == bytes[1] + 2) {
-					body_len = size - 2;
+			if (ksize > 3 && (bytes[0] == 0x03 || bytes[0] == 0x04)) {
+				if (bytes[1] <= 127 && ksize == (unsigned long)(bytes[1] + 2)) {
+					body_len = ksize - 2;
 				} else
-				if (bytes[1] = 0x81 && size == bytes[2] + 3) {
-					body_len = size - 3;
+				if (bytes[1] == 0x81 && size == ((unsigned long)bytes[2] + 3)) {
+					body_len = ksize - 3;
 				} else
-				if (bytes[1] = 0x82 && size == bytes[2] << 8 + bytes[3] + 4) {
-					body_len = size - 4;
+				if (bytes[1] == 0x82 && size == ((unsigned long)(bytes[2] << 8) + (unsigned long)bytes[3] + 4)) {
+					body_len = ksize - 4;
 				}
 			}
 			/* With BIT STRING remove unused bits in last byte indicator */
@@ -5853,6 +5854,7 @@ static int read_object(CK_SESSION_HANDLE session)
 
 			value = getEC_POINT(session, obj, &len);
 			/* PKCS#11-compliant modules should return ASN1_OCTET_STRING */
+			/* TODO DEE Should be returned encoded in BIT STRING, Need to accept both */
 			a = value;
 			os = d2i_ASN1_OCTET_STRING(NULL, &a, (long)len);
 #if OPENSSL_VERSION_NUMBER < 0x30000000L
@@ -5959,6 +5961,7 @@ static int read_object(CK_SESSION_HANDLE session)
 
 			value = getEC_POINT(session, obj, &len);
 			/* PKCS#11-compliant modules should return ASN1_OCTET_STRING */
+			/* TODO DEE should be in BIT STRING accept both */
 			a = value;
 			os = d2i_ASN1_OCTET_STRING(NULL, &a, (long)len);
 			if (!os) {
@@ -8184,6 +8187,7 @@ static void test_ec(CK_SLOT_ID slot, CK_SESSION_HANDLE session)
 		return;
 	}
 	getEC_POINT(session, pub_key, &ec_point_len);
+	/* TODO only looking at length of encoded EC_POINT. May be in BIT STRING or OCTET STRING */
 	if (ec_point_len < 5 || ec_point_len > 10000) {
 		printf("ERR: GetAttribute(pubkey, CKA_EC_POINT) doesn't seem to work\n");
 		return;

src/tools/pkcs15-init.c

@@ -778,7 +778,11 @@ parse_alg_spec(const struct alg_spec *types, const char *spec, unsigned int *key
 
 	if (*spec)   {
 		if (isalpha((unsigned char)*spec)
-				&& (algorithm == SC_ALGORITHM_EC || algorithm == SC_ALGORITHM_EDDSA || SC_ALGORITHM_XEDDSA)
+			&& algorithm == SC_ALGORITHM_EC && prkey)
+			prkey->u.ec.params.named_curve = strdup(spec);
+		else 
+		if (isalpha((unsigned char)*spec)
+				&& (algorithm == SC_ALGORITHM_EDDSA || SC_ALGORITHM_XEDDSA)
 				&& prkey) {
 			prkey->u.ec.params.named_curve = strdup(types[types_idx].spec); /* copy correct case */
 		} else {