ECC ecpointQ Fixes

The original ECC code in OpenSC stored the ecpointQ as a DER encoded OCTET STRING.
Shortly before 0.13.0, code changes where made to store the ecpointQ as raw data
without the DER encoding.

Only some of the code was changed to support this but not all, and the comments
that said the ecpointQ was in DER where not changed either.

Some card drivers continued to work, using the original code in all place,
while some cards failed, as they where using a mixture of original code and
0.13.0 code.

This commit fixes these problems.

The ecpointQ is stored in raw format

A new structure type sc_pkcs15_u8 is defined.

The ecpointQ are changed to use the struct sc_pkcs15_u8. This was done to avoid
 the confusion of using struct sc_pkcs15_der to hold non-DER encoded data.
(There may be other uses for this too...)

Comments are change is many places.

sc_pkcs15_decode_pubkey_ec was fixed to store the raw ecpointQ correctly.

sc_pkcs15_pubkey_from_spki was change to get the sc_ec_params from the alg_id
and fix up u.ec.params. Unfortunately the OpenSC code has two places EC parameters
are stored. They can get out of sync, or there may still be code
that looks in the wrng oplace. o(TODO get it to only only place.)

The u.ec.params.field_length is now set in a number of places, as this is need
in many of the PKCS#11 routines.

framework-pkcs15.c will now correctly return the DER encode ecpointQ,
for the CKA_EC_POINT attribute using pubkey->data which has the DER encoding
for the ecpointQ.

framework-pkcs15.c will look for the EC parameters in either the u.ec.params.der,
or in the alg_id->params. (TODO get it to only only place.)

pkcs15-myeid.c has some comments, as it looks like the code is storing a TLV
rather then a DER encoding of the ecpointQ. With the wrong encoding PKCS#11 will
return the wrong attribute for CKA_ECDSA_PARAMS.

pkcs15-piv.c is changed so emulation of a pubkey taken from a certificate will
work correctly.

src/libopensc/pkcs15-piv.c

@@ -912,7 +912,7 @@ sc_debug(card->ctx, SC_LOG_DEBUG_NORMAL, "DEE Adding pin %d label=%s",i, label);
 		}
 		else if (ckis[i].pubkey_from_cert && ckis[i].pubkey_from_cert->data.value) {
 		    sc_der_copy(&pubkey_obj.content, &ckis[i].pubkey_from_cert->data);
-		    sc_pkcs15_free_pubkey(ckis[i].pubkey_from_cert);
+		    pubkey_obj.emulated = ckis[i].pubkey_from_cert;
 		}
 
 		sc_debug(card->ctx, SC_LOG_DEBUG_NORMAL,"adding pubkey for %d keyalg=%d",i, ckis[i].key_alg);

src/libopensc/pkcs15-pubkey.c

@@ -602,9 +602,7 @@ sc_pkcs15_encode_pubkey_gostr3410(sc_context_t *ctx,
 }
 
 /*
- * We are storing the ec_pointQ as a octet string.
- * Thus we will just copy the string.
- * But to get the field length we decode it.
+ * We are storing the ec_pointQ as u8 string. not as DER
  */
 int
 sc_pkcs15_decode_pubkey_ec(sc_context_t *ctx,
@@ -620,23 +618,18 @@ sc_pkcs15_decode_pubkey_ec(sc_context_t *ctx,
 	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)
-		LOG_TEST_RET(ctx, r, "ASN.1 encoding failed");
+		LOG_TEST_RET(ctx, r, "ASN.1 decoding failed");
 
-	sc_log(ctx, "DEE-EC key=%p, buf=%p, buflen=%d", key, buf, buflen);
-	key->ecpointQ.value = malloc(buflen);
-	if (key->ecpointQ.value == NULL)
-		return SC_ERROR_OUT_OF_MEMORY;
+	sc_log(ctx, "decode-EC key=%p, buf=%p, buflen=%d", key, buf, buflen);
 
-	key->ecpointQ.len = buflen;
-	memcpy(key->ecpointQ.value, buf, buflen);
+	key->ecpointQ.len = ecpoint_len;
+	key->ecpointQ.value = ecpoint_data; 
 
 	/* An uncompressed ecpoint is of the form 04||x||y
 	 * The 04 indicates uncompressed
 	 * x and y are same size, and field_length = sizeof(x) in bits. */
 	/* TODO: -DEE  support more then uncompressed */
 	key->params.field_length = (ecpoint_len - 1)/2 * 8;
-	if (ecpoint_data)
-		free (ecpoint_data);
 
 	return r;
 }
