From 7bb53423a16dc1321be008b5d0757c330f9fcd10 Mon Sep 17 00:00:00 2001 From: Hannu Honkanen Date: Thu, 18 Oct 2018 16:56:06 +0300 Subject: [PATCH] Code cleanup and minor corrections according to review. pkcs15-lib: Extractable keys are now marked as native. Check return value of check_key_compatibility in more explicit way to avoid misunderstandings. --- src/libopensc/card-myeid.c | 4 ++-- src/libopensc/padding.c | 2 +- src/libopensc/pkcs15-sec.c | 19 +++++++++---------- src/libopensc/pkcs15-skey.c | 6 ++---- src/pkcs11/framework-pkcs15.c | 15 +++++++-------- src/pkcs11/mechanism.c | 6 ++---- src/pkcs11/pkcs11-object.c | 2 +- src/pkcs15init/pkcs15-lib.c | 20 +++++++------------- 8 files changed, 31 insertions(+), 43 deletions(-) diff --git a/src/libopensc/card-myeid.c b/src/libopensc/card-myeid.c index d74f0090..c652973c 100644 --- a/src/libopensc/card-myeid.c +++ b/src/libopensc/card-myeid.c @@ -1358,7 +1358,7 @@ static int myeid_unwrap_key(struct sc_card *card, const u8 *crgram, size_t crgra } else { - if (crgram_len > MYEID_MAX_APDU_DATA_LEN) + if (crgram_len >= MYEID_MAX_APDU_DATA_LEN) { /* padding indicator byte, 0x81 = first half of 2048 bit cryptogram */ sbuf[0] = 0x81; @@ -1380,7 +1380,7 @@ static int myeid_unwrap_key(struct sc_card *card, const u8 *crgram, size_t crgra LOG_TEST_RET(card->ctx, r, "APDU transmit failed"); if (apdu.sw1 == 0x90 && apdu.sw2 == 0x00) { - if (crgram_len > MYEID_MAX_APDU_DATA_LEN) + if (crgram_len >= MYEID_MAX_APDU_DATA_LEN) { sc_format_apdu(card, &apdu, SC_APDU_CASE_3_SHORT, 0x2A, 0x00, p2); diff --git a/src/libopensc/padding.c b/src/libopensc/padding.c index cefc10c9..f4e6a7e4 100644 --- a/src/libopensc/padding.c +++ b/src/libopensc/padding.c @@ -505,7 +505,7 @@ int sc_get_encoding_flags(sc_context_t *ctx, *sflags = SC_ALGORITHM_RSA_PAD_PKCS1 | SC_ALGORITHM_RSA_HASH_NONE; *pflags = iflags & SC_ALGORITHM_RSA_HASHES; - } else if ((iflags & SC_ALGORITHM_AES) == SC_ALGORITHM_AES) { /* TODO: seems like this constant does not belong to the same set of flags used form asymmetric algos. Fix this! */ + } else if ((iflags & SC_ALGORITHM_AES) == SC_ALGORITHM_AES) { /* TODO: seems like this constant does not belong to the same set of flags used form asymmetric algos. Fix this! */ *sflags = 0; *pflags = 0; diff --git a/src/libopensc/pkcs15-sec.c b/src/libopensc/pkcs15-sec.c index 47e8e3ad..b71e069f 100644 --- a/src/libopensc/pkcs15-sec.c +++ b/src/libopensc/pkcs15-sec.c @@ -534,8 +534,8 @@ int sc_pkcs15_wrap(struct sc_pkcs15_card *p15card, senv.algorithm_flags = sec_flags; if ((sec_flags & (SC_ALGORITHM_AES_CBC | SC_ALGORITHM_AES_CBC_PAD)) > 0) { - senv_param = (sc_sec_env_param_t) { SC_SEC_ENV_PARAM_IV, (void*) param, paramlen }; - LOG_TEST_RET(ctx, sec_env_add_param(&senv, &senv_param), "failed to add IV to security environment"); + senv_param = (sc_sec_env_param_t) { SC_SEC_ENV_PARAM_IV, (void*) param, paramlen }; + LOG_TEST_RET(ctx, sec_env_add_param(&senv, &senv_param), "failed to add IV to security environment"); } out = cryptogram; @@ -544,14 +544,13 @@ int sc_pkcs15_wrap(struct sc_pkcs15_card *p15card, *poutlen); if (r > -1) { - if (*crgram_len < (unsigned) r) { - *poutlen = r; - if (out != NULL) /* if NULL, return success and required buffer length by PKCS#11 convention */ - LOG_TEST_RET(ctx, SC_ERROR_BUFFER_TOO_SMALL, "Buffer too small to hold the wrapped key."); - } - - *poutlen = r; - } + if (*crgram_len < (unsigned) r) { + *poutlen = r; + if (out != NULL) /* if NULL, return success and required buffer length by PKCS#11 convention */ + LOG_TEST_RET(ctx, SC_ERROR_BUFFER_TOO_SMALL, "Buffer too small to hold the wrapped key."); + } + *poutlen = r; + } LOG_FUNC_RETURN(ctx, r); } diff --git a/src/libopensc/pkcs15-skey.c b/src/libopensc/pkcs15-skey.c index be19f3d7..4687d8c7 100644 --- a/src/libopensc/pkcs15-skey.c +++ b/src/libopensc/pkcs15-skey.c @@ -152,10 +152,8 @@ sc_pkcs15_decode_skdf_entry(struct sc_pkcs15_card *p15card, struct sc_pkcs15_obj if (asn1_skey_choice[0].flags & SC_ASN1_PRESENT) { obj->type = SC_PKCS15_TYPE_SKEY_GENERIC; - /* info.key_type = CKK_AES;*/ /* defaults to CKK_AES. TODO: implement choice 15 and GenericKeyAttributes like defined in ISO7816-15 and set key type accordingly. */ - - /* Check key type. framework-pkcs15 recognizes one type per key, and AES is the only algorithm supported for - * SKEY_GENERIC type keys, so just check if this key is AES compatible. */ + /* Check key type. framework-pkcs15 recognizes one type per key, and AES is the only algorithm supported for + * SKEY_GENERIC type keys, so just check if this key is AES compatible. */ for (i = 0; i < SC_MAX_SUPPORTED_ALGORITHMS && info.algo_refs[i] != 0; i++) { for (ii = 0; ii < SC_MAX_SUPPORTED_ALGORITHMS && p15card->tokeninfo != 0; ii++) { diff --git a/src/pkcs11/framework-pkcs15.c b/src/pkcs11/framework-pkcs15.c index ba63b25a..ec2d5a6b 100644 --- a/src/pkcs11/framework-pkcs15.c +++ b/src/pkcs11/framework-pkcs15.c @@ -131,11 +131,10 @@ struct pkcs15_skey_object { struct sc_pkcs15_skey *valueXXXX; }; -#define is_secret_key(obj) ((__p15_type(obj) & SC_PKCS15_TYPE_CLASS_MASK) == SC_PKCS15_TYPE_SKEY) +#define is_skey(obj) ((__p15_type(obj) & SC_PKCS15_TYPE_CLASS_MASK) == SC_PKCS15_TYPE_SKEY) #define skey_flags base.base.flags #define skey_p15obj base.p15_object -#define is_skey(obj) ((__p15_type(obj) & SC_PKCS15_TYPE_CLASS_MASK) == SC_PKCS15_TYPE_SKEY_OBJECT) extern struct sc_pkcs11_object_ops pkcs15_cert_ops; extern struct sc_pkcs11_object_ops pkcs15_prkey_ops; @@ -1346,7 +1345,7 @@ _add_pin_related_objects(struct sc_pkcs11_slot *slot, struct sc_pkcs15_object *p sc_log(context, "Slot:%p Adding cert object %d to PIN '%.*s'", slot, i, (int) sizeof pin_obj->label, pin_obj->label); pkcs15_add_object(slot, obj, NULL); } - else if (is_secret_key(obj)) { + else if (is_skey(obj)) { sc_log(context, "Slot:%p Adding secret key object %d to PIN '%.*s'", slot, i, (int) sizeof pin_obj->label, pin_obj->label); pkcs15_add_object(slot, obj, NULL); } @@ -2361,7 +2360,7 @@ pkcs15_create_secret_key(struct sc_pkcs11_slot *slot, struct sc_profile *profile if(_token == FALSE) args.session_object = 1; /* store the object on card for duration of the session. */ - args.value_len = args.value_len * 8; /* CKA_VALUE_LEN is number of bytes, PKCS#15 needs key length in bits */ + args.value_len = args.value_len * 8; /* CKA_VALUE_LEN is number of bytes, PKCS#15 needs key length in bits */ rc = sc_pkcs15init_store_secret_key(fw_data->p15_card, profile, &args, &key_obj); if (rc < 0) { rv = sc_to_cryptoki_error(rc, "C_CreateObject"); @@ -4768,15 +4767,15 @@ pkcs15_skey_get_attribute(struct sc_pkcs11_session *session, break; case CKA_TOKEN: check_attribute_buffer(attr, sizeof(CK_BBOOL)); - *(CK_BBOOL*)attr->pValue = skey->base.p15_object->session_object == 0; + *(CK_BBOOL*)attr->pValue = skey->base.p15_object->session_object == 0 ? CK_TRUE : CK_FALSE; break; case CKA_PRIVATE: check_attribute_buffer(attr, sizeof(CK_BBOOL)); - *(CK_BBOOL*)attr->pValue = (skey->base.p15_object->flags & SC_PKCS15_CO_FLAG_PRIVATE) != 0; + *(CK_BBOOL*)attr->pValue = (skey->base.p15_object->flags & SC_PKCS15_CO_FLAG_PRIVATE) != 0 ? CK_TRUE : CK_FALSE; break; case CKA_MODIFIABLE: check_attribute_buffer(attr, sizeof(CK_BBOOL)); - *(CK_BBOOL*)attr->pValue = (skey->base.p15_object->flags & 0x02) != 0; + *(CK_BBOOL*)attr->pValue = (skey->base.p15_object->flags & 0x02) != 0 ? CK_TRUE : CK_FALSE; /*TODO Why no definition of the flag */ break; case CKA_LABEL: @@ -4814,7 +4813,7 @@ pkcs15_skey_get_attribute(struct sc_pkcs11_session *session, check_attribute_buffer(attr, sizeof(CK_BBOOL)); *(CK_BBOOL*)attr->pValue = (((skey->base.p15_object->flags & SC_PKCS15_PRKEY_ACCESS_EXTRACTABLE) == SC_PKCS15_PRKEY_ACCESS_EXTRACTABLE) && (skey->base.p15_object->flags & SC_PKCS15_PRKEY_ACCESS_NEVEREXTRACTABLE) == 0 - && (skey->base.p15_object->flags & SC_PKCS15_PRKEY_ACCESS_ALWAYSSENSITIVE) == 0); + && (skey->base.p15_object->flags & SC_PKCS15_PRKEY_ACCESS_ALWAYSSENSITIVE) == 0) ? CK_TRUE : CK_FALSE; break; case CKA_VALUE_LEN: check_attribute_buffer(attr, sizeof(CK_ULONG)); diff --git a/src/pkcs11/mechanism.c b/src/pkcs11/mechanism.c index 48551120..609416d4 100644 --- a/src/pkcs11/mechanism.c +++ b/src/pkcs11/mechanism.c @@ -844,8 +844,7 @@ sc_pkcs11_wrap(struct sc_pkcs11_session *session, sc_pkcs11_mechanism_type_t *mt; CK_RV rv; - if (!session || !session->slot - || !(p11card = session->slot->p11card)) + if (!session || !session->slot || !(p11card = session->slot->p11card)) return CKR_ARGUMENTS_BAD; /* See if we support this mechanism type */ @@ -891,8 +890,7 @@ sc_pkcs11_unwrap(struct sc_pkcs11_session *session, CK_RV rv; - if (!session || !session->slot - || !(p11card = session->slot->p11card)) + if (!session || !session->slot || !(p11card = session->slot->p11card)) return CKR_ARGUMENTS_BAD; /* See if we support this mechanism type */ diff --git a/src/pkcs11/pkcs11-object.c b/src/pkcs11/pkcs11-object.c index 82e65caa..719386df 100644 --- a/src/pkcs11/pkcs11-object.c +++ b/src/pkcs11/pkcs11-object.c @@ -1041,7 +1041,7 @@ CK_RV C_WrapKey(CK_SESSION_HANDLE hSession, /* the session's handle */ { /* receives byte size of wrapped key */ CK_RV rv; CK_BBOOL can_wrap, - can_be_wrapped; + can_be_wrapped; CK_KEY_TYPE key_type; CK_ATTRIBUTE wrap_attribute = { CKA_WRAP, &can_wrap, sizeof(can_wrap) }; CK_ATTRIBUTE extractable_attribute = { CKA_EXTRACTABLE, &can_be_wrapped, sizeof(can_be_wrapped) }; diff --git a/src/pkcs15init/pkcs15-lib.c b/src/pkcs15init/pkcs15-lib.c index eda7071e..a3ae08c8 100644 --- a/src/pkcs15init/pkcs15-lib.c +++ b/src/pkcs15init/pkcs15-lib.c @@ -1215,7 +1215,6 @@ sc_pkcs15init_init_prkdf(struct sc_pkcs15_card *p15card, struct sc_profile *prof if (keyargs->access_flags & SC_PKCS15_PRKEY_ACCESS_EXTRACTABLE) { key_info->access_flags &= ~SC_PKCS15_PRKEY_ACCESS_NEVEREXTRACTABLE; - key_info->native = 0; } /* Select a Key ID if the user didn't specify one, @@ -1348,10 +1347,6 @@ sc_pkcs15init_init_skdf(struct sc_pkcs15_card *p15card, struct sc_profile *profi if (keyargs->access_flags & SC_PKCS15_PRKEY_ACCESS_EXTRACTABLE) { key_info->access_flags &= ~SC_PKCS15_PRKEY_ACCESS_NEVEREXTRACTABLE; - /* The following commented out. I interpret PKCS#15 so that native only means that a key - * can be used in on card crypto. Such key can be extractable (can be wrapped), so IMO this - logic is not correct. */ - /* key_info->native = 0; */ } if (keyargs->session_object > 0) @@ -1461,7 +1456,7 @@ sc_pkcs15init_generate_key(struct sc_pkcs15_card *p15card, struct sc_profile *pr if (check_key_compatibility(p15card, keygen_args->prkey_args.key.algorithm, &keygen_args->prkey_args.key, keygen_args->prkey_args.x509_usage, - keybits, SC_ALGORITHM_ONBOARD_KEY_GEN)) + keybits, SC_ALGORITHM_ONBOARD_KEY_GEN) != SC_SUCCESS) LOG_TEST_RET(ctx, SC_ERROR_NOT_SUPPORTED, "Cannot generate key with the given parameters"); if (profile->ops->generate_key == NULL) @@ -1585,7 +1580,7 @@ sc_pkcs15init_generate_secret_key(struct sc_pkcs15_card *p15card, struct sc_prof LOG_TEST_RET(ctx, r, "Invalid key size"); if (check_key_compatibility(p15card, skey_args->algorithm, NULL, 0, - keybits, SC_ALGORITHM_ONBOARD_KEY_GEN)) + keybits, SC_ALGORITHM_ONBOARD_KEY_GEN) != SC_SUCCESS) LOG_TEST_RET(ctx, SC_ERROR_NOT_SUPPORTED, "Cannot generate key with the given parameters"); if (profile->ops->generate_key == NULL) @@ -1654,11 +1649,11 @@ sc_pkcs15init_store_private_key(struct sc_pkcs15_card *p15card, struct sc_profil LOG_TEST_RET(ctx, keybits, "Invalid private key size"); /* Now check whether the card is able to handle this key */ - if (check_key_compatibility(p15card, key.algorithm, &key, keyargs->x509_usage, keybits, 0)) { + if (check_key_compatibility(p15card, key.algorithm, &key, keyargs->x509_usage, keybits, 0) != SC_SUCCESS) { /* Make sure the caller explicitly tells us to store * the key as extractable. */ if (!(keyargs->access_flags & SC_PKCS15_PRKEY_ACCESS_EXTRACTABLE)) - LOG_TEST_RET(ctx, SC_ERROR_INCOMPATIBLE_KEY, "Card does not support this key."); + LOG_TEST_RET(ctx, SC_ERROR_INCOMPATIBLE_KEY, "Card does not support this key for crypto. Cannot store it as non extractable."); } /* Select a intrinsic Key ID if user didn't specify one */ @@ -1887,12 +1882,11 @@ sc_pkcs15init_store_secret_key(struct sc_pkcs15_card *p15card, struct sc_profile LOG_FUNC_CALLED(ctx); /* Now check whether the card is able to handle this key */ - if (check_key_compatibility(p15card, keyargs->algorithm, NULL, 0, keyargs->key.data_len * 8, 0)) { + if (check_key_compatibility(p15card, keyargs->algorithm, NULL, 0, keyargs->key.data_len * 8, 0) != SC_SUCCESS) { /* Make sure the caller explicitly tells us to store * the key as extractable. */ - /* Commented out. I don't understand why one couldn't store a key as non extractable. -HH */ -/* if (!(keyargs->access_flags & SC_PKCS15_PRKEY_ACCESS_EXTRACTABLE)) - LOG_TEST_RET(ctx, SC_ERROR_INCOMPATIBLE_KEY, "Card does not support this key.");*/ + if (!(keyargs->access_flags & SC_PKCS15_PRKEY_ACCESS_EXTRACTABLE)) + LOG_TEST_RET(ctx, SC_ERROR_INCOMPATIBLE_KEY, "Card does not support this key for crypto. Cannot store it as non extractable."); } #ifdef ENABLE_OPENSSL