linux-integrity.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/4] IMA: restrict the accepted digest algorithms for the security.ima xattr
@ 2021-07-27 10:23 THOBY Simon
  2021-07-27 10:23 ` [PATCH v3 1/4] IMA: block writes of the security.ima xattr with unsupported algorithms THOBY Simon
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: THOBY Simon @ 2021-07-27 10:23 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 v2:
- remove the SecureBoot-specific behavior
- 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
- change the order of the patches to be bisect-safe while retaining
  the guarantee that a policy cannot be accepted but not enforced

Changelog since v1:
- Remove the two boot parameters
- filter out hash algorithms not compiled in the kernel
  on xattr writes
- 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

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

 Documentation/ABI/testing/ima_policy  |  15 ++-
 security/integrity/ima/ima.h          |  10 +-
 security/integrity/ima/ima_api.c      |   6 +-
 security/integrity/ima/ima_appraise.c |  65 ++++++++++-
 security/integrity/ima/ima_main.c     |  22 +++-
 security/integrity/ima/ima_policy.c   | 153 +++++++++++++++++++++++++-
 6 files changed, 254 insertions(+), 17 deletions(-)

-- 
2.31.1

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

* [PATCH v3 1/4] IMA: block writes of the security.ima xattr with unsupported algorithms
  2021-07-27 10:23 [PATCH v3 0/4] IMA: restrict the accepted digest algorithms for the security.ima xattr THOBY Simon
@ 2021-07-27 10:23 ` THOBY Simon
  2021-07-27 14:04   ` Mimi Zohar
  2021-07-27 10:23 ` [PATCH v3 2/4] IMA: add support to restrict the hash algorithms used for file appraisal THOBY Simon
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 9+ messages in thread
From: THOBY Simon @ 2021-07-27 10:23 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).

This patch prevents 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.

The idea behind this patch is that a user can disable weak hashes
when building the kernel, and thereby prevent their use in IMA
(these hash algorithms will not only be blocked for setxattr per
this patch, but they also won't be allowed for measurement/appraisal
either as the kernel isn't able to measure files hashed with them).

Note however that CONFIG_IMA depends on CONFIG_CRYPTO_MD5 and
CONFIG_CRYPTO_SHA1, which hampers the security benefits of this
measure.

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

diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index ef9dcfce45d4..b5b11f5ec90a 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -575,6 +575,42 @@ static void ima_reset_appraise_flags(struct inode *inode, int digsig)
 		clear_bit(IMA_DIGSIG, &iint->atomic_flags);
 }
 
