Message ID | 20210816205835.76183-1-bmeneg@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [v2,ima-evm-utils] libimaevm: make SHA-256 the default hash algorithm | expand |
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
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
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)?
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);
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(-)