linux-integrity.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 0/5] IMA: restrict the accepted digest algorithms for
@ 2021-07-27 16:33 THOBY Simon
  2021-07-27 16:33 ` [PATCH v4 1/5] IMA: remove the dependency on CRYPTO_MD5 THOBY Simon
                   ` (5 more replies)
  0 siblings, 6 replies; 17+ messages in thread
From: THOBY Simon @ 2021-07-27 16:33 UTC (permalink / raw)
  To: zohar, dmitry.kasatkin, linux-integrity, BARVAUX Didier; +Cc: 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, as was outlined in
https://lore.kernel.org/linux-integrity/10dde047d76b447f32ca91356599be679b8a76e5.camel@linux.ibm.com/t/#m0f8127c6982ef94aa42f5cc13ea83b9f9000917e

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', update the "ima_hash" parameter to
'new_algo', reboot, relabel all your files with 'new_algo', alter your
policy_rule from 'appraise_hash=old_algo,new_algo' to
'appraise_hash=new_algo', reboot again and you're done.
Agreed, it's quite a lot of churn - I don't know if this can be reduced -
but this is technically doable.


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 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 (4):
  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

Simon Thoby (4):
  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

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 |  79 +++++++++++-
 security/integrity/ima/ima_main.c     |  19 ++-
 security/integrity/ima/ima_policy.c   | 168 +++++++++++++++++++++++++-
 7 files changed, 278 insertions(+), 20 deletions(-)

-- 
2.31.1

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v4 1/5] IMA: remove the dependency on CRYPTO_MD5
  2021-07-27 16:33 [PATCH v4 0/5] IMA: restrict the accepted digest algorithms for THOBY Simon
@ 2021-07-27 16:33 ` THOBY Simon
  2021-07-27 17:57   ` Mimi Zohar
  2021-07-27 16:33 ` [PATCH v4 2/5] IMA: block writes of the security.ima xattr with unsupported algorithms THOBY Simon
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 17+ messages in thread
From: THOBY Simon @ 2021-07-27 16:33 UTC (permalink / raw)
  To: zohar, dmitry.kasatkin, linux-integrity, BARVAUX Didier; +Cc: 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, 2 insertions(+), 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
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 287b90509006..d171764230b7 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -53,8 +53,10 @@ 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;
+#ifdef CONFIG_CRYPTO_MD5
 		} else if (strncmp(str, "md5", 3) == 0) {
 			ima_hash_algo = HASH_ALGO_MD5;
+#endif
 		} else {
 			pr_err("invalid hash algorithm \"%s\" for template \"%s\"",
 				str, IMA_TEMPLATE_IMA_NAME);
-- 
2.31.1

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v4 2/5] IMA: block writes of the security.ima xattr with unsupported algorithms
  2021-07-27 16:33 [PATCH v4 0/5] IMA: restrict the accepted digest algorithms for THOBY Simon
  2021-07-27 16:33 ` [PATCH v4 1/5] IMA: remove the dependency on CRYPTO_MD5 THOBY Simon
@ 2021-07-27 16:33 ` THOBY Simon
  2021-07-27 20:32   ` Mimi Zohar
  2021-07-27 16:33 ` [PATCH v4 3/5] IMA: add support to restrict the hash algorithms used for file appraisal THOBY Simon
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 17+ messages in thread
From: THOBY Simon @ 2021-07-27 16:33 UTC (permalink / raw)
  To: zohar, dmitry.kasatkin, linux-integrity, BARVAUX Didier; +Cc: 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 | 51 ++++++++++++++++++++++++++-
 1 file changed, 50 insertions(+), 1 deletion(-)

diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index ef9dcfce45d4..989da2fbf496 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -575,12 +575,55 @@ static void ima_reset_appraise_flags(struct inode *inode, int digsig)
 		clear_bit(IMA_DIGSIG, &iint->atomic_flags);
 }
 
+/**
+ * ima_setxattr_validate_hash_alg() - Block setxattr with invalid digests
+ * @dentry: file being altered
+ * @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.
+ */
+int ima_setxattr_validate_hash_alg(struct dentry *dentry,
+				   const void *xattr_value,
+				   size_t xattr_value_len)
+{
+	int res = -EACCES;
+	char *path = NULL, *pathbuf = NULL;
+	enum hash_algo dentry_hash;
+
+	dentry_hash = ima_get_hash_algo((struct evm_ima_xattr_data *)xattr_value,
+				     xattr_value_len);
+
+	if (likely(dentry_hash == ima_hash_algo
+	    || crypto_has_alg(hash_algo_name[dentry_hash], 0, 0)))
+		return 0;
+
+	pathbuf = kmalloc(PATH_MAX, GFP_KERNEL);
+	/* no memory available ? no file path for you */
+	if (pathbuf)
+		path = dentry_path(dentry, pathbuf, PATH_MAX);
+
+	/* disallow xattr writes with algorithms not built in the kernel */
+	integrity_audit_msg(AUDIT_INTEGRITY_DATA, d_inode(dentry),
+		path, "collect_data", "unavailable-hash-algorithm", res, 0);
+
+	kfree(pathbuf);
+
+	return res;
+}
+
 int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
 		       const void *xattr_value, size_t xattr_value_len)
 {
 	const struct evm_ima_xattr_data *xvalue = xattr_value;
 	int digsig = 0;
-	int result;
+	int result, rc;
 
 	result = ima_protect_xattr(dentry, xattr_name, xattr_value,
 				   xattr_value_len);
@@ -592,6 +635,12 @@ 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)) {
+		/* the user-supplied xattr must use an allowed hash algo */
+		rc = ima_setxattr_validate_hash_alg(dentry, xattr_value,
+							xattr_value_len);
+		if (!rc)
+			return rc;
+
 		ima_reset_appraise_flags(d_backing_inode(dentry), digsig);
 		if (result == 1)
 			result = 0;
-- 
2.31.1

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v4 3/5] IMA: add support to restrict the hash algorithms used for file appraisal
  2021-07-27 16:33 [PATCH v4 0/5] IMA: restrict the accepted digest algorithms for THOBY Simon
  2021-07-27 16:33 ` [PATCH v4 1/5] IMA: remove the dependency on CRYPTO_MD5 THOBY Simon
  2021-07-27 16:33 ` [PATCH v4 2/5] IMA: block writes of the security.ima xattr with unsupported algorithms THOBY Simon
@ 2021-07-27 16:33 ` THOBY Simon
  2021-07-27 20:38   ` Mimi Zohar
  2021-07-27 16:33 ` [PATCH v4 4/5] IMA: add a policy option to restrict xattr hash algorithms on appraisal THOBY Simon
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 17+ messages in thread
From: THOBY Simon @ 2021-07-27 16:33 UTC (permalink / raw)
  To: zohar, dmitry.kasatkin, linux-integrity, BARVAUX Didier; +Cc: 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>
