[PATCH v2 ima-evm-utils] libimaevm: make SHA-256 the default hash algorithm

[PATCH v2 ima-evm-utils] libimaevm: make SHA-256 the default hash algorithm

[Bruno Meneguele]

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.

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);
-- 
2.31.1

Re: [PATCH v2 ima-evm-utils] libimaevm: make SHA-256 the default hash algorithm

[THOBY Simon]

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.

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@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.

Thanks,
Simon

Re: [PATCH v2 ima-evm-utils] libimaevm: make SHA-256 the default hash algorithm

[Mimi Zohar]

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.
> 
> 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@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.

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

Re: [PATCH v2 ima-evm-utils] libimaevm: make SHA-256 the default hash algorithm

[Bruno Meneguele]

[-- Attachment #1: Type: text/plain, Size: 4870 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]