No internal kv store for storing Root Of Trust key when using TDB_EXTERNAL_NO_RBP

[Jaco-Liebenberg]

Description of defect

I might be missing something but this looks and smells like a bug to me.

No internal kvstore gets mapped in KVMap in which to store Root of Trust key when storage.storage_type gets set to TDB_EXTERNAL_NO_RBP. I am well aware that because there is no Roll Back Protection an internal kvstore is not needed. But if we want to encrypt anything in the kvstore on the external TDB we need encryption keys. These encryption keys should be derived from the Root of Trust key which gets saved (and should be saved) in a kvstore in the internal memory of the processor. As such it is my understanding that an internal kvstore is still needed. Like I said I might be missing something but from examining SecureStore and DeviceKey, what I've said does seem to be the case.

I came across this bug while playing around with the different storage configurations. I wanted to see the encryption of values stored in the kvstore in action and as such, I adapted the kv_store_global_api_example application as follows:

#include "mbed.h"
#include <stdio.h>
#include <string.h>
#include "KVStore.h"
#include "kvstore_global_api.h"
#include "kv_config.h"
#include "mbed_error.h"
#include "mbed_trace.h"
#include "FlashIAPBlockDevice.h"

using namespace mbed;

#define EXAMPLE_KV_VALUE_LENGTH 64
#define EXAMPLE_KV_KEY_LENGTH 32
#define err_code(res) MBED_GET_ERROR_CODE(res)

void kv_store_global_api_example();

static inline uint32_t align_up(uint64_t val, uint64_t size)
{
    return (((val - 1) / size) + 1) * size;
}

int get_flashiap_bd_addresses(bd_addr_t *start_address)
{
    int ret = MBED_SUCCESS;

    FlashIAP flash;
    static const int STORE_SECTORS = 4;

    flash.init();

    // Lets work from end of the flash backwards
    bd_addr_t curr_addr = flash.get_flash_start() + flash.get_flash_size();

    for (int i = STORE_SECTORS; i; i--) {
        bd_size_t sector_size = flash.get_sector_size(curr_addr - 1);
        curr_addr -= sector_size;
    }

    // bd and application-sectors mustn't overlap
    uint32_t first_wrtbl_sector_addr =
        (uint32_t)(align_up(FLASHIAP_APP_ROM_END_ADDR, flash.get_sector_size(FLASHIAP_APP_ROM_END_ADDR)));

    MBED_ASSERT(curr_addr >= first_wrtbl_sector_addr);
    if (curr_addr < first_wrtbl_sector_addr) {
        ret = MBED_ERROR_MEDIA_FULL;
    } else {
        *start_address = curr_addr;
    }

    flash.deinit();

    return ret;
}

BlockDevice *get_flash_bd(bd_addr_t start_address, bd_size_t *size)
{
    bd_addr_t flash_end_address;
    bd_addr_t flash_start_address;
    FlashIAP flash;

    int ret = flash.init();

    //Get flash parameters before starting
    flash_start_address = flash.get_flash_start();
    flash_end_address = flash_start_address + flash.get_flash_size();
    flash.deinit();

    //The block device will have all space from start address to the end of the flash
    *size = (flash_end_address - start_address);
    static FlashIAPBlockDevice bd(start_address, *size);

    return &bd;
}

BlockDevice *get_other_blockdevice()
{
    bd_size_t internal_size = 0;
    bd_addr_t internal_start_address = 0;
    int ret;

    ret = get_flashiap_bd_addresses(&internal_start_address);
    if (ret != MBED_SUCCESS) {
        return NULL;
    }

    //Get internal memory FLASHIAP block device.
    return get_flash_bd(internal_start_address, &internal_size);
}

int main()
{
    /* KV Store Static API Example */
    kv_store_global_api_example();
}

void kv_store_global_api_example()
{
    char kv_value_in[EXAMPLE_KV_VALUE_LENGTH] = {"kvstore_dummy_value_hello_world"};
    char kv_key_in[EXAMPLE_KV_KEY_LENGTH] = {"/kv/dummy_key1"};

    /* kv store iterator */
    kv_iterator_t kvstore_it;

    int res = MBED_ERROR_NOT_READY;

    /* Start By Resetting the KV Storage */
    printf("kv_reset\n");
    res = kv_reset("/kv/");
    printf("kv_reset -> %d\n", err_code(res));

    /* Set First 'Dummy' Key/Value pair with unprotected clear value data */
    printf("kv_set first dummy key\n");
    res = kv_set(kv_key_in, kv_value_in, strlen(kv_value_in), 0);
    printf("kv_set -> %d\n", err_code(res));

}

with the following mbed_app.json

{
    "target_overrides": {
        "*": {
            "platform.all-stats-enabled": false,
            "platform.error-filename-capture-enabled": false,
            "storage.storage_type": "TDB_EXTERNAL_NO_RBP",
            "storage_tdb_external_no_rbp.blockdevice": "other"            
        }
    }
}

I am aware that in this application I am creating a FlashAIPBlockDevice at the end of the internal flash memory of the processor and that I am then "tricking" the application into using that BlockDevice as if it is an external BlockDevice. That is by design. I am also aware that in doing so I might be causing the bug rather than it being an inherent bug of the overall system design, although from going through the code it is my understanding that this is not the case. I am also aware that should an internal kvstore be created in which the Root of Trust key be stored that that kvstore's memory would most probably coincide with the memory of my self-created FlashAIPBlockDevice.

Be all of that as it may, I still believe this example application is a good example to demonstrate the perceived bug I am reporting. Towards that goal please examine the call stack below:

