Improvements to FIT ciphering
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
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
+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
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
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++; }
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
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---sciss...
Patrick
On Thu, Jul 30, 2020 at 02:30:47PM +1000, patrick.oppenlander@gmail.com wrote:
From: Patrick Oppenlander patrick.oppenlander@gmail.com
This patch addresses issue #2 for signed configurations.
Applied to u-boot/master, thanks!
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
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
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
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
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
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
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
participants (5)
-
Patrick Oppenlander -
patrick.oppenlander＠gmail.com
-
Philippe REYNES -
Simon Glass -
Tom Rini