Merge pull request #8987 from davidsaada/david_securestore_fixes

Fix a few SecureStore issues (following preliminary security review)

Author: 0xc0170

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),
 };
  

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;

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},

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);

features/storage/kvstore/KVStore.h

@@ -32,7 +32,7 @@ class KVStore {
     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 @@ class KVStore {
          * The Key flags, possible flags combination:
          * WRITE_ONCE_FLAG,
          * REQUIRE_CONFIDENTIALITY_FLAG,
-         * REQUIRE_INTEGRITY_FLAG,
          * REQUIRE_REPLAY_PROTECTION_FLAG
          */
         uint32_t flags;

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();

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;