+/**
+ * ima_setxattr_validate_hash_alg
+ *
+ * Called when the user tries to write the security.ima xattr.
+ * The xattr value maps to the hash algorithm hash_alg, and this function
+ * returns whether this setxattr should be allowed, emitting an audit
+ * message if necessary.
+ */
+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 hash_alg =
+		ima_get_hash_algo((struct evm_ima_xattr_data *)xattr_value,
+				  xattr_value_len);
+
+	/* disallow xattr writes with algorithms not built in the kernel */
+	if (likely(hash_alg == ima_hash_algo
+	    || crypto_has_alg(hash_algo_name[hash_alg], 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);
+
+	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)
 {
@@ -592,6 +628,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 */
+		int rc = ima_setxattr_validate_hash_alg(dentry, xattr_value,
+							xattr_value_len);
+		if (rc != 0)
+			return rc;
+
 		ima_reset_appraise_flags(d_backing_inode(dentry), digsig);
 		if (result == 1)
 			result = 0;
-- 
2.31.1

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

* [PATCH v3 2/4] IMA: add support to restrict the hash algorithms used for file appraisal
  2021-07-27 10:23 [PATCH v3 0/4] IMA: restrict the accepted digest algorithms for the security.ima xattr THOBY Simon
  2021-07-27 10:23 ` [PATCH v3 1/4] IMA: block writes of the security.ima xattr with unsupported algorithms THOBY Simon
@ 2021-07-27 10:23 ` THOBY Simon
  2021-07-27 15:29   ` Mimi Zohar
  2021-07-27 10:23 ` [PATCH v3 3/4] IMA: add a policy option to restrict xattr hash algorithms on appraisal THOBY Simon
  2021-07-27 10:23 ` [PATCH v3 4/4] IMA: introduce a new policy option func=SETXATTR_CHECK THOBY Simon
  3 siblings, 1 reply; 9+ messages in thread
From: THOBY Simon @ 2021-07-27 10:23 UTC (permalink / raw)
  To: zohar, dmitry.kasatkin, linux-integrity, BARVAUX Didier; +Cc: THOBY Simon

This patch plugs in support for restricting the hash algorithms
accepted for protecting the security.ima xattr when appraising
files.

Each ima policy rule can have a list of allowed hash algorithms,
and a file matching the policy but whose IMA hash is
not explicitly in the list will not pass appraisal.

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     | 22 +++++++++++++++++++---
 security/integrity/ima/ima_policy.c   | 18 ++++++++++++++++--
 5 files changed, 45 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 b5b11f5ec90a..6d121819ae9e 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 287b90509006..d7289164807e 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -210,6 +210,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
 	int xattr_len = 0;
 	bool violation_check;
 	enum hash_algo hash_algo;
+	unsigned int appraisal_allowed_hashes = 0;
 
 	if (!ima_policy_flag || !S_ISREG(inode->i_mode))
 		return 0;
@@ -219,7 +220,8 @@ static int process_measurement(struct file *file, const struct cred *cred,
 	 * Included is the appraise submask.
 	 */
 	action = ima_get_action(file_mnt_user_ns(file), inode, cred, secid,
-				mask, func, &pcr, &template_desc, NULL);
+				mask, func, &pcr, &template_desc, NULL,
+				&appraisal_allowed_hashes);
 	violation_check = ((func == FILE_CHECK || func == MMAP_CHECK) &&
 			   (ima_policy_flag & IMA_MEASURE));
 	if (!action && !violation_check)
@@ -327,6 +329,20 @@ static int process_measurement(struct file *file, const struct cred *cred,
 
 	hash_algo = ima_get_hash_algo(xattr_value, xattr_len);
 
+	/* Ensure that the digest was generated using an allowed algorithm */
+	if (appraisal_allowed_hashes &&
+	    !(appraisal_allowed_hashes & (1U << hash_algo))) {
+		rc = -EACCES;
+
+		if (!pathbuf)	/* ima_rdwr_violation possibly pre-fetched */
+			pathname = ima_d_path(&file->f_path, &pathbuf, filename);
+
+		integrity_audit_msg(AUDIT_INTEGRITY_DATA, file_inode(file),
+			pathname, "collect_data", "forbidden-hash-algorithm", rc, 0);
+
+		goto out_locked;
+	}
+
 	rc = ima_collect_measurement(iint, file, buf, size, hash_algo, modsig);
 	if (rc != 0 && rc != -EBADF && rc != -EINVAL)
 		goto out_locked;
@@ -433,7 +449,7 @@ int ima_file_mprotect(struct vm_area_struct *vma, unsigned long prot)
 	inode = file_inode(vma->vm_file);
 	action = ima_get_action(file_mnt_user_ns(vma->vm_file), inode,
 				current_cred(), secid, MAY_EXEC, MMAP_CHECK,
-				&pcr, &template, NULL);
+				&pcr, &template, NULL, NULL);
 
 	/* Is the mmap'ed file in policy? */
 	if (!(action & (IMA_MEASURE | IMA_APPRAISE_SUBMASK)))
@@ -882,7 +898,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	[flat|nested] 9+ messages in thread

* [PATCH v3 3/4] IMA: add a policy option to restrict xattr hash algorithms on appraisal
  2021-07-27 10:23 [PATCH v3 0/4] IMA: restrict the accepted digest algorithms for the security.ima xattr THOBY Simon
  2021-07-27 10:23 ` [PATCH v3 1/4] IMA: block writes of the security.ima xattr with unsupported algorithms THOBY Simon
  2021-07-27 10:23 ` [PATCH v3 2/4] IMA: add support to restrict the hash algorithms used for file appraisal THOBY Simon
@ 2021-07-27 10:23 ` THOBY Simon
  2021-07-27 16:24   ` Mimi Zohar
  2021-07-27 10:23 ` [PATCH v3 4/4] IMA: introduce a new policy option func=SETXATTR_CHECK THOBY Simon
  3 siblings, 1 reply; 9+ messages in thread
From: THOBY Simon @ 2021-07-27 10:23 UTC (permalink / raw)
  To: zohar, dmitry.kasatkin, linux-integrity, BARVAUX Didier; +Cc: THOBY Simon

This patch defines a new IMA policy rule option "appraise_hash=",
that restricts the hash algorithms accepted for the extended attribute
security.ima when appraising.

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  | 72 ++++++++++++++++++++++++++--
 2 files changed, 74 insertions(+), 4 deletions(-)

diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index 070779e8d836..365e4c91719e 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=] [keyrings=] [appraise_hash=]
 		  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..a7f110cbbff0 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -92,6 +92,7 @@ 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
@@ -962,7 +963,7 @@ enum {
 	Opt_uid_lt, Opt_euid_lt, Opt_fowner_lt,
 	Opt_appraise_type, Opt_appraise_flag,
 	Opt_permit_directio, Opt_pcr, Opt_template, Opt_keyrings,
-	Opt_label, Opt_err
+	Opt_label, Opt_appraise_hash, Opt_err
 };
 
 static const match_table_t policy_tokens = {
@@ -1000,6 +1001,7 @@ static const match_table_t policy_tokens = {
 	{Opt_template, "template=%s"},
 	{Opt_keyrings, "keyrings=%s"},
 	{Opt_label, "label=%s"},
+	{Opt_appraise_hash, "appraise_hash=%s"},
 	{Opt_err, NULL}
 };
 
@@ -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,27 @@ 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;
+	char *token;
+
+	while ((token = strsep(&arg, ",")) != NULL) {
+		int 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 +1227,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;
@@ -1556,6 +1580,23 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 						 &(template_desc->fields),
 						 &(template_desc->num_fields));
 			entry->template = template_desc;
+			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);
+			if (!entry->allowed_hashes) {
+				result = -EINVAL;
+				break;
+			}
+
+			entry->flags |= IMA_VALIDATE_HASH;
+
 			break;
 		case Opt_err:
 			ima_log_string(ab, "UNKNOWN", p);
@@ -1714,6 +1755,25 @@ 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;
+	bool first = true;
+
+	for (idx = 0; idx < HASH_ALGO__LAST; idx++) {
+		if (!(allowed_hashes & (1U << idx)))
+			continue;
+
+		if (!first)
+			seq_puts(m, ",");
+		first = false;
+
+		seq_puts(m, hash_algo_name[idx]);
+	}
+
+}
+
 int ima_policy_show(struct seq_file *m, void *v)
 {
 	struct ima_rule_entry *entry = v;
@@ -1825,6 +1885,12 @@ int ima_policy_show(struct seq_file *m, void *v)
 		seq_puts(m, " ");
 	}
 
