From mboxrd@z Thu Jan 1 00:00:00 1970 From: Philippe REYNES Date: Mon, 24 Aug 2020 17:57:02 +0200 (CEST) Subject: Improvements to FIT ciphering In-Reply-To: References: <638649134.465093.1596819787967.JavaMail.zimbra@softathome.com> Message-ID: <719781660.569529.1598284622393.JavaMail.zimbra@softathome.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 >> > 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