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

[PATCH v7 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 formats like MD4 and MD5.

The kernel itself will only write to 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 allowlist hash algorithms for
ppraising thanks to the new 'appraise_algos' 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_algos=old_algo' to
'appraise_algos=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_algos=old_algo,new_algo' to 'appraise_algos=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.


Here is also a short description of the new audit messages, but I can
send it in a followup mail if that is not the proper place:

When writing the xattr with an algorithm not built in the kernel (here the
kernel was built with CONFIG_CRYPTO_MD5 unset), e.g. with
"evmctl ima_hash -a md5 /usr/bin/strace":
	audit(1628066120.418:121): pid=1344 uid=0 auid=0 ses=1 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 op=set_data cause=unavailable-hash-algorithm comm="evmctl" name="/usr/bin/strace" dev="dm-0" ino=2632657 res=0 errno=0

With the same command and the policy rule
"appraise func=SETXATTR_CHECK appraise_algos=sha256,sha384,sha512", we get:
	audit(1628066210.141:127): pid=1362 uid=0 auid=0 ses=1 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 op=set_data cause=denied-hash-algorithm comm="evmctl" name="/usr/bin/strace" dev="dm-0" ino=2632657 res=0 errno=0

Note that the cause is now 'denied-hash-algorithm' instead of
'unavailable-hash-algorithm'. We get that audit message for any algorithm
outside of sha256/384/512 (including algorithms not compiled in the kernel
like MD5). In a sense, 'denied-hash-algorithm' takes predecence over
'unavailable-hash-algorithm'.

When appraising files, e.g. trying to execute a file whose xattr was hashed
with sha1 while the policy rule
"appraise func=BPRM_CHECK fowner=0 appraise_algos=sha256" is enabled:
	audit(1628066349.230:130): pid=1369 uid=0 auid=0 ses=1 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 op=collect_data cause=denied-hash-algorithm comm="bash" name="/usr/bin/strace" dev="dm-0" ino=2632657 res=0 errno=0


This series is based on the following repo/branch:
 repo: https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git
 branch: next-integrity-testing
 commit e37be5343ae2b9419aea1442b07e5d2428b437b4 ("Merge branch 'ima-buffer-measurement-changes-v4' into next-integrity")

Changelog since v6:
- removed the IS_ENABLED(CONFIG_CRYPTO_MD5) as IMA fails gracefully and
  already defaults to sha256 in that case (with a more helpful error
  message than "invalid hash algorithm for template")
- Update the commit message of patch 2/5 to fix a typo (reported by Mimi
  Zohar and Lakshmi Ramasubramanian)
- Update the kernel-doc (I decided to keep it as it is acceptable to document
  static fucntions according to 'Documentation/doc-guide/kernel-doc.rst')
  (reported by Mimi Zohar)
- Prevent file re-validation when the hash algorithm is invalid on setxattr()
  (reported by Mimi Zohar)
- Update the operation in the audit log from 'collect_data' to 'set_data' when
  performing setxatttr()s. (reported by Mimi Zohar)
- Rename the user options from 'appraise_hash' to 'appraise_algos' (reported
  by Mimi Zohar)
- Reject the policy update when the 'appraise_algos' parameter cannot be parsed
  (reported by Lakshmi Ramasubramanian)
- Kept the (single) whitespace->tab change in
  'Documentation/ABI/testing/ima_policy' for consistency

Changelog since v5:
- Adapt a few places where lines were longer than 80 characters (suggested by
 Mimi Zohar)
- Rebase on top on next-integrity-testing (suggested by Mimi Zohar)
- Replace "whitelist" with "allowlist" (suggested by both Mimi Zohar and
  Lakshmi Ramasubramanian)
- Fix a broken kernel-doc (suggested by Mimi Zohar)
- Prune the feature that updated "func=SETXATTR_CHECK" (suggested by Mimi
  Zohar)
- Update the commit messages with suggestions from Lakshmi Ramasubramanian
- Short-circuit a bit the evaluation in ima_appraise.c/validate_hash_algo
  to return earlier in case of memory exhaustion (suggested by Lakshmi
  Ramasubramanian)
- Renamed "forbidden-hash-algorithm" in ima_main.c to reduce the number of

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 initialization
  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          |  14 ++-
 security/integrity/ima/ima_api.c      |   6 +-
 security/integrity/ima/ima_appraise.c |  73 ++++++++++-
 security/integrity/ima/ima_main.c     |  20 ++-
 security/integrity/ima/ima_policy.c   | 168 +++++++++++++++++++++++---
 7 files changed, 262 insertions(+), 35 deletions(-)

-- 
2.31.1

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

[THOBY Simon]

MD5 is a weak digest algorithm that shouldn't be used for cryptographic
operation. It hinders the efficiency of a patch set that aims to limit
the digests allowed for the extended file attribute namely security.ima.
MD5 is no longer a requirement for IMA, nor should it be used there.

The sole place where we still use the MD5 algorithm inside IMA (setting
the ima_hash algorithm to MD5 if the user supply the 'ima_hash=md5'
parameter on the command line) fails gracefully when CRYPTO_MD5 is not set:
	ima: Can not allocate md5 (reason: -2)
	ima: Allocating md5 failed, going to use default hash algorithm sha256

Remove the CRYPTO_MD5 dependency for IMA.

Reviewed-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
Signed-off-by: Simon Thoby <simon.thoby@viveris.fr>
---
 security/integrity/ima/Kconfig | 1 -
 1 file changed, 1 deletion(-)

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

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

[THOBY Simon]

By default, writes to the extended attributes security.ima will be
allowed even if the hash algorithm used for the xattr 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 and audit writes to the security.ima xattr if the hash algorithm
used in the new value is not available in the current kernel.

Reviewed-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
Signed-off-by: Simon Thoby <simon.thoby@viveris.fr>
---
 security/integrity/ima/ima.h          |  2 +-
 security/integrity/ima/ima_appraise.c | 49 +++++++++++++++++++++++++--
 2 files changed, 47 insertions(+), 4 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 2f4c20b16ad7..829478dabeeb 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -319,7 +319,7 @@ int ima_must_appraise(struct user_namespace *mnt_userns, struct inode *inode,
 void ima_update_xattr(struct integrity_iint_cache *iint, struct file *file);
 enum integrity_status ima_get_cache_status(struct integrity_iint_cache *iint,
 					   enum ima_hooks func);
-enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value,
+enum hash_algo ima_get_hash_algo(const struct evm_ima_xattr_data *xattr_value,
 				 int xattr_len);
 int ima_read_xattr(struct dentry *dentry,
 		   struct evm_ima_xattr_data **xattr_value);
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 63bec42c353f..baeb10efbf51 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -171,7 +171,7 @@ static void ima_cache_flags(struct integrity_iint_cache *iint,
 	}
 }
 
-enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value,
+enum hash_algo ima_get_hash_algo(const struct evm_ima_xattr_data *xattr_value,
 				 int xattr_len)
 {
 	struct signature_v2_hdr *sig;
@@ -575,6 +575,47 @@ static void ima_reset_appraise_flags(struct inode *inode, int digsig)
 		clear_bit(IMA_DIGSIG, &iint->atomic_flags);
 }
 
+/**
+ * validate_hash_algo() - Block setxattr with unsupported hash algorithms
+ * @dentry: object of the setxattr()
+ * @xattr_value: userland supplied xattr value
+ * @xattr_value_len: length of xattr_value
+ *
+ * The xattr value is mapped to its 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 struct evm_ima_xattr_data *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(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)
+		return -EACCES;
+
+	path = dentry_path(dentry, pathbuf, PATH_MAX);
+
+	integrity_audit_msg(AUDIT_INTEGRITY_DATA, d_inode(dentry), path,
+			    "set_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)
 {
@@ -592,9 +633,11 @@ int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
 		digsig = (xvalue->type == EVM_XATTR_PORTABLE_DIGSIG);
 	}
 	if (result == 1 || evm_revalidate_status(xattr_name)) {
+		result = validate_hash_algo(dentry, xvalue, xattr_value_len);
+		if (result)
+			return result;
+
 		ima_reset_appraise_flags(d_backing_inode(dentry), digsig);
-		if (result == 1)
-			result = 0;
 	}
 	return result;
 }
-- 
2.31.1

[PATCH v7 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 signatures stored in
security.ima xattr.

Reviewed-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
Signed-off-by: Simon Thoby <simon.thoby@viveris.fr>
---
 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     | 18 +++++++++++++++---
 security/integrity/ima/ima_policy.c   | 18 ++++++++++++++++--
 5 files changed, 41 insertions(+), 12 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 829478dabeeb..bcaf818fb647 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_algos);
 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_algos);
 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..2c6c3a5228b5 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_algos: allowlist 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_algos)
 {
 	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_algos);
 }
 
 /*
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index baeb10efbf51..e2edef8a9185 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 1cba6beb5a60..af6367ba34ee 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -215,6 +215,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_algos = 0;
 
 	if (!ima_policy_flag || !S_ISREG(inode->i_mode))
 		return 0;
@@ -224,7 +225,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_algos);
 	violation_check = ((func == FILE_CHECK || func == MMAP_CHECK) &&
 			   (ima_policy_flag & IMA_MEASURE));
 	if (!action && !violation_check)
@@ -361,6 +363,16 @@ 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_algos != 0 &&
+	    (allowed_algos & (1U << hash_algo)) == 0) {
+		rc = -EACCES;
+
+		integrity_audit_msg(AUDIT_INTEGRITY_DATA, file_inode(file),
+				    pathname, "collect_data",
+				    "denied-hash-algorithm", rc, 0);
+	}
 out_locked:
 	if ((mask & MAY_WRITE) && test_bit(IMA_DIGSIG, &iint->atomic_flags) &&
 	     !(iint->flags & IMA_NEW_FILE))
@@ -438,7 +450,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)))
@@ -896,7 +908,7 @@ int 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) && !digest)
 			return -ENOENT;
 	}
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index fd5d46e511f1..1536e6f5eb22 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_ALGOS	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_algos; /* bitfield of allowed hash algorithms */
 	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_algos 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_algos: allowlist of hash algorithms 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_algos)
 {
 	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_algos &&
+			    entry->flags & IMA_VALIDATE_ALGOS)
+				*allowed_algos = entry->allowed_algos;
+		}
 
 		if (entry->action & IMA_DO_MASK)
 			actmask &= ~(entry->action | entry->action << 1);