---
 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 989da2fbf496..f751410930a5 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 d171764230b7..8aa0a3fe00a1 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -212,6 +212,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;
@@ -221,7 +222,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)
@@ -358,6 +360,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 that 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))
@@ -435,7 +446,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)))
@@ -884,7 +895,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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v4 4/5] IMA: add a policy option to restrict xattr hash algorithms on appraisal
  2021-07-27 16:33 [PATCH v4 0/5] IMA: restrict the accepted digest algorithms for THOBY Simon
                   ` (2 preceding siblings ...)
  2021-07-27 16:33 ` [PATCH v4 3/5] IMA: add support to restrict the hash algorithms used for file appraisal THOBY Simon
@ 2021-07-27 16:33 ` THOBY Simon
  2021-07-27 21:07   ` Mimi Zohar
  2021-07-27 16:33 ` [PATCH v4 5/5] IMA: introduce a new policy option func=SETXATTR_CHECK THOBY Simon
  2021-07-27 17:47 ` [PATCH v4 0/5] IMA: restrict the accepted digest algorithms for Mimi Zohar
  5 siblings, 1 reply; 17+ messages in thread
From: THOBY Simon @ 2021-07-27 16:33 UTC (permalink / raw)
  To: zohar, dmitry.kasatkin, linux-integrity, BARVAUX Didier; +Cc: THOBY Simon

The kernel have 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 mentionned 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>
---
 Documentation/ABI/testing/ima_policy |  6 ++-
 security/integrity/ima/ima_policy.c  | 75 ++++++++++++++++++++++++++--
 2 files changed, 76 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..aadd95753229 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;
@@ -1204,6 +1228,7 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 	entry->uid_op = &uid_eq;
 	entry->fowner_op = &uid_eq;
 	entry->action = UNKNOWN;
+	entry->allowed_hashes = 0;
 	while ((p = strsep(&rule, " \t")) != NULL) {
 		substring_t args[MAX_OPT_ARGS];
 		int token;
@@ -1522,6 +1547,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 +1758,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 +1886,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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v4 5/5] IMA: introduce a new policy option func=SETXATTR_CHECK
  2021-07-27 16:33 [PATCH v4 0/5] IMA: restrict the accepted digest algorithms for THOBY Simon
                   ` (3 preceding siblings ...)
  2021-07-27 16:33 ` [PATCH v4 4/5] IMA: add a policy option to restrict xattr hash algorithms on appraisal THOBY Simon
