Skip to content

Fix a few SecureStore issues (following preliminary security review) #8987

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 11, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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),
};

Expand Down Expand Up @@ -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),
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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

Expand Down Expand Up @@ -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.
Expand All @@ -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`.
Expand Down Expand Up @@ -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.
Expand All @@ -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.
Expand All @@ -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`.
Expand Down Expand Up @@ -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;
Expand Down
17 changes: 2 additions & 15 deletions features/storage/TESTS/kvstore/general_tests_phase_1/main.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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()
{
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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},
Expand Down
33 changes: 15 additions & 18 deletions features/storage/TESTS/kvstore/securestore_whitebox/main.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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
Expand All @@ -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);
Expand Down Expand Up @@ -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
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand Down
3 changes: 1 addition & 2 deletions features/storage/kvstore/KVStore.h
Original file line number Diff line number Diff line change
Expand Up @@ -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),
};

Expand All @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion features/storage/kvstore/conf/kv_config.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
3 changes: 1 addition & 2 deletions features/storage/kvstore/global_api/kvstore_global_api.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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;
Expand Down
Loading