[PATCH v5 0/5] IMA: restrict the accepted digest algorithms for the security.ima xattr

[PATCH v5 0/5] IMA: restrict the accepted digest algorithms for the security.ima xattr

[THOBY Simon]

IMA protects files by storing a hash (or a signature thereof) of their
content in the security.ima xattr. While the security.ima xattr itself
is protected by EVM with either a HMAC or a digital signature, no
mechanism is currently in place to ensure that the security.ima xattr
was generated with a strong digest algorithm, and any hash defined
in the kernel will be accepted, even obsolete format like MD4 and MD5.

The kernel itself will only write this xattr with the 'ima_hash' parameter,
fixed at init, but it will also happily accept userland writes for said
xattr, and those writes may use arbitrary hash algorithms as long as the
kernel have support for it.

One important point is safeguarding users from mislabelling their
files when using userland utilities to update their files, as this
is the kind of behavior one can observe with evmctl (`evmctl ima_hash`
defaults to sha1). Another group that may be interested is those
that have deployed IMA years ago, possibly using algorithms that
was then deemed sufficiently collision-resistant, but that proved
to be weak with the passage of time (note that this could also
happen in the future with algorithms considered safe today).
This patch provides a migration path of sorts for these users.

This patch series gives users the ability to restrict the algorithms
accepted by their system, both when writing/updating xattrs, and
when appraising files, while retaining a permissive behavior by default
to preserve backward compatibility.

To provide these features, alter the behavior of setxattr to
only accept hashes built in the kernel, instead of any hash listed
in the kernel (complete list crypto/hash_info.c). In addition, the
user can define in his IMA policy the list of digest algorithms
allowed for writing to the security.ima xattr. In that case,
only algorithms present in that list are accepted for writing.

In addition, users may opt-in to whitelisting the hash
algorithms accepted when appraising thanks to the new
"appraise_hash" IMA policy option.
By default IMA will keep accepting any hash algorithm, but specifying
that option will make appraisal of files hashed with another algorithm
fail.


Even when using this option to restrict accepted hashes, a migration
to a new algorithm is still possible. Suppose your policy states you
must migrate from 'old_algo' (e.g. sha1) to 'new_algo' (e.g. one of
sha256/384/512). You can upgrade without relaxing the hash requirements:
alter your policy rules from 'appraise_hash=old_algo' to
'appraise_hash=old_algo,new_algo', load a new SETXATTR_CHECK policy
rule that accept writes using 'new_algo', reboot, relabel
all your files with 'new_algo', alter your policy rules from
'appraise_hash=old_algo,new_algo' to 'appraise_hash=new_algo',
and you're done.
While this represent a significant amount of work, it is important to
showcase that this patchset is flexible enough to let users upgrade
if needed.


This series is based on the following repo/branch:
 repo: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
 branch: master
 commit ff1176468d368232b684f75e82563369208bc371 ("Linux 5.14-rc3")

Changelog since v4:
- Deleting the ability to remove SETXATTR_CHECK rules, as it added
  a lot of concurrency troubles while creating a special case for
  SETXATTR_CHECK rules only, which is rarely a good idea (suggested by Mimi Zohar)
- Change from #ifdef CONFIG_... to IS_ENABLED (suggested by Mimi Zohar)
- Various fixes (code style, english grammar errors, double initilization to
  zero) reported by Mimi Zohar
- Fixed a logic inversion error introduced in v4 where checks where
  performed when no SETXATTR_CHECK rule was enabled.
- Do not log partial audit messages under memory pressure (suggested by Mimi Zohar)

Changelog since v3:
- fixed an issue where the first write to the policy would ignore the
  SETXATTR_CHECK attribute
- fixed potential concurrency issues (I would greatly like external
  opinions on this, because I clearly don't know much about RCU. Beside
  maybe it's better to completely ignore the duplicates SETXATTR_CHECK
  issue and not update the IMA policy in any case)
- remove the CONFIG_CRYPTO_MD5 requirement for IMA (suggested by Mimi Zohar)
- updated commit messages to follow more closely the kernel style guide
  (suggested by Mimi Zohar)
- moved the hash verification code on appraisal a bit later, to prevent
  issues when using the code with IMA in a disable/auditing mode
  (suggested by Mimi Zohar)
- limit the 'appraise_hash' parameter to the 'appraise' action
  (suggested by Mimi Zohar)

Changelog since v2:
- remove the SecureBoot-specific behavior (suggested by Mimi Zohar)
- users can now tweak through policy both the algorithms for
  appraising files (a feature already present in v2) and for writing
  with the new SETXATTR_CHECK value for the 'func' ima policy flag
- updating 'forbidden-hash-algorithm' to 'denied-hash-algorithm' and
  'unsupported-hash-algorithm' to disambiguate cases when the user
  asked for an algorithm not present in the kernel and when the system
  vendor explicitly opted in to a restricted list of accepted
  algorithms (suggested by Mimi Zohar)
- change the order of the patches to be bisect-safe while retaining
  the guarantee that a policy cannot be accepted but not enforced
  (suggested by Mimi Zohar)

Changelog since v1:
- Remove the two boot parameters (suggested by Mimi Zohar)
- filter out hash algorithms not compiled in the kernel
  on xattr writes (suggested by Mimi Zohar)
- add a special case when secure boot is enabled: only the
  ima_hash algorithm is accepted on userland writes
- add a policy option to opt-in to restricting digest algorithms
  at a per-rule granularity (suggested by Mimi Zohar)

Simon Thoby (5):
  IMA: remove the dependency on CRYPTO_MD5
  IMA: block writes of the security.ima xattr with unsupported
    algorithms
  IMA: add support to restrict the hash algorithms used for file
    appraisal
  IMA: add a policy option to restrict xattr hash algorithms on
    appraisal
  IMA: introduce a new policy option func=SETXATTR_CHECK

 Documentation/ABI/testing/ima_policy  |  15 ++-
 security/integrity/ima/Kconfig        |   1 -
 security/integrity/ima/ima.h          |  10 +-
 security/integrity/ima/ima_api.c      |   6 +-
 security/integrity/ima/ima_appraise.c |  74 ++++++++++++-
 security/integrity/ima/ima_main.c     |  19 +++-
 security/integrity/ima/ima_policy.c   | 149 ++++++++++++++++++++++++--
 7 files changed, 254 insertions(+), 20 deletions(-)

-- 
2.31.1

[PATCH v5 1/5] IMA: remove the dependency on CRYPTO_MD5

[THOBY Simon]

Remove the CRYPTO_MD5 dependency for IMA, as it is not necessary
and it hinders the efficiency of a patchset that limit the digests
allowed for the security.ima xattr.

Signed-off-by: Simon Thoby <simon.thoby@viveris.fr>
---
 security/integrity/ima/Kconfig    | 1 -
 security/integrity/ima/ima_main.c | 2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index d0ceada99243..f3a9cc201c8c 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -6,7 +6,6 @@ config IMA
 	select SECURITYFS
 	select CRYPTO
 	select CRYPTO_HMAC