-- 
2.31.1

[PATCH v7 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_algos=",
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_algos' 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_algos=sha256,sha384
will block the execution of iptables if the xattr security.ima of its
executables were not hashed with either sha256 or sha384.

Reviewed-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
Signed-off-by: Simon Thoby <simon.thoby@viveris.fr>
---
 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..b0e3d278e799 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_algos=] [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_algos:= 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 1536e6f5eb22..cb86da0e562b 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_algos,
 	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_algos, "appraise_algos=%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_ALGOS))
 		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_ALGOS))
 			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_ALGOS))
 			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_algos(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\"",
+			       token);
+			return 0;
+		}
+
+		/* 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_algos:
+			ima_log_string(ab, "appraise_algos", args[0].from);
+
+			if (entry->allowed_algos) {
+				result = -EINVAL;
+				break;
+			}
+
+			entry->allowed_algos =
+				ima_parse_appraise_algos(args[0].from);
+			/* invalid or empty list of algorithms */
+			if (!entry->allowed_algos) {
+				result = -EINVAL;
+				break;
+			}
+
+			entry->flags |= IMA_VALIDATE_ALGOS;
+
+			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_algos(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_ALGOS) {
+		seq_puts(m, "appraise_algos=");
+		ima_policy_show_appraise_algos(m, entry->allowed_algos);
+		seq_puts(m, " ");
+	}
+
 	for (i = 0; i < MAX_LSM_RULES; i++) {
 		if (entry->lsm[i].rule) {
 			switch (i) {
-- 
2.31.1

[PATCH v7 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_algos=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).

Reviewed-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
Signed-off-by: Simon Thoby <simon.thoby@viveris.fr>
---
 Documentation/ABI/testing/ima_policy  |  9 +++-
 security/integrity/ima/ima.h          |  6 ++-
 security/integrity/ima/ima_appraise.c | 29 ++++++++--
 security/integrity/ima/ima_main.c     |  2 +-
 security/integrity/ima/ima_policy.c   | 76 +++++++++++++++++++++++----
 5 files changed, 104 insertions(+), 18 deletions(-)

diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index b0e3d278e799..5c2798534950 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -30,9 +30,10 @@ Description:
 				[appraise_flag=] [appraise_algos=] [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_algos=sha256,sha384,sha512
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index bcaf818fb647..be965a8715e4 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,
@@ -288,7 +292,7 @@ int ima_match_policy(struct user_namespace *mnt_userns, struct inode *inode,
 		     const char *func_data, unsigned int *allowed_algos);
 void ima_init_policy(void);
 void ima_update_policy(void);
-void ima_update_policy_flag(void);
+void ima_update_policy_flags(void);
 ssize_t ima_parse_add_rule(char *);
 void ima_delete_rules(void);
 int ima_check_policy(void);
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index e2edef8a9185..8f1eb7ef041e 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -595,12 +595,32 @@ 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(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 allowed 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)
@@ -609,8 +629,7 @@ static int validate_hash_algo(struct dentry *dentry,
 	path = dentry_path(dentry, pathbuf, PATH_MAX);
 
 	integrity_audit_msg(AUDIT_INTEGRITY_DATA, d_inode(dentry), path,
-			    "set_data", "unavailable-hash-algorithm",
-			    -EACCES, 0);
+			    "set_data", errmsg, -EACCES, 0);
 
 	kfree(pathbuf);
 
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index af6367ba34ee..a734f7d5292c 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -1051,7 +1051,7 @@ static int __init init_ima(void)
 		pr_warn("Couldn't register LSM notifier, error %d\n", error);
 
 	if (!error)
-		ima_update_policy_flag();
+		ima_update_policy_flags();
 
 	return error;
 }
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index cb86da0e562b..9eaa509f487a 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
@@ -720,24 +722,57 @@ int ima_match_policy(struct user_namespace *mnt_userns, struct inode *inode,
 	return action;
 }
 
-/*
- * Initialize the ima_policy_flag variable based on the currently
- * loaded policy.  Based on this flag, the decision to short circuit
- * out of a function or not call the function in the first place
- * can be made earlier.
+/**
+ * ima_update_policy_flags() - Update global IMA variables
+ *
+ * Update ima_policy_flag and ima_setxattr_allowed_hash_algorithms
+ * based on the currently loaded policy.
+ *
+ * With ima_policy_flag, the decision to short circuit out of a function
+ * or not call the function in the first place can be made earlier.
+ *
+ * With ima_setxattr_allowed_hash_algorithms, the policy can restrict the
+ * set of hash algorithms accepted when updating the security.ima xattr of
+ * a file.
+ *
+ * Context: called after a policy update and at system initialization.
  */
-void ima_update_policy_flag(void)
+void ima_update_policy_flags(void)
 {
 	struct ima_rule_entry *entry;
+	int new_policy_flag = 0;
 
+	rcu_read_lock();
 	list_for_each_entry(entry, ima_rules, list) {
+		/*
+		 * SETXATTR_CHECK rules do not implement a full policy check
+		 * because rule checking would probably have an important
+		 * performance impact on setxattr(). As a consequence, only one
+		 * SETXATTR_CHECK can be active at a given time.
+		 * Because we want to preserve that property, we set out to use
+		 * atomic_cmpxchg. Either:
+		 * - the atomic was non-zero: a setxattr hash policy is
+		 *   already enforced, we do nothing
+		 * - the atomic was zero: no setxattr policy was set, enable
+		 *   the setxattr hash policy
+		 */
+		if (entry->func == SETXATTR_CHECK) {
+			atomic_cmpxchg(&ima_setxattr_allowed_hash_algorithms,
+				       0, entry->allowed_algos);
+			/* SETXATTR_CHECK doesn't impact ima_policy_flag */
+			continue;
+		}
+
 		if (entry->action & IMA_DO_MASK)
-			ima_policy_flag |= entry->action;
+			new_policy_flag |= entry->action;
 	}
+	rcu_read_unlock();
 
 	ima_appraise |= (build_ima_appraise | temp_ima_appraise);
 	if (!ima_appraise)
-		ima_policy_flag &= ~IMA_APPRAISE;
+		new_policy_flag &= ~IMA_APPRAISE;
+
+	ima_policy_flag = new_policy_flag;
 }
 
 static int ima_appraise_flag(enum ima_hooks func)
@@ -903,7 +938,9 @@ void __init ima_init_policy(void)
 			  ARRAY_SIZE(critical_data_rules),
 			  IMA_DEFAULT_POLICY);
 
-	ima_update_policy_flag();
+	atomic_set(&ima_setxattr_allowed_hash_algorithms, 0);
+
+	ima_update_policy_flags();
 }
 
 /* Make sure we have a valid policy, at least containing some rules. */
@@ -943,7 +980,7 @@ void ima_update_policy(void)
 		 */
 		kfree(arch_policy_entry);
 	}
