Improvements to FIT ciphering

[Patrick Oppenlander]

On Sat, Aug 8, 2020 at 3:03 AM Philippe REYNES
<philippe.reynes at softathome.com> wrote:
>
> Hi Patrick,
>
> Sorry for this late anwser, I was very busy this week.

No problem!

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?

> > 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 at 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.

> >>
> >> 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.

> > 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.

> >> 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,

Patrick