-	select CRYPTO_MD5
 	select CRYPTO_SHA1
 	select CRYPTO_HASH_INFO
 	select TCG_TPM if HAS_IOMEM && !UML
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 287b90509006..7f2310f29789 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -53,7 +53,7 @@ static int __init hash_setup(char *str)
 	if (strcmp(template_desc->name, IMA_TEMPLATE_IMA_NAME) == 0) {
 		if (strncmp(str, "sha1", 4) == 0) {
 			ima_hash_algo = HASH_ALGO_SHA1;
-		} else if (strncmp(str, "md5", 3) == 0) {
+		} else if (IS_ENABLED(CONFIG_CRYPTO_MD5) && strncmp(str, "md5", 3) == 0) {
 			ima_hash_algo = HASH_ALGO_MD5;
 		} else {
 			pr_err("invalid hash algorithm \"%s\" for template \"%s\"",
-- 
2.31.1

Re: [PATCH v5 1/5] IMA: remove the dependency on CRYPTO_MD5

[Lakshmi Ramasubramanian]

Hi Simon,

On 7/28/2021 6:21 AM, THOBY Simon wrote:
> Remove the CRYPTO_MD5 dependency for IMA, as it is not necessary
> and it hinders the efficiency of a patchset that limit the digests
> allowed for the security.ima xattr.

In the patch description state the problem first and then describe how 
it is addressed in the patch. Maybe, something like the following:

MD5 is a weak digest algorithm. It hinders the efficiency of a patch set 
that aims to limit the digests allowed for the extended file attribute 
namely security.ima. MD5 should not be used for any crypto operations in 
IMA.

Remove the CRYPTO_MD5 dependency for IMA.

> 
> Signed-off-by: Simon Thoby <simon.thoby@viveris.fr>
> ---
>   security/integrity/ima/Kconfig    | 1 -
>   security/integrity/ima/ima_main.c | 2 +-
>   2 files changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
> index d0ceada99243..f3a9cc201c8c 100644
> --- a/security/integrity/ima/Kconfig
> +++ b/security/integrity/ima/Kconfig
> @@ -6,7 +6,6 @@ config IMA
>   	select SECURITYFS
>   	select CRYPTO
>   	select CRYPTO_HMAC
> -	select CRYPTO_MD5
>   	select CRYPTO_SHA1
>   	select CRYPTO_HASH_INFO
>   	select TCG_TPM if HAS_IOMEM && !UML
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 287b90509006..7f2310f29789 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -53,7 +53,7 @@ static int __init hash_setup(char *str)
>   	if (strcmp(template_desc->name, IMA_TEMPLATE_IMA_NAME) == 0) {
>   		if (strncmp(str, "sha1", 4) == 0) {
>   			ima_hash_algo = HASH_ALGO_SHA1;
> -		} else if (strncmp(str, "md5", 3) == 0) {
> +		} else if (IS_ENABLED(CONFIG_CRYPTO_MD5) && strncmp(str, "md5", 3) == 0) {
>   			ima_hash_algo = HASH_ALGO_MD5;
>   		} else {
>   			pr_err("invalid hash algorithm \"%s\" for template \"%s\"",
> 

Code change looks good.

  -lakshmi

[PATCH v5 2/5] IMA: block writes of the security.ima xattr with unsupported algorithms

[THOBY Simon]

By default, any write to the extended attributes security.ima will be
accepted, even if the xattr value uses a hash algorithm not compiled in
the kernel (which doesn't make sense, because the kernel wouldn't be able
to appraise that file, as it lacks support for validating the hash).

Prevent such writes: only writes using hash algorithms
available in the current kernel are now allowed. Any attempt to
perform these writes will be denied with an audit message.

Note however that CONFIG_IMA depends on CONFIG_CRYPTO_SHA1, which
somewhat hampers the security benefits of this measure (but MD4 and
MD5 can be disabled, which is already a significant improvement).

Signed-off-by: Simon Thoby <simon.thoby@viveris.fr>
---
 security/integrity/ima/ima_appraise.c | 50 +++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index ef9dcfce45d4..a5e0d3400bd1 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -575,6 +575,53 @@ static void ima_reset_appraise_flags(struct inode *inode, int digsig)
 		clear_bit(IMA_DIGSIG, &iint->atomic_flags);
 }
 
+/**
+ * validate_hash_algo() - Block setxattr with invalid digests
+ * @dentry: object being setxattr()'ed
+ * @xattr_value: value supplied by userland for the xattr
+ * @xattr_value_len: length of xattr_value
+ *
+ * Context: called when the user tries to write the security.ima xattr.
+ * The xattr value is mapped to some hash algorithm, and this algorithm
+ * must be built in the kernel for the setxattr to be allowed.
+ *
+ * Emit an audit message when the algorithm is invalid.
+ *
+ * Return: 0 on success, else an error.
+ */
+static int validate_hash_algo(struct dentry *dentry,
+				   const void *xattr_value,
+				   size_t xattr_value_len)
+{
+	char *path = NULL, *pathbuf = NULL;
+	enum hash_algo xattr_hash_algo;
+
+	xattr_hash_algo = ima_get_hash_algo((struct evm_ima_xattr_data *)xattr_value,
+					    xattr_value_len);
+
+	if (likely(xattr_hash_algo == ima_hash_algo ||
+		   crypto_has_alg(hash_algo_name[xattr_hash_algo], 0, 0)))
+		return 0;
+
+	pathbuf = kmalloc(PATH_MAX, GFP_KERNEL);
+	if (pathbuf) {
+		path = dentry_path(dentry, pathbuf, PATH_MAX);
+
+		/*
+		 * disallow xattr writes with algorithms disabled in the
+		 * kernel configuration
+		 */
+		integrity_audit_msg(AUDIT_INTEGRITY_DATA, d_inode(dentry),
+				    path, "collect_data",
+				    "unavailable-hash-algorithm",
+				    -EACCES, 0);
+
+		kfree(pathbuf);
+	}
+
+	return -EACCES;
+}
+
 int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
 		       const void *xattr_value, size_t xattr_value_len)
 {
@@ -595,6 +642,9 @@ int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
 		ima_reset_appraise_flags(d_backing_inode(dentry), digsig);
 		if (result == 1)
 			result = 0;
+
+		result = validate_hash_algo(dentry, xattr_value,
+					    xattr_value_len);
 	}
 	return result;
 }
-- 
2.31.1

Re: [PATCH v5 2/5] IMA: block writes of the security.ima xattr with unsupported algorithms

[Lakshmi Ramasubramanian]

Hi Simon,

On 7/28/2021 6:21 AM, THOBY Simon wrote:
> By default, any write to the extended attributes security.ima will be
> accepted, even if the xattr value uses a hash algorithm not compiled in
> the kernel (which doesn't make sense, because the kernel wouldn't be able
> to appraise that file, as it lacks support for validating the hash).

Some nit changes in the above paragraph:

By default, write to the extended file attribute, security.ima, will be 
allowed even if the hash algorithm used for the xattr value is not 
compiled in the kernel (which does not make sense because the kernel 
would not be able to appraise that file as it lacks support for 
validating the hash).

> 
> Prevent such writes: only writes using hash algorithms
> available in the current kernel are now allowed. Any attempt to
> perform these writes will be denied with an audit message.

Prevent writes to security.ima if the hash algorithm used for the xattr 
value is not available in the current kernel. Log an audit message if 
such an operation is attempted.

> 
> Note however that CONFIG_IMA depends on CONFIG_CRYPTO_SHA1, which
> somewhat hampers the security benefits of this measure (but MD4 and
> MD5 can be disabled, which is already a significant improvement).

I am not sure if the above paragraph adds any value for this patch.

> 
> Signed-off-by: Simon Thoby <simon.thoby@viveris.fr>
> ---
>   security/integrity/ima/ima_appraise.c | 50 +++++++++++++++++++++++++++
>   1 file changed, 50 insertions(+)
> 
> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> index ef9dcfce45d4..a5e0d3400bd1 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -575,6 +575,53 @@ static void ima_reset_appraise_flags(struct inode *inode, int digsig)
>   		clear_bit(IMA_DIGSIG, &iint->atomic_flags);
>   }
>   
> +/**
> + * validate_hash_algo() - Block setxattr with invalid digests
> + * @dentry: object being setxattr()'ed
> + * @xattr_value: value supplied by userland for the xattr
> + * @xattr_value_len: length of xattr_value
> + *
> + * Context: called when the user tries to write the security.ima xattr.
> + * The xattr value is mapped to some hash algorithm, and this algorithm
> + * must be built in the kernel for the setxattr to be allowed.
> + *
> + * Emit an audit message when the algorithm is invalid.
> + *
> + * Return: 0 on success, else an error.
> + */
> +static int validate_hash_algo(struct dentry *dentry,
> +				   const void *xattr_value,
> +				   size_t xattr_value_len)
> +{
> +	char *path = NULL, *pathbuf = NULL;
> +	enum hash_algo xattr_hash_algo;
> +
> +	xattr_hash_algo = ima_get_hash_algo((struct evm_ima_xattr_data *)xattr_value,
> +					    xattr_value_len);
> +
> +	if (likely(xattr_hash_algo == ima_hash_algo ||
> +		   crypto_has_alg(hash_algo_name[xattr_hash_algo], 0, 0)))
> +		return 0;
> +
> +	pathbuf = kmalloc(PATH_MAX, GFP_KERNEL);
> +	if (pathbuf) {
         if (!pathbuf)
            return -EACCESS;

Indentation for the block below can then be removed.

> +		path = dentry_path(dentry, pathbuf, PATH_MAX);
> +
> +		/*
> +		 * disallow xattr writes with algorithms disabled in the
> +		 * kernel configuration
> +		 */
The above comment is not relevant for the audit message call below. The 
purpose of this function is already stated in the function header. You 
could remove the above comment.

  -lakshmi

> +		integrity_audit_msg(AUDIT_INTEGRITY_DATA, d_inode(dentry),
> +				    path, "collect_data",
> +				    "unavailable-hash-algorithm",
> +				    -EACCES, 0);
> +
> +		kfree(pathbuf);
> +	}
> +
> +	return -EACCES;
> +}
> +
>   int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
>   		       const void *xattr_value, size_t xattr_value_len)
>   {
> @@ -595,6 +642,9 @@ int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
>   		ima_reset_appraise_flags(d_backing_inode(dentry), digsig);
>   		if (result == 1)
>   			result = 0;
> +
> +		result = validate_hash_algo(dentry, xattr_value,
> +					    xattr_value_len);
>   	}
>   	return result;
>   }
> 

