Improvements to FIT ciphering

Improvements to FIT ciphering

[Patrick Oppenlander]

Hi,

I recently posted some patches to the list [1], [2], [3] to address
some issues with the cipher support in mkimage. Hopefully someone gets
a chance to review these patches as I think mkimage is a bit broken
without them.

While considering using U-Boot cipher support in a product I work on,
I have convinced myself that the handling of the encryption IV could
be better, especially given that mkimage is using AES-CBC mode.
Please, correct me if I have missed something.

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.

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

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.

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.


Kind regards,

Patrick

[1] https://lists.denx.de/pipermail/u-boot/2020-July/420399.html
[2] https://lists.denx.de/pipermail/u-boot/2020-July/420400.html
[3] https://lists.denx.de/pipermail/u-boot/2020-July/420401.html

Improvements to FIT ciphering

[Patrick Oppenlander]

On Fri, Jul 24, 2020 at 12:06 PM Patrick Oppenlander
<patrick.oppenlander@gmail.com> wrote:
>
> Hi,
>
> I recently posted some patches to the list [1], [2], [3] to address
> some issues with the cipher support in mkimage. Hopefully someone gets
> a chance to review these patches as I think mkimage is a bit broken
> without them.
>
> While considering using U-Boot cipher support in a product I work on,
> I have convinced myself that the handling of the encryption IV could
> be better, especially given that mkimage is using AES-CBC mode.
> Please, correct me if I have missed something.
>
> 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.
>
> 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).
>
> 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.
>
> 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.
>
>
> Kind regards,
>
> Patrick
>
> [1] https://lists.denx.de/pipermail/u-boot/2020-July/420399.html
> [2] https://lists.denx.de/pipermail/u-boot/2020-July/420400.html
> [3] https://lists.denx.de/pipermail/u-boot/2020-July/420401.html

Hi Simon,

I posted this writeup to the u-boot list and forgot to CC you. Sorry about that.

Patrick

Improvements to FIT ciphering

[Simon Glass]

+Philippe Reynes too


On Mon, 27 Jul 2020 at 16:50, Patrick Oppenlander
<patrick.oppenlander@gmail.com> wrote:
>
> On Fri, Jul 24, 2020 at 12:06 PM Patrick Oppenlander
> <patrick.oppenlander@gmail.com> wrote:
> >
> > Hi,
> >
> > I recently posted some patches to the list [1], [2], [3] to address
> > some issues with the cipher support in mkimage. Hopefully someone gets
> > a chance to review these patches as I think mkimage is a bit broken
> > without them.
> >
> > While considering using U-Boot cipher support in a product I work on,
> > I have convinced myself that the handling of the encryption IV could
> > be better, especially given that mkimage is using AES-CBC mode.
> > Please, correct me if I have missed something.
> >
> > 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.
> >
> > 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).
> >
> > 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.
> >
> > 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.
> >
> >
> > Kind regards,
> >
> > Patrick
> >
> > [1] https://lists.denx.de/pipermail/u-boot/2020-July/420399.html
> > [2] https://lists.denx.de/pipermail/u-boot/2020-July/420400.html
> > [3] https://lists.denx.de/pipermail/u-boot/2020-July/420401.html
>
> Hi Simon,
>
> I posted this writeup to the u-boot list and forgot to CC you. Sorry about that.
>
> Patrick

Improvements to FIT ciphering

[Philippe REYNES]

Hi Simon and Patrick,

Sorry, I've missed this serie of patches.
I check them and send a review ASAP.

Regards,
Philippe


> +Philippe Reynes too
> 
> 
> On Mon, 27 Jul 2020 at 16:50, Patrick Oppenlander
> <patrick.oppenlander@gmail.com> wrote:
>> 
>> On Fri, Jul 24, 2020 at 12:06 PM Patrick Oppenlander
>> <patrick.oppenlander@gmail.com> wrote:
>> > 
>> > Hi,
>> > 
>> > I recently posted some patches to the list [1], [2], [3] to address
>> > some issues with the cipher support in mkimage. Hopefully someone gets
>> > a chance to review these patches as I think mkimage is a bit broken
>> > without them.
>> > 
>> > While considering using U-Boot cipher support in a product I work on,
>> > I have convinced myself that the handling of the encryption IV could
>> > be better, especially given that mkimage is using AES-CBC mode.
>> > Please, correct me if I have missed something.
>> > 
>> > 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.
>> > 
>> > 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).
>> > 
>> > 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.
>> > 
>> > 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.
>> > 
>> > 
>> > Kind regards,
>> > 
>> > Patrick
>> > 
>> > [1] https://lists.denx.de/pipermail/u-boot/2020-July/420399.html
>> > [2] https://lists.denx.de/pipermail/u-boot/2020-July/420400.html
>> > [3] https://lists.denx.de/pipermail/u-boot/2020-July/420401.html
>> 
>> Hi Simon,
>> 
>> I posted this writeup to the u-boot list and forgot to CC you. Sorry about that.
>> 
>> Patrick

