[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <9a2daab5-c9c8-4f62-8012-b851631d2b26@canonical.com>
Date: Fri, 10 Nov 2023 03:52:18 -0800
From: John Johansen <john.johansen@...onical.com>
To: Dimitri John Ledkov <dimitri.ledkov@...onical.com>,
Paul Moore <paul@...l-moore.com>,
James Morris <jmorris@...ei.org>,
"Serge E. Hallyn" <serge@...lyn.com>
Cc: linux-crypto@...r.kernel.org, apparmor@...ts.ubuntu.com,
linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] apparmor: switch SECURITY_APPARMOR_HASH from sha1 to
sha256
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@...onical.com>
Acked-by: John Johansen <john.johansen@...onical.com>
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;
> }
Powered by blists - more mailing lists