+	if (entry->flags & IMA_VALIDATE_HASH) {
+		seq_puts(m, "appraise_hash=");
+		ima_policy_show_appraise_hash(m, entry->allowed_hashes);
+		seq_puts(m, " ");
+	}
+
 	for (i = 0; i < MAX_LSM_RULES; i++) {
 		if (entry->lsm[i].rule) {
 			switch (i) {
-- 
2.31.1

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

* [PATCH v3 4/4] IMA: introduce a new policy option func=SETXATTR_CHECK
  2021-07-27 10:23 [PATCH v3 0/4] IMA: restrict the accepted digest algorithms for the security.ima xattr THOBY Simon
                   ` (2 preceding siblings ...)
  2021-07-27 10:23 ` [PATCH v3 3/4] IMA: add a policy option to restrict xattr hash algorithms on appraisal THOBY Simon
@ 2021-07-27 10:23 ` THOBY Simon
  2021-07-27 11:24   ` THOBY Simon
  3 siblings, 1 reply; 9+ messages in thread
From: THOBY Simon @ 2021-07-27 10:23 UTC (permalink / raw)
  To: zohar, dmitry.kasatkin, linux-integrity, BARVAUX Didier; +Cc: THOBY Simon

This patch defines a new value for the ima policy option 'func'.
That value restricts 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 | 28 +++++++++---
 security/integrity/ima/ima_policy.c   | 63 +++++++++++++++++++++++++++
 4 files changed, 98 insertions(+), 6 deletions(-)

diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index 365e4c91719e..c05a21007272 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -30,9 +30,10 @@ Description:
 				[appraise_flag=] [keyrings=] [appraise_hash=]
 		  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 6d121819ae9e..81c4259261bf 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -590,14 +590,32 @@ 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 hash_alg =
 		ima_get_hash_algo((struct evm_ima_xattr_data *)xattr_value,
 				  xattr_value_len);
+	unsigned int allowed_hashes = atomic_read(
+			&ima_setxattr_allowed_hash_algorithms);
 
-	/* disallow xattr writes with algorithms not built in the kernel */
-	if (likely(hash_alg == ima_hash_algo
-	    || crypto_has_alg(hash_algo_name[hash_alg], 0, 0)))
-		return 0;
+	if (allowed_hashes) {
+		/* success if the algorithm is whitelisted in the ima policy */
+		if (allowed_hashes & (1U << hash_alg))
+			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(hash_alg == ima_hash_algo))
+			return 0;
+
+		/* allow any xattr using an algorithm built in the kernel */
+		if (crypto_has_alg(hash_algo_name[hash_alg], 0, 0))
+			return 0;
+	}
 
 	pathbuf = kmalloc(PATH_MAX, GFP_KERNEL);
 	/* no memory available ? no file path for you */
@@ -605,7 +623,7 @@ int ima_setxattr_validate_hash_alg(struct dentry *dentry,
 		path = dentry_path(dentry, pathbuf, PATH_MAX);
 
 	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 a7f110cbbff0..dd5b2b00aa88 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
@@ -915,6 +917,49 @@ int ima_check_policy(void)
 	return 0;
 }
 
+/** ima_update_setxattr_allowed_hash_algorithms - cleanup SETXATTR_CHECK rules
+ * in the new ruleset
+ *
+ * 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;
+
+	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(&entry->list);
+			ima_free_rule(entry);
+		} 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;
+		}
+	}
+
+}
+
 /**
  * ima_update_policy - update default_rules with new measure rules
  *
@@ -932,9 +977,12 @@ 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;
+		atomic_xchg(&ima_setxattr_allowed_hash_algorithms, 0);
 
 		/*
 		 * IMA architecture specific policy rules are specified
@@ -1176,6 +1224,19 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
 		if (ima_rule_contains_lsm_cond(entry))
 			return false;
 
+		break;
+	case SETXATTR_CHECK:
+		/* any action other than APPRAISE is unsupported */
+		if (entry->action != APPRAISE)
+			return false;
+
+		/*
+		 * full policies are not supported, they would have too
+		 * much of a performance impact
+		 */
+		if (entry->flags & ~(IMA_FUNC | IMA_VALIDATE_HASH))
+			return false;
+
 		break;
 	default:
 		return false;
@@ -1332,6 +1393,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	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 4/4] IMA: introduce a new policy option func=SETXATTR_CHECK
  2021-07-27 10:23 ` [PATCH v3 4/4] IMA: introduce a new policy option func=SETXATTR_CHECK THOBY Simon
@ 2021-07-27 11:24   ` THOBY Simon
  0 siblings, 0 replies; 9+ messages in thread
From: THOBY Simon @ 2021-07-27 11:24 UTC (permalink / raw)
  To: zohar, dmitry.kasatkin, linux-integrity, BARVAUX Didier

Sorry, I forgot to run checkpatch on this one prior to sending :/
There was a few style issues in this patch, so here is the fixed version:

This patch defines a new value for the ima policy option 'func'.
That value restricts 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 | 28 +++++++++---
 security/integrity/ima/ima_policy.c   | 64 +++++++++++++++++++++++++++
 4 files changed, 99 insertions(+), 6 deletions(-)

diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index 365e4c91719e..c05a21007272 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -30,9 +30,10 @@ Description:
 				[appraise_flag=] [keyrings=] [appraise_hash=]
 		  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 6d121819ae9e..f3d52bbfdf0f 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -590,14 +590,32 @@ 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 hash_alg =
 		ima_get_hash_algo((struct evm_ima_xattr_data *)xattr_value,
 				  xattr_value_len);
+	unsigned int allowed_hashes = atomic_read(
+			&ima_setxattr_allowed_hash_algorithms);
 
-	/* disallow xattr writes with algorithms not built in the kernel */
-	if (likely(hash_alg == ima_hash_algo
-	    || crypto_has_alg(hash_algo_name[hash_alg], 0, 0)))
-		return 0;
+	if (allowed_hashes) {
+		/* success if the algorithm is whitelisted in the ima policy */
+		if (allowed_hashes & (1U << hash_alg))
+			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(hash_alg == ima_hash_algo))
+			return 0;
+
+		/* allow any xattr using an algorithm built in the kernel */
+		if (crypto_has_alg(hash_algo_name[hash_alg], 0, 0))
+			return 0;
+	}
 
 	pathbuf = kmalloc(PATH_MAX, GFP_KERNEL);
 	/* no memory available ? no file path for you */
