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.oppenlan...@gmail.com> wrote: >> >> On Fri, Jul 24, 2020 at 12:06 PM Patrick Oppenlander >> <patrick.oppenlan...@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