@@ -1010,26 +1003,45 @@ sc_pkcs15_pubkey_from_spki(sc_context_t *ctx, sc_pkcs15_pubkey_t ** outpubkey,
 	}
 	memcpy(pubkey->alg_id, &pk_alg, sizeof(struct sc_algorithm_id));
 	pubkey->algorithm = pk_alg.algorithm;
+	pk_alg.params = NULL;
 
 	sc_log(ctx, "DEE pk_alg.algorithm=%d", pk_alg.algorithm);
 
 	/* pk.len is in bits at this point */
 	switch (pk_alg.algorithm) {
 	case SC_ALGORITHM_EC:
+		/*
+		 * TODO we really should only have params in one place!
+		 * The alg_id may have the sc_ec_params, 
+		 * we want to get the curve OID into the 
+		 * u.ec.params and get the field length too.  
+		 */
+		if (pubkey->alg_id->params) {
+		    struct sc_ec_params  * ecp = (struct sc_ec_params *)pubkey->alg_id->params;
+		    pubkey->u.ec.params.der.value = malloc(ecp->der_len);
+		    if (pubkey->u.ec.params.der.value) {
+			memcpy(&pubkey->u.ec.params.der.value, &ecp->der, ecp->der_len);
+			pubkey->u.ec.params.der.len = ecp->der_len;
+			sc_pkcs15_fix_ec_parameters(ctx,&pubkey->u.ec.params);
+		    }
+		}
 		/*
 		 * For most keys, the above ASN.1 parsing of a key works, but for EC keys,
-		 * the ec_pointQ in a certificate is stored in a bitstring, but
-		 * in PKCS#11 it is an octet string and we just decoded its
-		 * contents from the bitstring in the certificate. So we need to encode it
-		 * back to an octet string so we can store it as an octet string.
+		 * the ec_pointQ in a certificate is stored in the bitstring, in its raw format.
+		 * RSA for example is stored in the  bitstring, as a ASN1 DER 
+		 * So we encoded the raw ecpointQ  into ASN1 DER as the pubkey->data 
+		 * and let the sc_pkcs15_decode_pubkey below get the ecpointQ out later. 
 		 */
 		pk.len >>= 3;  /* Assume it is multiple of 8 */
-/*		pubkey->u.ec.field_length = (pk.len - 1)/2 * 8;  */
+		if (pubkey->u.ec.params.field_length == 0) 
+		    pubkey->u.ec.params.field_length = (pk.len - 1)/2 * 8;
 
 		sc_copy_asn1_entry(c_asn1_ec_pointQ, asn1_ec_pointQ);
 		sc_format_asn1_entry(&asn1_ec_pointQ[0], pk.value, &pk.len, 1);
 		r = sc_asn1_encode(ctx, asn1_ec_pointQ, &pubkey->data.value, &pubkey->data.len);
 
+		pk.value = NULL;
+
 		sc_log(ctx, "DEE r=%d data=%p:%d", r, pubkey->data.value, pubkey->data.len);
 		break;
 	default:

src/libopensc/pkcs15.h

@@ -154,6 +154,12 @@ struct sc_pkcs15_der {
 };
 typedef struct sc_pkcs15_der sc_pkcs15_der_t;
 
+struct sc_pkcs15_u8 {
+	u8 *		value;
+	size_t		len;
+};
+typedef struct sc_pkcs15_u8 sc_pkcs15_u8_t;
+
 struct sc_pkcs15_pubkey_rsa {
 	sc_pkcs15_bignum_t modulus;
 	sc_pkcs15_bignum_t exponent;
@@ -214,13 +220,13 @@ struct sc_pkcs15_gost_parameters {
 
 struct sc_pkcs15_pubkey_ec {
 	struct sc_pkcs15_ec_parameters params;
-	struct sc_pkcs15_der		ecpointQ; /* note this is der */
+	struct sc_pkcs15_u8		ecpointQ; /* This is NOT DER, just value and length */
 };
 
 struct sc_pkcs15_prkey_ec {
 	struct sc_pkcs15_ec_parameters params;
 	sc_pkcs15_bignum_t	privateD; /* note this is bignum */
-	struct sc_pkcs15_der		ecpointQ; /* note this is der */
+	struct sc_pkcs15_u8		ecpointQ; /* This is NOT DER, just value and length */
 };
 
 struct sc_pkcs15_pubkey_gostr3410 {

src/pkcs11/framework-pkcs15.c

@@ -3294,10 +3294,10 @@ pkcs15_prkey_get_attribute(struct sc_pkcs11_session *session,
 		check_attribute_buffer(attr, sizeof(CK_ULONG));
 		switch (prkey->prv_p15obj->type) {
 			case SC_PKCS15_TYPE_PRKEY_EC:
-				if (key)
+				if (key && key->u.ec.params.field_length > 0)
 					*(CK_ULONG *) attr->pValue = key->u.ec.params.field_length;
 				else
-					*(CK_ULONG *) attr->pValue = 384; /* TODO -DEE needs work */
+					*(CK_ULONG *) attr->pValue = (key->u.ec.ecpointQ.len - 1) / 2 *8;
 				return CKR_OK;
 			default:
 				*(CK_ULONG *) attr->pValue = prkey->prv_info->modulus_length;
@@ -4251,6 +4251,13 @@ get_ec_pubkey_params(struct sc_pkcs15_pubkey *key, CK_ATTRIBUTE_PTR attr)
 
 	switch (key->algorithm) {
 	case SC_ALGORITHM_EC:
+		/* TODO parms should not be in two places */
+		/* ec_params may be in key->alg_id or in key->u.ec */
+		if (key->u.ec.params.der.value) {
+		    check_attribute_buffer(attr,key->u.ec.params.der.len);
+		    memcpy(attr->pValue, key->u.ec.params.der.value, key->u.ec.params.der.len);
+		    return CKR_OK;
+		}
 		check_attribute_buffer(attr, ecp->der_len);
 		memcpy(attr->pValue, ecp->der, ecp->der_len);
 		return CKR_OK;
@@ -4266,8 +4273,16 @@ get_ec_pubkey_point(struct sc_pkcs15_pubkey *key, CK_ATTRIBUTE_PTR attr)
 
 	switch (key->algorithm) {
 	case SC_ALGORITHM_EC:
-		check_attribute_buffer(attr, key->u.ec.ecpointQ.len);
-		memcpy(attr->pValue, key->u.ec.ecpointQ.value, key->u.ec.ecpointQ.len);
+		/*
+		 * TODO Verify that pubkey->data is DER encoded ecpointQ
+		 * It should be. If not we need to encode the ecpointQ to
+		 * DER. But can not use sc_pkcs15_encode_pubkey as it
+		 * needs ctx, and ctx is not available here. 
+		 * Original ECC code stored ecpointQ as DER, so there was
+		 * no issue.
+		 */
+		check_attribute_buffer(attr, key->data.len);
+		memcpy(attr->pValue, key->data.value, key->data.len);
 		return CKR_OK;
 	}
 	return CKR_ATTRIBUTE_TYPE_INVALID;

src/pkcs15init/pkcs15-myeid.c

@@ -651,6 +651,17 @@ myeid_generate_key(struct sc_profile *profile, struct sc_pkcs15_card *p15card,
 			r = sc_card_ctl(card, SC_CARDCTL_MYEID_GETDATA, &data_obj);
 			LOG_TEST_RET(ctx, r, "Cannot get EC public key: 'MYEID_GETDATA' failed");
 
+			/*
+			 * TODO DEE - this looks like a bug... 
+			 * pubkey->u.ec.ecpointQ.value is just value. "04||X||Y" 
+			 * pubkey->data.value should be DER OCTET STRING
+			 * but 
+			 * pubkey->data.value looks like TLV with TAG if 0x86
+			 * and single byte length.  
+			 * Could call sc_pkcs15_encode_pubkey
+			 * to set pubkey->data.value
+			 */
+
 			pubkey->u.ec.ecpointQ.value = malloc(data_obj.DataLen - 2);
 			pubkey->u.ec.ecpointQ.len = data_obj.DataLen - 2;
 			pubkey->data.value = malloc(data_obj.DataLen);