@@ -605,7 +623,7 @@ int ima_setxattr_validate_hash_alg(struct dentry *dentry,
 		path = dentry_path(dentry, pathbuf, PATH_MAX);
 
 	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 a7f110cbbff0..cfebf8b01cc0 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
@@ -915,6 +917,50 @@ int ima_check_policy(void)
 	return 0;
 }
 
+/** ima_update_setxattr_allowed_hash_algorithms - cleanup SETXATTR_CHECK rules
+ * in the new ruleset
+ *
+ * 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;
+
+	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(&entry->list);
+			ima_free_rule(entry);
+		} 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;
+		}
+	}
+
+}
+
 /**
  * ima_update_policy - update default_rules with new measure rules
  *
@@ -932,9 +978,12 @@ 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;
+		atomic_xchg(&ima_setxattr_allowed_hash_algorithms, 0);
 
 		/*
 		 * IMA architecture specific policy rules are specified
@@ -1176,6 +1225,19 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
 		if (ima_rule_contains_lsm_cond(entry))
 			return false;
 
+		break;
+	case SETXATTR_CHECK:
+		/* any action other than APPRAISE is unsupported */
+		if (entry->action != APPRAISE)
+			return false;
+
+		/*
+		 * full policies are not supported, they would have too
+		 * much of a performance impact
+		 */
+		if (entry->flags & ~(IMA_FUNC | IMA_VALIDATE_HASH))
+			return false;
+
 		break;
 	default:
 		return false;