[PATCH] mkimage: fit: include image cipher in configuration signature

[patrick.oppenlander at gmail.com]

From: Patrick Oppenlander <patrick.oppenlander@gmail.com>

This patch addresses issue #2 for signed configurations.

-----8<-----

Including the image cipher properties in the configuration signature
prevents an attacker from modifying cipher, key or iv properties.

Signed-off-by: Patrick Oppenlander <patrick.oppenlander@gmail.com>
---
 tools/image-host.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/tools/image-host.c b/tools/image-host.c
index e5417beee5..3d52593e36 100644
--- a/tools/image-host.c
+++ b/tools/image-host.c
@@ -744,6 +744,23 @@ static int fit_config_get_hash_list(void *fit, int conf_noffset,
 			return -ENOMSG;
 		}
 
+		/* Add this image's cipher node if present */
+		noffset = fdt_subnode_offset(fit, image_noffset,
+					     FIT_CIPHER_NODENAME);
+		if (noffset != -FDT_ERR_NOTFOUND) {
+			if (noffset < 0) {
+				printf("Failed to get cipher node in configuration '%s/%s' image '%s': %s\n",
+				       conf_name, sig_name, iname,
+				       fdt_strerror(noffset));
+				return -EIO;
+			}
+			ret = fdt_get_path(fit, noffset, path, sizeof(path));
+			if (ret < 0)
+				goto err_path;
+			if (strlist_add(node_inc, path))
+				goto err_mem;
+		}
+
 		image_count++;
 	}
 
-- 
2.27.0

[PATCH] mkimage: fit: include image cipher in configuration signature

[Philippe REYNES]

Hi Patrick,


> From: Patrick Oppenlander <patrick.oppenlander@gmail.com>
> 
> This patch addresses issue #2 for signed configurations.
> 
> -----8<-----

This "line" will be included in the commit message ;)

> Including the image cipher properties in the configuration signature
> prevents an attacker from modifying cipher, key or iv properties.
> 
> Signed-off-by: Patrick Oppenlander <patrick.oppenlander@gmail.com>


Reviewed-by: Philippe Reynes <philippe.reynes@softathome.com>


Regards,
Philippe

> ---
> tools/image-host.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
> 
> diff --git a/tools/image-host.c b/tools/image-host.c
> index e5417beee5..3d52593e36 100644
> --- a/tools/image-host.c
> +++ b/tools/image-host.c
> @@ -744,6 +744,23 @@ static int fit_config_get_hash_list(void *fit, int
> conf_noffset,
> return -ENOMSG;
> }
> 
> + /* Add this image's cipher node if present */
> + noffset = fdt_subnode_offset(fit, image_noffset,
> + FIT_CIPHER_NODENAME);
> + if (noffset != -FDT_ERR_NOTFOUND) {
> + if (noffset < 0) {
> + printf("Failed to get cipher node in configuration '%s/%s' image '%s': %s\n",
> + conf_name, sig_name, iname,
> + fdt_strerror(noffset));
> + return -EIO;
> + }
> + ret = fdt_get_path(fit, noffset, path, sizeof(path));
> + if (ret < 0)
> + goto err_path;
> + if (strlist_add(node_inc, path))
> + goto err_mem;
> + }
> +
> image_count++;
> }
> 
> --
> 2.27.0

[PATCH] mkimage: fit: include image cipher in configuration signature

[Patrick Oppenlander]

On Fri, Jul 31, 2020 at 12:59 AM Philippe REYNES
<philippe.reynes@softathome.com> wrote:
>
> Hi Patrick,
>
>
> > From: Patrick Oppenlander <patrick.oppenlander@gmail.com>
> >
> > This patch addresses issue #2 for signed configurations.
> >
> > -----8<-----
>
> This "line" will be included in the commit message ;)
>

