diff --git a/docs/design-documents/features/storage/KVStore/KVStore_design.md b/docs/design-documents/features/storage/KVStore/KVStore_design.md index f60fdc74ab..fc3fa08d6a 100644 --- a/docs/design-documents/features/storage/KVStore/KVStore_design.md +++ b/docs/design-documents/features/storage/KVStore/KVStore_design.md @@ -82,7 +82,6 @@ class KVStore { enum create_flags { WRITE_ONCE_FLAG = (1 << 0), REQUIRE_CONFIDENTIALITY_FLAG = (1 << 1), - REQUIRE_INTEGRITY_FLAG = (1 << 2), REQUIRE_REPLAY_PROTECTION_FLAG = (1 << 3), }; @@ -130,7 +129,6 @@ As mentioned above, each KVStore API has a parallel C-style API, used globally w enum kv_create_flags { KV_WRITE_ONCE_FLAG = (1 << 0), KV_REQUIRE_CONFIDENTIALITY_FLAG = (1 << 1), - KV_REQUIRE_INTEGRITY_FLAG = (1 << 2), KV_REQUIRE_REPLAY_PROTECTION_FLAG = (1 << 3), }; diff --git a/docs/design-documents/features/storage/SecureStore/SecureStore_design.md b/docs/design-documents/features/storage/SecureStore/SecureStore_design.md index fba01b4a11..385271c976 100644 --- a/docs/design-documents/features/storage/SecureStore/SecureStore_design.md +++ b/docs/design-documents/features/storage/SecureStore/SecureStore_design.md @@ -47,10 +47,11 @@ SecureStore is a storage class, derived from KVStore. It adds security features As such, it offers all KVStore APIs, with additional security options (which can be selected using the creation flags at set). These include: - Encryption: Data is encrypted using the AES-CTR encryption method, with a randomly generated 8-byte IV. Key is derived from [Device Key](../../../../../../mbed-os/features/device_key/README.md), using the NIST SP 800-108 KDF in counter mode spec, where salt is the key trimmed to 32 bytes, with "ENC" as prefix. Flag here is called "require confidentiality flag". -- Authentication: A 16-byte CMAC is calculated on all stored data (including metadata) and stored at the end of the record. When reading the record, calculated CMAC is compared with the stored one. In the case of encryption, CMAC is calculated on the encrypted data. The key used for generating the CMAC is derived from [Device Key](../../../../../../mbed-os/features/device_key/README.md), where salt is the key trimmed to 32 bytes, with "AUTH" as prefix. Flag here is called "Require integrity flag". - Rollback protection: (Requires authentication) CMAC is stored in a designated rollback protected storage (also of KVStore type) and compared to when reading the data under the same KVStore key. A missing or different key in the rollback protected storage results in an error. The flag here is called "Require replay protection flag". - Write once: Key can only be stored once and can't be removed. The flag here is called "Write once flag". +SecureStore maintains data integrity using a record CMAC. This 16-byte CMAC is calculated on all stored data (including key & metadata) and stored at the end of the record. When reading the record, SecureStore compares the calculated CMAC with the stored one. In the case of encryption, CMAC is calculated on the encrypted data. The key used for generating the CMAC is derived from [Device Key](../../../../../../mbed-os/features/device_key/README.md), where salt is the key trimmed to 32 bytes, with "AUTH" as prefix. + ![SecureStore Layers](./SecureStore_layers.jpg) ### Data layout @@ -73,7 +74,8 @@ Fields are: Because the code can't construct a single buffer to store all data (including metadata and possibly encrypted data) in one shot, setting the data occurs in chunks, using the incremental set APIs. Get uses the offset argument to extract metadata, data and CMAC separately. -Rollback protection (RBP) keys are stored in the designated rollback protection storage, which is also of KVStore type. RBP keys are the same as the SecureStore keys. +Rollback protection (RBP) keys are stored in the designated rollback protection storage, which is also of KVStore type. RBP keys are the same as the SecureStore keys. +This RBP storage is also used for storing the CMAC in write once case, as otherwise an attacker can delete this key from the underlying storage and modify this flag. # Detailed design @@ -227,14 +229,13 @@ Pseudo code: - Take `_mutex`. - Call `_underlying_kv` `get` API with `metadata` size into a `metadata` local structure. - If failure: - - If rollback protection flag set: + - If rollback protection or write once flag set: - Call `_rbp_kv` `get` API on a local `rbp_cmac` variable, key is `key`, size 16. - If no error, return "RBP authentication" error. - Return "Key not found error". -- If authentication flag set: - - Derive a key from device key and `key`. - - Allocate and initialize `auth_handle` CMAC calculation local handle with derived key. - - Using `auth_handle` handle, calculate CMAC on `key` and `metadata`. +- Derive a key from device key and `key`. +- Allocate and initialize `auth_handle` CMAC calculation local handle with derived key. +- Using `auth_handle` handle, calculate CMAC on `key` and `metadata`. - If encrypt flag set: - Derive a key from device key and `key`. - Allocate and initialize a local `enc_handle` AES-CTR local handle with derived key and `iv` field. @@ -247,13 +248,13 @@ Pseudo code: - Else: - Set `dest_buf` to `_scratch_buf` and `chunk_size` to `actual_size`. - Call `_underlying_kv` `get` API with `dest_buf` and `chunk_size`. - - If authentication flag set, calculate CMAC on `dest_buf`, using `_auth_handle` handle. + - Calculate CMAC on `dest_buf`, using `_auth_handle` handle. - If encrypt flag set, decrypt `dest_buf` (in place) using `_enc_handle` handle. - Decrement `data_size` by `chunk_size`. - Call `_underlying_kv` `get` API with on a local `read_cmac` variable, size 16. - Generate CMAC on local `cmac` variable . - Using `mbedtls_ssl_safer_memcmp` function, compare `read_cmac` with `cmac`. Return "data corrupt error" if no match. -- If rollback protection flag set: +- If rollback protection or write once flags set: - Call `_rbp_kv` `get` API on a local `rbp_cmac` variable, key is `key`, size 16. - If `rbp_cmac` doesn't match `cmac`, clear `buffer` and return "RBP authentication" error. - Deinitialize and free `auth_handle` and `enc_handle`. @@ -312,10 +313,9 @@ Pseudo code: - Using TLS entropy function on `_entropy` handle, randomly generate `iv` field. - Allocate and initialize `enc_handle` AES-CTR handle field with derived key and `iv` field. - Fill all available fields in `metadata`. -- If authentication flag set: - - Derive a key from device key and `key` as salt (trimmed to 32 bytes with "AUTH" as prefix). - - Allocate and initialize `auth_handle` CMAC calculation handle field with derived key. - - Using `auth_handle` handle, calculate CMAC on `key` and `metadata`. +- Derive a key from device key and `key` as salt (trimmed to 32 bytes with "AUTH" as prefix). +- Allocate and initialize `auth_handle` CMAC calculation handle field with derived key. +- Using `auth_handle` handle, calculate CMAC on `key` and `metadata`. - Call `_underlying_kv` `set_start` API. - Call `_underlying_kv` `set_add_data` API with `metadata` field. - Return OK. @@ -332,10 +332,10 @@ Pseudo code: - If flags include encryption: - Iterate over `value_data` field in chunks of `_scratch_buf` size. - Using `enc_handle` handle field, encrypt chunk into `_scratch_buf`. - - If authentication flag set, using `auth_handle` handle field, update CMAC of `_scratch_buf`. + - Using `auth_handle` handle field, update CMAC of `_scratch_buf`. - Call `_underlying_kv` `set_add_data` API with `_scratch_buf`. - Else: - - If authentication flag set, using `auth_handle` handle field, update CMAC of `value_data`. + - Using `auth_handle` handle field, update CMAC of `value_data`. - Call `_underlying_kv` `set_add_data` API with `value_data`. - Update `offset` field in handle. - Return OK. @@ -352,7 +352,7 @@ Pseudo code: - If authentication flag set, using `auth_handle` handle field, generate `cmac`. - Call `_underlying_kv` `set_add_data` API with `cmac`. - Call `_underlying_kv` `set_finalize`. -- If rollback protect flag set, call `_rbp_kv` `set` API with `key` as key and `cmac` as data. +- If rollback protect or write once flags set, call `_rbp_kv` `set` API with `key` as key and `cmac` as data. - Deinitialize and free `auth_handle` and `enc_handle`. - Free `handle`. - Release `_mutex`. @@ -426,10 +426,10 @@ res = secure_store.init(); const char *val1 = "Value of key 1"; const char *val2 = "Updated value of key 1"; -// Add "Key1" with encryption and authentication flags -res = secure_store.set("Key1", val1, sizeof(val1), KVSTore::REQUIRE_CONFIDENTIALITY_FLAG | KVSTore::REQUIRE_INTEGRITY_FLAG); +// Add "Key1" with encryption flag +res = secure_store.set("Key1", val1, sizeof(val1), KVSTore::REQUIRE_CONFIDENTIALITY_FLAG); // Update value of "Key1" (flags must be the same per key) -res = secure_store.set("Key1", val2, sizeof(val2), KVSTore::REQUIRE_CONFIDENTIALITY_FLAG | KVSTore::REQUIRE_INTEGRITY_FLAG); +res = secure_store.set("Key1", val2, sizeof(val2), KVSTore::REQUIRE_CONFIDENTIALITY_FLAG); uint_8 value[32]; size_t actual_size; diff --git a/features/storage/TESTS/kvstore/general_tests_phase_1/main.cpp b/features/storage/TESTS/kvstore/general_tests_phase_1/main.cpp index 44e23c32d1..5c299765ff 100644 --- a/features/storage/TESTS/kvstore/general_tests_phase_1/main.cpp +++ b/features/storage/TESTS/kvstore/general_tests_phase_1/main.cpp @@ -414,18 +414,6 @@ static void set_several_key_value_sizes() TEST_ASSERT_EQUAL_ERROR_CODE(MBED_SUCCESS, res); } -//set key with ROLLBACK flag without AUTHENTICATION flag -static void Sec_set_key_rollback_without_auth_flag() -{ - TEST_SKIP_UNLESS(kvstore != NULL); - if (kv_setup != SecStoreSet) { - return; - } - - int res = kvstore->set(key, data, data_size, KVStore::REQUIRE_REPLAY_PROTECTION_FLAG); - TEST_ASSERT_EQUAL_ERROR_CODE(MBED_ERROR_INVALID_ARGUMENT, res); -} - //set key with ROLLBACK flag and retrieve it, set it again with no ROLBACK static void Sec_set_key_rollback_set_again_no_rollback() { @@ -436,7 +424,7 @@ static void Sec_set_key_rollback_set_again_no_rollback() return; } - int res = kvstore->set(key_name, data, data_size, KVStore::REQUIRE_REPLAY_PROTECTION_FLAG | KVStore::REQUIRE_INTEGRITY_FLAG); + int res = kvstore->set(key_name, data, data_size, KVStore::REQUIRE_REPLAY_PROTECTION_FLAG); TEST_ASSERT_EQUAL_ERROR_CODE(MBED_SUCCESS, res); res = kvstore->get(key_name, buffer, sizeof(buffer), &actual_size, 0); @@ -479,7 +467,7 @@ static void Sec_set_key_auth() return; } - int res = kvstore->set(key, data, data_size, KVStore::REQUIRE_INTEGRITY_FLAG); + int res = kvstore->set(key, data, data_size, 0); TEST_ASSERT_EQUAL_ERROR_CODE(MBED_SUCCESS, res); res = kvstore->get(key, buffer, sizeof(buffer), &actual_size, 0); @@ -768,7 +756,6 @@ template_case_t template_cases[] = { {"set_key_value_seventeen_byte_size", set_key_value_seventeen_byte_size, greentea_failure_handler}, {"set_several_key_value_sizes", set_several_key_value_sizes, greentea_failure_handler}, - {"Sec_set_key_rollback_without_auth_flag", Sec_set_key_rollback_without_auth_flag, greentea_failure_handler}, {"Sec_set_key_rollback_set_again_no_rollback", Sec_set_key_rollback_set_again_no_rollback, greentea_failure_handler}, {"Sec_set_key_encrypt", Sec_set_key_encrypt, greentea_failure_handler}, {"Sec_set_key_auth", Sec_set_key_auth, greentea_failure_handler}, diff --git a/features/storage/TESTS/kvstore/securestore_whitebox/main.cpp b/features/storage/TESTS/kvstore/securestore_whitebox/main.cpp index 819acc4a49..ea619f0c16 100644 --- a/features/storage/TESTS/kvstore/securestore_whitebox/main.cpp +++ b/features/storage/TESTS/kvstore/securestore_whitebox/main.cpp @@ -148,13 +148,13 @@ static void white_box_test() elapsed = timer.read_ms(); printf("Elapsed time for reset is %d ms\n", elapsed); - result = sec_kv->set(key1, key1_val1, strlen(key1_val1), KVStore::REQUIRE_CONFIDENTIALITY_FLAG | KVStore::REQUIRE_INTEGRITY_FLAG); + result = sec_kv->set(key1, key1_val1, strlen(key1_val1), KVStore::REQUIRE_CONFIDENTIALITY_FLAG); TEST_ASSERT_EQUAL_ERROR_CODE(MBED_SUCCESS, result); - result = sec_kv->set(key2, key2_val1, strlen(key2_val1), KVStore::REQUIRE_INTEGRITY_FLAG); + result = sec_kv->set(key2, key2_val1, strlen(key2_val1), 0); TEST_ASSERT_EQUAL_ERROR_CODE(MBED_SUCCESS, result); - result = sec_kv->set(key2, key2_val2, strlen(key2_val2), KVStore::REQUIRE_CONFIDENTIALITY_FLAG | KVStore::REQUIRE_INTEGRITY_FLAG); + result = sec_kv->set(key2, key2_val2, strlen(key2_val2), KVStore::REQUIRE_CONFIDENTIALITY_FLAG); TEST_ASSERT_EQUAL_ERROR_CODE(MBED_SUCCESS, result); result = sec_kv->get(key2, get_buf, sizeof(get_buf), &actual_data_size); @@ -163,18 +163,17 @@ static void white_box_test() TEST_ASSERT_EQUAL_STRING_LEN(key2_val2, get_buf, strlen(key2_val2)); timer.reset(); - result = sec_kv->set(key2, key2_val3, strlen(key2_val3), KVStore::REQUIRE_INTEGRITY_FLAG | KVStore::REQUIRE_REPLAY_PROTECTION_FLAG); + result = sec_kv->set(key2, key2_val3, strlen(key2_val3), KVStore::REQUIRE_REPLAY_PROTECTION_FLAG); elapsed = timer.read_ms(); printf("Elapsed time for set is %d ms\n", elapsed); TEST_ASSERT_EQUAL_ERROR_CODE(MBED_SUCCESS, result); result = sec_kv->set(key3, key3_val1, strlen(key3_val1), - KVStore::REQUIRE_INTEGRITY_FLAG | KVStore::REQUIRE_CONFIDENTIALITY_FLAG | KVStore::REQUIRE_REPLAY_PROTECTION_FLAG); + KVStore::REQUIRE_CONFIDENTIALITY_FLAG | KVStore::REQUIRE_REPLAY_PROTECTION_FLAG); TEST_ASSERT_EQUAL_ERROR_CODE(MBED_SUCCESS, result); - result = sec_kv->set(key3, key3_val2, strlen(key3_val2), - KVStore::REQUIRE_INTEGRITY_FLAG | KVStore::REQUIRE_CONFIDENTIALITY_FLAG); + result = sec_kv->set(key3, key3_val2, strlen(key3_val2), KVStore::REQUIRE_CONFIDENTIALITY_FLAG); TEST_ASSERT_EQUAL_ERROR_CODE(MBED_ERROR_INVALID_ARGUMENT, result); result = sec_kv->get(key3, get_buf, sizeof(get_buf), &actual_data_size); @@ -183,18 +182,16 @@ static void white_box_test() TEST_ASSERT_EQUAL_STRING_LEN(key3_val1, get_buf, strlen(key3_val1)); for (int j = 0; j < 2; j++) { - result = sec_kv->set(key4, key4_val1, strlen(key4_val1), - KVStore::REQUIRE_INTEGRITY_FLAG | KVStore::REQUIRE_REPLAY_PROTECTION_FLAG); + result = sec_kv->set(key4, key4_val1, strlen(key4_val1), KVStore::REQUIRE_REPLAY_PROTECTION_FLAG); TEST_ASSERT_EQUAL_ERROR_CODE(MBED_SUCCESS, result); - result = sec_kv->set(key4, key4_val2, strlen(key4_val2), - KVStore::REQUIRE_INTEGRITY_FLAG | KVStore::REQUIRE_CONFIDENTIALITY_FLAG | KVStore::REQUIRE_REPLAY_PROTECTION_FLAG); + result = sec_kv->set(key4, key4_val2, strlen(key4_val2), KVStore::REQUIRE_CONFIDENTIALITY_FLAG | KVStore::REQUIRE_REPLAY_PROTECTION_FLAG); TEST_ASSERT_EQUAL_ERROR_CODE(MBED_SUCCESS, result); } result = sec_kv->get_info(key3, &info); TEST_ASSERT_EQUAL_ERROR_CODE(MBED_SUCCESS, result); - TEST_ASSERT_EQUAL(KVStore::REQUIRE_INTEGRITY_FLAG | KVStore::REQUIRE_CONFIDENTIALITY_FLAG | KVStore::REQUIRE_REPLAY_PROTECTION_FLAG, info.flags); + TEST_ASSERT_EQUAL(KVStore::REQUIRE_CONFIDENTIALITY_FLAG | KVStore::REQUIRE_REPLAY_PROTECTION_FLAG, info.flags); TEST_ASSERT_EQUAL(strlen(key3_val1), info.size); result = ul_kv->get_info(key3, &info); @@ -224,7 +221,7 @@ static void white_box_test() TEST_ASSERT_EQUAL_ERROR_CODE(MBED_ERROR_ITEM_NOT_FOUND, result); result = sec_kv->set(key5, key5_val1, strlen(key5_val1), - KVStore::REQUIRE_INTEGRITY_FLAG | KVStore::REQUIRE_REPLAY_PROTECTION_FLAG | KVStore::WRITE_ONCE_FLAG); + KVStore::REQUIRE_REPLAY_PROTECTION_FLAG | KVStore::WRITE_ONCE_FLAG); TEST_ASSERT_EQUAL_ERROR_CODE(MBED_SUCCESS, result); #ifndef NO_RBP_MODE @@ -234,7 +231,7 @@ static void white_box_test() #endif result = sec_kv->set(key5, key5_val2, strlen(key5_val2), - KVStore::REQUIRE_INTEGRITY_FLAG | KVStore::REQUIRE_REPLAY_PROTECTION_FLAG | KVStore::WRITE_ONCE_FLAG); + KVStore::REQUIRE_REPLAY_PROTECTION_FLAG | KVStore::WRITE_ONCE_FLAG); TEST_ASSERT_EQUAL_ERROR_CODE(MBED_ERROR_WRITE_PROTECTED, result); result = sec_kv->remove(key5); @@ -315,7 +312,7 @@ static void white_box_test() TEST_ASSERT_EQUAL_STRING_LEN(key4_val2, get_buf, strlen(key4_val2)); result = sec_kv->set(key6, key6_val1, strlen(key6_val1), - KVStore::REQUIRE_INTEGRITY_FLAG | KVStore::REQUIRE_CONFIDENTIALITY_FLAG | KVStore::REQUIRE_REPLAY_PROTECTION_FLAG); + KVStore::REQUIRE_CONFIDENTIALITY_FLAG | KVStore::REQUIRE_REPLAY_PROTECTION_FLAG); TEST_ASSERT_EQUAL_ERROR_CODE(MBED_SUCCESS, result); #ifndef NO_RBP_MODE @@ -326,7 +323,7 @@ static void white_box_test() TEST_ASSERT_EQUAL_ERROR_CODE(MBED_SUCCESS, result); result = sec_kv->set(key6, key6_val2, strlen(key6_val2), - KVStore::REQUIRE_INTEGRITY_FLAG | KVStore::REQUIRE_CONFIDENTIALITY_FLAG | KVStore::REQUIRE_REPLAY_PROTECTION_FLAG); + KVStore::REQUIRE_CONFIDENTIALITY_FLAG | KVStore::REQUIRE_REPLAY_PROTECTION_FLAG); TEST_ASSERT_EQUAL_ERROR_CODE(MBED_SUCCESS, result); result = ul_kv->set(key6, attack_buf, attack_size, 0); @@ -342,7 +339,7 @@ static void white_box_test() int cmac_size = info.size; uint8_t *cmac = new uint8_t[cmac_size]; - result = sec_kv->set(key7, key7_val1, strlen(key7_val1), KVStore::REQUIRE_INTEGRITY_FLAG); + result = sec_kv->set(key7, key7_val1, strlen(key7_val1), 0); TEST_ASSERT_EQUAL_ERROR_CODE(MBED_SUCCESS, result); result = ul_kv->get(key7, attack_buf, sizeof(attack_buf), &attack_size); @@ -351,7 +348,7 @@ static void white_box_test() int data_offset = attack_size - cmac_size - strlen(key7_val1); TEST_ASSERT_EQUAL(0, strncmp(key7_val1, attack_buf + data_offset, strlen(key7_val1))); - result = sec_kv->set(key7, key7_val1, strlen(key7_val1), KVStore::REQUIRE_INTEGRITY_FLAG | KVStore::REQUIRE_CONFIDENTIALITY_FLAG); + result = sec_kv->set(key7, key7_val1, strlen(key7_val1), KVStore::REQUIRE_CONFIDENTIALITY_FLAG); TEST_ASSERT_EQUAL_ERROR_CODE(MBED_SUCCESS, result); result = ul_kv->get(key7, attack_buf, sizeof(attack_buf), &attack_size); diff --git a/features/storage/kvstore/KVStore.h b/features/storage/kvstore/KVStore.h index ed07691456..a3383997da 100644 --- a/features/storage/kvstore/KVStore.h +++ b/features/storage/kvstore/KVStore.h @@ -32,7 +32,7 @@ public: enum create_flags { WRITE_ONCE_FLAG = (1 << 0), REQUIRE_CONFIDENTIALITY_FLAG = (1 << 1), - REQUIRE_INTEGRITY_FLAG = (1 << 2), + RESERVED_FLAG = (1 << 2), REQUIRE_REPLAY_PROTECTION_FLAG = (1 << 3), }; @@ -54,7 +54,6 @@ public: * The Key flags, possible flags combination: * WRITE_ONCE_FLAG, * REQUIRE_CONFIDENTIALITY_FLAG, - * REQUIRE_INTEGRITY_FLAG, * REQUIRE_REPLAY_PROTECTION_FLAG */ uint32_t flags; diff --git a/features/storage/kvstore/conf/kv_config.cpp b/features/storage/kvstore/conf/kv_config.cpp index 49a4451f52..ea9bdddd93 100644 --- a/features/storage/kvstore/conf/kv_config.cpp +++ b/features/storage/kvstore/conf/kv_config.cpp @@ -590,7 +590,7 @@ int _storage_config_TDB_INTERNAL() kvstore_config.internal_store; kvstore_config.flags_mask = ~(KVStore::REQUIRE_CONFIDENTIALITY_FLAG | - KVStore::REQUIRE_INTEGRITY_FLAG | KVStore::REQUIRE_REPLAY_PROTECTION_FLAG); + KVStore::REQUIRE_REPLAY_PROTECTION_FLAG); KVMap &kv_map = KVMap::get_instance(); ret = kv_map.init(); diff --git a/features/storage/kvstore/global_api/kvstore_global_api.h b/features/storage/kvstore/global_api/kvstore_global_api.h index e588ca292d..12cfe067f3 100644 --- a/features/storage/kvstore/global_api/kvstore_global_api.h +++ b/features/storage/kvstore/global_api/kvstore_global_api.h @@ -27,7 +27,7 @@ typedef struct _opaque_kv_key_iterator *kv_iterator_t; #define KV_WRITE_ONCE_FLAG (1 << 0) #define KV_REQUIRE_CONFIDENTIALITY_FLAG (1 << 1) -#define KV_REQUIRE_INTEGRITY_FLAG (1 << 2) +#define KV_RESERVED_FLAG (1 << 2) #define KV_REQUIRE_REPLAY_PROTECTION_FLAG (1 << 3) #define KV_MAX_KEY_LENGTH 128 @@ -44,7 +44,6 @@ typedef struct info { * The Key flags, possible flags combination: * WRITE_ONCE_FLAG, * REQUIRE_CONFIDENTIALITY_FLAG, - * REQUIRE_INTEGRITY_FLAG, * REQUIRE_REPLAY_PROTECTION_FLAG */ uint32_t flags; diff --git a/features/storage/kvstore/securestore/SecureStore.cpp b/features/storage/kvstore/securestore/SecureStore.cpp index 9d167cf32c..c4ac4b15c8 100644 --- a/features/storage/kvstore/securestore/SecureStore.cpp +++ b/features/storage/kvstore/securestore/SecureStore.cpp @@ -46,7 +46,7 @@ static const uint32_t derived_key_size = 16; static const char *const enc_prefix = "ENC"; static const char *const auth_prefix = "AUTH"; -static const uint32_t security_flags = KVStore::REQUIRE_INTEGRITY_FLAG | KVStore::REQUIRE_CONFIDENTIALITY_FLAG | KVStore::REQUIRE_REPLAY_PROTECTION_FLAG; +static const uint32_t security_flags = KVStore::REQUIRE_CONFIDENTIALITY_FLAG | KVStore::REQUIRE_REPLAY_PROTECTION_FLAG; typedef struct { uint16_t metadata_size; @@ -190,11 +190,6 @@ int SecureStore::set_start(set_handle_t *handle, const char *key, size_t final_d return MBED_ERROR_INVALID_ARGUMENT; } - // RP requires authentication - if ((create_flags & REQUIRE_REPLAY_PROTECTION_FLAG) && !(create_flags & REQUIRE_INTEGRITY_FLAG)) { - return MBED_ERROR_INVALID_ARGUMENT; - } - *handle = static_cast(_inc_set_handle); ih = reinterpret_cast(*handle); @@ -254,24 +249,22 @@ int SecureStore::set_start(set_handle_t *handle, const char *key, size_t final_d memset(ih->metadata.iv, 0, iv_size); } - if (create_flags & REQUIRE_INTEGRITY_FLAG) { - os_ret = cmac_calc_start(ih->auth_ctx, key, _scratch_buf, scratch_buf_size); - if (os_ret) { - ret = MBED_ERROR_FAILED_OPERATION; - goto fail; - } - auth_started = true; - // Although name is not part of the data, we calculate CMAC on it as well - os_ret = cmac_calc_data(ih->auth_ctx, key, strlen(key)); - if (os_ret) { - ret = MBED_ERROR_FAILED_OPERATION; - goto fail; - } - os_ret = cmac_calc_data(ih->auth_ctx, &ih->metadata, sizeof(record_metadata_t)); - if (os_ret) { - ret = MBED_ERROR_FAILED_OPERATION; - goto fail; - } + os_ret = cmac_calc_start(ih->auth_ctx, key, _scratch_buf, scratch_buf_size); + if (os_ret) { + ret = MBED_ERROR_FAILED_OPERATION; + goto fail; + } + auth_started = true; + // Although name is not part of the data, we calculate CMAC on it as well + os_ret = cmac_calc_data(ih->auth_ctx, key, strlen(key)); + if (os_ret) { + ret = MBED_ERROR_FAILED_OPERATION; + goto fail; + } + os_ret = cmac_calc_data(ih->auth_ctx, &ih->metadata, sizeof(record_metadata_t)); + if (os_ret) { + ret = MBED_ERROR_FAILED_OPERATION; + goto fail; } ih->offset_in_data = 0; @@ -291,7 +284,7 @@ int SecureStore::set_start(set_handle_t *handle, const char *key, size_t final_d goto fail; } - if (create_flags & REQUIRE_REPLAY_PROTECTION_FLAG) { + if (create_flags & (REQUIRE_REPLAY_PROTECTION_FLAG | WRITE_ONCE_FLAG)) { ih->key = new char[strlen(key) + 1]; strcpy(ih->key, key); } @@ -360,12 +353,10 @@ int SecureStore::set_add_data(set_handle_t handle, const void *value_data, size_ dst_ptr = static_cast (value_data); } - if (ih->metadata.create_flags & REQUIRE_INTEGRITY_FLAG) { - os_ret = cmac_calc_data(ih->auth_ctx, dst_ptr, chunk_size); - if (os_ret) { - ret = MBED_ERROR_FAILED_OPERATION; - goto fail; - } + os_ret = cmac_calc_data(ih->auth_ctx, dst_ptr, chunk_size); + if (os_ret) { + ret = MBED_ERROR_FAILED_OPERATION; + goto fail; } ret = _underlying_kv->set_add_data(ih->underlying_handle, dst_ptr, chunk_size); @@ -387,9 +378,7 @@ fail: mbedtls_aes_free(&ih->enc_ctx); } - if (ih->metadata.create_flags & REQUIRE_INTEGRITY_FLAG) { - mbedtls_cipher_free(&ih->auth_ctx); - } + mbedtls_cipher_free(&ih->auth_ctx); // mark handle as invalid by clearing metadata size field in header ih->metadata.metadata_size = 0; @@ -420,12 +409,10 @@ int SecureStore::set_finalize(set_handle_t handle) goto end; } - if (ih->metadata.create_flags & REQUIRE_INTEGRITY_FLAG) { - os_ret = cmac_calc_finish(ih->auth_ctx, cmac); - if (os_ret) { - ret = MBED_ERROR_FAILED_OPERATION; - goto end; - } + os_ret = cmac_calc_finish(ih->auth_ctx, cmac); + if (os_ret) { + ret = MBED_ERROR_FAILED_OPERATION; + goto end; } ret = _underlying_kv->set_add_data(ih->underlying_handle, cmac, cmac_size); @@ -438,9 +425,11 @@ int SecureStore::set_finalize(set_handle_t handle) goto end; } - if (_rbp_kv && (ih->metadata.create_flags & REQUIRE_REPLAY_PROTECTION_FLAG)) { + if (_rbp_kv && (ih->metadata.create_flags & (REQUIRE_REPLAY_PROTECTION_FLAG | WRITE_ONCE_FLAG))) { // In rollback protect case, we need to store CMAC in RBP store. // If it's also write once case, set write once flag in the RBP key as well. + // Use RBP storage also in write once case only - in order to prevent attacks removing + // a written once value from underlying KV. ret = _rbp_kv->set(ih->key, cmac, cmac_size, ih->metadata.create_flags & WRITE_ONCE_FLAG); delete[] ih->key; if (ret) { @@ -455,9 +444,7 @@ end: mbedtls_aes_free(&ih->enc_ctx); } - if (ih->metadata.create_flags & REQUIRE_INTEGRITY_FLAG) { - mbedtls_cipher_free(&ih->auth_ctx); - } + mbedtls_cipher_free(&ih->auth_ctx); _mutex.unlock(); return ret; @@ -493,7 +480,9 @@ int SecureStore::remove(const char *key) _mutex.lock(); int ret = do_get(key, 0, 0, 0, 0, &info); - if (ret) { + // Allow deleting key if read error is of our own errors + if ((ret != MBED_SUCCESS) && (ret != MBED_ERROR_AUTHENTICATION_FAILED) && + (ret != MBED_ERROR_RBP_AUTHENTICATION_FAILED)) { goto end; } @@ -570,30 +559,28 @@ int SecureStore::do_get(const char *key, void *buffer, size_t buffer_size, size_ } // Another potential attack case - key hasn't got the RP flag set, but exists in the RBP KV - if (rbp_key_exists && !(create_flags & REQUIRE_REPLAY_PROTECTION_FLAG)) { + if (rbp_key_exists && !(create_flags & (REQUIRE_REPLAY_PROTECTION_FLAG | WRITE_ONCE_FLAG))) { ret = MBED_ERROR_RBP_AUTHENTICATION_FAILED; goto end; } - if (create_flags & REQUIRE_INTEGRITY_FLAG) { - os_ret = cmac_calc_start(ih->auth_ctx, key, _scratch_buf, scratch_buf_size); - if (os_ret) { - ret = MBED_ERROR_FAILED_OPERATION; - goto end; - } - auth_started = true; + os_ret = cmac_calc_start(ih->auth_ctx, key, _scratch_buf, scratch_buf_size); + if (os_ret) { + ret = MBED_ERROR_FAILED_OPERATION; + goto end; + } + auth_started = true; - // Although name is not part of the data, we calculate CMAC on it as well - os_ret = cmac_calc_data(ih->auth_ctx, key, strlen(key)); - if (os_ret) { - ret = MBED_ERROR_FAILED_OPERATION; - goto end; - } - os_ret = cmac_calc_data(ih->auth_ctx, &ih->metadata, sizeof(record_metadata_t)); - if (os_ret) { - ret = MBED_ERROR_FAILED_OPERATION; - goto end; - } + // Although name is not part of the data, we calculate CMAC on it as well + os_ret = cmac_calc_data(ih->auth_ctx, key, strlen(key)); + if (os_ret) { + ret = MBED_ERROR_FAILED_OPERATION; + goto end; + } + os_ret = cmac_calc_data(ih->auth_ctx, &ih->metadata, sizeof(record_metadata_t)); + if (os_ret) { + ret = MBED_ERROR_FAILED_OPERATION; + goto end; } if (create_flags & REQUIRE_CONFIDENTIALITY_FLAG) { @@ -641,12 +628,10 @@ int SecureStore::do_get(const char *key, void *buffer, size_t buffer_size, size_ goto end; } - if (create_flags & REQUIRE_INTEGRITY_FLAG) { - os_ret = cmac_calc_data(ih->auth_ctx, dest_buf, chunk_size); - if (os_ret) { - ret = MBED_ERROR_FAILED_OPERATION; - goto end; - } + os_ret = cmac_calc_data(ih->auth_ctx, dest_buf, chunk_size); + if (os_ret) { + ret = MBED_ERROR_FAILED_OPERATION; + goto end; } if (create_flags & REQUIRE_CONFIDENTIALITY_FLAG) { @@ -672,35 +657,33 @@ int SecureStore::do_get(const char *key, void *buffer, size_t buffer_size, size_ *actual_size = actual_data_size; } - if (create_flags & REQUIRE_INTEGRITY_FLAG) { - uint8_t calc_cmac[cmac_size], read_cmac[cmac_size]; - os_ret = cmac_calc_finish(ih->auth_ctx, calc_cmac); - if (os_ret) { - ret = MBED_ERROR_FAILED_OPERATION; - goto end; - } + uint8_t calc_cmac[cmac_size], read_cmac[cmac_size]; + os_ret = cmac_calc_finish(ih->auth_ctx, calc_cmac); + if (os_ret) { + ret = MBED_ERROR_FAILED_OPERATION; + goto end; + } - // Check with record CMAC - ret = _underlying_kv->get(key, read_cmac, cmac_size, 0, - ih->metadata.metadata_size + ih->metadata.data_size); - if (ret) { - goto end; - } - if (memcmp(calc_cmac, read_cmac, cmac_size) != 0) { - ret = MBED_ERROR_AUTHENTICATION_FAILED; - goto end; - } + // Check with record CMAC + ret = _underlying_kv->get(key, read_cmac, cmac_size, 0, + ih->metadata.metadata_size + ih->metadata.data_size); + if (ret) { + goto end; + } + if (memcmp(calc_cmac, read_cmac, cmac_size) != 0) { + ret = MBED_ERROR_AUTHENTICATION_FAILED; + goto end; + } - // If rollback protect, check also CMAC stored in RBP store - if (create_flags & REQUIRE_REPLAY_PROTECTION_FLAG) { - if (!rbp_key_exists) { - ret = MBED_ERROR_RBP_AUTHENTICATION_FAILED; - goto end; - } - if (memcmp(calc_cmac, rbp_cmac, cmac_size) != 0) { - ret = MBED_ERROR_RBP_AUTHENTICATION_FAILED; - goto end; - } + // If rollback protect, check also CMAC stored in RBP store + if (_rbp_kv && (create_flags & (REQUIRE_REPLAY_PROTECTION_FLAG | WRITE_ONCE_FLAG))) { + if (!rbp_key_exists) { + ret = MBED_ERROR_RBP_AUTHENTICATION_FAILED; + goto end; + } + if (memcmp(calc_cmac, rbp_cmac, cmac_size) != 0) { + ret = MBED_ERROR_RBP_AUTHENTICATION_FAILED; + goto end; } } diff --git a/features/storage/kvstore/securestore/SecureStore.h b/features/storage/kvstore/securestore/SecureStore.h index 439319b62f..2dbda2dd2f 100644 --- a/features/storage/kvstore/securestore/SecureStore.h +++ b/features/storage/kvstore/securestore/SecureStore.h @@ -27,8 +27,8 @@ #define SECURESTORE_ENABLED 1 -// Whole class is not supported if entropy or device key are not enabled -#if !defined(MBEDTLS_ENTROPY_C) || !DEVICEKEY_ENABLED +// Whole class is not supported if entropy, device key or required mbed TLS features are not enabled +#if !defined(MBEDTLS_ENTROPY_C) || !defined(MBEDTLS_CIPHER_MODE_CTR) || !defined(MBEDTLS_CMAC_C) || !DEVICEKEY_ENABLED #undef SECURESTORE_ENABLED #define SECURESTORE_ENABLED 0 #endif