@@ -1332,6 +1394,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	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 1/4] IMA: block writes of the security.ima xattr with unsupported algorithms
  2021-07-27 10:23 ` [PATCH v3 1/4] IMA: block writes of the security.ima xattr with unsupported algorithms THOBY Simon
@ 2021-07-27 14:04   ` Mimi Zohar
  0 siblings, 0 replies; 9+ messages in thread
From: Mimi Zohar @ 2021-07-27 14:04 UTC (permalink / raw)
  To: THOBY Simon, dmitry.kasatkin, linux-integrity, BARVAUX Didier

Hi Simon,

On Tue, 2021-07-27 at 10:23 +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).
> 
> This patch prevents 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.
> 

Instead of "This patch", start with "Prevent".

From Documentation/process/submitting-patches.rst:
   Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
   instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
   to do frotz", as if you are giving orders to the codebase to change
   its behaviour.

> The idea behind this patch is that a user can disable weak hashes
> when building the kernel, and thereby prevent their use in IMA
> (these hash algorithms will not only be blocked for setxattr per
> this patch, but they also won't be allowed for measurement/appraisal
> either as the kernel isn't able to measure files hashed with them).

The motivation for this patch set is described in the cover letter,
which may be included as the merge message.  The above paragraph isn't
needed here in this particular patch description.

> Note however that CONFIG_IMA depends on CONFIG_CRYPTO_MD5 and
> CONFIG_CRYPTO_SHA1, which hampers the security benefits of this
> measure.

Unlike SHA1, which is still being used in the IMA measurement list,
there is no reason to automatically select MD5 in the Kconfig.  As a
separate patch, probably the first in this series so that it could be
backported, please remove the CRYPTO_MD5 select.  

> 
> Signed-off-by: Simon Thoby <simon.thoby@viveris.fr>
> ---
>  security/integrity/ima/ima_appraise.c | 42 +++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> index ef9dcfce45d4..b5b11f5ec90a 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -575,6 +575,42 @@ static void ima_reset_appraise_flags(struct inode *inode, int digsig)
>  		clear_bit(IMA_DIGSIG, &iint->atomic_flags);
>  }
>  
> +/**
> + * ima_setxattr_validate_hash_alg
> + *

"kernel-doc" has a specific format.   Please refer to the section
"Function documentation" in Documentation/doc-guide/kernel-doc.rst.

> + * Called when the user tries to write the security.ima xattr.
> + * The xattr value maps to the hash algorithm hash_alg, and this function
> + * returns whether this setxattr should be allowed, emitting an audit
> + * message if necessary.
> + */