-	ima_update_policy_flag();
+	ima_update_policy_flags();
 
 	/* Custom IMA policy has been loaded */
 	ima_process_queued_keys();
@@ -1176,6 +1213,23 @@ 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;
+
+		/* SETXATTR_CHECK requires an appraise_algos parameter */
+		if (!(entry->flags & IMA_VALIDATE_ALGOS))
+			return false;
+
+		/*
+		 * full policies are not supported, they would have too
+		 * much of a performance impact
+		 */
+		if (entry->flags & ~(IMA_FUNC | IMA_VALIDATE_ALGOS))
+			return false;
+
 		break;
 	default:
 		return false;
@@ -1332,6 +1386,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 v7 4/5] IMA: add a policy option to restrict xattr hash algorithms on appraisal

[Mimi Zohar]

Hi Simon,

On Wed, 2021-08-11 at 11:40 +0000, THOBY Simon wrote:
> +static unsigned int ima_parse_appraise_algos(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\"",
> +			       token);
> +			return 0;

Previous versions of this patch ignored unknown algorithms.  If not all
of the algorithms are defined in an older kernel, should loading the
policy fail?   As new IMA policy features are defined, older kernels
prevent loading newer policies with unknown features.   I hesitated to
equate the two scenarios.

> +		}
> +
> +		/* Add the hash algorithm to the 'allowed' bitfield */
> +		res |= (1U << idx);