Examining the call stack it can be seen that the kvstore_global_api has been used to set a value to the global kvstore, that the global kvstore referred it to the underlining SecureStore. That the SecureStore started the encryption process and requested a key derived from the Root Of Trust key. That the derived key generation process requested the Root of Trust, found there do not exist one yet, generated a new random key and started injecting this new Root of Trust. This was done calling DeviceKey::write_key_to_kvstore which in turn called KVMap::get_internal_kv_instance. The call stack is now at the point where it will return NULL for KVMap::get_internal_kv_instance because an internal kvstore was never mapped.

Like I said this might have happened because of my "trickery" above but examining _storage_config_TDB_EXTERNAL_NO_RBP() in conjunction with _storage_config_tdb_external_common() it would seem that the bug would have happened even if a true external BlockDevice was used.

Suggested solution

I do not at this point trust my understanding of the complete system design well enough to make anything more than a recommendation. That recommendation would be to always create a FlashAIPBlockDevice (even if it is just the last two sectors of the internal flash memory) and to always map that to the kvstore_config.internal_bd

Target(s) affected by this defect ?

This bug was discovered using a nucleo_f429zi but should affect all targets

Toolchain(s) (name and version) displaying this defect ?

This bug was discovered using gcc_arm version 8.2.1 but should affect all Toolchains

What version of Mbed-os are you using (tag or sha) ?

mbed-os-5.14.1, 679d248

What version(s) of tools are you using. List all that apply (E.g. mbed-cli)

mbed-cli version 1.10.1

How is this defect reproduced ?

This defect can be reproduced by running the application above or by setting storage.storage_type to TDB_EXTERNAL_NO_RBP (providing you have some form of external storage to your disposal) in mbed_app.json and then using the Global Static API to set a value in the global kvstore.

[0xc0170]

cc @ARMmbed/mbed-os-storage

[ciarmcom]

Thank you for raising this detailed GitHub issue. I am now notifying our internal issue triagers.
Internal Jira reference: https://jira.arm.com/browse/IOTOSM-2442

[LDong-Arm]

@Jaco-Liebenberg Thanks for your analysis, I can totally confirm it. The issue happens with TDB_EXTERNAL_NO_RBP and FILESYSTEM_NO_RBP, as no internal TDB is created to store DeviceKey RoT. The assumption is internal TDB only stores data for rollback checks, and it misses DeviceKey RoT.

I've created #13986 to fix this. Additionally, sometime after you created this issue, DeviceKey RoT needs to be manually generated in the main app now, so ARMmbed/mbed-os-example-kvstore#67 is also needed.

@evedon Both PRs are ready for review, thanks.

[evedon]

The analysis is correct but the proposed solution is a major breaking change.

In the current implementation, SecureStore can be created on top of external Flash only and in that configuration less security features are provided. Only encryption is possible. Authentication, rollback protection and "write once" are only possible with TDBStore on top of internal flash.

I think the solution is to use the direct_access_to_devicekey, see https://github.com/ARMmbed/mbed-os/blob/master/storage/kvstore/direct_access_devicekey/include/direct_access_devicekey/DirectAccessDevicekey.h/#L39

@davidsaada could you advise please?

[davidsaada]

@evedon This takes me long time back, so hopefully I'm not forgetting anything here.
First of all, Without going through all @Jaco-Liebenberg's comments, I believe his analysis is correct. This whole NO_RBP configuration is basically a design bug (both TDBStore and FileSystemStore). The internal storage is needed for both rollback protection and device key. The fact that we don't need RBP still doesn't mean we can give up the internal storage, due to the fact we still need it for encryption. This is why we have decided that the NO_RBP configurations will no longer be supported. Can't remember why we didn't remove them from the code as well.
As for direct_access_to_devicekey, it was simply created for bootloader usage. It's a very simple code, giving readonly access to the device key, without needing to use TDBStore, hence keeping the code much smaller. This is its sole purpose.

[LDong-Arm]

@evedon @davidsaada Thanks for the comments.

The analysis is correct but the proposed solution is a major breaking change.

In the current implementation, SecureStore can be created on top of external Flash only and in that configuration less security features are provided. Only encryption is possible. Authentication, rollback protection and "write once" are only possible with TDBStore on top of internal flash.

The PR doesn't alter the behaviour of anything. In the NO_RBP case, the internal TDB created purely for DeviceKey RoT, not for SecureStore itself. Just as before, rollback/replay protection remains disabled in SecureStore because of the flag:

mbed-os/storage/kvstore/kv_config/source/kv_config.cpp

Lines 946 to 948 in e77b1d8

	//Masking flag - Actually used to remove any KVStore flag which is not supported
	//in the chosen KVStore profile.
	kvstore_config.flags_mask = ~(KVStore::REQUIRE_REPLAY_PROTECTION_FLAG);

So I think it's a non-breaking change?

As for direct_access_to_devicekey, it was simply created for bootloader usage. It's a very simple code, giving readonly access to the device key, without needing to use TDBStore, hence keeping the code much smaller. This is its sole purpose.

Thanks for the explanation. Yea, from the DirectAccessDevicekey implementation, it's a lightweight API (useful for the bootloader) to only read a key from the expected location of an internal TDB. The main application still needs to create such an internal TDB (my PR), and the key is still set with the full-fledge DeviceKey.

[evedon]

Thanks for your reply @davidsaada
So I think the right thing is to remove the two NO_RBP configurations which are flawed but that's quite a significant change (in the code and documentation).

[LDong-Arm]

An intended benefit of NO_RBP is it doesn't require an internal TDB. If the fix involves creating one anyway, NO_RBP would be kind of pointless?

[evedon]

An intended benefit of NO_RBP is it doesn't require an internal TDB. If the fix involves creating one anyway, NO_RBP would be kind of pointless?

This is exactly why the design is flawed. It does not make sense to have a configuration which is specifically meant to work without internal TDB but creates one under the hood.

[Patater]

Fixed in #14490