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. > > 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. > > > > > 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. 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)? thanks, Mimi
