On Tue, Aug 17, 2021 at 07:26:02AM -0400, Mimi Zohar wrote: > On Tue, 2021-08-17 at 07:42 +0000, THOBY Simon wrote: > > Hi Bruno, > > > > On 8/16/21 10:58 PM, Bruno Meneguele wrote: > > > The SHA-1 algorithm is considered a weak hash algorithm and there has been > > > some movement within certain distros to drop its support completely or at > > > least drop it from the default behavior. ima-evm-utils uses it as the > > > default algorithm in case the user doesn't explicitly ask for another > > > through the --hashalgo/-a option. With that, make SHA-256 the default hash > > > algorithm instead. > > > > I'm really happy to see that patch! > > I contributed to the patchset https://lore.kernel.org/linux-integrity/20210816081056.24530-1-Simon.THOBY@xxxxxxxxxx/T/#m8372b2b55dc8e1517e37624829fc8cb4361baf4d > > because I ran into exactly this issue of (hashing files with SHA1 because of > > the "insecure" default of evmctl). > > So I'm definitely in favor of switching the default hash to sha256. > > I asked about changing the default algorithm for ima-evm-utils with Mimi right before you sent the patchset v1 to the list :) Then I left in a 3weeks vacation and when I was back you were already in the v6/v7 of that :D that's awesome. > > That said, considering that CONFIG_IMA (in the kernel) doesn't depend > > on CONFIG_CRYPTO_SHA256, isn't there a risk that files hashes/signed with > > this patch could break on a kernel where that option wasn't selected? > > (I also don't know how frequent kernels without CONFIG_CRYPTO_SHA256 > > may be - given that CONFIG_ENCRYPTED_KEYS require SHA256, probably > > not so much) > > I guess this boils down to: "do we know of any distribution not selecting > > CRYPTO_SHA256?". If we don't, then the backward compatibility impact should > > be nearly void. If we do, some decision must be taken. > > See my comments below... > > > > > > Signed-off-by: Bruno Meneguele <bmeneg@xxxxxxxxxx> > > > --- > > > Changelog: > > > v1: add ima-evm-utils to the [PATCH] part of the subject > > > > > > README | 2 +- > > > src/evmctl.c | 2 +- > > > src/libimaevm.c | 2 +- > > > 3 files changed, 3 insertions(+), 3 deletions(-) > > > > > > diff --git a/README b/README > > > index 87cd3b5cd7da..0dc02f551673 100644 > > > --- a/README > > > +++ b/README > > > @@ -41,7 +41,7 @@ COMMANDS > > > OPTIONS > > > ------- > > > > > > - -a, --hashalgo sha1 (default), sha224, sha256, sha384, sha512 > > > + -a, --hashalgo sha1, sha224, sha256 (default), sha384, sha512 > > > -s, --imasig make IMA signature > > > -d, --imahash make IMA hash > > > -f, --sigfile store IMA signature in .sig file instead of xattr > > > diff --git a/src/evmctl.c b/src/evmctl.c > > > index a8065bbe124a..e0e55bc0b122 100644 > > > --- a/src/evmctl.c > > > +++ b/src/evmctl.c > > > @@ -2496,7 +2496,7 @@ static void usage(void) > > > > > > printf( > > > "\n" > > > - " -a, --hashalgo sha1 (default), sha224, sha256, sha384, sha512, streebog256, streebog512\n" > > > + " -a, --hashalgo sha1, sha224, sha256 (default), sha384, sha512, streebog256, streebog512\n" > > > " -s, --imasig make IMA signature\n" > > > " -d, --imahash make IMA hash\n" > > > " -f, --sigfile store IMA signature in .sig file instead of xattr\n" > > > diff --git a/src/libimaevm.c b/src/libimaevm.c > > > index 8e9615796153..f6c72b878d88 100644 > > > --- a/src/libimaevm.c > > > +++ b/src/libimaevm.c > > > @@ -88,7 +88,7 @@ static const char *const pkey_hash_algo_kern[PKEY_HASH__LAST] = { > > > struct libimaevm_params imaevm_params = { > > > .verbose = LOG_INFO, > > > .x509 = 1, > > > - .hash_algo = "sha1", > > > + .hash_algo = "sha256", > > > }; > > > > > > static void __attribute__ ((constructor)) libinit(void); > > > > > > > No comments on the code change, it looks alright to me. > > Agreed with Simon's comments above. > Yes, I also agree. At first glance I would say that every distro nowadays have CONFIG_CRYPTO_SHA256 set, at least the major ones AFAICT (where ima-evm-utils have CI enabled for) so I didn't really bother too much. > Currently the default hash algorithm may be modified by supplying " > --hashalgo" on the command. We also know that whatever default hash > algorithm chosen now, will change in the future. Instead of hard > coding the default hash algorithm, does it make more sense to make it a > build time config option (e.g. DEFAULT_HASH_ALGO)? > +1 for the approach. We can make the algorithm availability check in configuration time instead of runtime, directly in evmctl source code, by grepping /proc/crypto maybe (not sure if that's the easiest way)? -- bmeneg PGP Key: http://bmeneg.com/pubkey.txt
Attachment:
signature.asc
Description: PGP signature