This assumes that all the hash algorithms are enabled in the kernel,
but nothing checks that they are.  In validate_hash_algo(), either the
allowed_hashes is checked or the hash algorithm must be configured.  Do
we really want a total separation like this?

thanks,

Mimi

> +	}
> +
> +	return res;
> +}
> +

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

[Mimi Zohar]

[Cc'ing linux-audit]

Hi Simon,

On Wed, 2021-08-11 at 11:40 +0000, THOBY Simon wrote:

Other than the two questions on " IMA: add a policy option to restrict
xattr hash algorithms on appraisal" patch, the patch set is looking
good.

thanks,

Mimi

> Here is also a short description of the new audit messages, but I can
> send it in a followup mail if that is not the proper place:
> 
> When writing the xattr with an algorithm not built in the kernel (here the
> kernel was built with CONFIG_CRYPTO_MD5 unset), e.g. with
> "evmctl ima_hash -a md5 /usr/bin/strace":
> 	audit(1628066120.418:121): pid=1344 uid=0 auid=0 ses=1 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 op=set_data cause=unavailable-hash-algorithm comm="evmctl" name="/usr/bin/strace" dev="dm-0" ino=2632657 res=0 errno=0
> 
> With the same command and the policy rule
> "appraise func=SETXATTR_CHECK appraise_algos=sha256,sha384,sha512", we get:
> 	audit(1628066210.141:127): pid=1362 uid=0 auid=0 ses=1 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 op=set_data cause=denied-hash-algorithm comm="evmctl" name="/usr/bin/strace" dev="dm-0" ino=2632657 res=0 errno=0
> 
> Note that the cause is now 'denied-hash-algorithm' instead of
> 'unavailable-hash-algorithm'. We get that audit message for any algorithm
> outside of sha256/384/512 (including algorithms not compiled in the kernel
> like MD5). In a sense, 'denied-hash-algorithm' takes predecence over
> 'unavailable-hash-algorithm'.
> 
> When appraising files, e.g. trying to execute a file whose xattr was hashed
> with sha1 while the policy rule
> "appraise func=BPRM_CHECK fowner=0 appraise_algos=sha256" is enabled:
> 	audit(1628066349.230:130): pid=1369 uid=0 auid=0 ses=1 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 op=collect_data cause=denied-hash-algorithm comm="bash" name="/usr/bin/strace" dev="dm-0" ino=2632657 res=0 errno=0
> 
> 
> This series is based on the following repo/branch:
>  repo: https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git
>  branch: next-integrity-testing
>  commit e37be5343ae2b9419aea1442b07e5d2428b437b4 ("Merge branch 'ima-buffer-measurement-changes-v4' into next-integrity")

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

[Mimi Zohar]

[Cc'ing linux-audit]

Hi Simon,

On Wed, 2021-08-11 at 11:40 +0000, THOBY Simon wrote:

Other than the two questions on " IMA: add a policy option to restrict
xattr hash algorithms on appraisal" patch, the patch set is looking
good.

thanks,

Mimi

> Here is also a short description of the new audit messages, but I can
> send it in a followup mail if that is not the proper place:
> 
> When writing the xattr with an algorithm not built in the kernel (here the
> kernel was built with CONFIG_CRYPTO_MD5 unset), e.g. with
> "evmctl ima_hash -a md5 /usr/bin/strace":
> 	audit(1628066120.418:121): pid=1344 uid=0 auid=0 ses=1 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 op=set_data cause=unavailable-hash-algorithm comm="evmctl" name="/usr/bin/strace" dev="dm-0" ino=2632657 res=0 errno=0
> 
> With the same command and the policy rule
> "appraise func=SETXATTR_CHECK appraise_algos=sha256,sha384,sha512", we get:
> 	audit(1628066210.141:127): pid=1362 uid=0 auid=0 ses=1 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 op=set_data cause=denied-hash-algorithm comm="evmctl" name="/usr/bin/strace" dev="dm-0" ino=2632657 res=0 errno=0
> 
> Note that the cause is now 'denied-hash-algorithm' instead of
> 'unavailable-hash-algorithm'. We get that audit message for any algorithm
> outside of sha256/384/512 (including algorithms not compiled in the kernel
> like MD5). In a sense, 'denied-hash-algorithm' takes predecence over
> 'unavailable-hash-algorithm'.
> 
> When appraising files, e.g. trying to execute a file whose xattr was hashed
> with sha1 while the policy rule
> "appraise func=BPRM_CHECK fowner=0 appraise_algos=sha256" is enabled:
> 	audit(1628066349.230:130): pid=1369 uid=0 auid=0 ses=1 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 op=collect_data cause=denied-hash-algorithm comm="bash" name="/usr/bin/strace" dev="dm-0" ino=2632657 res=0 errno=0
> 
> 
> This series is based on the following repo/branch:
>  repo: https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git
>  branch: next-integrity-testing
>  commit e37be5343ae2b9419aea1442b07e5d2428b437b4 ("Merge branch 'ima-buffer-measurement-changes-v4' into next-integrity")


--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit

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

[Steve Grubb]

Hello,

On Wednesday, August 11, 2021 3:40:51 PM EDT Mimi Zohar wrote:
> On Wed, 2021-08-11 at 11:40 +0000, THOBY Simon wrote:
> Other than the two questions on " IMA: add a policy option to restrict
> xattr hash algorithms on appraisal" patch, the patch set is looking
> good.
> 
> thanks,
> 
> Mimi
> 
> > Here is also a short description of the new audit messages, but I can
> > send it in a followup mail if that is not the proper place:
> > 
> > When writing the xattr with an algorithm not built in the kernel (here
> > the
> > kernel was built with CONFIG_CRYPTO_MD5 unset), e.g. with
> > 
> > "evmctl ima_hash -a md5 /usr/bin/strace":
> > 	audit(1628066120.418:121): pid=1344 uid=0 auid=0 ses=1
> > 	subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 op=set_data
> > 	cause=unavailable-hash-algorithm comm="evmctl" name="/usr/bin/strace"
> > 	dev="dm-0" ino=2632657 res=0 errno=0> 

Is this audit event accurate? I seem to be seeing name=value=value. I'm 
hoping this is a copy/paste/mail client issue.

-Steve


> > With the same command and the policy rule
> > 
> > "appraise func=SETXATTR_CHECK appraise_algos=sha256,sha384,sha512", we 
get:
> > 	audit(1628066210.141:127): pid=1362 uid=0 auid=0 ses=1
> > 	subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 op=set_data
> > 	cause=denied-hash-algorithm comm="evmctl" name="/usr/bin/strace"
> > 	dev="dm-0" ino=2632657 res=0 errno=0> 
> > Note that the cause is now 'denied-hash-algorithm' instead of
> > 'unavailable-hash-algorithm'. We get that audit message for any algorithm
> > outside of sha256/384/512 (including algorithms not compiled in the
> > kernel
> > like MD5). In a sense, 'denied-hash-algorithm' takes predecence over
> > 'unavailable-hash-algorithm'.
> > 
> > When appraising files, e.g. trying to execute a file whose xattr was
> > hashed with sha1 while the policy rule
> > 
> > "appraise func=BPRM_CHECK fowner=0 appraise_algos=sha256" is enabled:
> > 	audit(1628066349.230:130): pid=1369 uid=0 auid=0 ses=1
> > 	subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
> > 	op=collect_data cause=denied-hash-algorithm comm="bash"
> > 	name="/usr/bin/strace" dev="dm-0" ino=2632657 res=0 errno=0> 
> > This series is based on the following repo/branch:
> >  repo:
> >  https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.g
> >  it branch: next-integrity-testing
> >  commit e37be5343ae2b9419aea1442b07e5d2428b437b4 ("Merge branch
> >  'ima-buffer-measurement-changes-v4' into next-integrity")
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://listman.redhat.com/mailman/listinfo/linux-audit

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

[Steve Grubb]

Hello,

On Wednesday, August 11, 2021 3:40:51 PM EDT Mimi Zohar wrote:
> On Wed, 2021-08-11 at 11:40 +0000, THOBY Simon wrote:
> Other than the two questions on " IMA: add a policy option to restrict
> xattr hash algorithms on appraisal" patch, the patch set is looking
> good.
> 
> thanks,
> 
> Mimi
> 
> > Here is also a short description of the new audit messages, but I can
> > send it in a followup mail if that is not the proper place:
> > 
> > When writing the xattr with an algorithm not built in the kernel (here
> > the
> > kernel was built with CONFIG_CRYPTO_MD5 unset), e.g. with
> > 
> > "evmctl ima_hash -a md5 /usr/bin/strace":
> > 	audit(1628066120.418:121): pid=1344 uid=0 auid=0 ses=1
> > 	subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 op=set_data
> > 	cause=unavailable-hash-algorithm comm="evmctl" name="/usr/bin/strace"
> > 	dev="dm-0" ino=2632657 res=0 errno=0> 

Is this audit event accurate? I seem to be seeing name=value=value. I'm 
hoping this is a copy/paste/mail client issue.

-Steve


> > With the same command and the policy rule
> > 
> > "appraise func=SETXATTR_CHECK appraise_algos=sha256,sha384,sha512", we 
get:
> > 	audit(1628066210.141:127): pid=1362 uid=0 auid=0 ses=1
> > 	subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 op=set_data
> > 	cause=denied-hash-algorithm comm="evmctl" name="/usr/bin/strace"
> > 	dev="dm-0" ino=2632657 res=0 errno=0> 
> > Note that the cause is now 'denied-hash-algorithm' instead of
> > 'unavailable-hash-algorithm'. We get that audit message for any algorithm
> > outside of sha256/384/512 (including algorithms not compiled in the
> > kernel
> > like MD5). In a sense, 'denied-hash-algorithm' takes predecence over
> > 'unavailable-hash-algorithm'.
> > 
> > When appraising files, e.g. trying to execute a file whose xattr was
> > hashed with sha1 while the policy rule
> > 
> > "appraise func=BPRM_CHECK fowner=0 appraise_algos=sha256" is enabled:
> > 	audit(1628066349.230:130): pid=1369 uid=0 auid=0 ses=1
> > 	subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
> > 	op=collect_data cause=denied-hash-algorithm comm="bash"
> > 	name="/usr/bin/strace" dev="dm-0" ino=2632657 res=0 errno=0> 
> > This series is based on the following repo/branch:
> >  repo:
> >  https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.g
> >  it branch: next-integrity-testing
> >  commit e37be5343ae2b9419aea1442b07e5d2428b437b4 ("Merge branch
> >  'ima-buffer-measurement-changes-v4' into next-integrity")
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://listman.redhat.com/mailman/listinfo/linux-audit




--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit

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

[Steve Grubb]

Hello,

On Wednesday, August 11, 2021 7:53:15 PM EDT Steve Grubb wrote:
> On Wednesday, August 11, 2021 3:40:51 PM EDT Mimi Zohar wrote:
> > On Wed, 2021-08-11 at 11:40 +0000, THOBY Simon wrote:
> > Other than the two questions on " IMA: add a policy option to restrict
> > xattr hash algorithms on appraisal" patch, the patch set is looking
> > good.
> > 
> > thanks,
> > 
> > Mimi
> > 
> > > Here is also a short description of the new audit messages, but I can
> > > send it in a followup mail if that is not the proper place:
> > > 
> > > When writing the xattr with an algorithm not built in the kernel (here
> > > the kernel was built with CONFIG_CRYPTO_MD5 unset), e.g. with
> > > 
> > > "evmctl ima_hash -a md5 /usr/bin/strace":
> > > 	audit(1628066120.418:121): pid=1344 uid=0 auid=0 ses=1
> > > 	subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 op=set_data
> > > 	cause=unavailable-hash-algorithm comm="evmctl" name="/usr/bin/
strace"
> > > 	dev="dm-0" ino=2632657 res=0 errno=0>
> 
> Is this audit event accurate? I seem to be seeing name=value=value. I'm
> hoping this is a copy/paste/mail client issue.

Sorry for the noise...I see there is a space in there.

-Steve

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

[Steve Grubb]

Hello,

On Wednesday, August 11, 2021 7:53:15 PM EDT Steve Grubb wrote:
> On Wednesday, August 11, 2021 3:40:51 PM EDT Mimi Zohar wrote:
> > On Wed, 2021-08-11 at 11:40 +0000, THOBY Simon wrote:
> > Other than the two questions on " IMA: add a policy option to restrict
> > xattr hash algorithms on appraisal" patch, the patch set is looking
> > good.
> > 
> > thanks,
> > 
> > Mimi
> > 
> > > Here is also a short description of the new audit messages, but I can
> > > send it in a followup mail if that is not the proper place:
> > > 
> > > When writing the xattr with an algorithm not built in the kernel (here
> > > the kernel was built with CONFIG_CRYPTO_MD5 unset), e.g. with
> > > 
> > > "evmctl ima_hash -a md5 /usr/bin/strace":
> > > 	audit(1628066120.418:121): pid=1344 uid=0 auid=0 ses=1
> > > 	subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 op=set_data
> > > 	cause=unavailable-hash-algorithm comm="evmctl" name="/usr/bin/
strace"
> > > 	dev="dm-0" ino=2632657 res=0 errno=0>
> 
> Is this audit event accurate? I seem to be seeing name=value=value. I'm
> hoping this is a copy/paste/mail client issue.

Sorry for the noise...I see there is a space in there.

-Steve


--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit

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

[THOBY Simon]

Hi Mimi,


On 8/11/21 6:26 PM, Mimi Zohar wrote:
> Hi Simon,
> 
> On Wed, 2021-08-11 at 11:40 +0000, THOBY Simon wrote:
>> +static unsigned int ima_parse_appraise_algos(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\"",
>> +			       token);
>> +			return 0;
> 
> Previous versions of this patch ignored unknown algorithms.  If not all
> of the algorithms are defined in an older kernel, should loading the
> policy fail?   As new IMA policy features are defined, older kernels
> prevent loading newer policies with unknown features.   I hesitated to
> equate the two scenarios.

Yes, that choice isn't easy. I changed the behavior in response to Lakshmi's
remark on v6. It's true that failing to load the policy on an old kernel because
of an unknown algorithm is not very desirable, but loading a partial policy may
be worse.

As an exampe, if we load the policy rule
	appraise func=BPRM_CHECK fowner=0 appraise_algos=sha256,<new_algo>
in a version of the kernel that doesn't know about <new_algo>, a permissive interpretation
would yield
	appraise func=BPRM_CHECK fowner=0 appraise_algos=sha256.
Now if the the system files were already hashes with <new_algo>, then the user is in
for a world of pain as the system can't boot (trying to appraise every executable root
owns - that is, pretty much all of them - will fail).
Admittedly, if the kernel doesn't know about <new_algo>, there is little chances the
filesystem was protected with that algorithm, unless the user did migrate to <new_algo>
at some point and is now trying to rollback to an older kernel for some reason.
So I think that remains a very niche issue.


On the other hand, rejecting the policy protects against typos and human errors
(while trying this patch, I once wrote 'appraise [...] appraise_hashes=sha256,sha384;sha512'
which was accepted and silently updated to 'appraise [...] appraise_hashes=sha256',
and I didn't understood my error until trying to launch a command and being greeted with the
infamous "Permission denied". Had I been monitoring the kernel log, I would have seen the error
right away, but as the policy was accepted I assumed it did what I expected and didn't thought
any more of it until seeing failures).
It is also more consistent, as I think it is a desirable feature to know when writing a policy
to the kernel that reading it back will return the exact same policy, modulo the order of the
fields in the policy rules. Ignoring unknown algorithms breaks that behavior.


Additionally, I don't think digest algorithms are added very frequently to the
kernel (or else the list would be much longer), which mitigate the potential impact
of either solution.


After some thought, I am personally inclined to prefer strict checking, as I think failing fast
and early can save great ordeals later. Of course it's not always easy in the case of the kernel,
where failure is not an option except in some rare catastrophic situations. But as rejecting
invalid algorithms is coherent with the IMA behavior with regard to new features, and rejecting
a policy cannot break a system (although it definitely reduces the trust you can have in the
integrity of the system), I think that's an acceptable behavior.

> 
>> +		}
>> +
>> +		/* Add the hash algorithm to the 'allowed' bitfield */
>> +		res |= (1U << idx);
> 
> This assumes that all the hash algorithms are enabled in the kernel,
> but nothing checks that they are.  In validate_hash_algo(), either the
> allowed_hashes is checked or the hash algorithm must be configured.  Do
> we really want a total separation like this?

I kind of assumed that if the user allowlist some algorithms with 'appraise_algos', he is basically
saying "I know what I'm doing, trust these values", and thus these values take precedence on
the algorithms compiled in the kernel.

In addition, validate_hash_algo() is only ever used for setxattr, not for appraisal
(which is the subject of this specific patch). If a user try to run a file appraised
with an algorithm not present in the kernel, ima_collect_measurement() would fail
before we even checked that the algorithm is in the allowlist (process_measurement()->
ima_collect_measurement()->ima_calc_file_hash()->ima_calc_file_ahash()->ima_alloc_atfm(<algo>)
would fail and log an error message like "Can not allocate <algo> (reason: <result>)").
So that check is already done "for free" on appraisal.

However your comment does applies to the next patch ("IMA: introduce a new policy
option func=SETXATTR_CHECK"), and we probably could restrict the algorithms referenced in
ima_setxattr_allowed_hash_algorithms to ones the current kernel can use. 
The easiest way to enforce this would probably be to check that when parsing 'appraise_algos'
in ima_parse_appraise_algos(), we only add algorithms that are available, ignoring - or
rejecting, according to the outcome of the discussion above - the other algorithms. That way,
we do not have to pay the price of allocating a hash object every time validate_hash_algo() is
called.

Would it be ok if I did that?

> 
> thanks,
> 
> Mimi
> 
>> +	}
>> +
>> +	return res;
>> +}
>> +
> 

Thanks,
Simon

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

[Mimi Zohar]

Hi Simon,

On Thu, 2021-08-12 at 08:06 +0000, THOBY Simon wrote:
> On 8/11/21 6:26 PM, Mimi Zohar wrote:
> > On Wed, 2021-08-11 at 11:40 +0000, THOBY Simon wrote:
> >> +static unsigned int ima_parse_appraise_algos(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\"",
> >> +			       token);
> >> +			return 0;
> > 
> > Previous versions of this patch ignored unknown algorithms.  If not all
> > of the algorithms are defined in an older kernel, should loading the
> > policy fail?   As new IMA policy features are defined, older kernels
> > prevent loading newer policies with unknown features.   I hesitated to
> > equate the two scenarios.
> 
> Yes, that choice isn't easy. I changed the behavior in response to Lakshmi's
> remark on v6. It's true that failing to load the policy on an old kernel because
> of an unknown algorithm is not very desirable, but loading a partial policy may
> be worse.

Somehow I missed Lakshmi's comment.  Thank you for the really well
written and thought out explanation below.

> 
> As an exampe, if we load the policy rule
> 	appraise func=BPRM_CHECK fowner=0 appraise_algos=sha256,<new_algo>
> in a version of the kernel that doesn't know about <new_algo>, a permissive interpretation
> would yield
> 	appraise func=BPRM_CHECK fowner=0 appraise_algos=sha256.
> Now if the the system files were already hashes with <new_algo>, then the user is in
> for a world of pain as the system can't boot (trying to appraise every executable root
> owns - that is, pretty much all of them - will fail).
> Admittedly, if the kernel doesn't know about <new_algo>, there is little chances the
> filesystem was protected with that algorithm, unless the user did migrate to <new_algo>
> at some point and is now trying to rollback to an older kernel for some reason.
> So I think that remains a very niche issue.
> 
> 
> On the other hand, rejecting the policy protects against typos and human errors
> (while trying this patch, I once wrote 'appraise [...] appraise_hashes=sha256,sha384;sha512'
> which was accepted and silently updated to 'appraise [...] appraise_hashes=sha256',
> and I didn't understood my error until trying to launch a command and being greeted with the
> infamous "Permission denied". Had I been monitoring the kernel log, I would have seen the error
> right away, but as the policy was accepted I assumed it did what I expected and didn't thought
> any more of it until seeing failures).
> It is also more consistent, as I think it is a desirable feature to know when writing a policy
> to the kernel that reading it back will return the exact same policy, modulo the order of the
> fields in the policy rules. Ignoring unknown algorithms breaks that behavior.
> 
> 
> Additionally, I don't think digest algorithms are added very frequently to the
> kernel (or else the list would be much longer), which mitigate the potential impact
> of either solution.
> 
> 
> After some thought, I am personally inclined to prefer strict checking, as I think failing fast
> and early can save great ordeals later. Of course it's not always easy in the case of the kernel,
> where failure is not an option except in some rare catastrophic situations. But as rejecting
> invalid algorithms is coherent with the IMA behavior with regard to new features, and rejecting
> a policy cannot break a system (although it definitely reduces the trust you can have in the
> integrity of the system), I think that's an acceptable behavior.
> 
> > 
> >> +		}
> >> +
> >> +		/* Add the hash algorithm to the 'allowed' bitfield */
> >> +		res |= (1U << idx);
> > 
> > This assumes that all the hash algorithms are enabled in the kernel,
> > but nothing checks that they are.  In validate_hash_algo(), either the
> > allowed_hashes is checked or the hash algorithm must be configured.  Do
> > we really want a total separation like this?
> 
> I kind of assumed that if the user allowlist some algorithms with 'appraise_algos', he is basically
> saying "I know what I'm doing, trust these values", and thus these values take precedence on
> the algorithms compiled in the kernel.