Hi Philippe,

apply using "git am --scissors" [1].

[1] https://git-scm.com/docs/git-mailinfo#Documentation/git-mailinfo.txt---scissors

Patrick

[PATCH] mkimage: fit: include image cipher in configuration signature

[Tom Rini]

On Thu, Jul 30, 2020 at 02:30:47PM +1000, patrick.oppenlander at gmail.com wrote:

> From: Patrick Oppenlander <patrick.oppenlander@gmail.com>
> 
> This patch addresses issue #2 for signed configurations.
> 

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200808/0ce5f4a9/attachment.sig>

Improvements to FIT ciphering

[Patrick Oppenlander]

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.

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

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

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

Patrick

[1] https://lists.denx.de/pipermail/u-boot/2020-July/421989.html

Improvements to FIT ciphering

[Philippe REYNES]

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

Improvements to FIT ciphering

[Patrick Oppenlander]

On Sat, Aug 8, 2020 at 3:03 AM Philippe REYNES
<philippe.reynes@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@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

Improvements to FIT ciphering

[Philippe REYNES]

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

Improvements to FIT ciphering

[Patrick Oppenlander]

On Tue, Aug 25, 2020 at 1:57 AM Philippe REYNES
<philippe.reynes@softathome.com> wrote:
>
> 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).

Hi Philippe,

here is what I had in mind, in the .its we would put:

cipher {
    algo = "aes256";
    key-name-hint = "aeskey";
};

when mkimage processes this it opens /dev/urandom to generate a unique
IV. It then uses this IV to perform the encryption and writes it IV to
the .fit image like so:

cipher {
    algo = "aes256";
    key-name-hint = "aeskey";
    iv = <0xa16e090c 0x7e116bf8 0x75c44329 0x3278c74d>;
}

I don't think there is a need for a "iv-in-fit" property and
"iv-name-hint" can be deprecated.

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

Unfortunately right now it is crunch time at $DAYJOB to meet a
deadline by the end of September, so I don't have much (if any) time
to dedicate to working on U-Boot right now.

There are actually five issues on my list to address in U-Boot/mkimage:

* mkimage needs to generate encryption IV using /dev/urandom
* FIT image signatures need to include cipher node
* AES-GCM cipher support
* mkimage -B option doesn't zero padding bytes
* mkimage -B option unnecessarily pads the end of the image

I was planning on working through these when I get time, but I have
not started on any of them yet. So, if you have time (and energy),
please, go ahead :)

Best regards,

Patrick

Improvements to FIT ciphering

[Philippe REYNES]

Hi Patrick,

Sorry for the late answer, I was very busy in the beginning of september

>> 
>> 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).
> 
> Hi Philippe,
> 
> here is what I had in mind, in the .its we would put:
> 
> cipher {
> algo = "aes256";
> key-name-hint = "aeskey";
> };
> 
> when mkimage processes this it opens /dev/urandom to generate a unique
> IV. It then uses this IV to perform the encryption and writes it IV to
> the .fit image like so:
> 
> cipher {
> algo = "aes256";
> key-name-hint = "aeskey";
> iv = <0xa16e090c 0x7e116bf8 0x75c44329 0x3278c74d>;
> }
> 
> I don't think there is a need for a "iv-in-fit" property and
> "iv-name-hint" can be deprecated.

I think that we should keep the compatibility with previous code.
If a company/project has started to used iv in the u-boot device tree,
may be they want to continue without changing the format.

Idea 1:
if there is a property "iv-name-hint" in the FIT image, mkimage uses
the old format, and put the iv in the u-boot device tree. Otherwise,
mkimage generate a random iv an put it in the FIT image (recommanded solution).

Idea 2:
We manage four cases according to the properties in the its file:
- property "iv-name-hint" and no flag "iv-in-fit" :
  => the iv is static and added in the u-boot device tree (actual scheme)
- property "iv-name-hint" and flag "iv-in-fit" : 
  => the iv is static and added in the FIT image
- no property "iv-name-hint" and no flag "iv-in-fit" :
  => the iv is generated and added to the u-boot device tree
