Improvements to FIT ciphering

[Philippe REYNES <philippe.reynes@softathome.com>]

Hi Patrick,

Sorry for this late anwser, I was very busy this week.

> Hi Simon & Philippe,
> 
> I've been thinking about this some more and have added a few points
> below. I will need feedback before proposing any patches for the
> remaining issues.
> 
> On Fri, Jul 24, 2020 at 12:06 PM Patrick Oppenlander
> <patrick.oppenlander@gmail.com> wrote:
>> 
>> Issue #1
>> ========
>> 
>> Currently, mkimage treats the IV in the same manner as the encryption
>> key. There is an iv-name-hint property which mkimage uses to read the
>> IV from a file in the keys directory. This can then be written to
>> u-boot.dtb along with the encryption key.
>> 
>> The problem with that is that u-boot.dtb is baked in at production
>> time and is generally not field upgradable. That means that the IV is
>> also baked in which is considered bad practice especially when using
>> CBC mode (see CBC IV attack). In general it is my understanding that
>> you should never use a key+IV twice regardless of cipher or mode.
>> 
>> In my opinion a better solution would have been to write the IV into
>> the FIT image instead of iv-name-hint (it's only 16 bytes!), and
>> regenerate it (/dev/random?) each and every time the data is ciphered.
>> 
> 
> If U-Boot needs to continue supporting AES-CBC I think the only option
> here is to deprecate the "iv-name-hint" property and replace it with
> an "iv" property. This should be possible in a backward-compatible
> manner.

I prefer to keep the support of aes-cbc, and I like the idea of storing
the IV in the FIT image.

But I don't really understand the issue with iv-name-hint. To stay
compatible, for example, we could simply add a propert "iv-store-in-fit"
in the device tree.


>> 
>> An even better solution is to use AES-GCM (or something similar) as
>> this includes the IV with the ciphertext, simplifying the above, and
>> also provides authentication addressing another issue (see below).
>> 
> 
> In my opinion it would be better to deprecate AES-CBC and replace it
> with AES-GCM. I can see no advantages to supporting both, and can see
> no reason to use AES-CBC over AES-GCM apart from a minor performance
> advantage.

I also think that AES-GCM is a really good idea.

But I prefer to keep aes-cbc support. And to go further, I think we may
support several  algo (for example AES-CTR). The algo choice may change
depending on the project. The boot speed may be very important (or not).

 
>> Issue #2
>> =======
>> 
>> The current implementation uses encrypt-then-sign. I like this
>> approach as it means that the FIT image can be verified outside of
>> U-Boot without requiring encryption keys. It is also considered best
>> practise.
>> 
>> However, for this to be secure, the details of the cipher need to be
>> included in the signature, otherwise an attacker can change the cipher
>> or key/iv properties.
>> 
>> I do not believe that properties in the cipher node are currently
>> included when signing a FIT configuration including an encrypted
>> image. That should be a simple fix. Fixing it for image signatures
>> might be a bit more tricky.
> 
> I have posted a patch [1] which Philippe has reviewed which includes
> the cipher node when signing a configuration.
> 
> It looks to be a much more intrusive (and incompatible) change to
> include the cipher node in an image signature. Perhaps it would be
> better for mkimage to issue a warning or error in this case and
> document why it is not recommended?

I don't see the issue, but I haven't looked deeply ....

> I don't personally have a use case for signing an image. All of my FIT
> images use configuration signatures instead. Is there a common use
> case for which this needs to be solved or could we say that signing an
> encrypted image is simply not supported?

We may provide a warning, but not allowing it seems a bit "hard".
Is it really problematic to not sign the cipher node ?

>> Issue #3
>> =======
>> 
>> Due to the nature of encrypt-then-sign U-Boot can verify that the
>> ciphertext is unmodified, but it has no way of making sure that the
>> key used to encrypt the image matches the key in u-boot.fit used for
>> decryption. This can result in an attempt to boot gibberish and I
>> think it can open up certain attack vectors.
>> 
>> The best way I know of to fix this is to use an authenticated
>> encryption mode such as AES-GCM or something similar.
> 
> I still think this is the best approach.

I agree that supporting AES-GCM would increase the security,
so it is a really good idea.
But, I think that we should not impose aes-gcm.
 
> Patrick
> 
> [1] https://lists.denx.de/pipermail/u-boot/2020-July/421989.html


In few words, I like the idea of supporting AES-GCM.


Best regards,
Philippe