Agreed, but having this discussion is important too.

> 
> In addition, validate_hash_algo() is only ever used for setxattr, not for appraisal
> (which is the subject of this specific patch). If a user try to run a file appraised
> with an algorithm not present in the kernel, ima_collect_measurement() would fail
> before we even checked that the algorithm is in the allowlist (process_measurement()->
> ima_collect_measurement()->ima_calc_file_hash()->ima_calc_file_ahash()->ima_alloc_atfm(<algo>)
> would fail and log an error message like "Can not allocate <algo> (reason: <result>)").
> So that check is already done "for free" on appraisal.
> 
> However your comment does applies to the next patch ("IMA: introduce a new policy
> option func=SETXATTR_CHECK"), and we probably could restrict the algorithms referenced in
> ima_setxattr_allowed_hash_algorithms to ones the current kernel can use. 
> The easiest way to enforce this would probably be to check that when parsing 'appraise_algos'
> in ima_parse_appraise_algos(), we only add algorithms that are available, ignoring - or
> rejecting, according to the outcome of the discussion above - the other algorithms. That way,
> we do not have to pay the price of allocating a hash object every time validate_hash_algo() is
> called.
> 
> Would it be ok if I did that?

Without knowing and understanding all the environments in which IMA is
enabled (e.g. front end, back end build system), you're correct -
protecting the user from themselves -might not be the right answer.

What you suggested, above, would be the correct solution.  Perhaps post
that change as a separate patch, on top of this patch set, for
additional discussion.

thanks,

Mimi

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

[Mimi Zohar]

Hi Simon,

On Thu, 2021-08-12 at 09:16 -0400, Mimi Zohar wrote:
> On Thu, 2021-08-12 at 08:06 +0000, THOBY Simon wrote:

> > However your comment does applies to the next patch ("IMA: introduce a new policy
> > option func=SETXATTR_CHECK"), and we probably could restrict the algorithms referenced in
> > ima_setxattr_allowed_hash_algorithms to ones the current kernel can use. 
> > The easiest way to enforce this would probably be to check that when parsing 'appraise_algos'
> > in ima_parse_appraise_algos(), we only add algorithms that are available, ignoring - or
> > rejecting, according to the outcome of the discussion above - the other algorithms. That way,
> > we do not have to pay the price of allocating a hash object every time validate_hash_algo() is
> > called.
> > 
> > Would it be ok if I did that?
> 
> Without knowing and understanding all the environments in which IMA is
> enabled (e.g. front end, back end build system), you're correct -
> protecting the user from themselves -might not be the right answer.
> 
> What you suggested, above, would be the correct solution.  Perhaps post
> that change as a separate patch, on top of this patch set, for
> additional discussion.

Before posting the patch, please fix your user name and email address
in the git configuration.  scripts/checkpatch.pl is complaining:

ERROR: Missing Signed-off-by: line by nominal patch author 'THOBY Simon
<Simon.THOBY@viveris.fr>'

total: 1 errors, 0 warnings, 218 lines checked

thanks,

Mimi

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

[THOBY Simon]

Hi Mimi,

On 8/12/21 8:31 PM, Mimi Zohar wrote:
> Hi Simon,
> 
> On Thu, 2021-08-12 at 09:16 -0400, Mimi Zohar wrote:
>> On Thu, 2021-08-12 at 08:06 +0000, THOBY Simon wrote:
> 
>>> However your comment does applies to the next patch ("IMA: introduce a new policy
>>> option func=SETXATTR_CHECK"), and we probably could restrict the algorithms referenced in
>>> ima_setxattr_allowed_hash_algorithms to ones the current kernel can use. 
>>> The easiest way to enforce this would probably be to check that when parsing 'appraise_algos'
>>> in ima_parse_appraise_algos(), we only add algorithms that are available, ignoring - or
>>> rejecting, according to the outcome of the discussion above - the other algorithms. That way,
>>> we do not have to pay the price of allocating a hash object every time validate_hash_algo() is
>>> called.
>>>
>>> Would it be ok if I did that?
>>
>> Without knowing and understanding all the environments in which IMA is
>> enabled (e.g. front end, back end build system), you're correct -
>> protecting the user from themselves -might not be the right answer.
>>
>> What you suggested, above, would be the correct solution.  Perhaps post
>> that change as a separate patch, on top of this patch set, for
>> additional discussion.
> 
> Before posting the patch, please fix your user name and email address
> in the git configuration.  scripts/checkpatch.pl is complaining:
> 
> ERROR: Missing Signed-off-by: line by nominal patch author 'THOBY Simon
> <Simon.THOBY@viveris.fr>'

> 
> total: 1 errors, 0 warnings, 218 lines checked

I guess Microsoft Exchange strikes again :/
On my end, the patches have the correct 'From: Simon Thoby <simon.thoby@viveris.fr>'
header, but it seems Outlook likes to rewrite headers to match my Microsoft work account
information. I will update my git config, as I can't edit the name and email of the corporate
account.
Sorry about that.


> 
> thanks,
> 
> Mimi
> 

Thanks,
Simon

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

[Mimi Zohar]

On Fri, 2021-08-13 at 07:17 +0000, THOBY Simon wrote:
> On 8/12/21 8:31 PM, Mimi Zohar wrote:
> > Before posting the patch, please fix your user name and email address
> > in the git configuration.  scripts/checkpatch.pl is complaining:
> > 
> > ERROR: Missing Signed-off-by: line by nominal patch author 'THOBY Simon
> > <Simon.THOBY@viveris.fr>'
> 
> > 
> > total: 1 errors, 0 warnings, 218 lines checked
> 
> I guess Microsoft Exchange strikes again :/
> On my end, the patches have the correct 'From: Simon Thoby <simon.thoby@viveris.fr>'
> header, but it seems Outlook likes to rewrite headers to match my Microsoft work account
> information. I will update my git config, as I can't edit the name and email of the corporate
> account.
> Sorry about that.

If commit 48ca2d8ac8a1 ("checkpatch: add new warnings to author signoff
checks") wasn't case sensitive, it would have flagged  "<
simon.thoby@viveris.fr>" as a warning, not an error.

Mimi