Improvements to FIT ciphering

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

Hi Patrick,

>> 
>> Hi Patrick,
>> 
>> Sorry for this late anwser, I was very busy this week.
> 
> No problem!

Again, sorry I was off and then busy.

> Before I address your comments below, is there anything else I need to
> do with the previous patches I posted? I haven't contributed to U-Boot
> before and I'm not entirely sure of the correct process. Do I need to
> resubmit the patches with your reviewed-by tags included?

You have followed the correct process.
Your patches (and proposal) are fine.

>> > 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.
> 
> The IV must change each and every time the encryption is run. I am not
> sure how this can be achieved using "iv-name-hint" and storing the IV
> in the U-Boot FDT. Maybe I have missed something?
> 
> In the simplest case encrypting the same plaintext twice must not
> result in the same ciphertext as this leaks important information to
> an attacker (the plaintext payload is the same!). There are many
> examples of attacks against encryption where the IV is kept constant.
> 
> This is why I suggested replacing "iv-name-hint" with "iv". U-Boot can
> then search for "iv" first and (optionally?) fall back to
> "iv-name-hint" if "Iv" is not found. This should be a simple change.

I agree that IV should be set in the FIT.

So in the dts, we may have:
	cipher {
		algo = "aes256";
		key-name-hint = "aeskey";
		iv = "aesiv";
	};
or (I propose) :
	cipher {
		algo = "aes256";
		key-name-hint = "aeskey";
		iv-name-hint = "aesiv";
		iv-in-fit;
	};

I think that both solution should work ...

Have you planned to implement this change/feature ?
(otherwise I will try to found some time for it,
it is a really nice improvement).

>> >> 
>> >> 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).
> 
> That makes sense. There is also a code size advantage which may matter
> in some circumstances.
> 
> Perhaps we should add a "mode" property: "cbc", "ctr", "gcm", etc.
> 
>> >> 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 ....
> 
> In order to include the "cipher" node in an image signature we would
> need to change the way the image signatures work to match the way the
> configuration signatures work, i.e include "hashed-nodes" and
> "hashed-strings" properties.
> 
> I was wrong in my previous assertion that this can't be done in a
> backward compatible way. The presence of the "hashed-nodes" property
> could be a trigger to use the same algorithm as is used for
> configurations.
> 
> I think this can work nicely.

excellent :)
 
>> > 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 ?
> 
> There are many problems with not signing the cipher node. It allows an
> attacker to do all kinds of nasty things:
> * Delete the cipher node!
> * Change key-name-hint to work out which keys are supported.
> * Change the cipher used to work out which ciphers are supported.
> * Decrypt the image using an alternate cipher. This could potentially
> leak information about the decryption key.
> * More that I probably haven't thought of.
> 
> Allowing an attacker to control these properties can open the system
> up to all kinds of nasty side-channel (timing, power consumption,
> acoustic, etc.) attacks.
> 
> However, if adding "hashed-nodes" and "hashed-strings" properties to
> the image signature is acceptable we can still support signing
> ciphered images with no problems.

I think that everything should be added to the signature. I think it's
simpler and more safe.

Have you planned to implement this/propose a patch please ?
(of course, if not, I will try to found some time)

>> >> 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.
> 
> OK great!
> 
>> > 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
> 
> Thanks for your feedback,

Thanks for your ideas/proposals.
They will improve the security.

> Patrick

Regards,
Philippe