Reapply "boot: Add MCUBOOT_HW_KEY support for image encryption" with modifications

[davidvincze]

Reapply commit 0fa4627 with requested modification after reverting it in #1996.

Original PR: #1722

[davidvincze]

@DineshDK03 could you please review?

[davidvincze]

CI is failing due to the missing signoff in the original commit.

[de-nordic]

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.

[davidvincze]

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?

[de-nordic]

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.

[davidvincze]

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.

Would it be okay to merge this PR as this is a different topic?

I've opened #2027 as a follow-up issue.

[de-nordic]

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.

Would it be okay to merge this PR as this is a different topic?

Yes, yes, yes! That is why I ack the PR, let have it in and then follow with improvements.

I've opened #2027 as a follow-up issue.

Thanks.