@ 2021-07-27 16:33 ` THOBY Simon
  2021-07-27 17:25   ` Mimi Zohar
  2021-07-27 17:47 ` [PATCH v4 0/5] IMA: restrict the accepted digest algorithms for Mimi Zohar
  5 siblings, 1 reply; 17+ messages in thread
From: THOBY Simon @ 2021-07-27 16:33 UTC (permalink / raw)
  To: zohar, dmitry.kasatkin, linux-integrity, BARVAUX Didier; +Cc: 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).

On policy update, the latest SETXATTR_CHECK rule is the only one
that apply, and other SETXATTR_CHECK rules are deleted.

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 | 33 ++++++++++--
 security/integrity/ima/ima_policy.c   | 75 +++++++++++++++++++++++++++
 4 files changed, 115 insertions(+), 6 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 f751410930a5..b938a053366c 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -596,23 +596,46 @@ int ima_setxattr_validate_hash_alg(struct dentry *dentry,
 {
 	int res = -EACCES;
 	char *path = NULL, *pathbuf = NULL;
+	const char *errmsg = "unavailable-hash-algorithm";
 	enum hash_algo dentry_hash;
+	unsigned int allowed_hashes;
 
 	dentry_hash = ima_get_hash_algo((struct evm_ima_xattr_data *)xattr_value,
 				     xattr_value_len);
 
-	if (likely(dentry_hash == ima_hash_algo
-	    || crypto_has_alg(hash_algo_name[dentry_hash], 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 << dentry_hash))
+			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(dentry_hash == ima_hash_algo))
+			return 0;
+
+		/* allow any xattr using an algorithm built in the kernel */
+		if (crypto_has_alg(hash_algo_name[dentry_hash], 0, 0))
+			return 0;
+	}
 
 	pathbuf = kmalloc(PATH_MAX, GFP_KERNEL);
 	/* no memory available ? no file path for you */
 	if (pathbuf)
 		path = dentry_path(dentry, pathbuf, PATH_MAX);
 
-	/* disallow xattr writes with algorithms not built in the kernel */
+	/*
+	 * disallow xattr writes with algorithms not built in the kernel or
+	 * denied by policy
+	 */
 	integrity_audit_msg(AUDIT_INTEGRITY_DATA, d_inode(dentry),
-		path, "collect_data", "unavailable-hash-algorithm", res, 0);
+		path, "collect_data", errmsg, res, 0);
 
 	kfree(pathbuf);
 
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index aadd95753229..a74b78e1746e 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
@@ -87,6 +89,7 @@ struct ima_rule_entry {
 		int type;	/* audit type */
 	} lsm[MAX_LSM_RULES];
 	char *fsname;
+	struct rcu_head rcu;
 	struct ima_rule_opt_list *keyrings; /* Measure keys added to these keyrings */
 	struct ima_rule_opt_list *label; /* Measure data grouped under this label */
 	struct ima_template_desc *template;
@@ -368,6 +371,13 @@ static void ima_free_rule(struct ima_rule_entry *entry)
 	kfree(entry);
 }
 
+static void ima_free_rule_rcu(struct rcu_head *rcu)
+{
+	struct ima_rule_entry *entry = container_of(rcu, struct ima_rule_entry, rcu);
+
+	ima_free_rule(entry);
+}
+
 static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
 {
 	struct ima_rule_entry *nentry;
@@ -903,6 +913,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 +926,52 @@ int ima_check_policy(void)
 	return 0;
 }
 
+/** ima_update_setxattr_allowed_hash_algorithms - cleanup SETXATTR_CHECK rules
+ * in the new ruleset
+ * @policy: the list of ima_rules_entry to clean
+ *
+ * Context: called when updating the IMA policy. Delete non-applicable
+ * rules with 'func' set to SETXATTR_CHECK and update the atomic variable
+ * to hold the list of allowed hash algorithms for the security.ima xattr.
+ *
+ * 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. To prevent confusion, on policy updates, if a new SETXATTR_CHECK
+ * is defined, other SETXATTR_CHECK rules are remove from the ruleset.
+ */
+void ima_update_setxattr_allowed_hash_algorithms(struct list_head *policy)
+{
+	struct ima_rule_entry *entry, *tmp;
+	bool setxattr_check_already_defined = false;
+
+	rcu_read_lock();
+	list_for_each_entry_safe_reverse(entry, tmp, policy, list) {
+		if (entry->func != SETXATTR_CHECK)
+			continue;
+
+		if (setxattr_check_already_defined) {
+			/*
+			 * delete old SETXATTR_CHECK entries when a newer
+			 * one already exists
+			 */
+			list_del_rcu(&entry->list);
+			call_rcu(&entry->rcu, ima_free_rule_rcu);
+		} else {
+			/*
+			 * only the last entry with the SETXATTR_CHECK func
+			 * apply: this allows runtime upgrades of the
+			 * digest algorithm policy, unlike the other IMA
+			 * rules
+			 */
+			atomic_xchg(&ima_setxattr_allowed_hash_algorithms,
+				    entry->allowed_hashes);
+			setxattr_check_already_defined = true;
+		}
+	}
+	rcu_read_unlock();
+}
+
 /**
  * ima_update_policy - update default_rules with new measure rules
  *
@@ -931,6 +989,8 @@ void ima_update_policy(void)
 
 	list_splice_tail_init_rcu(&ima_temp_rules, policy, synchronize_rcu);
 
+	ima_update_setxattr_allowed_hash_algorithms(policy);
+
 	if (ima_rules != policy) {
 		ima_policy_flag = 0;
 		ima_rules = policy;
@@ -1176,6 +1236,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;
@@ -1333,6 +1406,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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH v4 5/5] IMA: introduce a new policy option func=SETXATTR_CHECK
  2021-07-27 16:33 ` [PATCH v4 5/5] IMA: introduce a new policy option func=SETXATTR_CHECK THOBY Simon
