Hi Sumit,
I've started providing some early review comments on your patches in Gerrit but these are mainly about implementation details. I would like to continue the higher-level design discussions here in parallel on the mailing list, if that's fine with you.
On 12/6/19 1:29 PM, Sumit Garg via TF-A wrote:
Hi Sandrine,
On Thu, 5 Dec 2019 at 20:31, Sandrine Bailleux Sandrine.Bailleux@arm.com wrote:
Also, I am still trying to get my head around how this would integrate with a cryptographic engine where the key does not leave the chip. I can imagine that we could get the address of the encrypted firmware image from the FIP, pass that to a cryptographic engine, request it to decrypt it and store the result somewhere in Trusted RAM. In this case, we wouldn't call plat_get_fip_encryption_key(). Do you have any idea how we would pull this off? Like how the different modules (IO layer, crypto module, image parser module, ...) would integrate together?
In this case, I would expect platform to provide key identifier rather than actual key as part of plat_get_fip_encryption_key() which is then passed onto auth_decrypt() that is implementation specific to cryptographic engine in similar terms as currently done for mbedTLS backend.
Ah I see, so plat_get_fip_encryption_key() could return either the key itself or a key identifier. Just like plat_get_rotpk_info() can return either the key or a hash of it today. However, in the case of plat_get_rotpk_info(), it also returns some flags indicating which one it is. Don't we need something similar for plat_get_fip_encryption_key()? How will the crypto module be able to tell the difference between a key and a key ID otherwise? Or would you expect a given crypto module backend to always use either keys or key IDs, but not both?
It perfectly makes sense for crypto module backend to support both key ID and actual key. But I was thinking from use-case perspective that why would one like to expose key to the firmware if the crypto module could better protect it. And I could come up with one use-case where the SSK (burnt in SoC fuses) is provided as an actual key and BSSK (accessible only to crypto module) is provided via a key ID.
Yes, that makes sense to me.
So yes I agree with you to provide additional flags field along with key buffer. Also, I think it makes sense to rename "plat_get_fip_encryption_key()" to "plat_get_fip_enc_key_info()".
Agree.
I have some concerns around the generation of the initialization vectors in the encrypt_fw tool. Right now, IVs are simply a random sequence of bytes (obtained through a call to OpenSSL's RAND_bytes() API). Now, I would imagine that RAND_bytes() is typically based on a good random number generator and thus will generate different sequences every time it is called. At least, as long as it is called from the same machine every time. But what if we encrypt a new FIP bundle from a different machine, say in the context of a firmware update? Is it not possible that it might choose the same IV out of bad luck?
Perhaps that's an issue left to provisioning/manufacturing time and is out of the scope here. But it worries me because AFAIU, the security of AES-GCM is critically undermined if the same nonce is used multiple times with the same key (see section 5.1.1. "Nonce reuse" in RFC 5116). If the encryption key is the SSK (rather than the BSSK) then I guess the probability is even higher, as it is shared amongst a class of devices.
Agree that "nonce" should be unique and using a random number generator available on build machine was an effort towards that. But thinking about the case that you have mentioned, I think we could have an optional user provided "nonce" as an input to "encrypt_fw" tool, so that the user is aware to randomly generate and provide a unique nonce.
Yes, I like your suggestion of specifying the nonce to the tool. But I think it should be the default behaviour then. You mention an *optional* user-provided nonce, I would like to suggest we make this mandatory instead. We could provide an option to request the tool to generate the nonce, intended for development purposes.
Making nonce as default and mandatory command line option seems to be overkill for a user. It may also lead to unsafe practices of hardcoding a nonce in Makefile, shell scripts etc. So I think we should keep this as optional only for a narrow use-case where people might not trust uniqueness property of RNG from multiple build machines.
I don't know... Hard-coding a nonce in the makefile is unsafe for sure but I would think that a nonce generated by the tool is also unsafe without proper management of these nonces across keys and devices, but in a much more subtle manner. I still think the nonce generation would be better left outside of the scope of this tool.
BTW, I have already added nonce command line option as part of v3 patch-set [1]. Please have a look.
[1] https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/2496/2..3
Impact on memory footprint and performance
Do you know what the performance impact is when this feature is enabled in TF-A, to decrypt images at boot time? Obviously it depends on the platform and whether there is a dedicated cryptographic engine, and I suppose you cannot really get any relevant measurements out of QEMU but I would be interested if you've got any rough numbers.
Following are measurements based on qemu for mbedTLS software library based authenticated decryption:
BL31 plain: NOTICE: Load image time: 137us, size: 28KB BL31 encrypted: NOTICE: Load image time: 3979us, size: 28KB
BL32 plain: NOTICE: Load image time: 1791us, size: 360KB BL32 encrypted: NOTICE: Load image time: 36339us, size: 360KB
Thanks. So it's a 29% increase for BL31 and 20% for BL32. I would have naively expected similar percentages, any idea why we get such a difference between BL31 and BL32? I am just curious. Maybe it's down to how the crypto algorithm/mode of operation works under the hood?
Yes this additional increase for BL31 seems to be due to one time initialization operations carried on first invocation of mbedTLS gcm APIs, especially time taken to generate AES tables (see call to aes_gen_tables() in mbedtls/library/aes.c +576).
Ah I see, thanks for the explanation.
Regarding the memory impact, given that it's adding quite a bit of code and data (around 9KB for BL1 and 12KB for BL2 from what you were saying in a previous email), I think we need to make this feature optional and wrap it under a build flag. Not everyone will need to encrypt firmware images, having only authentication is a valid use case as well. If I am not mistaken, your patches right now unconditionally pull the mbedTLS primitives into BL1 and BL2 (as long as TBBR is enabled of course).
As a matter of fact, I am not able to build BL1 on FVP platform with your patches, presumably due to the unconditional size increase. The following build command:
make TRUSTED_BOARD_BOOT=1 ARM_ROTPK_LOCATION=devel_rsa
MBEDTLS_DIR=mbedtls bl1
... fails at link time:
aarch64-linux-gnu-ld.bfd: build/fvp/release/bl1/bl1.elf section `xlat_table' will not fit in region `RAM' aarch64-linux-gnu-ld.bfd: BL1's RW section has exceeded its limit. aarch64-linux-gnu-ld.bfd: region `RAM' overflowed by 4096 bytes Makefile:889: recipe for target 'build/fvp/release/bl1/bl1.elf' failed
Regards, Sandrine