Hi Sandrine,
On Wed, 18 Dec 2019 at 21:07, Sandrine Bailleux Sandrine.Bailleux@arm.com wrote:
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.
Yes that's perfectly fine with me.
<snip>
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.
Ok fine then let me remove nonce generation from the tool, make it a mandatory command line option and add an additional built option for TF-A to provide nonce as well.
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).
I understand your concern regarding memory impact. How about if we just pull mbedTLS encryption primitives as backend under a compile time flag? And just do an assert() at runtime if no backend is present. As I think flags in the FIP header are sufficient to make this firmware encryption feature optional.
-Sumit
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