@ 2021-07-27 17:25   ` Mimi Zohar
  2021-07-27 17:58     ` THOBY Simon
  0 siblings, 1 reply; 17+ messages in thread
From: Mimi Zohar @ 2021-07-27 17:25 UTC (permalink / raw)
  To: THOBY Simon, dmitry.kasatkin, linux-integrity, BARVAUX Didier

Hi Simon,

On Tue, 2021-07-27 at 16:33 +0000, THOBY Simon wrote:
> 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).
> 
> On policy update, the latest SETXATTR_CHECK rule is the only one
> that apply, and other SETXATTR_CHECK rules are deleted.
> 
> Signed-off-by: Simon Thoby <simon.thoby@viveris.fr>

Sorry, I was just getting to this patch, when you re-posted the patch
set.  In the future, I'll make sure the responses are sent in quick
succession.

There are a number of assumptions related to the IMA policy:
- A builtin policy may be replaced by a custom policy.
- Depending on the Kconfig, the policy rules may not change be updated.
- Subsequent to replacing the builtin policy with a custom policy,
rules may only be appended, based on the Kconfig.

The locking around the policy rules is dependent on these assumptions. 
Removing policy rules is a major change that needs to be considered
carefully.  Why should "func=SETXATTR_CHECK"  be treated any
differently than any other policy rule?  How would an attestation
server know which setxattr rule was enabled at the time the file was
appraised?

thanks,

Mimi


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4 0/5] IMA: restrict the accepted digest algorithms for
  2021-07-27 16:33 [PATCH v4 0/5] IMA: restrict the accepted digest algorithms for THOBY Simon
                   ` (4 preceding siblings ...)
  2021-07-27 16:33 ` [PATCH v4 5/5] IMA: introduce a new policy option func=SETXATTR_CHECK THOBY Simon
@ 2021-07-27 17:47 ` Mimi Zohar
  5 siblings, 0 replies; 17+ messages in thread
From: Mimi Zohar @ 2021-07-27 17:47 UTC (permalink / raw)
  To: THOBY Simon, dmitry.kasatkin, linux-integrity, BARVAUX Didier

Hi Simon,

On Tue, 2021-07-27 at 16:33 +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, as was outlined in
> https://lore.kernel.org/linux-integrity/10dde047d76b447f32ca91356599be679b8a76e5.camel@linux.ibm.com/t/#m0f8127c6982ef94aa42f5cc13ea83b9f9000917e

Discussions should be summarized inline.  A reference to the thread
discussion may be included in a "Link" tag.  When including a "Link"
tag use the "permalink" as listed on the linux-integrity thread.  Once
the discussion is summarized, will a reference to the link really be
necessary?   Maybe fold in the subsequent paragraphs below.  Remember,
the entire cover letter or part of it, may be used as the git merge
text.

> 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', update the "ima_hash" parameter to
> 'new_algo', reboot, relabel all your files with 'new_algo', alter your
> policy_rule from 'appraise_hash=old_algo,new_algo' to
> 'appraise_hash=new_algo', reboot again and you're done.
> Agreed, it's quite a lot of churn - I don't know if this can be reduced -
> but this is technically doable.

Perhaps update the last line?

thanks,

Mimi


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4 1/5] IMA: remove the dependency on CRYPTO_MD5
  2021-07-27 16:33 ` [PATCH v4 1/5] IMA: remove the dependency on CRYPTO_MD5 THOBY Simon
@ 2021-07-27 17:57   ` Mimi Zohar
  0 siblings, 0 replies; 17+ messages in thread
From: Mimi Zohar @ 2021-07-27 17:57 UTC (permalink / raw)
  To: THOBY Simon, dmitry.kasatkin, linux-integrity, BARVAUX Didier

Hi Simon,

On Tue, 2021-07-27 at 16:33 +0000, THOBY Simon wrote:
<snip>

> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 287b90509006..d171764230b7 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -53,8 +53,10 @@ 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;
> +#ifdef CONFIG_CRYPTO_MD5

As much as possible ifdef's should be avoided in C code.   If
necessary, using the IS_ENABLED macro is preferred.

>  		} else if (strncmp(str, "md5", 3) == 0) {
>  			ima_hash_algo = HASH_ALGO_MD5;
> +#endif
>  		} else {
>  			pr_err("invalid hash algorithm \"%s\" for template \"%s\"",
>  				str, IMA_TEMPLATE_IMA_NAME);

thanks,

Mimi


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4 5/5] IMA: introduce a new policy option func=SETXATTR_CHECK
  2021-07-27 17:25   ` Mimi Zohar