This is called by an LSM/IMA hook.  On success return 0.  On failure,
return errno.

> +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 hash_alg =
> +		ima_get_hash_algo((struct evm_ima_xattr_data *)xattr_value,
> +				  xattr_value_len);

Programmatically it is the same to define a variable and assign it on
the same line, but in this case, it might be cleaner to split it up.

> +
> +	/* disallow xattr writes with algorithms not built in the kernel */
> +	if (likely(hash_alg == ima_hash_algo
> +	    || crypto_has_alg(hash_algo_name[hash_alg], 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);
> +
> +	integrity_audit_msg(AUDIT_INTEGRITY_DATA, d_inode(dentry),
> +		path, "collect_data", "unavailable-hash-algorithm", res, 0);
> +

The comment is applicable to integrity_audit_msg().  Why not move it
prior to integrity_audit_msg().

> +	kfree(pathbuf);
> +
> +	return res;
> +}
> +
>  int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
>  		       const void *xattr_value, size_t xattr_value_len)
>  {
> @@ -592,6 +628,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 */
> +		int rc = ima_setxattr_validate_hash_alg(dentry, xattr_value,
> +							xattr_value_len);

Variables should be defined at the beginning of the function.

> +		if (rc != 0)
> +			return rc;
> +

"rc" should be 0 or < 1.

thanks,

Mimi

>  		ima_reset_appraise_flags(d_backing_inode(dentry), digsig);
>  		if (result == 1)
>  			result = 0;



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

* Re: [PATCH v3 2/4] IMA: add support to restrict the hash algorithms used for file appraisal
  2021-07-27 10:23 ` [PATCH v3 2/4] IMA: add support to restrict the hash algorithms used for file appraisal THOBY Simon
@ 2021-07-27 15:29   ` Mimi Zohar
  0 siblings, 0 replies; 9+ messages in thread
From: Mimi Zohar @ 2021-07-27 15:29 UTC (permalink / raw)
  To: THOBY Simon, dmitry.kasatkin, linux-integrity, BARVAUX Didier

Hi Simon,

On Tue, 2021-07-27 at 10:23 +0000, THOBY Simon wrote:
> This patch plugs in support for restricting the hash algorithms
> accepted for protecting the security.ima xattr when appraising
> files.

The first sentence should provide the motivation for the patch.  For
example, start this paragraph by saying that the hash algorithms are
currently not restricted.  Then continue with "Restrict ..." or maybe
"Provide the plumbing to restrict ...".

> 
> Each ima policy rule can have a list of allowed hash algorithms,
> and a file matching the policy but whose IMA hash is
> not explicitly in the list will not pass appraisal.

Belongs in the patch associated with the IMA policy "appraise_hash"
rule option.
> 
> 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.

Instead of phrasing this paragraph in the negative, the content could
be combined with the first paragraph in the positive.   For example,
"Restrict the permitted set of hash algorithms used for verifying file
signatures stored in security.ima xattr."

> 
> Signed-off-by: Simon Thoby <simon.thoby@viveris.fr>
> ---

<snip>

> @@ -327,6 +329,20 @@ static int process_measurement(struct file *file, const struct cred *cred,
>  
>  	hash_algo = ima_get_hash_algo(xattr_value, xattr_len);
>  
> +	/* Ensure that the digest was generated using an allowed algorithm */
> +	if (appraisal_allowed_hashes &&
> +	    !(appraisal_allowed_hashes & (1U << hash_algo))) {
> +		rc = -EACCES;
> +
> +		if (!pathbuf)	/* ima_rdwr_violation possibly pre-fetched */
> +			pathname = ima_d_path(&file->f_path, &pathbuf, filename);
> +
> +		integrity_audit_msg(AUDIT_INTEGRITY_DATA, file_inode(file),
> +			pathname, "collect_data", "forbidden-hash-algorithm", rc, 0);
> +
> +		goto out_locked;
> +	}
> +

This doesn't look like the right place for checking the hash algorithm.
IMA may be configured differently on different systems.  Some might
only enable measurement without appraisal, appraisal without
measurement, or both.  Only appraisal returns a failure and prevents
read, execute, mmap, etc.   The hash algorithm check probably should be
deferred to appraisal.  Placing the test here would skip measurements.

thanks,

Mimi

>  	rc = ima_collect_measurement(iint, file, buf, size, hash_algo, modsig);
>  	if (rc != 0 && rc != -EBADF && rc != -EINVAL)
>  		goto out_locked;



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

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

Hi Simon,

On Tue, 2021-07-27 at 10:23 +0000, THOBY Simon wrote:
> This patch defines a new IMA policy rule option "appraise_hash=",
> that restricts the hash algorithms accepted for the extended attribute
> security.ima when appraising.

"Define ..."

> 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  | 72 ++++++++++++++++++++++++++--
>  2 files changed, 74 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
> index 070779e8d836..365e4c91719e 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=] [keyrings=] [appraise_hash=]

Nit:
Probably nicer to keep the "appraise_" options together.  How about
placing it after "appraise_flag", instead of at the end.

>  		  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..a7f110cbbff0 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -92,6 +92,7 @@ struct ima_rule_entry {
>  	struct ima_template_desc *template;
>  };
>  
> +

Is this extra blank line intentional?

>  /*
>   * sanity check in case the kernels gains more hash algorithms that can
>   * fit in an unsigned int
> @@ -962,7 +963,7 @@ enum {
>  	Opt_uid_lt, Opt_euid_lt, Opt_fowner_lt,
>  	Opt_appraise_type, Opt_appraise_flag,
>  	Opt_permit_directio, Opt_pcr, Opt_template, Opt_keyrings,
> -	Opt_label, Opt_err
> +	Opt_label, Opt_appraise_hash, Opt_err

Nit: move Opt_appraise_hash after Opt_appraise_type.
>  };
>  
>  static const match_table_t policy_tokens = {
> @@ -1000,6 +1001,7 @@ static const match_table_t policy_tokens = {
>  	{Opt_template, "template=%s"},
>  	{Opt_keyrings, "keyrings=%s"},
>  	{Opt_label, "label=%s"},
> +	{Opt_appraise_hash, "appraise_hash=%s"},

ditto

>  	{Opt_err, NULL}
>  };
>  
> @@ -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,27 @@ 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;
> +	char *token;
> +
> +	while ((token = strsep(&arg, ",")) != NULL) {
> +		int idx = match_string(hash_algo_name, HASH_ALGO__LAST, token);

Move the variable definition to the beginning of the function.
> +
> +		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 +1227,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;
> @@ -1556,6 +1580,23 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
>  						 &(template_desc->fields),
>  						 &(template_desc->num_fields));
>  			entry->template = template_desc;
> +			break;
> +		case Opt_appraise_hash:

ditto

"appraise_hash=" should be limited to appraise rules.  Please update
ima_validate_rule().

thanks,

Mimi

> +			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);
> +			if (!entry->allowed_hashes) {
> +				result = -EINVAL;
> +				break;
> +			}
> +
> +			entry->flags |= IMA_VALIDATE_HASH;
> +
>  			break;
>  		case Opt_err:
>  			ima_log_string(ab, "UNKNOWN", p);
> 
<trim>


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

end of thread, other threads:[~2021-07-27 16:24 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-27 10:23 [PATCH v3 0/4] IMA: restrict the accepted digest algorithms for the security.ima xattr THOBY Simon
2021-07-27 10:23 ` [PATCH v3 1/4] IMA: block writes of the security.ima xattr with unsupported algorithms THOBY Simon
2021-07-27 14:04   ` Mimi Zohar
2021-07-27 10:23 ` [PATCH v3 2/4] IMA: add support to restrict the hash algorithms used for file appraisal THOBY Simon
2021-07-27 15:29   ` Mimi Zohar
2021-07-27 10:23 ` [PATCH v3 3/4] IMA: add a policy option to restrict xattr hash algorithms on appraisal THOBY Simon
2021-07-27 16:24   ` Mimi Zohar
2021-07-27 10:23 ` [PATCH v3 4/4] IMA: introduce a new policy option func=SETXATTR_CHECK THOBY Simon
2021-07-27 11:24   ` THOBY Simon

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