From 51ff4f9015e4fce6eaf542f38dd7130539fdd456 Mon Sep 17 00:00:00 2001 From: Aveen Ismail Date: Thu, 21 Mar 2024 10:19:20 +0100 Subject: [PATCH] PKCS11: More code optimization --- pkcs11/tests/ecdh_sp800_test.c | 48 ++++++++-------------- pkcs11/util_pkcs11.c | 70 +++++++++++-------------------- pkcs11/util_pkcs11.h | 4 +- pkcs11/yubihsm_pkcs11.c | 75 +++++++++++++++------------------- 4 files changed, 75 insertions(+), 122 deletions(-) diff --git a/pkcs11/tests/ecdh_sp800_test.c b/pkcs11/tests/ecdh_sp800_test.c index 297af51b..96cc383b 100644 --- a/pkcs11/tests/ecdh_sp800_test.c +++ b/pkcs11/tests/ecdh_sp800_test.c @@ -197,8 +197,9 @@ static bool yh_derive_ecdh(CK_OBJECT_HANDLE priv_key, EVP_PKEY *peer_keypair, return true; } -static size_t do_hash(const EVP_MD *md, uint8_t *hashed, - unsigned char *raw_derived, size_t raw_derived_len) { +static unsigned int do_hash(const EVP_MD *md, uint8_t *hashed, + unsigned char *raw_derived, + size_t raw_derived_len) { EVP_MD_CTX *mdctx = NULL; unsigned int len = 0; @@ -211,13 +212,11 @@ static size_t do_hash(const EVP_MD *md, uint8_t *hashed, if (EVP_DigestInit_ex(mdctx, md, NULL) == 0) { fail("Failed to initialize digest"); - len = 0; goto h_free; } if (EVP_DigestUpdate(mdctx, raw_derived, raw_derived_len) != 1) { fail("Failed to update digest"); - len = 0; goto h_free; } if (EVP_DigestFinal_ex(mdctx, hashed, &len) != 1) { @@ -230,7 +229,7 @@ static size_t do_hash(const EVP_MD *md, uint8_t *hashed, if (mdctx != NULL) { EVP_MD_CTX_destroy(mdctx); } - return (size_t) len; + return len; } static size_t openssl_derive(CK_ULONG kdf, EVP_PKEY *private_key, @@ -517,34 +516,19 @@ int main(int argc, char **argv) { run_test(handle, CURVES[2], CKD_NULL, 256, yh_pubkey[2], yh_pubkey[2], true); run_test(handle, CURVES[3], CKD_NULL, 256, yh_pubkey[3], yh_pubkey[3], true); + CK_ULONG key_lens[3] = {128, 192, 256}; + for (int i = 0; i < CURVE_COUNT; i++) { - run_test(handle, CURVES[i], CKD_YUBICO_SHA1_KDF_SP800, 128, yh_pubkey[i], - yh_pubkey[i], true); - run_test(handle, CURVES[i], CKD_YUBICO_SHA1_KDF_SP800, 192, yh_pubkey[i], - yh_pubkey[i], true); - run_test(handle, CURVES[i], CKD_YUBICO_SHA1_KDF_SP800, 256, yh_pubkey[i], - yh_pubkey[i], true); - - run_test(handle, CURVES[i], CKD_YUBICO_SHA256_KDF_SP800, 128, yh_pubkey[i], - yh_pubkey[i], true); - run_test(handle, CURVES[i], CKD_YUBICO_SHA256_KDF_SP800, 192, yh_pubkey[i], - yh_pubkey[i], true); - run_test(handle, CURVES[i], CKD_YUBICO_SHA256_KDF_SP800, 256, yh_pubkey[i], - yh_pubkey[i], true); - - run_test(handle, CURVES[i], CKD_YUBICO_SHA384_KDF_SP800, 128, yh_pubkey[i], - yh_pubkey[i], true); - run_test(handle, CURVES[i], CKD_YUBICO_SHA384_KDF_SP800, 192, yh_pubkey[i], - yh_pubkey[i], true); - run_test(handle, CURVES[i], CKD_YUBICO_SHA384_KDF_SP800, 256, yh_pubkey[i], - yh_pubkey[i], true); - - run_test(handle, CURVES[i], CKD_YUBICO_SHA512_KDF_SP800, 128, yh_pubkey[i], - yh_pubkey[i], true); - run_test(handle, CURVES[i], CKD_YUBICO_SHA512_KDF_SP800, 192, yh_pubkey[i], - yh_pubkey[i], true); - run_test(handle, CURVES[i], CKD_YUBICO_SHA512_KDF_SP800, 256, yh_pubkey[i], - yh_pubkey[i], true); + for (size_t j = 0; j < 3; j++) { + run_test(handle, CURVES[i], CKD_YUBICO_SHA1_KDF_SP800, key_lens[j], + yh_pubkey[i], yh_pubkey[i], true); + run_test(handle, CURVES[i], CKD_YUBICO_SHA256_KDF_SP800, key_lens[j], + yh_pubkey[i], yh_pubkey[i], true); + run_test(handle, CURVES[i], CKD_YUBICO_SHA384_KDF_SP800, key_lens[j], + yh_pubkey[i], yh_pubkey[i], true); + run_test(handle, CURVES[i], CKD_YUBICO_SHA512_KDF_SP800, key_lens[j], + yh_pubkey[i], yh_pubkey[i], true); + } } printf("\n"); diff --git a/pkcs11/util_pkcs11.c b/pkcs11/util_pkcs11.c index 2e339809..977ff1a3 100644 --- a/pkcs11/util_pkcs11.c +++ b/pkcs11/util_pkcs11.c @@ -5308,78 +5308,54 @@ bool match_meta_attributes(yubihsm_pkcs11_session *session, return true; } -size_t ecdh_with_kdf(ecdh_session_key *shared_secret, size_t shared_secret_len, - CK_ULONG kdf, size_t value_len) { +CK_RV ecdh_with_kdf(ecdh_session_key *shared_secret, uint8_t *fixed_info, + size_t fixed_len, CK_ULONG kdf, size_t value_len) { - size_t out_len = 0; - size_t output_bits = 0; - - hash_t hash = _NONE; + hash_ctx hash = NULL; switch (kdf) { case CKD_NULL: // Do nothing break; case CKD_YUBICO_SHA1_KDF_SP800: - hash = _SHA1; - output_bits = 160; + hash_create(&hash, _SHA1); break; case CKD_YUBICO_SHA256_KDF_SP800: - hash = _SHA256; - output_bits = 256; + hash_create(&hash, _SHA256); break; case CKD_YUBICO_SHA384_KDF_SP800: - hash = _SHA384; - output_bits = 384; + hash_create(&hash, _SHA384); break; case CKD_YUBICO_SHA512_KDF_SP800: - hash = _SHA512; - output_bits = 512; + hash_create(&hash, _SHA512); break; default: DBG_ERR("Unsupported KDF"); return 0; } - if (hash == _NONE) { - out_len = shared_secret_len; - } else { - size_t l = value_len * 8; - size_t reps = 1 + l / output_bits; - if (reps > INT32_MAX) { - DBG_ERR("Too many repetitions"); - return 0; - } - - size_t ctr_len = 4; - uint8_t res[1024] = {0}; + if (hash) { + uint8_t ctr[sizeof(uint32_t)] = {0}; + uint8_t res[ECDH_KEY_BUF_SIZE] = {0}; size_t res_len = 0; - size_t hashed_len = sizeof(res); - - size_t k_len = shared_secret_len + ctr_len; - uint8_t *k = malloc(k_len); - memset(k, 0, ctr_len); - memcpy(k + ctr_len, shared_secret->ecdh_key, shared_secret_len); - for (size_t i = 0; i < reps; i++) { - increment_ctr(k, ctr_len); - - if (!hash_bytes(k, k_len, hash, res + res_len, &hashed_len)) { - DBG_ERR("Failed to apply hash function"); - return 0; - } - res_len += hashed_len; - } - - if (value_len > res_len) { - DBG_ERR("Derived key is too short"); - return 0; + while (res_len < value_len) { + increment_ctr(ctr, sizeof(ctr)); + hash_init(hash); + hash_update(hash, ctr, sizeof(ctr)); + hash_update(hash, shared_secret->ecdh_key, shared_secret->len); + hash_update(hash, fixed_info, fixed_len); + size_t len = sizeof(res) - res_len; + hash_final(hash, res + res_len, &len); + res_len += len; } memcpy(shared_secret->ecdh_key, res, value_len); memset(shared_secret->ecdh_key + value_len, 0, sizeof(shared_secret->ecdh_key) - value_len); - out_len = value_len; + shared_secret->len = value_len; + } else if (kdf != CKD_NULL) { + return CKR_MECHANISM_PARAM_INVALID; } - return out_len; + return CKR_OK; } diff --git a/pkcs11/util_pkcs11.h b/pkcs11/util_pkcs11.h index d8bcd07b..e2ebc6dd 100644 --- a/pkcs11/util_pkcs11.h +++ b/pkcs11/util_pkcs11.h @@ -188,6 +188,6 @@ CK_RV parse_meta_label_template(yubihsm_pkcs11_object_template *template, uint8_t *value, size_t value_len); bool match_byte_array(uint8_t *a, uint16_t a_len, uint8_t *b, uint16_t b_len); -size_t ecdh_with_kdf(ecdh_session_key *shared_secret, size_t shared_secret_len, - CK_ULONG kdf, size_t value_len); +CK_RV ecdh_with_kdf(ecdh_session_key *shared_secret, uint8_t *fixed_info, + size_t fixed_len, CK_ULONG kdf, size_t value_len); #endif diff --git a/pkcs11/yubihsm_pkcs11.c b/pkcs11/yubihsm_pkcs11.c index 06589ee6..a9cc541e 100644 --- a/pkcs11/yubihsm_pkcs11.c +++ b/pkcs11/yubihsm_pkcs11.c @@ -5612,14 +5612,14 @@ CK_DEFINE_FUNCTION(CK_RV, C_DeriveKey) goto c_drv_out; } - int basekey_type = hBaseKey >> 16; + CK_ULONG basekey_type = hBaseKey >> 16; if (basekey_type == ECDH_KEY_TYPE) { DBG_ERR("Cannot derive an ECDH key from another ECDH key"); rv = CKR_ARGUMENTS_BAD; goto c_drv_out; } - char *label_buf = NULL; + char *label = NULL; size_t label_len = 0; size_t value_len = 0; for (CK_ULONG i = 0; i < ulAttributeCount; i++) { @@ -5632,7 +5632,7 @@ CK_DEFINE_FUNCTION(CK_RV, C_DeriveKey) rv = CKR_ATTRIBUTE_VALUE_INVALID; goto c_drv_out; } - label_buf = pTemplate[i].pValue; + label = pTemplate[i].pValue; label_len = pTemplate[i].ulValueLen; break; default: @@ -5647,30 +5647,12 @@ CK_DEFINE_FUNCTION(CK_RV, C_DeriveKey) CK_ECDH1_DERIVE_PARAMS *params = pMechanism->pParameter; - if (params->kdf == CKD_NULL || params->kdf == CKD_YUBICO_SHA1_KDF_SP800 || - params->kdf == CKD_YUBICO_SHA256_KDF_SP800 || - params->kdf == CKD_YUBICO_SHA384_KDF_SP800 || - params->kdf == CKD_YUBICO_SHA512_KDF_SP800) { - if ((params->pSharedData != NULL) || (params->ulSharedDataLen != 0)) { - DBG_ERR("Mechanism parameters incompatible with key derivation function " - "CKD_NULL"); - rv = CKR_MECHANISM_PARAM_INVALID; - goto c_drv_out; - } - } else { - DBG_ERR("Unsupported value of mechanism parameter key derivation function"); + if ((params->pSharedData != NULL) || (params->ulSharedDataLen != 0)) { + DBG_ERR("Mechanism parameters incompatible with key derivation function"); rv = CKR_MECHANISM_PARAM_INVALID; goto c_drv_out; } - size_t in_len = params->ulPublicDataLen; - if (in_len != params->ulPublicDataLen) { - DBG_ERR("Invalid parameter"); - return CKR_ARGUMENTS_BAD; - } - - CK_BYTE_PTR pubkey = params->pPublicData; - int seq = session->ecdh_session_keys.length + 1; if (seq > MAX_ECDH_SESSION_KEYS) { DBG_ERR("There are already %d ECDH keys available for this session. " @@ -5680,54 +5662,65 @@ CK_DEFINE_FUNCTION(CK_RV, C_DeriveKey) goto c_drv_out; } - // Read the base key as the private keyID - uint16_t privkey_id = hBaseKey & 0xffff; - ecdh_session_key ecdh_key = {0}; - size_t out_len = sizeof(ecdh_key.ecdh_key); + ecdh_key.id = ECDH_KEY_TYPE << 16 | seq; + ecdh_key.len = sizeof(ecdh_key.ecdh_key); + + DBG_INFO("ecdh_key.id = %zu", ecdh_key.id); + DBG_INFO("ecdh_key.len = %zu", ecdh_key.len); - if (value_len > out_len) { + if (value_len > ecdh_key.len) { DBG_ERR("Requested derived key is too long"); rv = CKR_ATTRIBUTE_VALUE_INVALID; goto c_drv_out; } + // Read the base key as the private keyID + uint16_t privkey_id = hBaseKey & 0xffff; + yh_rc rc = yh_util_derive_ecdh(session->slot->device_session, privkey_id, - pubkey, in_len, ecdh_key.ecdh_key, &out_len); + params->pPublicData, params->ulPublicDataLen, + ecdh_key.ecdh_key, &ecdh_key.len); if (rc != YHR_SUCCESS) { DBG_ERR("Unable to derive raw ECDH key: %s", yh_strerror(rc)); rv = yrc_to_rv(rc); goto c_drv_out; } - out_len = ecdh_with_kdf(&ecdh_key, out_len, params->kdf, value_len); - if (out_len == 0) { - DBG_ERR("Failed to derive ECDH key with KDF"); + DBG_INFO("ECDH ecdh_key.len = %zu", ecdh_key.len); + + rv = ecdh_with_kdf(&ecdh_key, params->pSharedData, params->ulSharedDataLen, + params->kdf, value_len); + if (rv != CKR_OK) { + DBG_ERR("Failed to derive ECDH key with KDF %lu", params->kdf); goto c_drv_out; } + DBG_INFO("KDF ecdh_key.len = %zu", ecdh_key.len); + if (value_len > 0) { - if (out_len < value_len) { - DBG_ERR("Failed to derive a key with the expected length"); + if (ecdh_key.len < value_len) { + DBG_ERR("Failed to derive a key with the requested length"); rv = CKR_DATA_LEN_RANGE; goto c_drv_out; } - if (out_len > value_len) { + if (ecdh_key.len > value_len) { // Truncate from the left - size_t offset = out_len - value_len; + size_t offset = ecdh_key.len - value_len; memmove(ecdh_key.ecdh_key, ecdh_key.ecdh_key + offset, value_len); memset(ecdh_key.ecdh_key + value_len, 0, offset); - out_len = value_len; + ecdh_key.len = value_len; + DBG_INFO("Truncated ecdh_key.len = %zu", ecdh_key.len); } } - // Make a session variable to store the derived key - ecdh_key.id = ECDH_KEY_TYPE << 16 | seq; - ecdh_key.len = out_len; - memcpy(ecdh_key.label, label_buf, label_len); + memcpy(ecdh_key.label, label, label_len); + + // Copy the derived key as a session object list_append(&session->ecdh_session_keys, &ecdh_key); + // Clear the derived key insecure_memzero(ecdh_key.ecdh_key, sizeof(ecdh_key.ecdh_key)); *phKey = ecdh_key.id;