@ 2021-07-27 17:58     ` THOBY Simon
  0 siblings, 0 replies; 17+ messages in thread
From: THOBY Simon @ 2021-07-27 17:58 UTC (permalink / raw)
  To: Mimi Zohar, dmitry.kasatkin, linux-integrity, BARVAUX Didier

Hello Mimi,

On 7/27/21 7:25 PM, Mimi Zohar wrote:
> Hi Simon,
> 
> On Tue, 2021-07-27 at 16:33 +0000, THOBY Simon wrote:
>> 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).
>>
>> On policy update, the latest SETXATTR_CHECK rule is the only one
>> that apply, and other SETXATTR_CHECK rules are deleted.
>>
>> Signed-off-by: Simon Thoby <simon.thoby@viveris.fr>
> 
> Sorry, I was just getting to this patch, when you re-posted the patch
> set.  In the future, I'll make sure the responses are sent in quick
> succession.
> 

The mistake is mine, I will wait more between submission next time (and it would also
be more reasonable of me to spam less the ML), sorry!

> There are a number of assumptions related to the IMA policy:
> - A builtin policy may be replaced by a custom policy.
> - Depending on the Kconfig, the policy rules may not change be updated.
> - Subsequent to replacing the builtin policy with a custom policy,
> rules may only be appended, based on the Kconfig.
> 
> The locking around the policy rules is dependent on these assumptions. 
> Removing policy rules is a major change that needs to be considered
> carefully.  Why should "func=SETXATTR_CHECK"  be treated any
> differently than any other policy rule?  How would an attestation
> server know which setxattr rule was enabled at the time the file was
> appraised?
> 

You're right, the "user convenience" gain is probably not much anyway, while the complexity burden
arising from deleting nodes from a list in reverse order with potentially multiple concurrent readers
is high, even with RCU doing most of the work.

I will drop that in a future revision of the patchset.

> thanks,
> 
> Mimi
> 

Many thanks for all the time you already spent on reviewing this work,
Have a nice evening (or day, with timezones and all),
Simon

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4 2/5] IMA: block writes of the security.ima xattr with unsupported algorithms
  2021-07-27 16:33 ` [PATCH v4 2/5] IMA: block writes of the security.ima xattr with unsupported algorithms THOBY Simon
@ 2021-07-27 20:32   ` Mimi Zohar
  2021-07-28  7:00     ` THOBY Simon
  0 siblings, 1 reply; 17+ messages in thread
From: Mimi Zohar @ 2021-07-27 20:32 UTC (permalink / raw)
  To: THOBY Simon, dmitry.kasatkin, linux-integrity, BARVAUX Didier; +Cc: Paul Moore

[Cc'ing Paul Moore]

Hi Simon,

On Tue, 2021-07-27 at 16:33 +0000, 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).
> 
> 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 | 51 ++++++++++++++++++++++++++-
>  1 file changed, 50 insertions(+), 1 deletion(-)
> 
> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> index ef9dcfce45d4..989da2fbf496 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -575,12 +575,55 @@ static void ima_reset_appraise_flags(struct inode *inode, int digsig)
>  		clear_bit(IMA_DIGSIG, &iint->atomic_flags);
>  }
>  
> +/**
> + * ima_setxattr_validate_hash_alg() - Block setxattr with invalid digests
> + * @dentry: file being altered

There isn't a one to one mapping betwen dentry and "file".   Perhaps
something like used in  __vfs_setxattr_locked:

*  @dentry: object to perform setxattr on

> + * @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.
> + */
> +int ima_setxattr_validate_hash_alg(struct dentry *dentry,
> +				   const void *xattr_value,
> +				   size_t xattr_value_len)

Should this be static?  If it is a local function, then it doesn't
really need to be prefixed with "ima_".  It could even be trimmed to
validate_hash_algo().