[PATCH v5 3/5] IMA: add support to restrict the hash algorithms used for file appraisal

[THOBY Simon]

The kernel accepts any hash algorithm as a value for the security.ima
xattr. Users may wish to restrict the accepted algorithms to only
support strong cryptographic ones.

Provide the plumbing to restrict the permitted set of hash algorithms
used for verifying file hashes and digest algorithms stored in
security.ima xattr.

This do not apply only to IMA in hash mode, it also works with digital
signatures, in which case it checks that the hash (which was then
signed by the trusted private key) have been generated with one of
the algortihms whitelisted for this specific rule.

Signed-off-by: Simon Thoby <simon.thoby@viveris.fr>
Reviewed-by:  Mimi Zohar <zohar@linux.ibm.com>
---
 security/integrity/ima/ima.h          |  6 +++---
 security/integrity/ima/ima_api.c      |  6 ++++--
 security/integrity/ima/ima_appraise.c |  5 +++--
 security/integrity/ima/ima_main.c     | 17 ++++++++++++++---
 security/integrity/ima/ima_policy.c   | 18 ++++++++++++++++--
 5 files changed, 40 insertions(+), 12 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index f0e448ed1f9f..7ef1b214d358 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -47,7 +47,7 @@ enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8, TPM_PCR10 = 10 };
 extern int ima_policy_flag;
 
 /* set during initialization */
-extern int ima_hash_algo;
+extern int ima_hash_algo __ro_after_init;
 extern int ima_sha1_idx __ro_after_init;
 extern int ima_hash_algo_idx __ro_after_init;
 extern int ima_extra_slots __ro_after_init;
@@ -254,7 +254,7 @@ int ima_get_action(struct user_namespace *mnt_userns, struct inode *inode,
 		   const struct cred *cred, u32 secid, int mask,
 		   enum ima_hooks func, int *pcr,
 		   struct ima_template_desc **template_desc,
-		   const char *func_data);
+		   const char *func_data, unsigned int *allowed_hashes);
 int ima_must_measure(struct inode *inode, int mask, enum ima_hooks func);
 int ima_collect_measurement(struct integrity_iint_cache *iint,
 			    struct file *file, void *buf, loff_t size,
@@ -285,7 +285,7 @@ int ima_match_policy(struct user_namespace *mnt_userns, struct inode *inode,
 		     const struct cred *cred, u32 secid, enum ima_hooks func,
 		     int mask, int flags, int *pcr,
 		     struct ima_template_desc **template_desc,
-		     const char *func_data);
+		     const char *func_data, unsigned int *allowed_hashes);
 void ima_init_policy(void);
 void ima_update_policy(void);
 void ima_update_policy_flag(void);
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index d8e321cc6936..c91c2c402498 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -172,6 +172,7 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
  * @pcr: pointer filled in if matched measure policy sets pcr=
  * @template_desc: pointer filled in if matched measure policy sets template=
  * @func_data: func specific data, may be NULL
+ * @allowed_hashes: whitelist of hash algorithms allowed for the IMA xattr
  *
  * The policy is defined in terms of keypairs:
  *		subj=, obj=, type=, func=, mask=, fsmagic=
@@ -188,14 +189,15 @@ int ima_get_action(struct user_namespace *mnt_userns, struct inode *inode,
 		   const struct cred *cred, u32 secid, int mask,
 		   enum ima_hooks func, int *pcr,
 		   struct ima_template_desc **template_desc,
-		   const char *func_data)
+		   const char *func_data, unsigned int *allowed_hashes)
 {
 	int flags = IMA_MEASURE | IMA_AUDIT | IMA_APPRAISE | IMA_HASH;
 
 	flags &= ima_policy_flag;
 
 	return ima_match_policy(mnt_userns, inode, cred, secid, func, mask,
-				flags, pcr, template_desc, func_data);
+				flags, pcr, template_desc, func_data,
+				allowed_hashes);
 }
 
 /*
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index a5e0d3400bd1..12d406b5ab35 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -77,8 +77,9 @@ int ima_must_appraise(struct user_namespace *mnt_userns, struct inode *inode,
 		return 0;
 
 	security_task_getsecid_subj(current, &secid);
-	return ima_match_policy(mnt_userns, inode, current_cred(), secid, func,
-				mask, IMA_APPRAISE | IMA_HASH, NULL, NULL, NULL);
+	return ima_match_policy(mnt_userns, inode, current_cred(), secid,
+				func, mask, IMA_APPRAISE | IMA_HASH, NULL,
+				NULL, NULL, NULL);
 }
 
 static int ima_fix_xattr(struct dentry *dentry,
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 7f2310f29789..85b079c1a19b 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -210,6 +210,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
 	int xattr_len = 0;
 	bool violation_check;
 	enum hash_algo hash_algo;
+	unsigned int allowed_hashes = 0;
 
 	if (!ima_policy_flag || !S_ISREG(inode->i_mode))
 		return 0;
@@ -219,7 +220,8 @@ static int process_measurement(struct file *file, const struct cred *cred,
 	 * Included is the appraise submask.
 	 */
 	action = ima_get_action(file_mnt_user_ns(file), inode, cred, secid,
-				mask, func, &pcr, &template_desc, NULL);
+				mask, func, &pcr, &template_desc, NULL,
+				&allowed_hashes);
 	violation_check = ((func == FILE_CHECK || func == MMAP_CHECK) &&
 			   (ima_policy_flag & IMA_MEASURE));
 	if (!action && !violation_check)
@@ -356,6 +358,15 @@ static int process_measurement(struct file *file, const struct cred *cred,
 
 	if ((file->f_flags & O_DIRECT) && (iint->flags & IMA_PERMIT_DIRECTIO))
 		rc = 0;
+
+	/* Ensure the digest was generated using an allowed algorithm */
+	if (rc == 0 && must_appraise && allowed_hashes != 0 &&
+	    (allowed_hashes & (1U << hash_algo)) == 0) {
+		rc = -EACCES;
+
+		integrity_audit_msg(AUDIT_INTEGRITY_DATA, file_inode(file),
+			pathname, "collect_data", "forbidden-hash-algorithm", rc, 0);
+	}
 out_locked:
 	if ((mask & MAY_WRITE) && test_bit(IMA_DIGSIG, &iint->atomic_flags) &&
 	     !(iint->flags & IMA_NEW_FILE))
@@ -433,7 +444,7 @@ int ima_file_mprotect(struct vm_area_struct *vma, unsigned long prot)
 	inode = file_inode(vma->vm_file);
 	action = ima_get_action(file_mnt_user_ns(vma->vm_file), inode,
 				current_cred(), secid, MAY_EXEC, MMAP_CHECK,
-				&pcr, &template, NULL);
+				&pcr, &template, NULL, NULL);
 
 	/* Is the mmap'ed file in policy? */
 	if (!(action & (IMA_MEASURE | IMA_APPRAISE_SUBMASK)))
@@ -882,7 +893,7 @@ void process_buffer_measurement(struct user_namespace *mnt_userns,
 		security_task_getsecid_subj(current, &secid);
 		action = ima_get_action(mnt_userns, inode, current_cred(),
 					secid, 0, func, &pcr, &template,
-					func_data);
+					func_data, NULL);
 		if (!(action & IMA_MEASURE))
 			return;
 	}
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index fd5d46e511f1..344b5b0dc1a1 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -35,6 +35,7 @@
 #define IMA_FSNAME	0x0200
 #define IMA_KEYRINGS	0x0400
 #define IMA_LABEL	0x0800
+#define IMA_VALIDATE_HASH	0x1000
 
 #define UNKNOWN		0
 #define MEASURE		0x0001	/* same as IMA_MEASURE */
