Thanks Sandrine for your comments.
On Fri, 29 Nov 2019 at 20:49, Sandrine Bailleux Sandrine.Bailleux@arm.com wrote:
Hello Sumit,
Sorry for not getting back to you earlier on this.
No worries.
I started looking at your patches and although I've not completely got my head around them yet, I've got some early comments and questions. I've tried to classify them by themes below.
First of all, let me say that I'm no security expert and I did not know about the concept of authenticated encryption/decryption before looking at your patches. I spent a couple of hours reading about such algorithms in general and AES-GCM in particular. Most of what I've learned so far is based on my understanding of RFC 5116 [1].
What's the motivation for choosing GCM to start with? I've seen that it is free of patent [2], which I am guessing was a strong argument for it. I've also read that it is supposed to be quite lightweight and can take full advantage of parallel processing, although I've not looked into the details. Were these the reasons? Any other reasons?
First of all, the authenticated encryption framework for FIP payloads allows for algorithm parameter to be passed in the header (see struct fip_toc_dec_data here [1]). So it should be easy to add support for CCM algorithm too.
Now coming on to additional reason to choose AES-GCM only (apart from reasons that you have already mentioned) being: - Currently mbedTLS only exposes partial decryption APIs for GCM (mbedtls_gcm_starts(), mbedtls_gcm_update() and mbedtls_gcm_finish()) but not CCM. And we need to rely on stack based partial decryption approach (see gcm_decrypt() here [2]) as I think we can't afford large buffers for both encrypted and plain firmware image.
[1] https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/2495/2/includ... [2] https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/2494/2/driver...
Key management
fip_file_read() retrieves the key from the platform and stores it in a buffer on the stack. I don't see any code wiping it out of memory once we're done with it. Did I miss it? Unlike the root of trust public key, this is a (symmetric) secret key so it is sensitive data that we must not leave for grabs, even if the stack is in Trusted RAM and that it's likely to be overwritten by subsequent stack usage.
Good catch. Will zeroise stack area used for secret key after decryption is done.
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.
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.
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
Following is the patch I used to take measurements in case someone is interested to try it out on actual platform:
diff --git a/common/bl_common.c b/common/bl_common.c index e6f9802..d7303d2 100644 --- a/common/bl_common.c +++ b/common/bl_common.c @@ -148,6 +148,7 @@ static int load_auth_image_internal(unsigned int image_id, int is_parent_image) { int rc; + unsigned long int ticks;
#if TRUSTED_BOARD_BOOT if (dyn_is_auth_disabled() == 0) { @@ -164,11 +165,16 @@ static int load_auth_image_internal(unsigned int image_id, } #endif /* TRUSTED_BOARD_BOOT */
+ ticks = read_cntpct_el0(); /* Load the image */ rc = load_image(image_id, image_data); if (rc != 0) { return rc; } +#define SYS_COUNTER_FREQ_IN_TICKS ((1000 * 1000 * 1000) / 16) + NOTICE("Load image time: %ldus, size: %dKB\n", + (read_cntpct_el0() - ticks) * 1000000 / SYS_COUNTER_FREQ_IN_TICKS, + image_data->image_size / 1024);
#if TRUSTED_BOARD_BOOT if (dyn_is_auth_disabled() == 0) {
And what's the memory footprint impact? IIUC, AES-GCM almost does not inflate the size of the data it encrypts. The size of the ciphertext seems to be the same as the plaintext + the size of the authentication tag. So I guess there's no real impact on flash storage and Trusted RAM usage to hold decrypted firmware. But what about the mbedTLS primitives to decrypt the images? How much code and data does this add?
Following is my analysis on code and data increase due to mbedTLS primitives:
Binary size: =========
$ ls -lh build/qemu/release/*.bin -rwxrwxr-x 1 sumit sumit 58K Dec 2 12:53 build/qemu/release/bl1.bin -rwxrwxr-x 1 sumit sumit 66K Dec 2 12:53 build/qemu/release/bl2.bin -rwxrwxr-x 1 sumit sumit 29K Dec 2 12:53 build/qemu/release/bl31.bin -rw-rw-r-- 1 sumit sumit 2.5M Dec 2 12:53 build/qemu/release/fip.bin -rw-rw-r-- 1 sumit sumit 32 Dec 2 12:53 build/qemu/release/rotpk_sha256.bin
After importing mbedTLS primitives to support AES-GCM algo:
$ ls -lh build/qemu/release/*.bin -rwxrwxr-x 1 sumit sumit 67K Dec 2 12:56 build/qemu/release/bl1.bin -rwxrwxr-x 1 sumit sumit 78K Dec 2 12:56 build/qemu/release/bl2.bin -rwxrwxr-x 1 sumit sumit 29K Dec 2 12:56 build/qemu/release/bl31.bin -rw-rw-r-- 1 sumit sumit 2.5M Dec 2 12:56 build/qemu/release/fip.bin -rw-rw-r-- 1 sumit sumit 32 Dec 2 12:53 build/qemu/release/rotpk_sha256.bin
Stack and heap: Works fine with default allocations on qemu. ============
encrypt_fw tool
We have some floating ideas around re-implementing the tools (fiptool, certtool) in a scripting language (possibly python) in the future and also doing a better job at sharing a common description of the list of images to boot/authenticate between the firmware and the host tools. But we're not there yet, so I agree that implementing this new tool in C from the same "mold" as fiptool and certtool is what makes the most sense today. It's just another tool we will have to rework if and when we get there.
Sounds like a good idea to have these tools being python based.
I did not understand why this new tool needs to know what image it is encrypting. For example, one possible invocation could be:
tools/encrypt_fw/encrypt_fw \ -k 1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef \ --soc-fw bl31.bin \ --soc-fw-enc bl31_enc.bin \ --tos-fw bl32.bin \ --tos-fw-enc bl32_enc.bin
Why not invoking the tool once per image instead? As in:
encrypt_fw -k key -in ifile -out ofile
for BL31, then for BL32? Does the tool do anything different based on the type of image it receives?
"encrypt_fw" tool doesn't infer anything based on image type but image types were added for more user visibility and ease of use as follows: - Provides the capability to encrypt multiple firmwares on single invocation. - Restricts usage of tool for FIP payloads only. - Better align with Makefile framework to build command line args while building different images and finally invoke tool at once before creating FIP payload.
Regards, Sumit
Regards, Sandrine
[1] https://tools.ietf.org/html/rfc5116 [2] https://csrc.nist.gov/csrc/media/projects/block-cipher-techniques/documents/...