> +{
> +	int res = -EACCES;

I know there isn't any variable naming consistency.  The original code
used rc.  Subsequently it was replaced with result or ret.  Let's not
introduce yet another variable name here.

> +	char *path = NULL, *pathbuf = NULL;
> +	enum hash_algo dentry_hash;
> +
> +	dentry_hash = ima_get_hash_algo((struct evm_ima_xattr_data *)xattr_value,
> +				     xattr_value_len);

The hash algorithm is extracted from the xattr_value.  Perhaps rename
the variable to xattr_hash, xattr_hash_algo, or simply hash_algo?

> +
> +	if (likely(dentry_hash == ima_hash_algo
> +	    || crypto_has_alg(hash_algo_name[dentry_hash], 0, 0)))
> +		return 0;
> +
> +	pathbuf = kmalloc(PATH_MAX, GFP_KERNEL);
> +	/* no memory available ? no file path for you */

The comment here is unnecessary.  Avoid or limit comments inside a
function.  Refer to the section "8) Commenting" in
Documentation/process/coding-style.rst

> +	if (pathbuf)
> +		path = dentry_path(dentry, pathbuf, PATH_MAX);
> +
> +	/* disallow xattr writes with algorithms not built in the kernel */
> +	integrity_audit_msg(AUDIT_INTEGRITY_DATA, d_inode(dentry),
> +		path, "collect_data", "unavailable-hash-algorithm", res, 0);

This will emit an audit message without the filename when !path.  Is
this what you intended?

Now that this patch set is getting closer, examples of each new type of
audit message needs to be provided to the audit mailing list.  Paul,
any suggestions as to how/when to provide them?

> +
> +	kfree(pathbuf);
> +
> +	return res;
> +}
> +
>  int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
>  		       const void *xattr_value, size_t xattr_value_len)
>  {
>  	const struct evm_ima_xattr_data *xvalue = xattr_value;
>  	int digsig = 0;
> -	int result;
> +	int result, rc;
>  
>  	result = ima_protect_xattr(dentry, xattr_name, xattr_value,
>  				   xattr_value_len);
> @@ -592,6 +635,12 @@ 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)) {
> +		/* the user-supplied xattr must use an allowed hash algo */

With a function name containing "validate_hash_alg", this comment is
unnecessary.

thanks,

Mimi

> +		rc = ima_setxattr_validate_hash_alg(dentry, xattr_value,
> +							xattr_value_len);
> +		if (!rc)
> +			return rc;
> +
>  		ima_reset_appraise_flags(d_backing_inode(dentry), digsig);
>  		if (result == 1)
>  			result = 0;



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4 3/5] IMA: add support to restrict the hash algorithms used for file appraisal
  2021-07-27 16:33 ` [PATCH v4 3/5] IMA: add support to restrict the hash algorithms used for file appraisal THOBY Simon
@ 2021-07-27 20:38   ` Mimi Zohar
  0 siblings, 0 replies; 17+ messages in thread
From: Mimi Zohar @ 2021-07-27 20:38 UTC (permalink / raw)
  To: THOBY Simon, dmitry.kasatkin, linux-integrity, BARVAUX Didier

Hi Simon,

On Tue, 2021-07-27 at 16:33 +0000, 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.
> 
> Signed-off-by: Simon Thoby <simon.thoby@viveris.fr>

I haven't yet tested building the kernel after applying each patch. 
Assuming that it compiles properly: 

Reviewed-by:  Mimi Zohar <zohar@linux.ibm.com>


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4 4/5] IMA: add a policy option to restrict xattr hash algorithms on appraisal
  2021-07-27 16:33 ` [PATCH v4 4/5] IMA: add a policy option to restrict xattr hash algorithms on appraisal THOBY Simon
@ 2021-07-27 21:07   ` Mimi Zohar
  0 siblings, 0 replies; 17+ messages in thread
From: Mimi Zohar @ 2021-07-27 21:07 UTC (permalink / raw)
  To: THOBY Simon, dmitry.kasatkin, linux-integrity, BARVAUX Didier

On Tue, 2021-07-27 at 16:33 +0000, THOBY Simon wrote:
> The kernel have the ability to restrict the set of hash algorithms

^kernel has

> it accepts for the security.ima xattr when it appraises files.
> 
> Define a new IMA policy rule option "appraise_hash=",
> using the mentionned mechanism to expose a user-toggable policy

^mentioned

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

> ---

<snip>

> @@ -1204,6 +1228,7 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
>  	entry->uid_op = &uid_eq;
>  	entry->fowner_op = &uid_eq;
>  	entry->action = UNKNOWN;
> +	entry->allowed_hashes = 0;

"entry" is zeroed when allocated.


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4 2/5] IMA: block writes of the security.ima xattr with unsupported algorithms
  2021-07-27 20:32   ` Mimi Zohar
@ 2021-07-28  7:00     ` THOBY Simon
  2021-07-28 12:43       ` Mimi Zohar
  0 siblings, 1 reply; 17+ messages in thread
From: THOBY Simon @ 2021-07-28  7:00 UTC (permalink / raw)
  To: Mimi Zohar, dmitry.kasatkin, linux-integrity, BARVAUX Didier; +Cc: Paul Moore

Hi Mimi,

On 7/27/21 10:32 PM, Mimi Zohar wrote:
> [Cc'ing Paul Moore]
> 
> Hi Simon,
> 

[snip]

> 
>> +
>> +	if (likely(dentry_hash == ima_hash_algo
>> +	    || crypto_has_alg(hash_algo_name[dentry_hash], 0, 0)))
>> +		return 0;
>> +
>> +	pathbuf = kmalloc(PATH_MAX, GFP_KERNEL);
>> +	/* no memory available ? no file path for you */
> 
> The comment here is unnecessary.  Avoid or limit comments inside a
> function.  Refer to the section "8) Commenting" in
> Documentation/process/coding-style.rst
> 
>> +	if (pathbuf)
>> +		path = dentry_path(dentry, pathbuf, PATH_MAX);
>> +
>> +	/* disallow xattr writes with algorithms not built in the kernel */
>> +	integrity_audit_msg(AUDIT_INTEGRITY_DATA, d_inode(dentry),
>> +		path, "collect_data", "unavailable-hash-algorithm", res, 0);
> 
> This will emit an audit message without the filename when !path.  Is
> this what you intended?
> 

This is what I was clumsily trying to explain in the previous comment: if we cannot
allocate memory for a file path, I thought it best to log the audit message without
the path than fail with a -ENOMEM (auditing will also try to allocate a memory buffer
too, but a bit smaller, and memory could have been reclaimed between the two calls,
so the auditing operation may succeed).

Of course I could also return -ENOMEM, and it would happily propagate back to the user.

What do you think ?

Thanks,
Simon

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4 2/5] IMA: block writes of the security.ima xattr with unsupported algorithms
  2021-07-28  7:00     ` THOBY Simon
