Re: [PATCH] apparmor: switch SECURITY_APPARMOR_HASH from sha1 to sha256

[John Johansen <john.johansen@xxxxxxxxxxxxx>]

On 10/22/23 12:40, Dimitri John Ledkov wrote:
> sha1 is insecure and has colisions, thus it is not useful for even
> lightweight policy hash checks. Switch to sha256, which on modern
> hardware is fast enough.
>
> Separately as per NIST Policy on Hash Functions, sha1 usage must be
> withdrawn by 2030. This config option currently is one of many that
> holds up sha1 usage.
>
> Signed-off-by: Dimitri John Ledkov <dimitri.ledkov@xxxxxxxxxxxxx>

Acked-by: John Johansen <john.johansen@xxxxxxxxxxxxx>

I am pulling this into apparmor-next-queue and plan to drop this into
apparmor-next as soon as 6.7-r1 is released.

> ---
>   security/apparmor/Kconfig      | 12 ++++++------
>   security/apparmor/apparmorfs.c | 16 ++++++++--------
>   security/apparmor/crypto.c     |  6 +++---
>   3 files changed, 17 insertions(+), 17 deletions(-)
>
> diff --git a/security/apparmor/Kconfig b/security/apparmor/Kconfig
> index e0d1dd0a19..64cc3044a4 100644
> --- a/security/apparmor/Kconfig
> +++ b/security/apparmor/Kconfig
> @@ -57,10 +57,10 @@ config SECURITY_APPARMOR_INTROSPECT_POLICY
>   	  cpu is paramount.
>   
>   config SECURITY_APPARMOR_HASH
> -	bool "Enable introspection of sha1 hashes for loaded profiles"
> +	bool "Enable introspection of sha256 hashes for loaded profiles"
>   	depends on SECURITY_APPARMOR_INTROSPECT_POLICY
>   	select CRYPTO
> -	select CRYPTO_SHA1
> +	select CRYPTO_SHA256
>   	default y
>   	help
>   	  This option selects whether introspection of loaded policy
> @@ -74,10 +74,10 @@ config SECURITY_APPARMOR_HASH_DEFAULT
>          depends on SECURITY_APPARMOR_HASH
>          default y
>          help
> -         This option selects whether sha1 hashing of loaded policy
> -	 is enabled by default. The generation of sha1 hashes for
> -	 loaded policy provide system administrators a quick way
> -	 to verify that policy in the kernel matches what is expected,
> +	 This option selects whether sha256 hashing of loaded policy
> +	 is enabled by default. The generation of sha256 hashes for
> +	 loaded policy provide system administrators a quick way to
> +	 verify that policy in the kernel matches what is expected,
>   	 however it can slow down policy load on some devices. In
>   	 these cases policy hashing can be disabled by default and
>   	 enabled only if needed.
> diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
> index a608a6bd76..082581397d 100644
> --- a/security/apparmor/apparmorfs.c
> +++ b/security/apparmor/apparmorfs.c
> @@ -1474,7 +1474,7 @@ int __aa_fs_create_rawdata(struct aa_ns *ns, struct aa_loaddata *rawdata)
>   	rawdata->dents[AAFS_LOADDATA_REVISION] = dent;
>   
>   	if (aa_g_hash_policy) {
> -		dent = aafs_create_file("sha1", S_IFREG | 0444, dir,
> +		dent = aafs_create_file("sha256", S_IFREG | 0444, dir,
>   					      rawdata, &seq_rawdata_hash_fops);
>   		if (IS_ERR(dent))
>   			goto fail;
> @@ -1644,11 +1644,11 @@ static const char *rawdata_get_link_base(struct dentry *dentry,
>   	return target;
>   }
>   
> -static const char *rawdata_get_link_sha1(struct dentry *dentry,
> +static const char *rawdata_get_link_sha256(struct dentry *dentry,
>   					 struct inode *inode,
>   					 struct delayed_call *done)
>   {
> -	return rawdata_get_link_base(dentry, inode, done, "sha1");
> +	return rawdata_get_link_base(dentry, inode, done, "sha256");
>   }
>   
>   static const char *rawdata_get_link_abi(struct dentry *dentry,
> @@ -1665,8 +1665,8 @@ static const char *rawdata_get_link_data(struct dentry *dentry,
>   	return rawdata_get_link_base(dentry, inode, done, "raw_data");
>   }
>   
> -static const struct inode_operations rawdata_link_sha1_iops = {
> -	.get_link	= rawdata_get_link_sha1,
> +static const struct inode_operations rawdata_link_sha256_iops = {
> +	.get_link	= rawdata_get_link_sha256,
>   };
>   
>   static const struct inode_operations rawdata_link_abi_iops = {
> @@ -1739,7 +1739,7 @@ int __aafs_profile_mkdir(struct aa_profile *profile, struct dentry *parent)
>   	profile->dents[AAFS_PROF_ATTACH] = dent;
>   
>   	if (profile->hash) {
> -		dent = create_profile_file(dir, "sha1", profile,
> +		dent = create_profile_file(dir, "sha256", profile,
>   					   &seq_profile_hash_fops);
>   		if (IS_ERR(dent))
>   			goto fail;
> @@ -1749,9 +1749,9 @@ int __aafs_profile_mkdir(struct aa_profile *profile, struct dentry *parent)
>   #ifdef CONFIG_SECURITY_APPARMOR_EXPORT_BINARY
>   	if (profile->rawdata) {
>   		if (aa_g_hash_policy) {
> -			dent = aafs_create("raw_sha1", S_IFLNK | 0444, dir,
> +			dent = aafs_create("raw_sha256", S_IFLNK | 0444, dir,
>   					   profile->label.proxy, NULL, NULL,
> -					   &rawdata_link_sha1_iops);
> +					   &rawdata_link_sha256_iops);
>   			if (IS_ERR(dent))
>   				goto fail;
>   			aa_get_proxy(profile->label.proxy);
> diff --git a/security/apparmor/crypto.c b/security/apparmor/crypto.c
> index 6724e2ff6d..aad486b2fc 100644
> --- a/security/apparmor/crypto.c
> +++ b/security/apparmor/crypto.c
> @@ -106,16 +106,16 @@ static int __init init_profile_hash(void)
>   	if (!apparmor_initialized)
>   		return 0;
>   
> -	tfm = crypto_alloc_shash("sha1", 0, 0);
> +	tfm = crypto_alloc_shash("sha256", 0, 0);
>   	if (IS_ERR(tfm)) {
>   		int error = PTR_ERR(tfm);
> -		AA_ERROR("failed to setup profile sha1 hashing: %d\n", error);
> +		AA_ERROR("failed to setup profile sha256 hashing: %d\n", error);
>   		return error;
>   	}
>   	apparmor_tfm = tfm;
>   	apparmor_hash_size = crypto_shash_digestsize(apparmor_tfm);
>   
> -	aa_info_message("AppArmor sha1 policy hashing enabled");
> +	aa_info_message("AppArmor sha256 policy hashing enabled");
>   
>   	return 0;
>   }