@@ -79,6 +80,7 @@ struct ima_rule_entry {
 	bool (*uid_op)(kuid_t, kuid_t);    /* Handlers for operators       */
 	bool (*fowner_op)(kuid_t, kuid_t); /* uid_eq(), uid_gt(), uid_lt() */
 	int pcr;
+	unsigned int allowed_hashes;
 	struct {
 		void *rule;	/* LSM file metadata specific */
 		char *args_p;	/* audit value */
@@ -90,6 +92,14 @@ struct ima_rule_entry {
 	struct ima_template_desc *template;
 };
 
+/*
+ * sanity check in case the kernels gains more hash algorithms that can
+ * fit in an unsigned int
+ */
+static_assert(
+	8 * sizeof(unsigned int) >= HASH_ALGO__LAST,
+	"The bitfield allowed_hashes in ima_rule_entry is too small to contain all the supported hash algorithms, consider using a bigger type");
+
 /*
  * Without LSM specific knowledge, the default policy can only be
  * written in terms of .action, .func, .mask, .fsmagic, .uid, and .fowner
@@ -646,6 +656,7 @@ static int get_subaction(struct ima_rule_entry *rule, enum ima_hooks func)
  * @pcr: set the pcr to extend
  * @template_desc: the template that should be used for this rule
  * @func_data: func specific data, may be NULL
+ * @allowed_hashes: whitelist of hash algorithms allowed for the IMA xattr
  *
  * Measure decision based on func/mask/fsmagic and LSM(subj/obj/type)
  * conditions.
@@ -658,7 +669,7 @@ int ima_match_policy(struct user_namespace *mnt_userns, struct inode *inode,
 		     const struct cred *cred, u32 secid, enum ima_hooks func,
 		     int mask, int flags, int *pcr,
 		     struct ima_template_desc **template_desc,
-		     const char *func_data)
+		     const char *func_data, unsigned int *allowed_hashes)
 {
 	struct ima_rule_entry *entry;
 	int action = 0, actmask = flags | (flags << 1);
@@ -684,8 +695,11 @@ int ima_match_policy(struct user_namespace *mnt_userns, struct inode *inode,
 			action &= ~IMA_HASH;
 			if (ima_fail_unverifiable_sigs)
 				action |= IMA_FAIL_UNVERIFIABLE_SIGS;
-		}
 
+			if (allowed_hashes &&
+			    entry->flags & IMA_VALIDATE_HASH)
+				*allowed_hashes = entry->allowed_hashes;
+		}
 
 		if (entry->action & IMA_DO_MASK)
 			actmask &= ~(entry->action | entry->action << 1);
-- 
2.31.1

Re: [PATCH v5 3/5] IMA: add support to restrict the hash algorithms used for file appraisal

[Lakshmi Ramasubramanian]

Hi Simon,

On 7/28/2021 6:21 AM, THOBY Simon wrote:
> The kernel accepts any hash algorithm as a value for the security.ima
> xattr. Users may wish to restrict the accepted algorithms to only
> support strong cryptographic ones.
> 
> Provide the plumbing to restrict the permitted set of hash algorithms
> used for verifying file hashes and digest algorithms stored in
> security.ima xattr.
> 
> This do not apply only to IMA in hash mode, it also works with digital
> signatures, in which case it checks that the hash (which was then
> signed by the trusted private key) have been generated with one of
> the algortihms whitelisted for this specific rule.
typo: algortihms => algorithms

Also, suggest using "allowed list" (for digest algorithms allowed)

> 
> Signed-off-by: Simon Thoby <simon.thoby@viveris.fr>
> Reviewed-by:  Mimi Zohar <zohar@linux.ibm.com>
> ---
>   security/integrity/ima/ima.h          |  6 +++---
>   security/integrity/ima/ima_api.c      |  6 ++++--
>   security/integrity/ima/ima_appraise.c |  5 +++--
>   security/integrity/ima/ima_main.c     | 17 ++++++++++++++---
>   security/integrity/ima/ima_policy.c   | 18 ++++++++++++++++--
>   5 files changed, 40 insertions(+), 12 deletions(-)
> 
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index f0e448ed1f9f..7ef1b214d358 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -47,7 +47,7 @@ enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8, TPM_PCR10 = 10 };
>   extern int ima_policy_flag;
>   
>   /* set during initialization */
> -extern int ima_hash_algo;
> +extern int ima_hash_algo __ro_after_init;
>   extern int ima_sha1_idx __ro_after_init;
>   extern int ima_hash_algo_idx __ro_after_init;
>   extern int ima_extra_slots __ro_after_init;
> @@ -254,7 +254,7 @@ int ima_get_action(struct user_namespace *mnt_userns, struct inode *inode,
>   		   const struct cred *cred, u32 secid, int mask,
>   		   enum ima_hooks func, int *pcr,
>   		   struct ima_template_desc **template_desc,
> -		   const char *func_data);
> +		   const char *func_data, unsigned int *allowed_hashes);
>   int ima_must_measure(struct inode *inode, int mask, enum ima_hooks func);
>   int ima_collect_measurement(struct integrity_iint_cache *iint,
>   			    struct file *file, void *buf, loff_t size,
> @@ -285,7 +285,7 @@ int ima_match_policy(struct user_namespace *mnt_userns, struct inode *inode,
>   		     const struct cred *cred, u32 secid, enum ima_hooks func,
>   		     int mask, int flags, int *pcr,
>   		     struct ima_template_desc **template_desc,
> -		     const char *func_data);
> +		     const char *func_data, unsigned int *allowed_hashes);
>   void ima_init_policy(void);
>   void ima_update_policy(void);
>   void ima_update_policy_flag(void);
> diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
> index d8e321cc6936..c91c2c402498 100644
> --- a/security/integrity/ima/ima_api.c
> +++ b/security/integrity/ima/ima_api.c
> @@ -172,6 +172,7 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
>    * @pcr: pointer filled in if matched measure policy sets pcr=
>    * @template_desc: pointer filled in if matched measure policy sets template=
>    * @func_data: func specific data, may be NULL
> + * @allowed_hashes: whitelist of hash algorithms allowed for the IMA xattr
"@allowed_hashes: allowed list of hash algorithms for the IMA xattr"

>    *
>    * The policy is defined in terms of keypairs:
>    *		subj=, obj=, type=, func=, mask=, fsmagic=
> @@ -188,14 +189,15 @@ int ima_get_action(struct user_namespace *mnt_userns, struct inode *inode,
>   		   const struct cred *cred, u32 secid, int mask,
>   		   enum ima_hooks func, int *pcr,
>   		   struct ima_template_desc **template_desc,
> -		   const char *func_data)
> +		   const char *func_data, unsigned int *allowed_hashes)
>   {
>   	int flags = IMA_MEASURE | IMA_AUDIT | IMA_APPRAISE | IMA_HASH;
>   
>   	flags &= ima_policy_flag;
>   
>   	return ima_match_policy(mnt_userns, inode, cred, secid, func, mask,
> -				flags, pcr, template_desc, func_data);
> +				flags, pcr, template_desc, func_data,
> +				allowed_hashes);
>   }
>   
>   /*
> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> index a5e0d3400bd1..12d406b5ab35 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -77,8 +77,9 @@ int ima_must_appraise(struct user_namespace *mnt_userns, struct inode *inode,
>   		return 0;
>   
>   	security_task_getsecid_subj(current, &secid);
> -	return ima_match_policy(mnt_userns, inode, current_cred(), secid, func,
> -				mask, IMA_APPRAISE | IMA_HASH, NULL, NULL, NULL);
> +	return ima_match_policy(mnt_userns, inode, current_cred(), secid,
> +				func, mask, IMA_APPRAISE | IMA_HASH, NULL,
> +				NULL, NULL, NULL);
>   }
>   
>   static int ima_fix_xattr(struct dentry *dentry,
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 7f2310f29789..85b079c1a19b 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -210,6 +210,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
>   	int xattr_len = 0;
>   	bool violation_check;
>   	enum hash_algo hash_algo;
> +	unsigned int allowed_hashes = 0;
>   
>   	if (!ima_policy_flag || !S_ISREG(inode->i_mode))
>   		return 0;
> @@ -219,7 +220,8 @@ static int process_measurement(struct file *file, const struct cred *cred,
>   	 * Included is the appraise submask.
>   	 */
>   	action = ima_get_action(file_mnt_user_ns(file), inode, cred, secid,
> -				mask, func, &pcr, &template_desc, NULL);
> +				mask, func, &pcr, &template_desc, NULL,
> +				&allowed_hashes);
>   	violation_check = ((func == FILE_CHECK || func == MMAP_CHECK) &&
>   			   (ima_policy_flag & IMA_MEASURE));
>   	if (!action && !violation_check)
> @@ -356,6 +358,15 @@ static int process_measurement(struct file *file, const struct cred *cred,
>   
>   	if ((file->f_flags & O_DIRECT) && (iint->flags & IMA_PERMIT_DIRECTIO))
>   		rc = 0;
> +
> +	/* Ensure the digest was generated using an allowed algorithm */
> +	if (rc == 0 && must_appraise && allowed_hashes != 0 &&
> +	    (allowed_hashes & (1U << hash_algo)) == 0) {
> +		rc = -EACCES;
> +
> +		integrity_audit_msg(AUDIT_INTEGRITY_DATA, file_inode(file),
> +			pathname, "collect_data", "forbidden-hash-algorithm", rc, 0);
> +	}
>   out_locked:
>   	if ((mask & MAY_WRITE) && test_bit(IMA_DIGSIG, &iint->atomic_flags) &&
>   	     !(iint->flags & IMA_NEW_FILE))
> @@ -433,7 +444,7 @@ int ima_file_mprotect(struct vm_area_struct *vma, unsigned long prot)
>   	inode = file_inode(vma->vm_file);
>   	action = ima_get_action(file_mnt_user_ns(vma->vm_file), inode,
>   				current_cred(), secid, MAY_EXEC, MMAP_CHECK,
> -				&pcr, &template, NULL);
> +				&pcr, &template, NULL, NULL);
>   
>   	/* Is the mmap'ed file in policy? */
>   	if (!(action & (IMA_MEASURE | IMA_APPRAISE_SUBMASK)))
> @@ -882,7 +893,7 @@ void process_buffer_measurement(struct user_namespace *mnt_userns,
>   		security_task_getsecid_subj(current, &secid);
>   		action = ima_get_action(mnt_userns, inode, current_cred(),
>   					secid, 0, func, &pcr, &template,
> -					func_data);
> +					func_data, NULL);
>   		if (!(action & IMA_MEASURE))
>   			return;
>   	}
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index fd5d46e511f1..344b5b0dc1a1 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -35,6 +35,7 @@
>   #define IMA_FSNAME	0x0200
>   #define IMA_KEYRINGS	0x0400
>   #define IMA_LABEL	0x0800
> +#define IMA_VALIDATE_HASH	0x1000
>   
>   #define UNKNOWN		0
>   #define MEASURE		0x0001	/* same as IMA_MEASURE */
> @@ -79,6 +80,7 @@ struct ima_rule_entry {
>   	bool (*uid_op)(kuid_t, kuid_t);    /* Handlers for operators       */
>   	bool (*fowner_op)(kuid_t, kuid_t); /* uid_eq(), uid_gt(), uid_lt() */
>   	int pcr;
> +	unsigned int allowed_hashes;
>   	struct {
>   		void *rule;	/* LSM file metadata specific */
>   		char *args_p;	/* audit value */
> @@ -90,6 +92,14 @@ struct ima_rule_entry {
>   	struct ima_template_desc *template;
>   };
>   
> +/*
> + * sanity check in case the kernels gains more hash algorithms that can
> + * fit in an unsigned int
> + */
> +static_assert(
> +	8 * sizeof(unsigned int) >= HASH_ALGO__LAST,
> +	"The bitfield allowed_hashes in ima_rule_entry is too small to contain all the supported hash algorithms, consider using a bigger type");
> +
>   /*
>    * Without LSM specific knowledge, the default policy can only be
>    * written in terms of .action, .func, .mask, .fsmagic, .uid, and .fowner
> @@ -646,6 +656,7 @@ static int get_subaction(struct ima_rule_entry *rule, enum ima_hooks func)
>    * @pcr: set the pcr to extend
>    * @template_desc: the template that should be used for this rule
>    * @func_data: func specific data, may be NULL
> + * @allowed_hashes: whitelist of hash algorithms allowed for the IMA xattr
"@allowed_hashes: allowed list of hash algorithms for the IMA xattr"

  -lakshmi

>    *
>    * Measure decision based on func/mask/fsmagic and LSM(subj/obj/type)
>    * conditions.
> @@ -658,7 +669,7 @@ int ima_match_policy(struct user_namespace *mnt_userns, struct inode *inode,
>   		     const struct cred *cred, u32 secid, enum ima_hooks func,
>   		     int mask, int flags, int *pcr,
>   		     struct ima_template_desc **template_desc,
> -		     const char *func_data)
> +		     const char *func_data, unsigned int *allowed_hashes)
>   {
>   	struct ima_rule_entry *entry;
>   	int action = 0, actmask = flags | (flags << 1);
> @@ -684,8 +695,11 @@ int ima_match_policy(struct user_namespace *mnt_userns, struct inode *inode,
>   			action &= ~IMA_HASH;
>   			if (ima_fail_unverifiable_sigs)
>   				action |= IMA_FAIL_UNVERIFIABLE_SIGS;
> -		}
>   
> +			if (allowed_hashes &&
> +			    entry->flags & IMA_VALIDATE_HASH)
> +				*allowed_hashes = entry->allowed_hashes;
> +		}
>   
>   		if (entry->action & IMA_DO_MASK)
>   			actmask &= ~(entry->action | entry->action << 1);
> 

[PATCH v5 4/5] IMA: add a policy option to restrict xattr hash algorithms on appraisal

[THOBY Simon]

The kernel has the ability to restrict the set of hash algorithms
it accepts for the security.ima xattr when it appraises files.

Define a new IMA policy rule option "appraise_hash=",
using the mentioned mechanism to expose a user-toggable policy
knob to opt-in to that restriction and select the desired set of
algorithms that must be accepted.

When a policy rule uses the 'appraise_hash' option, appraisal of a
file referenced by that rule will now fail if the digest algorithm
employed to hash the file was not one of those explicitly listed
in the option. In its absence, any hash algorithm compiled in the
kernel will be accepted.

For example, on a system where SELinux is properly deployed, the rule
  appraise func=BPRM_CHECK obj_type=iptables_exec_t appraise_hash=sha256,sha384
will block the execution of iptables if the xattr security.ima of its
executables were not hashed with either sha256 or sha384.

Signed-off-by: Simon Thoby <simon.thoby@viveris.fr>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
---
 Documentation/ABI/testing/ima_policy |  6 ++-
 security/integrity/ima/ima_policy.c  | 74 ++++++++++++++++++++++++++--
 2 files changed, 75 insertions(+), 5 deletions(-)

diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index 070779e8d836..aeb622698047 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -27,7 +27,7 @@ Description:
 			lsm:	[[subj_user=] [subj_role=] [subj_type=]
 				 [obj_user=] [obj_role=] [obj_type=]]
 			option:	[[appraise_type=]] [template=] [permit_directio]
-				[appraise_flag=] [keyrings=]
+				[appraise_flag=] [appraise_hash=] [keyrings=]
 		  base:
 			func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK]
 			        [FIRMWARE_CHECK]
@@ -55,6 +55,10 @@ Description:
 			label:= [selinux]|[kernel_info]|[data_label]
 			data_label:= a unique string used for grouping and limiting critical data.
 			For example, "selinux" to measure critical data for SELinux.
+			appraise_hash:= comma-separated list of hash algorithms
+			For example, "sha256,sha512" to only accept to appraise
+			files where the security.ima xattr was hashed with one
+			of these two algorithms.
 
 		  default policy:
 			# PROC_SUPER_MAGIC
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 344b5b0dc1a1..f944c3e7f340 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -960,7 +960,7 @@ enum {
 	Opt_fsuuid, Opt_uid_eq, Opt_euid_eq, Opt_fowner_eq,
 	Opt_uid_gt, Opt_euid_gt, Opt_fowner_gt,
 	Opt_uid_lt, Opt_euid_lt, Opt_fowner_lt,
-	Opt_appraise_type, Opt_appraise_flag,
+	Opt_appraise_type, Opt_appraise_flag, Opt_appraise_hash,
 	Opt_permit_directio, Opt_pcr, Opt_template, Opt_keyrings,
 	Opt_label, Opt_err
 };
@@ -995,6 +995,7 @@ static const match_table_t policy_tokens = {
 	{Opt_fowner_lt, "fowner<%s"},
 	{Opt_appraise_type, "appraise_type=%s"},
 	{Opt_appraise_flag, "appraise_flag=%s"},
+	{Opt_appraise_hash, "appraise_hash=%s"},
 	{Opt_permit_directio, "permit_directio"},
 	{Opt_pcr, "pcr=%s"},
 	{Opt_template, "template=%s"},
@@ -1095,7 +1096,8 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
 		return false;
 
 	if (entry->action != APPRAISE &&
-	    entry->flags & (IMA_DIGSIG_REQUIRED | IMA_MODSIG_ALLOWED | IMA_CHECK_BLACKLIST))
+	    entry->flags & (IMA_DIGSIG_REQUIRED | IMA_MODSIG_ALLOWED |
+			    IMA_CHECK_BLACKLIST | IMA_VALIDATE_HASH))
 		return false;
 
 	/*
@@ -1125,7 +1127,7 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
 				     IMA_UID | IMA_FOWNER | IMA_FSUUID |
 				     IMA_INMASK | IMA_EUID | IMA_PCR |
 				     IMA_FSNAME | IMA_DIGSIG_REQUIRED |
-				     IMA_PERMIT_DIRECTIO))
+				     IMA_PERMIT_DIRECTIO | IMA_VALIDATE_HASH))
 			return false;
 
 		break;
@@ -1137,7 +1139,7 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
 				     IMA_INMASK | IMA_EUID | IMA_PCR |
 				     IMA_FSNAME | IMA_DIGSIG_REQUIRED |
 				     IMA_PERMIT_DIRECTIO | IMA_MODSIG_ALLOWED |
-				     IMA_CHECK_BLACKLIST))
+				     IMA_CHECK_BLACKLIST | IMA_VALIDATE_HASH))
 			return false;
 
 		break;
@@ -1187,6 +1189,28 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
 	return true;
 }
 
+static unsigned int ima_parse_appraise_hash(char *arg)
+{
+	unsigned int res = 0;
+	int idx;
+	char *token;
+
+	while ((token = strsep(&arg, ",")) != NULL) {
+		idx = match_string(hash_algo_name, HASH_ALGO__LAST, token);
+
+		if (idx < 0) {
+			pr_err("unknown hash algorithm \"%s\", ignoring",
+			       token);
+			continue;
+		}
+
+		/* Add the hash algorithm to the 'allowed' bitfield */
+		res |= (1U << idx);
+	}
+
+	return res;
+}
+
 static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 {
 	struct audit_buffer *ab;
@@ -1522,6 +1546,25 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 			else
 				result = -EINVAL;
 			break;
+		case Opt_appraise_hash:
+			ima_log_string(ab, "appraise_hash", args[0].from);
+
+			if (entry->allowed_hashes) {
+				result = -EINVAL;
+				break;
+			}
+
+			entry->allowed_hashes = ima_parse_appraise_hash(args[0].from);
+
+			/* invalid or empty list of algorithms */
+			if (!entry->allowed_hashes) {
+				result = -EINVAL;
+				break;
+			}
+
+			entry->flags |= IMA_VALIDATE_HASH;
+
+			break;
 		case Opt_permit_directio:
 			entry->flags |= IMA_PERMIT_DIRECTIO;
 			break;
@@ -1714,6 +1757,23 @@ static void ima_show_rule_opt_list(struct seq_file *m,
 		seq_printf(m, "%s%s", i ? "|" : "", opt_list->items[i]);
 }
 
+static void ima_policy_show_appraise_hash(struct seq_file *m,
+					  unsigned int allowed_hashes)
+{
+	int idx, list_size = 0;
+
+	for (idx = 0; idx < HASH_ALGO__LAST; idx++) {
+		if (!(allowed_hashes & (1U << idx)))
+			continue;
+
+		/* only add commas if the list contains multiple entries */
+		if (list_size++)
+			seq_puts(m, ",");
+
+		seq_puts(m, hash_algo_name[idx]);
+	}
+}
+
 int ima_policy_show(struct seq_file *m, void *v)
 {
 	struct ima_rule_entry *entry = v;
@@ -1825,6 +1885,12 @@ int ima_policy_show(struct seq_file *m, void *v)
 		seq_puts(m, " ");
 	}
 
+	if (entry->flags & IMA_VALIDATE_HASH) {
+		seq_puts(m, "appraise_hash=");
+		ima_policy_show_appraise_hash(m, entry->allowed_hashes);
+		seq_puts(m, " ");
+	}
+
 	for (i = 0; i < MAX_LSM_RULES; i++) {
 		if (entry->lsm[i].rule) {
 			switch (i) {
-- 
2.31.1

[PATCH v5 5/5] IMA: introduce a new policy option func=SETXATTR_CHECK

[THOBY Simon]

While users can restrict the accepted hash algorithms for the
security.ima xattr file signature when appraising said file, users
cannot restrict the algorithms that can be set on that attribute:
any algorithm built in the kernel is accepted on a write.

Define a new value for the ima policy option 'func' that restricts
globally the hash algorithms accepted when writing the security.ima
xattr.

When a policy contains a rule of the form
	appraise func=SETXATTR_CHECK appraise_hash=sha256,sha384,sha512
only values corresponding to one of these three digest algorithms
will be accepted for writing the security.ima xattr.
Attempting to write the attribute using another algorithm (or "free-form"
data) will be denied with an audit log message.
In the absence of such a policy rule, the default is still to only
accept hash algorithms built in the kernel (with all the limitations
that entails).

Signed-off-by: Simon Thoby <simon.thoby@viveris.fr>
---
 Documentation/ABI/testing/ima_policy  |  9 ++++-
 security/integrity/ima/ima.h          |  4 ++
 security/integrity/ima/ima_appraise.c | 31 ++++++++++++---
 security/integrity/ima/ima_policy.c   | 57 +++++++++++++++++++++++++++
 4 files changed, 94 insertions(+), 7 deletions(-)

diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index aeb622698047..537be0e1720e 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -30,9 +30,10 @@ Description:
 				[appraise_flag=] [appraise_hash=] [keyrings=]
 		  base:
 			func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK]
-			        [FIRMWARE_CHECK]
+				[FIRMWARE_CHECK]
 				[KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK]
 				[KEXEC_CMDLINE] [KEY_CHECK] [CRITICAL_DATA]
+				[SETXATTR_CHECK]
 			mask:= [[^]MAY_READ] [[^]MAY_WRITE] [[^]MAY_APPEND]
 			       [[^]MAY_EXEC]
 			fsmagic:= hex value
@@ -138,3 +139,9 @@ Description:
 		keys added to .builtin_trusted_keys or .ima keyring:
 
 			measure func=KEY_CHECK keyrings=.builtin_trusted_keys|.ima
+
+		Example of the special SETXATTR_CHECK appraise rule, that
+		restricts the hash algorithms allowed when writing to the
+		security.ima xattr of a file:
+
+			appraise func=SETXATTR_CHECK appraise_hash=sha256,sha384,sha512
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 7ef1b214d358..aeb3bf30c0f9 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -46,6 +46,9 @@ enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8, TPM_PCR10 = 10 };
 /* current content of the policy */
 extern int ima_policy_flag;
 
+/* bitset of digests algorithms allowed in the setxattr hook */
+extern atomic_t ima_setxattr_allowed_hash_algorithms;
+
 /* set during initialization */
 extern int ima_hash_algo __ro_after_init;
 extern int ima_sha1_idx __ro_after_init;
@@ -198,6 +201,7 @@ static inline unsigned int ima_hash_key(u8 *digest)
 	hook(KEXEC_CMDLINE, kexec_cmdline)		\
 	hook(KEY_CHECK, key)				\
 	hook(CRITICAL_DATA, critical_data)		\
+	hook(SETXATTR_CHECK, setxattr_check)		\
 	hook(MAX_CHECK, none)
 
 #define __ima_hook_enumify(ENUM, str)	ENUM,
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 12d406b5ab35..c4a43c84da5f 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -596,13 +596,33 @@ static int validate_hash_algo(struct dentry *dentry,
 {
 	char *path = NULL, *pathbuf = NULL;
 	enum hash_algo xattr_hash_algo;
+	const char *errmsg = "unavailable-hash-algorithm";
+	unsigned int allowed_hashes;
 
 	xattr_hash_algo = ima_get_hash_algo((struct evm_ima_xattr_data *)xattr_value,
 					    xattr_value_len);
 
-	if (likely(xattr_hash_algo == ima_hash_algo ||
-		   crypto_has_alg(hash_algo_name[xattr_hash_algo], 0, 0)))
-		return 0;
+	allowed_hashes = atomic_read(&ima_setxattr_allowed_hash_algorithms);
+
+	if (allowed_hashes) {
+		/* success if the algorithm is whitelisted in the ima policy */
+		if (allowed_hashes & (1U << xattr_hash_algo))
+			return 0;
+
+		/*
+		 * We use a different audit message when the hash algorithm
+		 * is denied by a policy rule, instead of not being built
+		 * in the kernel image
+		 */
+		errmsg = "denied-hash-algorithm";
+	} else {
+		if (likely(xattr_hash_algo == ima_hash_algo))
+			return 0;
+
+		/* allow any xattr using an algorithm built in the kernel */
+		if (crypto_has_alg(hash_algo_name[xattr_hash_algo], 0, 0))
+			return 0;
+	}
 
 	pathbuf = kmalloc(PATH_MAX, GFP_KERNEL);
 	if (pathbuf) {
@@ -610,11 +630,10 @@ static int validate_hash_algo(struct dentry *dentry,
 
 		/*
 		 * disallow xattr writes with algorithms disabled in the
-		 * kernel configuration
+		 * kernel configuration or denied by policy
 		 */
 		integrity_audit_msg(AUDIT_INTEGRITY_DATA, d_inode(dentry),
-				    path, "collect_data",
-				    "unavailable-hash-algorithm",
+				    path, "collect_data", errmsg,
 				    -EACCES, 0);
 
 		kfree(pathbuf);
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index f944c3e7f340..4e2410b826e2 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -53,6 +53,8 @@ int ima_policy_flag;
 static int temp_ima_appraise;
 static int build_ima_appraise __ro_after_init;
 
+atomic_t ima_setxattr_allowed_hash_algorithms;
+
 #define MAX_LSM_RULES 6
 enum lsm_rule_types { LSM_OBJ_USER, LSM_OBJ_ROLE, LSM_OBJ_TYPE,
 	LSM_SUBJ_USER, LSM_SUBJ_ROLE, LSM_SUBJ_TYPE
@@ -903,6 +905,8 @@ void __init ima_init_policy(void)
 			  ARRAY_SIZE(critical_data_rules),
 			  IMA_DEFAULT_POLICY);
 
+	atomic_xchg(&ima_setxattr_allowed_hash_algorithms, 0);
+
 	ima_update_policy_flag();
 }
 
@@ -914,6 +918,42 @@ int ima_check_policy(void)
 	return 0;
 }
 
+/** update_allowed_hash_algorithms - update the hash algorithms allowed
+ * for setxattr writes
+ *
+ * Update the atomic variable holding the set of allowed hash algorithms
+ * that can be used to update the security.ima xattr of a file.
+ *
+ * Context: called when updating the IMA policy.
+ *
+ * SETXATTR_CHECK rules do not implement a full policy check because of
+ * the performance impact performing rules checking on setxattr() would
+ * have. The consequence is that only one SETXATTR_CHECK can be active at
+ * a time.
+ */
+static void update_allowed_hash_algorithms(void)
+{
+	struct ima_rule_entry *entry;
+
+	/*
+	 * We scan in reverse order because only the last entry with the
+	 * 'func=SETXATTR_CHECK' apply: this allows runtime upgrades of the
+	 * digest algorithm policy, unlike the other IMA rules that are
+	 * usually append-only. Old rules will still be present in the
+	 * ruleset, but inactive.
+	 */
+	rcu_read_lock();
+	list_for_each_entry_reverse(entry, ima_rules, list) {
+		if (entry->func != SETXATTR_CHECK)
+			continue;
+
+		atomic_xchg(&ima_setxattr_allowed_hash_algorithms,
+			    entry->allowed_hashes);
+		break;
+	}
+	rcu_read_unlock();
+}
+
 /**
  * ima_update_policy - update default_rules with new measure rules
  *
@@ -931,6 +971,7 @@ void ima_update_policy(void)
 
 	list_splice_tail_init_rcu(&ima_temp_rules, policy, synchronize_rcu);
 
+
 	if (ima_rules != policy) {
 		ima_policy_flag = 0;
 		ima_rules = policy;
@@ -944,6 +985,7 @@ void ima_update_policy(void)
 		kfree(arch_policy_entry);
 	}
 	ima_update_policy_flag();
+	update_allowed_hash_algorithms();
 
 	/* Custom IMA policy has been loaded */
 	ima_process_queued_keys();
@@ -1176,6 +1218,19 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
 		if (ima_rule_contains_lsm_cond(entry))
 			return false;
 
+		break;
+	case SETXATTR_CHECK:
+		/* any action other than APPRAISE is unsupported */
+		if (entry->action != APPRAISE)
+			return false;
+
+		/*
+		 * full policies are not supported, they would have too
+		 * much of a performance impact
+		 */
+		if (entry->flags & ~(IMA_FUNC | IMA_VALIDATE_HASH))
+			return false;
+
 		break;
 	default:
 		return false;
@@ -1332,6 +1387,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 				entry->func = KEY_CHECK;
 			else if (strcmp(args[0].from, "CRITICAL_DATA") == 0)
 				entry->func = CRITICAL_DATA;
+			else if (strcmp(args[0].from, "SETXATTR_CHECK") == 0)
+				entry->func = SETXATTR_CHECK;
 			else
 				result = -EINVAL;
 			if (!result)
-- 
2.31.1

Re: [PATCH v5 5/5] IMA: introduce a new policy option func=SETXATTR_CHECK

[Mimi Zohar]

Hi Simon,

On Wed, 2021-07-28 at 13:21 +0000, THOBY Simon wrote:
 
> @@ -914,6 +918,42 @@ int ima_check_policy(void)
>  	return 0;
>  }
>  
> +/** update_allowed_hash_algorithms - update the hash algorithms allowed

The first line of kernel-doc is just "/**" by itself, followed by the
function name and a brief description.  The brief description should
not wrap to the next line.  Refer to Documentation/doc-guide/kernel-
doc.rst.

> + * for setxattr writes
> + *
> + * Update the atomic variable holding the set of allowed hash algorithms
> + * that can be used to update the security.ima xattr of a file.
> + *
> + * Context: called when updating the IMA policy.
> + *
> + * SETXATTR_CHECK rules do not implement a full policy check because of
> + * the performance impact performing rules checking on setxattr() would
> + * have. The consequence is that only one SETXATTR_CHECK can be active at
> + * a time.
> + */
> +static void update_allowed_hash_algorithms(void)
> +{
> +	struct ima_rule_entry *entry;
> +
> +	/*
> +	 * We scan in reverse order because only the last entry with the
> +	 * 'func=SETXATTR_CHECK' apply: this allows runtime upgrades of the
> +	 * digest algorithm policy, unlike the other IMA rules that are
> +	 * usually append-only. Old rules will still be present in the
> +	 * ruleset, but inactive.
> +	 */

Oh, my!  I really hope this won't be used as precedent.  Before
agreeing to this, the existing policy rules must require loading of
only signed IMA policies.

thanks,

Mimi
  
> +	rcu_read_lock();
> +	list_for_each_entry_reverse(entry, ima_rules, list) {
> +		if (entry->func != SETXATTR_CHECK)
> +			continue;
> +
> +		atomic_xchg(&ima_setxattr_allowed_hash_algorithms,
> +			    entry->allowed_hashes);
> +		break;
> +	}
> +	rcu_read_unlock();
> +}
> +

Re: [PATCH v5 5/5] IMA: introduce a new policy option func=SETXATTR_CHECK

[THOBY Simon]

Hi Mimi,

On 7/29/21 12:57 AM, Mimi Zohar wrote:
> Hi Simon,
> 
> On Wed, 2021-07-28 at 13:21 +0000, THOBY Simon wrote:
>  
>> @@ -914,6 +918,42 @@ int ima_check_policy(void)
>>  	return 0;
>>  }
>>  
>> +/** update_allowed_hash_algorithms - update the hash algorithms allowed
> 
> The first line of kernel-doc is just "/**" by itself, followed by the
> function name and a brief description.  The brief description should
> not wrap to the next line.  Refer to Documentation/doc-guide/kernel-
> doc.rst.
> 

Thanks, I will fix that in the next revision.

>> + * for setxattr writes
>> + *
>> + * Update the atomic variable holding the set of allowed hash algorithms
>> + * that can be used to update the security.ima xattr of a file.
>> + *
>> + * Context: called when updating the IMA policy.
>> + *
>> + * SETXATTR_CHECK rules do not implement a full policy check because of
>> + * the performance impact performing rules checking on setxattr() would
>> + * have. The consequence is that only one SETXATTR_CHECK can be active at
>> + * a time.
>> + */
>> +static void update_allowed_hash_algorithms(void)
>> +{
>> +	struct ima_rule_entry *entry;
>> +
>> +	/*
>> +	 * We scan in reverse order because only the last entry with the
>> +	 * 'func=SETXATTR_CHECK' apply: this allows runtime upgrades of the
>> +	 * digest algorithm policy, unlike the other IMA rules that are
>> +	 * usually append-only. Old rules will still be present in the
>> +	 * ruleset, but inactive.
>> +	 */
> 
> Oh, my!  I really hope this won't be used as precedent.  Before
> agreeing to this, the existing policy rules must require loading of
> only signed IMA policies.
> 


After some though, I think you're right, there is not much point to do anything
different from the other IMA rules, 

Below is the modified version that I will submit in the next patch.

However given the similarities between this function and ima_update_policy_flag,
I think maybe I should merge them together: they are mostly called from the
same places and they both serve the same role: updating some of the global ima
variables after a policy update or at system initialization.

Do you think it would be ok to add that functionality to ima_update_policy_flag?
Maybe updating the name to reflect that the function updates multiples flags?

 
+/**
+ * update_allowed_hash_algorithms() - update the hash allowlist for setxattr
+ *
+ * Update the atomic variable holding the set of allowed hash algorithms
+ * that can be used to update the security.ima xattr of a file.
+ *
+ * SETXATTR_CHECK rules do not implement a full policy check because
+ * of the performance impact performing rules checking on setxattr()
+ * would have. The consequence is that only one SETXATTR_CHECK can be
+ * active at a given time.
+ *
+ * Context: Called when updating the IMA policy.
+ */
+static void update_allowed_hash_algorithms(void)
+{
+	struct ima_rule_entry *entry;
+
+	rcu_read_lock();
+	list_for_each_entry(entry, ima_rules, list) {
+		if (entry->func != SETXATTR_CHECK)
+			continue;
+
+		/*
+		 * Two possibilities:
+		 * - the atomic was non-zero: a setxattr hash policy is
+		 *   already enforced -> do nothing
+		 * - the atomic was zero -> enable the setxattr hash policy
+		 *
+		 * While we could check if the atomic is non-zero at the
+		 * beginning of the function, doing it here prevent TOCTOU
+		 * races (not that I think there would be much interest for
+		 * an attacker to perform a TOCTOU race here)
+		 */
+		atomic_cmpxchg(&ima_setxattr_allowed_hash_algorithms, 0,
+			       entry->allowed_hashes);
+		break;
+	}
+	rcu_read_unlock();
+}
+

As a side note on the patchset, maybe I should refrain from posting for a few days to give people time
to comment on it, instead of sending new versions in such a quick succession?

Thanks,
Simon

Re: [PATCH v5 5/5] IMA: introduce a new policy option func=SETXATTR_CHECK

[Mimi Zohar]

Hi Simon,

On Thu, 2021-07-29 at 07:47 +0000, THOBY Simon wrote:
> 
> >> + * for setxattr writes
> >> + *
> >> + * Update the atomic variable holding the set of allowed hash algorithms
> >> + * that can be used to update the security.ima xattr of a file.
> >> + *
> >> + * Context: called when updating the IMA policy.
> >> + *
> >> + * SETXATTR_CHECK rules do not implement a full policy check because of
> >> + * the performance impact performing rules checking on setxattr() would
> >> + * have. The consequence is that only one SETXATTR_CHECK can be active at
> >> + * a time.
> >> + */
> >> +static void update_allowed_hash_algorithms(void)
> >> +{
> >> +	struct ima_rule_entry *entry;
> >> +
> >> +	/*
> >> +	 * We scan in reverse order because only the last entry with the
> >> +	 * 'func=SETXATTR_CHECK' apply: this allows runtime upgrades of the
> >> +	 * digest algorithm policy, unlike the other IMA rules that are
> >> +	 * usually append-only. Old rules will still be present in the
> >> +	 * ruleset, but inactive.
> >> +	 */
> > 
> > Oh, my!  I really hope this won't be used as precedent.  Before
> > agreeing to this, the existing policy rules must require loading of
> > only signed IMA policies.
> > 
> 
> 
> After some though, I think you're right, there is not much point to do anything
> different from the other IMA rules, 
> 
> Below is the modified version that I will submit in the next patch.
> 
> However given the similarities between this function and ima_update_policy_flag,
> I think maybe I should merge them together: they are mostly called from the
> same places and they both serve the same role: updating some of the global ima
> variables after a policy update or at system initialization.
> 
> Do you think it would be ok to add that functionality to ima_update_policy_flag?
> Maybe updating the name to reflect that the function updates multiples flags?

We've gone through a couple of iterations of this patch.  At this
point, it's defining a single set of setxattr permitted hash
algorithms.  Renaming and adding the change to
ima_update_policy_flags() definitely makes sense.

At the same time, I'd appreciate your fixing the RCU locking there.

> As a side note on the patchset, maybe I should refrain from posting for a few days to give people time
> to comment on it, instead of sending new versions in such a quick succession?

Definitely.

thanks,

Mimi

Re: [PATCH v5 0/5] IMA: restrict the accepted digest algorithms for the security.ima xattr

[Mimi Zohar]

On Wed, 2021-07-28 at 13:21 +0000, THOBY Simon wrote:
> IMA protects files by storing a hash (or a signature thereof) of their
> content in the security.ima xattr. While the security.ima xattr itself
> is protected by EVM with either a HMAC or a digital signature, no
> mechanism is currently in place to ensure that the security.ima xattr
> was generated with a strong digest algorithm, and any hash defined
> in the kernel will be accepted, even obsolete format like MD4 and MD5.
> 
> The kernel itself will only write this xattr with the 'ima_hash' parameter,
> fixed at init, but it will also happily accept userland writes for said
> xattr, and those writes may use arbitrary hash algorithms as long as the
> kernel have support for it.
> 
> One important point is safeguarding users from mislabelling their
> files when using userland utilities to update their files, as this
> is the kind of behavior one can observe with evmctl (`evmctl ima_hash`
> defaults to sha1). Another group that may be interested is those
> that have deployed IMA years ago, possibly using algorithms that
> was then deemed sufficiently collision-resistant, but that proved
> to be weak with the passage of time (note that this could also
> happen in the future with algorithms considered safe today).
> This patch provides a migration path of sorts for these users.
> 
> This patch series gives users the ability to restrict the algorithms
> accepted by their system, both when writing/updating xattrs, and
> when appraising files, while retaining a permissive behavior by default
> to preserve backward compatibility.
> 
> To provide these features, alter the behavior of setxattr to
> only accept hashes built in the kernel, instead of any hash listed
> in the kernel (complete list crypto/hash_info.c). In addition, the
> user can define in his IMA policy the list of digest algorithms
> allowed for writing to the security.ima xattr. In that case,
> only algorithms present in that list are accepted for writing.
> 
> In addition, users may opt-in to whitelisting the hash
> algorithms accepted when appraising thanks to the new
> "appraise_hash" IMA policy option.
> By default IMA will keep accepting any hash algorithm, but specifying
> that option will make appraisal of files hashed with another algorithm
> fail.
> 
> 
> Even when using this option to restrict accepted hashes, a migration
> to a new algorithm is still possible. Suppose your policy states you
> must migrate from 'old_algo' (e.g. sha1) to 'new_algo' (e.g. one of
> sha256/384/512). You can upgrade without relaxing the hash requirements:
> alter your policy rules from 'appraise_hash=old_algo' to
> 'appraise_hash=old_algo,new_algo', load a new SETXATTR_CHECK policy
> rule that accept writes using 'new_algo', reboot, relabel
> all your files with 'new_algo', alter your policy rules from
> 'appraise_hash=old_algo,new_algo' to 'appraise_hash=new_algo',
> and you're done.
> While this represent a significant amount of work, it is important to
> showcase that this patchset is flexible enough to let users upgrade
> if needed.
> 
> 
> This series is based on the following repo/branch:
>  repo: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
>  branch: master
>  commit ff1176468d368232b684f75e82563369208bc371 ("Linux 5.14-rc3")

A few high level comments:

- I recently accepted a couple of patches, which are now in the next-
integrity-testing branch.  When reposting, please rebase this patch set
on top of it.

- The code uses the term "allowed lists", not "white lists", but the
cover letter, patch descriptions, and/or comments still refer to "white
lists".  For an explanation refer to the new section "Naming" in
Documentation/process/coding-style.rst.

- There was some discussion about allowing code longer than 80 columns,
but the section on  "Breaking long lines and strings" in
Documentation/process/coding-style.rst hasn't been updated.  Please
make sure that the new code has a max line length of 80.

thanks,

Mimi