@ 2021-07-28 12:43       ` Mimi Zohar
  2021-07-28 12:53         ` THOBY Simon
  0 siblings, 1 reply; 17+ messages in thread
From: Mimi Zohar @ 2021-07-28 12:43 UTC (permalink / raw)
  To: THOBY Simon, dmitry.kasatkin, linux-integrity, BARVAUX Didier; +Cc: Paul Moore

Hi Simon,

On Wed, 2021-07-28 at 07:00 +0000, THOBY Simon wrote: 
> >> +
> >> +	if (likely(dentry_hash == ima_hash_algo
> >> +	    || crypto_has_alg(hash_algo_name[dentry_hash], 0, 0)))
> >> +		return 0;
> >> +
> >> +	pathbuf = kmalloc(PATH_MAX, GFP_KERNEL);
> >> +	/* no memory available ? no file path for you */
> > 
> > The comment here is unnecessary.  Avoid or limit comments inside a
> > function.  Refer to the section "8) Commenting" in
> > Documentation/process/coding-style.rst
> > 
> >> +	if (pathbuf)
> >> +		path = dentry_path(dentry, pathbuf, PATH_MAX);
> >> +
> >> +	/* disallow xattr writes with algorithms not built in the kernel */
> >> +	integrity_audit_msg(AUDIT_INTEGRITY_DATA, d_inode(dentry),
> >> +		path, "collect_data", "unavailable-hash-algorithm", res, 0);
> > 
> > This will emit an audit message without the filename when !path.  Is
> > this what you intended?
> > 
> 
> This is what I was clumsily trying to explain in the previous comment: if we cannot
> allocate memory for a file path, I thought it best to log the audit message without
> the path than fail with a -ENOMEM (auditing will also try to allocate a memory buffer
> too, but a bit smaller, and memory could have been reclaimed between the two calls,
> so the auditing operation may succeed).
> 
> Of course I could also return -ENOMEM, and it would happily propagate back to the user.
> 
> What do you think ?

Memory pressure isn't the reason for preventing the xattr write.  It's
the reason for not being able to audit the setxattr failure.

thanks,

Mimi


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4 2/5] IMA: block writes of the security.ima xattr with unsupported algorithms
  2021-07-28 12:43       ` Mimi Zohar
@ 2021-07-28 12:53         ` THOBY Simon
  2021-07-28 13:09           ` Mimi Zohar
  0 siblings, 1 reply; 17+ messages in thread
From: THOBY Simon @ 2021-07-28 12:53 UTC (permalink / raw)
  To: Mimi Zohar, dmitry.kasatkin, linux-integrity, BARVAUX Didier; +Cc: Paul Moore

Hi Mimi,

On 7/28/21 2:43 PM, Mimi Zohar wrote:
> Hi Simon,
> 
> On Wed, 2021-07-28 at 07:00 +0000, THOBY Simon wrote: 
>>>> +
>>>> +	if (likely(dentry_hash == ima_hash_algo
>>>> +	    || crypto_has_alg(hash_algo_name[dentry_hash], 0, 0)))
>>>> +		return 0;
>>>> +
>>>> +	pathbuf = kmalloc(PATH_MAX, GFP_KERNEL);
>>>> +	/* no memory available ? no file path for you */
>>>
>>> The comment here is unnecessary.  Avoid or limit comments inside a
>>> function.  Refer to the section "8) Commenting" in
>>> Documentation/process/coding-style.rst
>>>
>>>> +	if (pathbuf)
>>>> +		path = dentry_path(dentry, pathbuf, PATH_MAX);
>>>> +
>>>> +	/* disallow xattr writes with algorithms not built in the kernel */
>>>> +	integrity_audit_msg(AUDIT_INTEGRITY_DATA, d_inode(dentry),
>>>> +		path, "collect_data", "unavailable-hash-algorithm", res, 0);
>>>
>>> This will emit an audit message without the filename when !path.  Is
>>> this what you intended?
>>>
>>
>> This is what I was clumsily trying to explain in the previous comment: if we cannot
>> allocate memory for a file path, I thought it best to log the audit message without
>> the path than fail with a -ENOMEM (auditing will also try to allocate a memory buffer
>> too, but a bit smaller, and memory could have been reclaimed between the two calls,
>> so the auditing operation may succeed).
>>
>> Of course I could also return -ENOMEM, and it would happily propagate back to the user.
>>
>> What do you think ?
> 
> Memory pressure isn't the reason for preventing the xattr write.  It's
> the reason for not being able to audit the setxattr failure.
> 

I completely agree with you, but I'm not quite sure I get the action you want to take
from there.
Does this mean you prefer the mechanism already implemented in this patch (not printing
the file path and trying to audit the setxattr failure anyway)?

> thanks,
> 
> Mimi
> 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4 2/5] IMA: block writes of the security.ima xattr with unsupported algorithms
  2021-07-28 12:53         ` THOBY Simon