- no property "iv-name-hint" and flag "iv-in-fit" :
  => the iv is generated and added in the FIT image (recomanded scheme)


>> > 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)
> 
> Unfortunately right now it is crunch time at $DAYJOB to meet a
> deadline by the end of September, so I don't have much (if any) time
> to dedicate to working on U-Boot right now.
> 
> There are actually five issues on my list to address in U-Boot/mkimage:
> 
> * mkimage needs to generate encryption IV using /dev/urandom
> * FIT image signatures need to include cipher node
> * AES-GCM cipher support
> * mkimage -B option doesn't zero padding bytes
> * mkimage -B option unnecessarily pads the end of the image

I've got a lot of work too,  so I can't do all those features.
But I'll try to work on the (random) IV generation and set it in
the FIT image.


> I was planning on working through these when I get time, but I have
> not started on any of them yet. So, if you have time (and energy),
> please, go ahead :)

I'll do my best to start this work.

 
> Best regards,
> 
> Patrick

Best regards,
Philippe

Improvements to FIT ciphering

[Patrick Oppenlander]

Hi Philippe,

On Fri, Sep 11, 2020 at 2:08 AM Philippe REYNES
<philippe.reynes@softathome.com> wrote:
>
> Hi Patrick,
>
> Sorry for the late answer, I was very busy in the beginning of september

No problem at all. I have a product deadline at the end of September
rapidly approaching so I am also extremely busy at the moment.

> >>
> >> 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).
> >
> > Hi Philippe,
> >
> > here is what I had in mind, in the .its we would put:
> >
> > cipher {
> > algo = "aes256";
> > key-name-hint = "aeskey";
> > };
> >
> > when mkimage processes this it opens /dev/urandom to generate a unique
> > IV. It then uses this IV to perform the encryption and writes it IV to
> > the .fit image like so:
> >
> > cipher {
> > algo = "aes256";
> > key-name-hint = "aeskey";
> > iv = <0xa16e090c 0x7e116bf8 0x75c44329 0x3278c74d>;
> > }
> >
> > I don't think there is a need for a "iv-in-fit" property and
> > "iv-name-hint" can be deprecated.
>
> I think that we should keep the compatibility with previous code.
> If a company/project has started to used iv in the u-boot device tree,
> may be they want to continue without changing the format.
>
> Idea 1:
> if there is a property "iv-name-hint" in the FIT image, mkimage uses
> the old format, and put the iv in the u-boot device tree. Otherwise,
> mkimage generate a random iv an put it in the FIT image (recommanded solution).
>
> Idea 2:
> We manage four cases according to the properties in the its file:
> - property "iv-name-hint" and no flag "iv-in-fit" :
>   => the iv is static and added in the u-boot device tree (actual scheme)
> - property "iv-name-hint" and flag "iv-in-fit" :
>   => the iv is static and added in the FIT image
> - no property "iv-name-hint" and no flag "iv-in-fit" :
>   => the iv is generated and added to the u-boot device tree
> - no property "iv-name-hint" and flag "iv-in-fit" :
>   => the iv is generated and added in the FIT image (recomanded scheme)

My opinion Idea 1 is simpler both in implementation & documentation,
which is probably enough for me to say it's the way we should go.
Perhaps others could contribute an opinion here?

>
> >> > 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)
> >
> > Unfortunately right now it is crunch time at $DAYJOB to meet a
> > deadline by the end of September, so I don't have much (if any) time
> > to dedicate to working on U-Boot right now.
> >
> > There are actually five issues on my list to address in U-Boot/mkimage:
> >
> > * mkimage needs to generate encryption IV using /dev/urandom
> > * FIT image signatures need to include cipher node
> > * AES-GCM cipher support
> > * mkimage -B option doesn't zero padding bytes
> > * mkimage -B option unnecessarily pads the end of the image
>
> I've got a lot of work too,  so I can't do all those features.
> But I'll try to work on the (random) IV generation and set it in
> the FIT image.

I didn't mean to dump my whole bug list on you :)

In my opinion the image signature covering the cipher node is also
quite important if you have time.

I'll make sure to send you an email if I start on any of this to make
sure we aren't duplicating effort.

> > I was planning on working through these when I get time, but I have
> > not started on any of them yet. So, if you have time (and energy),
> > please, go ahead :)
>
> I'll do my best to start this work.

That's great news,

Patrick