Code restructuring: bootutil/encrypted

[davidvincze]

Original comments (from #2022):

I am fine with the changes. I think that we should have some rework here because the code for key decoding keeps repeating in one form or another.
Could you elaborate on this? Do you mean the:

• bootutil_rsa_oaep_decrypt,
• key_unwrap,
• parse_ec256_enckey
• parse_x25519_enckey,
  functions?

Yes. Since only one type of encryption is used at once, we could define some key processing API that will link same name functions, but processing different keys, depending on what algorithm is selected. This can be later extended to take MCUboot identifier for key, if we decide to support more than one alg at once.

In the end the key ends in enckey buffer anyway, in byte string form.
Which is actually a problem also: I am currently working on making the x25519 to work with PSA and have found out that the best way would be to load key to PSA storage and use key id there, this requires change in key type, as passed in boot_status.
We also lack boot_enc_ type function for key destruction.

---

We also lack boot_enc_ type function for key destruction.

I believe it was not needed before as the key-encryption key is embedded as static data and the encryption key was simply decrypted from the TLV are into the boot_status struct. But it's different with PSA as you want to destroy the imported key after it's no longer needed. Correct?

Yes. Since only one type of encryption is used at once, we could define some key processing API that will link same name functions, but processing different keys, depending on what algorithm is selected.

I believe the boot_enc_load() > boot_decrypt_key() serve this purpose. boot_decrypt_key() acts as the key processing API with the difference that it's not implemented separately for the different algorithms but it's put together with the ifdef tsunami.

[davidvincze]

Copying further conversations:

We also lack boot_enc_ type function for key destruction.

I believe it was not needed before as the key-encryption key is embedded as static data and the encryption key was simply decrypted from the TLV are into the boot_status struct. But it's different with PSA as you want to destroy the imported key after it's no longer needed. Correct?

Yes and no.
Generally we do not maintain the key lifetime properly. Currently there is no clear point where key could be discarded so maintaining the proper key lifetime load->use->discard is hard to do.
In my case, once I decrypt the key with boot_decrypt_key I should import it into PSA storage and then only pass the key id, otherwise every time any encryption/decryption, with the key, starts I need to import it and destroy it afterwards to avoid fillin the key storage with unattended keys.

There is also lack of general crypto init, before any crypto start, nor general crypto deinit (if that would be required); that would also be a point where we should zero all key memory, before passing execution further.

I believe the boot_enc_load() > boot_decrypt_key() serve this purpose. boot_decrypt_key() acts as the key processing API with the difference that it's not implemented separately for the different algorithms but it's put together with the ifdef tsunami.

Yes, that is what I mean; with the separate key processing functions the boot_enc_load could be done nicer.

Another problem is that with how the boot_enc_key_load is defined there are places, like boot_image_validate_encrypted, where the boot_status variable is only defined to get the key, and there is probably a bug where the thing works only thanks to memset, but will fail with more than one image.

[davidvincze]

@de-nordic you obviously have a better understanding of the current situation :)
We should break down this topic into tasks that can be solved with separate PRs.
For example such as:

• Implement clear encryption key lifetime management,
• Refactor code structure: encryption-key_handling,
• Supervise the boot_enc_<> API.......

[de-nordic]

Yes, regarding the encryption.

But I would rather start with defining, at least, minimal API for handling boot_loader_status, so that we can pinpoint the start and end of usage. Also some of the key management, or encryption context management, would touch it because it holds the encryption context array, of misleading type enc_key_data.

To discard the array, you have to call the boot_enc_zeorize directly passing it the array! That should be done by some API serving the boot_loader_status, not by picking the thing out of its envelope and calling other API. And the boot_enc_zeroize has to actually know the size of the array stored within the boot_loader_status, which it does calculate independently from the actual the one stored in boot_loader_status.

Regarding the boot_enc_key_load: it is, usually, called to load the key to some instance of boot_state and immediately followed by the boot_enc_set_key that applies the key to proper context, in boot_loader_status (I do not think boot_enc_key_load should even know what the boot_state looks like.)
At this point it may be assumed that key that has been loaded to boot_status is not needed anymore and can be discarded, but then why even load it to boot_status? We still hold space for array[images][slots per image] of keys in each boot_state instance.
This is also why functions like boot_image_validate_encrypted define local boot_state instance: because there is no other way to apply key to boot_loader_state.

Maybe then boot_enc API should completely discard usage of boot_state and operate on key pointer, whatever it would be, and the boot_state type should be discarded completely and its contents become part of boot_loader_status?

Additional problem is with the boot_enc_set_key, or rather calling it, is that we assume that encryption context is initialized once for entire image encryption/decryption; the the boot_encrypt uses the same context re-setting IV every time it encrypts/decrypts chunk of data.
In case of PSA, here, it does not work that way: to be able to pas IV, we have to restart the encryption/decryption operation for each chunk, which means that we have to pass the key again. So from the point of view of the PSA, the function, boot_enc_set_key, does not operate on encryption context at all, it just copies key to some intermediate storage.

I also do not think that boot_enc_ should have slot parameter, and instead be directly fed the context it is supposed to work on. Most of callers of these functions know the slot, because they have to pass it, so they can just pass the proper context pointer anyway.

[de-nordic]

We need to clean up mess in key length definitions.
Now there are following keys sizes used in code:

TC_AES_KEY_SIZE
BOOT_ENC_KEY_SIZE
BOOTUTIL_CRYPTO_AES_CTR_KEY_SIZE
BOOT_ENC_KEY_ALIGN_SIZE

The TC_AES_KEY_SIZE is of course used only with tinycrypt and is hardcoded to 16 bytes.
BOOT_ENC_KEY_SIZE is defined in bootutil headers and depends on type of AES, and can have 16 or 32 bytes.
BOOTUTIL_CRYPTO_AES_CTR_KEY_SIZE is defined in aes_ctr.h, depending on encryption backend, as alias for TC_AES_KEY_SIZE or BOOT_ENC_KEY_SIZE; the BOOT_ENC_KEY_ALIGN_SIZE is write block aligned BOOT_ENC_KEY_SIZE, and is used in flash read/write operations.
Now, there is problem. In common code we either use BOOT_ENC_KEY_SIZE, BOOTUTIL_CRYPTO_AES_CTR_KEY_SIZE or BOOTUTIL_CRYPTO_AES_CTR_KEY_SIZE, which means that when tinycrypt is used, the TC_AES_KEY_SIZE only affects BOOTUTIL_CRYPTO_AES_CTR_KEY_SIZE.

Additionally ,for example within operations like boot_decrypt_key, we write to buffers that have been defined, outside of function, with size representing with one identifier but the boot_decrypt_key is hardcoded with other. This works, because all the buffers have been defined with highest possible size BOOT_ENC_KEY_ALIGN_SIZE.