@ 2021-07-28 13:09           ` Mimi Zohar
  0 siblings, 0 replies; 17+ messages in thread
From: Mimi Zohar @ 2021-07-28 13:09 UTC (permalink / raw)
  To: THOBY Simon, dmitry.kasatkin, linux-integrity, BARVAUX Didier; +Cc: Paul Moore

On Wed, 2021-07-28 at 12:53 +0000, THOBY Simon wrote:
> Hi Mimi,
> 
> On 7/28/21 2:43 PM, Mimi Zohar wrote:
> > Hi Simon,
> > 
> > On Wed, 2021-07-28 at 07:00 +0000, THOBY Simon wrote: 
> >>>> +
> >>>> +	if (likely(dentry_hash == ima_hash_algo
> >>>> +	    || crypto_has_alg(hash_algo_name[dentry_hash], 0, 0)))
> >>>> +		return 0;
> >>>> +
> >>>> +	pathbuf = kmalloc(PATH_MAX, GFP_KERNEL);
> >>>> +	/* no memory available ? no file path for you */
> >>>
> >>> The comment here is unnecessary.  Avoid or limit comments inside a
> >>> function.  Refer to the section "8) Commenting" in
> >>> Documentation/process/coding-style.rst
> >>>
> >>>> +	if (pathbuf)
> >>>> +		path = dentry_path(dentry, pathbuf, PATH_MAX);
> >>>> +
> >>>> +	/* disallow xattr writes with algorithms not built in the kernel */
> >>>> +	integrity_audit_msg(AUDIT_INTEGRITY_DATA, d_inode(dentry),
> >>>> +		path, "collect_data", "unavailable-hash-algorithm", res, 0);
> >>>
> >>> This will emit an audit message without the filename when !path.  Is
> >>> this what you intended?
> >>>
> >>
> >> This is what I was clumsily trying to explain in the previous comment: if we cannot
> >> allocate memory for a file path, I thought it best to log the audit message without
> >> the path than fail with a -ENOMEM (auditing will also try to allocate a memory buffer
> >> too, but a bit smaller, and memory could have been reclaimed between the two calls,
> >> so the auditing operation may succeed).
> >>
> >> Of course I could also return -ENOMEM, and it would happily propagate back to the user.
> >>
> >> What do you think ?
> > 
> > Memory pressure isn't the reason for preventing the xattr write.  It's
> > the reason for not being able to audit the setxattr failure.

Return the existing errno, not -ENOMEM.

> 
> I completely agree with you, but I'm not quite sure I get the action you want to take
> from there.
> Does this mean you prefer the mechanism already implemented in this patch (not printing
> the file path and trying to audit the setxattr failure anyway)?

Under memory pressure, I don't think partially auditing the setxattr,
without the pathname, makes sense.   Not being able to audit the
setxattr failure shouldn't affect the returned result.  It would be the
same in either case.

   if (pathbuf) {
      < audit message >
      < free pathbuf >
   }

   return <failure>

thanks,

Mimi



^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2021-07-28 13:12 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-27 16:33 [PATCH v4 0/5] IMA: restrict the accepted digest algorithms for THOBY Simon
2021-07-27 16:33 ` [PATCH v4 1/5] IMA: remove the dependency on CRYPTO_MD5 THOBY Simon
2021-07-27 17:57   ` Mimi Zohar
2021-07-27 16:33 ` [PATCH v4 2/5] IMA: block writes of the security.ima xattr with unsupported algorithms THOBY Simon
2021-07-27 20:32   ` Mimi Zohar
2021-07-28  7:00     ` THOBY Simon
2021-07-28 12:43       ` Mimi Zohar
2021-07-28 12:53         ` THOBY Simon
2021-07-28 13:09           ` Mimi Zohar
2021-07-27 16:33 ` [PATCH v4 3/5] IMA: add support to restrict the hash algorithms used for file appraisal THOBY Simon
2021-07-27 20:38   ` Mimi Zohar
2021-07-27 16:33 ` [PATCH v4 4/5] IMA: add a policy option to restrict xattr hash algorithms on appraisal THOBY Simon
2021-07-27 21:07   ` Mimi Zohar
2021-07-27 16:33 ` [PATCH v4 5/5] IMA: introduce a new policy option func=SETXATTR_CHECK THOBY Simon
2021-07-27 17:25   ` Mimi Zohar
2021-07-27 17:58     ` THOBY Simon
2021-07-27 17:47 ` [PATCH v4 0/5] IMA: restrict the accepted digest algorithms for Mimi Zohar

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).