From 6cd9af7070c4b904fe883b31635708a0a5264592 Mon Sep 17 00:00:00 2001 From: Michael Schwarcz Date: Tue, 12 Jun 2018 11:38:08 +0300 Subject: [PATCH] Review fixes --- .../library_api/secure_time_test_utils.cpp | 1 + features/storage/nvstore/source/nvstore.h | 6 +-- secure_time/crypto/secure_time_crypto.h | 9 ++++ secure_time/crypto/secure_time_mbedtls.c | 29 +++++------ secure_time/mbed_lib.json | 2 +- secure_time/secure_time_client_spe.h | 49 +------------------ secure_time/secure_time_client_spe_direct.c | 2 +- secure_time/secure_time_impl.c | 2 +- secure_time/secure_time_impl.h | 2 +- secure_time/secure_time_utils.h | 36 ++++++++++++++ secure_time/storage/secure_time_nvstore.cpp | 2 +- 11 files changed, 67 insertions(+), 73 deletions(-) diff --git a/TESTS/secure_time/library_api/secure_time_test_utils.cpp b/TESTS/secure_time/library_api/secure_time_test_utils.cpp index 44b4e0219f..2b62c36e35 100644 --- a/TESTS/secure_time/library_api/secure_time_test_utils.cpp +++ b/TESTS/secure_time/library_api/secure_time_test_utils.cpp @@ -15,6 +15,7 @@ #include "secure_time_test_utils.h" #include "secure_time_client_spe.h" +#include "secure_time_utils.h" #include "unity.h" #include diff --git a/features/storage/nvstore/source/nvstore.h b/features/storage/nvstore/source/nvstore.h index 275d8c53a1..806e689c42 100644 --- a/features/storage/nvstore/source/nvstore.h +++ b/features/storage/nvstore/source/nvstore.h @@ -49,10 +49,10 @@ typedef enum { // All predefined keys used for internal features should be defined here - NVSTORE_STORED_TIME_KEY = 2, - NVSTORE_STORED_BACK_TIME_KEY = 3, + NVSTORE_STORED_TIME_KEY = 2, + NVSTORE_STORED_BACK_TIME_KEY = 3, NVSTORE_DEVICEKEY_KEY = 4, - NVSTORE_CA_PUBKEY_KEY = 9, + NVSTORE_CA_PUBKEY_KEY = 9, NVSTORE_LAST_PREDEFINED_KEY = 15, NVSTORE_NUM_PREDEFINED_KEYS diff --git a/secure_time/crypto/secure_time_crypto.h b/secure_time/crypto/secure_time_crypto.h index aa29465c67..652ee11fa0 100644 --- a/secure_time/crypto/secure_time_crypto.h +++ b/secure_time/crypto/secure_time_crypto.h @@ -22,6 +22,15 @@ extern "C" { #endif +/* + * Enumeration for the possible blob signature algorithms + */ +typedef enum signature_alg { + SIGNATURE_ALG_INVALID = 0, /**< Invalid algorithm type */ + SIGNATURE_ALG_SHA256_ECDSA = 1, /**< ECDSA on a SHA256 hash */ + SIGNATURE_ALG_MAX = SIGNATURE_ALG_SHA256_ECDSA +} SignatureAlg; + /* * Verify the data buffer signature. * diff --git a/secure_time/crypto/secure_time_mbedtls.c b/secure_time/crypto/secure_time_mbedtls.c index 0c9df7232d..e8826d4512 100644 --- a/secure_time/crypto/secure_time_mbedtls.c +++ b/secure_time/crypto/secure_time_mbedtls.c @@ -22,17 +22,20 @@ #include "mbedtls/entropy.h" #include "mbedtls/ctr_drbg.h" +#include + #if SECURE_TIME_ENABLED /* * Structure containing contexts for random number generation. */ typedef struct secure_time_random_ctx { + bool initialized; mbedtls_ctr_drbg_context ctr_drbg_ctx; /* CTR_DRBG context structure. */ mbedtls_entropy_context entropy_ctx; /* Entropy context structure. */ } secure_time_random_ctx_t; -secure_time_random_ctx_t *random_ctx = NULL; +static secure_time_random_ctx_t random_ctx = {0}; static void random_ctx_init(secure_time_random_ctx_t *ctx) { @@ -85,7 +88,7 @@ static void calculate_hash( error("mbedtls_md_info_from_type() returned NULL!"); } - rc = mbedtls_md(md_info, (const unsigned char *)data, data_size, hash); + rc = mbedtls_md(md_info, data, data_size, hash); if (SECURE_TIME_SUCCESS != rc) { error("mbedtls_md() failed! (rc=%d)", rc); } @@ -101,7 +104,7 @@ int32_t secure_time_verify_signature( ) { int rc = SECURE_TIME_SUCCESS; - uint8_t hash[MBEDTLS_MD_MAX_SIZE] = {0}; + uint8_t hash[MBEDTLS_MD_MAX_SIZE]; mbedtls_pk_context pubkey_ctx = {0}; mbedtls_md_type_t md_type = md_type_from_signature_alg(SIGNATURE_ALG_SHA256_ECDSA); @@ -122,14 +125,7 @@ int32_t secure_time_verify_signature( error("Unable to verify signature"); } - rc = mbedtls_pk_verify( - &pubkey_ctx, - md_type, - hash, - 0, - (const unsigned char *)sign, - sign_size - ); + rc = mbedtls_pk_verify(&pubkey_ctx, md_type, hash, 0, sign, sign_size); if (SECURE_TIME_SUCCESS != rc) { rc = SECURE_TIME_SIGNATURE_VERIFICATION_FAILED; } @@ -142,15 +138,12 @@ void secure_time_generate_random_bytes(size_t size, void *random_buf) { int rc = SECURE_TIME_SUCCESS; - if (NULL == random_ctx) { - random_ctx = (secure_time_random_ctx_t *)malloc(sizeof(*random_ctx)); - if (NULL == random_ctx) { - error("Failed to allocate memory for random_ctx!"); - } - random_ctx_init(random_ctx); + if (false == random_ctx.initialized) { + random_ctx_init(&random_ctx); + random_ctx.initialized = true; } - rc = mbedtls_ctr_drbg_random(&(random_ctx->ctr_drbg_ctx), (unsigned char *)random_buf, size); + rc = mbedtls_ctr_drbg_random(&(random_ctx.ctr_drbg_ctx), random_buf, size); if (SECURE_TIME_SUCCESS != rc) { error("mbedtls_ctr_drbg_random() failed! (rc=%d)", rc); } diff --git a/secure_time/mbed_lib.json b/secure_time/mbed_lib.json index 7ba53fbe90..42c9e4e2a7 100644 --- a/secure_time/mbed_lib.json +++ b/secure_time/mbed_lib.json @@ -9,7 +9,7 @@ }, "target_overrides": { "NUCLEO_F410RB": { - "enabled": 0 + "enabled": false } } } diff --git a/secure_time/secure_time_client_spe.h b/secure_time/secure_time_client_spe.h index 177d7b8891..89c562258a 100644 --- a/secure_time/secure_time_client_spe.h +++ b/secure_time/secure_time_client_spe.h @@ -38,51 +38,6 @@ extern "C" { #endif -/**< Maximal allowed blob size in bytes. */ -#define SECURE_TIME_MAX_BLOB_SIZE_BYTES (10 * 1024UL) - -/**< Timestamp size in bytes. */ -#define SECURE_TIME_TIMESTAMP_SIZE_BYTES (8UL) - -/**< Nonce size in bytes. */ -#define SECURE_TIME_NONCE_SIZE_BYTES (8UL) - -/**< The size of delegation length in bytes. */ -#define SECURE_TIME_DELEGATION_LENGTH_SIZE_BYTES (2UL) - -/**< The size of public key length field in bytes. */ -#define SECURE_TIME_PUBKEY_LENGTH_SIZE_BYTES (2UL) - -/**< The size of signature length field in bytes. */ -#define SECURE_TIME_SIGNATURE_LENGTH_SIZE_BYTES (2UL) - -/**< The size of public key length field in bytes. */ -#define SECURE_TIME_PUBKEY_LENGTH_SIZE_BYTES (2UL) - -/**< The size of the constant length blob header. */ -#define SECURE_TIME_BLOB_HEADER_SIZE_BYTES \ - ( \ - SECURE_TIME_TIMESTAMP_SIZE_BYTES + \ - SECURE_TIME_NONCE_SIZE_BYTES + \ - SECURE_TIME_DELEGATION_LENGTH_SIZE_BYTES \ - ) - -/**< The location of the delegation length field in the blob. */ -#define SECURE_TIME_DELEGATION_LENGTH_OFFSET \ - ( \ - SECURE_TIME_TIMESTAMP_SIZE_BYTES + \ - SECURE_TIME_NONCE_SIZE_BYTES \ - ) - -/** - * Enumeration for the possible blob signature algorithms - */ -typedef enum signature_alg { - SIGNATURE_ALG_INVALID = 0, /**< Invalid algorithm type */ - SIGNATURE_ALG_SHA256_ECDSA = 1, /**< ECDSA on a SHA256 hash */ - SIGNATURE_ALG_MAX = SIGNATURE_ALG_SHA256_ECDSA -} SignatureAlg; - /** * Factory-setup provisioning of public key to be used by secure_time_set_trusted(). * Defined as a weak function which by default tries to write the public key to NVStore. @@ -94,7 +49,7 @@ typedef enum signature_alg { * @param[in] key_size Size in bytes of public key. * @return 0 or negative error code if failed. */ -int32_t secure_time_set_stored_public_key(const void* pubkey, size_t key_size); +int32_t secure_time_set_stored_public_key(const void *pubkey, size_t key_size); /** * Return the previously-provisioned public key. @@ -107,7 +62,7 @@ int32_t secure_time_set_stored_public_key(const void* pubkey, size_t key_size); * @param[out] actual_size Actual size in bytes of the returned public key. * @return 0 or negative error code if failed. */ -int32_t secure_time_get_stored_public_key(uint8_t *pubkey, size_t size, size_t *actual_size); +int32_t secure_time_get_stored_public_key(void *pubkey, size_t size, size_t *actual_size); /** * Return the size in bytes of the previously-provisioned public key. diff --git a/secure_time/secure_time_client_spe_direct.c b/secure_time/secure_time_client_spe_direct.c index 5a8b3b26ac..a933cc1a57 100644 --- a/secure_time/secure_time_client_spe_direct.c +++ b/secure_time/secure_time_client_spe_direct.c @@ -31,7 +31,7 @@ int32_t secure_time_set_stored_public_key(const void* pubkey, size_t key_size) } int32_t secure_time_get_stored_public_key( - uint8_t *pubkey, + void *pubkey, size_t size, size_t *actual_size ) diff --git a/secure_time/secure_time_impl.c b/secure_time/secure_time_impl.c index bc18a947c9..5942b48153 100644 --- a/secure_time/secure_time_impl.c +++ b/secure_time/secure_time_impl.c @@ -66,7 +66,7 @@ static void invalidate_nonce(nonce_data_t *nonce_data) nonce_data->nonce = 0; } -static bool is_nonce_valid(nonce_data_t *nonce) +static bool is_nonce_valid(const nonce_data_t *nonce) { return (SECURE_TIME_NONCE_GENERATION_TIME_INVALID != nonce->generation_time); } diff --git a/secure_time/secure_time_impl.h b/secure_time/secure_time_impl.h index bb58becf56..c665afd2f1 100644 --- a/secure_time/secure_time_impl.h +++ b/secure_time/secure_time_impl.h @@ -36,7 +36,7 @@ uint64_t secure_time_get_impl(void); int32_t secure_time_set_stored_public_key_impl(const void* pubkey, size_t key_size); int32_t secure_time_get_stored_public_key_impl( - uint8_t *pubkey, + void *pubkey, size_t size, size_t *actual_size ); diff --git a/secure_time/secure_time_utils.h b/secure_time/secure_time_utils.h index 376de8d9ec..37f88ab5e9 100644 --- a/secure_time/secure_time_utils.h +++ b/secure_time/secure_time_utils.h @@ -22,6 +22,42 @@ extern "C" { #endif +/*< Maximal allowed blob size in bytes. */ +#define SECURE_TIME_MAX_BLOB_SIZE_BYTES (10 * 1024UL) + +/*< Timestamp size in bytes. */ +#define SECURE_TIME_TIMESTAMP_SIZE_BYTES (8UL) + +/*< Nonce size in bytes. */ +#define SECURE_TIME_NONCE_SIZE_BYTES (8UL) + +/*< The size of delegation length in bytes. */ +#define SECURE_TIME_DELEGATION_LENGTH_SIZE_BYTES (2UL) + +/*< The size of public key length field in bytes. */ +#define SECURE_TIME_PUBKEY_LENGTH_SIZE_BYTES (2UL) + +/*< The size of signature length field in bytes. */ +#define SECURE_TIME_SIGNATURE_LENGTH_SIZE_BYTES (2UL) + +/*< The size of public key length field in bytes. */ +#define SECURE_TIME_PUBKEY_LENGTH_SIZE_BYTES (2UL) + +/*< The size of the constant length blob header. */ +#define SECURE_TIME_BLOB_HEADER_SIZE_BYTES \ + ( \ + SECURE_TIME_TIMESTAMP_SIZE_BYTES + \ + SECURE_TIME_NONCE_SIZE_BYTES + \ + SECURE_TIME_DELEGATION_LENGTH_SIZE_BYTES \ + ) + +/*< The location of the delegation length field in the blob. */ +#define SECURE_TIME_DELEGATION_LENGTH_OFFSET \ + ( \ + SECURE_TIME_TIMESTAMP_SIZE_BYTES + \ + SECURE_TIME_NONCE_SIZE_BYTES \ + ) + #define SECURE_TIME_MIN_RTC_LATENCY_SEC (100UL) /* diff --git a/secure_time/storage/secure_time_nvstore.cpp b/secure_time/storage/secure_time_nvstore.cpp index bb48784d07..f052700f8e 100644 --- a/secure_time/storage/secure_time_nvstore.cpp +++ b/secure_time/storage/secure_time_nvstore.cpp @@ -33,7 +33,7 @@ MBED_WEAK int32_t secure_time_set_stored_public_key_impl(const void* pubkey, siz return rc; } -MBED_WEAK int32_t secure_time_get_stored_public_key_impl(uint8_t *pubkey, size_t size, size_t *actual_size) +MBED_WEAK int32_t secure_time_get_stored_public_key_impl(void *pubkey, size_t size, size_t *actual_size) { if (NULL == pubkey) { error("pubkey is NULL!");