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@viveris.fr/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@redhat.com>
> > > ---
> > > 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