From 6cd86779522fe8874e5171fce621b51707b3c019 Mon Sep 17 00:00:00 2001 From: Matt Feddersen Date: Fri, 8 Dec 2023 18:35:46 -0800 Subject: [PATCH] Bug fix ECC and persistent storage in OPTEE Fixes two bugs in the OP-TEE port 1. WTPI_GetBootCertificateChain() did not correctly zero pad ECC keys that are smaller than 32 bytes. 2. wtpi_persistent_storage_layer2.c:write_raw_object() would overwrite objects if an initial read failed. --- .../wtpi_persistent_storage_layer2.c | 35 +++++++++++++------ .../ta/common/wtpi_impl/wtpi_provisioning_4.c | 18 +++++++--- 2 files changed, 38 insertions(+), 15 deletions(-) diff --git a/oemcrypto/opk/ports/optee/ta/common/wtpi_impl/wtpi_persistent_storage_layer2.c b/oemcrypto/opk/ports/optee/ta/common/wtpi_impl/wtpi_persistent_storage_layer2.c index 1a60282..16f0859 100644 --- a/oemcrypto/opk/ports/optee/ta/common/wtpi_impl/wtpi_persistent_storage_layer2.c +++ b/oemcrypto/opk/ports/optee/ta/common/wtpi_impl/wtpi_persistent_storage_layer2.c @@ -26,18 +26,25 @@ static TEE_Result write_raw_object(const char* obj_id, size_t obj_id_length, * Create object in secure storage and fill with data */ obj_data_flag = - TEE_DATA_FLAG_ACCESS_READ | /* we can later read the oject */ - TEE_DATA_FLAG_ACCESS_WRITE | /* we can later write into the object */ - TEE_DATA_FLAG_ACCESS_WRITE_META | /* we can later destroy or rename the - object */ - TEE_DATA_FLAG_OVERWRITE; /* destroy existing object of same ID */ + TEE_DATA_FLAG_ACCESS_READ | /* we can later read the oject */ + TEE_DATA_FLAG_ACCESS_WRITE | /* we can later write into the object */ + TEE_DATA_FLAG_ACCESS_WRITE_META; /* we can later destroy or rename the + object */ - res = TEE_CreatePersistentObject(TEE_STORAGE_PRIVATE, obj_id, obj_id_length, - obj_data_flag, TEE_HANDLE_NULL, NULL, - 0, /* we may not fill it right now */ - &object); - if (res != TEE_SUCCESS) { - EMSG("TEE_CreatePersistentObject failed 0x%08x", res); + res = TEE_OpenPersistentObject(TEE_STORAGE_PRIVATE, obj_id, + obj_id_length, obj_data_flag, &object); + if (res == TEE_ERROR_ITEM_NOT_FOUND) { + res = TEE_CreatePersistentObject( + TEE_STORAGE_PRIVATE, obj_id, obj_id_length, obj_data_flag, + TEE_HANDLE_NULL, NULL, 0, /* we may not fill it right now */ + &object); + if (res != TEE_SUCCESS) { + EMSG("TEE_CreatePersistentObject failed 0x%08x", res); + TEE_Free(data); + return res; + } + } else if (res != TEE_SUCCESS) { + EMSG("Failed to open persistent object, res=0x%08x", res); TEE_Free(data); return res; } @@ -86,6 +93,12 @@ static TEE_Result read_raw_object(const char* obj_id, size_t obj_id_length, goto exit; } + if (object_info.dataSize == 0) { + res = TEE_ERROR_ITEM_NOT_FOUND; + EMSG("Failed to read persistent object, res=0x%08x, dataSize=0", res); + goto exit; + } + res = TEE_ReadObjectData(object, data, object_info.dataSize, &read_bytes); if (res == TEE_SUCCESS) TEE_MemMove(out, data, read_bytes); if (res != TEE_SUCCESS || read_bytes != object_info.dataSize) { diff --git a/oemcrypto/opk/ports/optee/ta/common/wtpi_impl/wtpi_provisioning_4.c b/oemcrypto/opk/ports/optee/ta/common/wtpi_impl/wtpi_provisioning_4.c index 6507ba3..06fc831 100644 --- a/oemcrypto/opk/ports/optee/ta/common/wtpi_impl/wtpi_provisioning_4.c +++ b/oemcrypto/opk/ports/optee/ta/common/wtpi_impl/wtpi_provisioning_4.c @@ -293,26 +293,36 @@ OEMCryptoResult WTPI_GetBootCertificateChain(uint8_t* out, size_t* out_length) { } // extract public key, P256 hardcoded to 64 bytes long - uint8_t public_key[64]; + uint8_t public_key[64] = {0}; + uint8_t temp[32] = {0}; size_t public_key_length = sizeof(public_key); size_t buf_size = 32; TEE_Result tee_res = TEE_GetObjectBufferAttribute( - private_key_handle->key_handle, TEE_ATTR_ECC_PUBLIC_VALUE_X, public_key, + private_key_handle->key_handle, TEE_ATTR_ECC_PUBLIC_VALUE_X, temp, &buf_size); if (tee_res != TEE_SUCCESS) { EMSG("TEE_GetObjectBufferAttribute() failed with result 0x%x", tee_res); WTPI_FreeAsymmetricKeyHandle(private_key_handle); return OEMCrypto_ERROR_UNKNOWN_FAILURE; } + + // buf_size is modified to indicate the actual size of the X component. + // Move the data so that it has leading zeros if it is less than 32 bytes. + TEE_MemMove(public_key + 32 - buf_size, temp, buf_size); + + buf_size = 32; + TEE_MemFill(temp, 0, sizeof(temp)); tee_res = TEE_GetObjectBufferAttribute(private_key_handle->key_handle, - TEE_ATTR_ECC_PUBLIC_VALUE_Y, - public_key + 32, &buf_size); + TEE_ATTR_ECC_PUBLIC_VALUE_Y, temp, + &buf_size); if (tee_res != TEE_SUCCESS) { EMSG("TEE_GetObjectBufferAttribute() failed with result 0x%x", tee_res); WTPI_FreeAsymmetricKeyHandle(private_key_handle); return OEMCrypto_ERROR_UNKNOWN_FAILURE; } + TEE_MemMove(public_key + 64 - buf_size, temp, buf_size); + result = BuildBootCertificateChain(public_key, public_key_length, key_type, private_key_handle, out, out_length); WTPI_FreeAsymmetricKeyHandle(private_key_handle);