From a32ad90426a9c8eb3915eed26e08ce133bd9e0da Mon Sep 17 00:00:00 2001 From: Austin Kim Date: Tue, 29 Jun 2021 14:50:50 +0100 Subject: IMA: remove -Wmissing-prototypes warning MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With W=1 build, the compiler throws warning message as below: security/integrity/ima/ima_mok.c:24:12: warning: no previous prototype for ‘ima_mok_init’ [-Wmissing-prototypes] __init int ima_mok_init(void) Silence the warning by adding static keyword to ima_mok_init(). Signed-off-by: Austin Kim Fixes: 41c89b64d718 ("IMA: create machine owner and blacklist keyrings") Cc: stable@vger.kernel.org Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_mok.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'security') diff --git a/security/integrity/ima/ima_mok.c b/security/integrity/ima/ima_mok.c index 1e5c01916173..95cc31525c57 100644 --- a/security/integrity/ima/ima_mok.c +++ b/security/integrity/ima/ima_mok.c @@ -21,7 +21,7 @@ struct key *ima_blacklist_keyring; /* * Allocate the IMA blacklist keyring */ -__init int ima_mok_init(void) +static __init int ima_mok_init(void) { struct key_restriction *restriction; -- cgit v1.2.3-59-g8ed1b From 5d1ef2ce13a9098b4e0d31c50e4c79763a57b444 Mon Sep 17 00:00:00 2001 From: Roberto Sassu Date: Fri, 23 Jul 2021 10:53:02 +0200 Subject: ima: Introduce ima_get_current_hash_algo() Buffer measurements, unlike file measurements, are not accessible after the measurement is done, as buffers are not suitable for use with the integrity_iint_cache structure (there is no index, for files it is the inode number). In the subsequent patches, the measurement (digest) will be returned directly by the functions that perform the buffer measurement, ima_measure_critical_data() and process_buffer_measurement(). A caller of those functions also needs to know the algorithm used to calculate the digest. Instead of adding the algorithm as a new parameter to the functions, this patch provides it separately with the new function ima_get_current_hash_algo(). Since the hash algorithm does not change after the IMA setup phase, there is no risk of races (obtaining a digest calculated with a different algorithm than the one returned). Signed-off-by: Roberto Sassu Reviewed-by: Lakshmi Ramasubramanian [zohar@linux.ibm.com: annotate ima_hash_algo as __ro_after_init] Signed-off-by: Mimi Zohar --- include/linux/ima.h | 7 +++++++ security/integrity/ima/ima_main.c | 7 ++++++- 2 files changed, 13 insertions(+), 1 deletion(-) (limited to 'security') diff --git a/include/linux/ima.h b/include/linux/ima.h index 61d5723ec303..81e830d01ced 100644 --- a/include/linux/ima.h +++ b/include/linux/ima.h @@ -11,9 +11,11 @@ #include #include #include +#include struct linux_binprm; #ifdef CONFIG_IMA +extern enum hash_algo ima_get_current_hash_algo(void); extern int ima_bprm_check(struct linux_binprm *bprm); extern int ima_file_check(struct file *file, int mask); extern void ima_post_create_tmpfile(struct user_namespace *mnt_userns, @@ -64,6 +66,11 @@ static inline const char * const *arch_get_ima_policy(void) #endif #else +static inline enum hash_algo ima_get_current_hash_algo(void) +{ + return HASH_ALGO__LAST; +} + static inline int ima_bprm_check(struct linux_binprm *bprm) { return 0; diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 287b90509006..634e4709d8af 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -35,7 +35,7 @@ int ima_appraise = IMA_APPRAISE_ENFORCE; int ima_appraise; #endif -int ima_hash_algo = HASH_ALGO_SHA1; +int __ro_after_init ima_hash_algo = HASH_ALGO_SHA1; static int hash_setup_done; static struct notifier_block ima_lsm_policy_notifier = { @@ -76,6 +76,11 @@ out: } __setup("ima_hash=", hash_setup); +enum hash_algo ima_get_current_hash_algo(void) +{ + return ima_hash_algo; +} + /* Prevent mmap'ing a file execute that is already mmap'ed write */ static int mmap_violation_check(enum ima_hooks func, struct file *file, char **pathbuf, const char **pathname, -- cgit v1.2.3-59-g8ed1b From ce5bb5a86e5ebcd3c2e40e6dd1382027b5d43caf Mon Sep 17 00:00:00 2001 From: Roberto Sassu Date: Fri, 23 Jul 2021 10:53:03 +0200 Subject: ima: Return int in the functions to measure a buffer ima_measure_critical_data() and process_buffer_measurement() currently don't return a result as, unlike appraisal-related functions, the result is not used by callers to deny an operation. Measurement-related functions instead rely on the audit subsystem to notify the system administrator when an error occurs. However, ima_measure_critical_data() and process_buffer_measurement() are a special case, as these are the only functions that can return a buffer measurement (for files, there is ima_file_hash()). In a subsequent patch, they will be modified to return the calculated digest. In preparation to return the result of the digest calculation, this patch modifies the return type from void to int, and returns 0 if the buffer has been successfully measured, a negative value otherwise. Given that the result of the measurement is still not necessary, this patch does not modify the behavior of existing callers by processing the returned value. For those, the return value is ignored. Signed-off-by: Roberto Sassu Reviewed-by: Lakshmi Ramasubramanian Acked-by: Paul Moore (for the SELinux bits) Signed-off-by: Mimi Zohar --- include/linux/ima.h | 15 +++++++++------ security/integrity/ima/ima.h | 10 +++++----- security/integrity/ima/ima_main.c | 40 ++++++++++++++++++++++----------------- 3 files changed, 37 insertions(+), 28 deletions(-) (limited to 'security') diff --git a/include/linux/ima.h b/include/linux/ima.h index 81e830d01ced..60492263aa64 100644 --- a/include/linux/ima.h +++ b/include/linux/ima.h @@ -35,10 +35,10 @@ extern void ima_post_path_mknod(struct user_namespace *mnt_userns, extern int ima_file_hash(struct file *file, char *buf, size_t buf_size); extern int ima_inode_hash(struct inode *inode, char *buf, size_t buf_size); extern void ima_kexec_cmdline(int kernel_fd, const void *buf, int size); -extern void ima_measure_critical_data(const char *event_label, - const char *event_name, - const void *buf, size_t buf_len, - bool hash); +extern int ima_measure_critical_data(const char *event_label, + const char *event_name, + const void *buf, size_t buf_len, + bool hash); #ifdef CONFIG_IMA_APPRAISE_BOOTPARAM extern void ima_appraise_parse_cmdline(void); @@ -144,10 +144,13 @@ static inline int ima_inode_hash(struct inode *inode, char *buf, size_t buf_size static inline void ima_kexec_cmdline(int kernel_fd, const void *buf, int size) {} -static inline void ima_measure_critical_data(const char *event_label, +static inline int ima_measure_critical_data(const char *event_label, const char *event_name, const void *buf, size_t buf_len, - bool hash) {} + bool hash) +{ + return -ENOENT; +} #endif /* CONFIG_IMA */ diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index f0e448ed1f9f..03db221324c3 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -264,11 +264,11 @@ void ima_store_measurement(struct integrity_iint_cache *iint, struct file *file, struct evm_ima_xattr_data *xattr_value, int xattr_len, const struct modsig *modsig, int pcr, struct ima_template_desc *template_desc); -void process_buffer_measurement(struct user_namespace *mnt_userns, - struct inode *inode, const void *buf, int size, - const char *eventname, enum ima_hooks func, - int pcr, const char *func_data, - bool buf_hash); +int process_buffer_measurement(struct user_namespace *mnt_userns, + struct inode *inode, const void *buf, int size, + const char *eventname, enum ima_hooks func, + int pcr, const char *func_data, + bool buf_hash); void ima_audit_measurement(struct integrity_iint_cache *iint, const unsigned char *filename); int ima_alloc_init_template(struct ima_event_data *event_data, diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 634e4709d8af..c814738caaca 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -827,7 +827,7 @@ int ima_post_load_data(char *buf, loff_t size, return 0; } -/* +/** * process_buffer_measurement - Measure the buffer or the buffer data hash * @mnt_userns: user namespace of the mount the inode was found from * @inode: inode associated with the object being measured (NULL for KEY_CHECK) @@ -840,12 +840,15 @@ int ima_post_load_data(char *buf, loff_t size, * @buf_hash: measure buffer data hash * * Based on policy, either the buffer data or buffer data hash is measured + * + * Return: 0 if the buffer has been successfully measured, a negative value + * otherwise. */ -void process_buffer_measurement(struct user_namespace *mnt_userns, - struct inode *inode, const void *buf, int size, - const char *eventname, enum ima_hooks func, - int pcr, const char *func_data, - bool buf_hash) +int process_buffer_measurement(struct user_namespace *mnt_userns, + struct inode *inode, const void *buf, int size, + const char *eventname, enum ima_hooks func, + int pcr, const char *func_data, + bool buf_hash) { int ret = 0; const char *audit_cause = "ENOMEM"; @@ -867,7 +870,7 @@ void process_buffer_measurement(struct user_namespace *mnt_userns, u32 secid; if (!ima_policy_flag) - return; + return -ENOENT; template = ima_template_desc_buf(); if (!template) { @@ -889,7 +892,7 @@ void process_buffer_measurement(struct user_namespace *mnt_userns, secid, 0, func, &pcr, &template, func_data); if (!(action & IMA_MEASURE)) - return; + return -ENOENT; } if (!pcr) @@ -937,7 +940,7 @@ out: func_measure_str(func), audit_cause, ret, 0, ret); - return; + return ret; } /** @@ -977,18 +980,21 @@ void ima_kexec_cmdline(int kernel_fd, const void *buf, int size) * and extend the pcr. Examples of critical data could be various data * structures, policies, and states stored in kernel memory that can * impact the integrity of the system. + * + * Return: 0 if the buffer has been successfully measured, a negative value + * otherwise. */ -void ima_measure_critical_data(const char *event_label, - const char *event_name, - const void *buf, size_t buf_len, - bool hash) +int ima_measure_critical_data(const char *event_label, + const char *event_name, + const void *buf, size_t buf_len, + bool hash) { if (!event_name || !event_label || !buf || !buf_len) - return; + return -ENOPARAM; - process_buffer_measurement(&init_user_ns, NULL, buf, buf_len, event_name, - CRITICAL_DATA, 0, event_label, - hash); + return process_buffer_measurement(&init_user_ns, NULL, buf, buf_len, + event_name, CRITICAL_DATA, 0, + event_label, hash); } static int __init init_ima(void) -- cgit v1.2.3-59-g8ed1b From ca3c9bdb101d9b9eb3ed8a85cc0fe55915ba49de Mon Sep 17 00:00:00 2001 From: Roberto Sassu Date: Fri, 23 Jul 2021 10:53:04 +0200 Subject: ima: Add digest and digest_len params to the functions to measure a buffer This patch performs the final modification necessary to pass the buffer measurement to callers, so that they provide a functionality similar to ima_file_hash(). It adds the 'digest' and 'digest_len' parameters to ima_measure_critical_data() and process_buffer_measurement(). These functions calculate the digest even if there is no suitable rule in the IMA policy and, in this case, they simply return 1 before generating a new measurement entry. Signed-off-by: Roberto Sassu Reviewed-by: Lakshmi Ramasubramanian Signed-off-by: Mimi Zohar --- include/linux/ima.h | 5 ++-- security/integrity/ima/ima.h | 2 +- security/integrity/ima/ima_appraise.c | 2 +- security/integrity/ima/ima_asymmetric_keys.c | 2 +- security/integrity/ima/ima_init.c | 3 ++- security/integrity/ima/ima_main.c | 36 ++++++++++++++++++++-------- security/integrity/ima/ima_queue_keys.c | 2 +- security/selinux/ima.c | 6 +++-- 8 files changed, 39 insertions(+), 19 deletions(-) (limited to 'security') diff --git a/include/linux/ima.h b/include/linux/ima.h index 60492263aa64..b6ab66a546ae 100644 --- a/include/linux/ima.h +++ b/include/linux/ima.h @@ -38,7 +38,7 @@ extern void ima_kexec_cmdline(int kernel_fd, const void *buf, int size); extern int ima_measure_critical_data(const char *event_label, const char *event_name, const void *buf, size_t buf_len, - bool hash); + bool hash, u8 *digest, size_t digest_len); #ifdef CONFIG_IMA_APPRAISE_BOOTPARAM extern void ima_appraise_parse_cmdline(void); @@ -147,7 +147,8 @@ static inline void ima_kexec_cmdline(int kernel_fd, const void *buf, int size) { static inline int ima_measure_critical_data(const char *event_label, const char *event_name, const void *buf, size_t buf_len, - bool hash) + bool hash, u8 *digest, + size_t digest_len) { return -ENOENT; } diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index 03db221324c3..2f4c20b16ad7 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -268,7 +268,7 @@ int process_buffer_measurement(struct user_namespace *mnt_userns, struct inode *inode, const void *buf, int size, const char *eventname, enum ima_hooks func, int pcr, const char *func_data, - bool buf_hash); + bool buf_hash, u8 *digest, size_t digest_len); void ima_audit_measurement(struct integrity_iint_cache *iint, const unsigned char *filename); int ima_alloc_init_template(struct ima_event_data *event_data, diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index ef9dcfce45d4..63bec42c353f 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -357,7 +357,7 @@ int ima_check_blacklist(struct integrity_iint_cache *iint, if ((rc == -EPERM) && (iint->flags & IMA_MEASURE)) process_buffer_measurement(&init_user_ns, NULL, digest, digestsize, "blacklisted-hash", NONE, - pcr, NULL, false); + pcr, NULL, false, NULL, 0); } return rc; diff --git a/security/integrity/ima/ima_asymmetric_keys.c b/security/integrity/ima/ima_asymmetric_keys.c index c985418698a4..f6aa0b47a772 100644 --- a/security/integrity/ima/ima_asymmetric_keys.c +++ b/security/integrity/ima/ima_asymmetric_keys.c @@ -62,5 +62,5 @@ void ima_post_key_create_or_update(struct key *keyring, struct key *key, */ process_buffer_measurement(&init_user_ns, NULL, payload, payload_len, keyring->description, KEY_CHECK, 0, - keyring->description, false); + keyring->description, false, NULL, 0); } diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c index 5076a7d9d23e..b26fa67476b4 100644 --- a/security/integrity/ima/ima_init.c +++ b/security/integrity/ima/ima_init.c @@ -154,7 +154,8 @@ int __init ima_init(void) ima_init_key_queue(); ima_measure_critical_data("kernel_info", "kernel_version", - UTS_RELEASE, strlen(UTS_RELEASE), false); + UTS_RELEASE, strlen(UTS_RELEASE), false, + NULL, 0); return rc; } diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index c814738caaca..1cba6beb5a60 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -838,17 +838,20 @@ int ima_post_load_data(char *buf, loff_t size, * @pcr: pcr to extend the measurement * @func_data: func specific data, may be NULL * @buf_hash: measure buffer data hash + * @digest: buffer digest will be written to + * @digest_len: buffer length * * Based on policy, either the buffer data or buffer data hash is measured * - * Return: 0 if the buffer has been successfully measured, a negative value - * otherwise. + * Return: 0 if the buffer has been successfully measured, 1 if the digest + * has been written to the passed location but not added to a measurement entry, + * a negative value otherwise. */ int process_buffer_measurement(struct user_namespace *mnt_userns, struct inode *inode, const void *buf, int size, const char *eventname, enum ima_hooks func, int pcr, const char *func_data, - bool buf_hash) + bool buf_hash, u8 *digest, size_t digest_len) { int ret = 0; const char *audit_cause = "ENOMEM"; @@ -869,7 +872,10 @@ int process_buffer_measurement(struct user_namespace *mnt_userns, int action = 0; u32 secid; - if (!ima_policy_flag) + if (digest && digest_len < digest_hash_len) + return -EINVAL; + + if (!ima_policy_flag && !digest) return -ENOENT; template = ima_template_desc_buf(); @@ -891,7 +897,7 @@ int process_buffer_measurement(struct user_namespace *mnt_userns, action = ima_get_action(mnt_userns, inode, current_cred(), secid, 0, func, &pcr, &template, func_data); - if (!(action & IMA_MEASURE)) + if (!(action & IMA_MEASURE) && !digest) return -ENOENT; } @@ -922,6 +928,12 @@ int process_buffer_measurement(struct user_namespace *mnt_userns, event_data.buf_len = digest_hash_len; } + if (digest) + memcpy(digest, iint.ima_hash->digest, digest_hash_len); + + if (!ima_policy_flag || (func && !(action & IMA_MEASURE))) + return 1; + ret = ima_alloc_init_template(&event_data, &entry, template); if (ret < 0) { audit_cause = "alloc_entry"; @@ -964,7 +976,7 @@ void ima_kexec_cmdline(int kernel_fd, const void *buf, int size) process_buffer_measurement(file_mnt_user_ns(f.file), file_inode(f.file), buf, size, "kexec-cmdline", KEXEC_CMDLINE, 0, - NULL, false); + NULL, false, NULL, 0); fdput(f); } @@ -975,26 +987,30 @@ void ima_kexec_cmdline(int kernel_fd, const void *buf, int size) * @buf: pointer to buffer data * @buf_len: length of buffer data (in bytes) * @hash: measure buffer data hash + * @digest: buffer digest will be written to + * @digest_len: buffer length * * Measure data critical to the integrity of the kernel into the IMA log * and extend the pcr. Examples of critical data could be various data * structures, policies, and states stored in kernel memory that can * impact the integrity of the system. * - * Return: 0 if the buffer has been successfully measured, a negative value - * otherwise. + * Return: 0 if the buffer has been successfully measured, 1 if the digest + * has been written to the passed location but not added to a measurement entry, + * a negative value otherwise. */ int ima_measure_critical_data(const char *event_label, const char *event_name, const void *buf, size_t buf_len, - bool hash) + bool hash, u8 *digest, size_t digest_len) { if (!event_name || !event_label || !buf || !buf_len) return -ENOPARAM; return process_buffer_measurement(&init_user_ns, NULL, buf, buf_len, event_name, CRITICAL_DATA, 0, - event_label, hash); + event_label, hash, digest, + digest_len); } static int __init init_ima(void) diff --git a/security/integrity/ima/ima_queue_keys.c b/security/integrity/ima/ima_queue_keys.c index 979ef6c71f3d..93056c03bf5a 100644 --- a/security/integrity/ima/ima_queue_keys.c +++ b/security/integrity/ima/ima_queue_keys.c @@ -165,7 +165,7 @@ void ima_process_queued_keys(void) entry->keyring_name, KEY_CHECK, 0, entry->keyring_name, - false); + false, NULL, 0); list_del(&entry->list); ima_free_key_entry(entry); } diff --git a/security/selinux/ima.c b/security/selinux/ima.c index 34d421861bfc..727c4e43219d 100644 --- a/security/selinux/ima.c +++ b/security/selinux/ima.c @@ -86,7 +86,8 @@ void selinux_ima_measure_state_locked(struct selinux_state *state) } ima_measure_critical_data("selinux", "selinux-state", - state_str, strlen(state_str), false); + state_str, strlen(state_str), false, + NULL, 0); kfree(state_str); @@ -103,7 +104,8 @@ void selinux_ima_measure_state_locked(struct selinux_state *state) } ima_measure_critical_data("selinux", "selinux-policy-hash", - policy, policy_len, true); + policy, policy_len, true, + NULL, 0); vfree(policy); } -- cgit v1.2.3-59-g8ed1b From 8510505d55e194d3f6c9644c9f9d12c4f6b0395a Mon Sep 17 00:00:00 2001 From: THOBY Simon Date: Mon, 16 Aug 2021 08:10:59 +0000 Subject: IMA: remove the dependency on CRYPTO_MD5 MD5 is a weak digest algorithm that shouldn't be used for cryptographic operation. It hinders the efficiency of a patch set that aims to limit the digests allowed for the extended file attribute namely security.ima. MD5 is no longer a requirement for IMA, nor should it be used there. The sole place where we still use the MD5 algorithm inside IMA is setting the ima_hash algorithm to MD5, if the user supplies 'ima_hash=md5' parameter on the command line. With commit ab60368ab6a4 ("ima: Fallback to the builtin hash algorithm"), setting "ima_hash=md5" fails gracefully when CRYPTO_MD5 is not set: ima: Can not allocate md5 (reason: -2) ima: Allocating md5 failed, going to use default hash algorithm sha256 Remove the CRYPTO_MD5 dependency for IMA. Signed-off-by: THOBY Simon Reviewed-by: Lakshmi Ramasubramanian [zohar@linux.ibm.com: include commit number in patch description for stable.] Cc: stable@vger.kernel.org # 4.17 Signed-off-by: Mimi Zohar --- security/integrity/ima/Kconfig | 1 - 1 file changed, 1 deletion(-) (limited to 'security') 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 -- cgit v1.2.3-59-g8ed1b From 50f742dd91474e7f4954bf88d094eede59783883 Mon Sep 17 00:00:00 2001 From: THOBY Simon Date: Mon, 16 Aug 2021 08:10:59 +0000 Subject: IMA: block writes of the security.ima xattr with unsupported algorithms By default, writes to the extended attributes security.ima will be allowed even if the hash algorithm used for the xattr is not compiled in the kernel (which does not make sense because the kernel would not be able to appraise that file as it lacks support for validating the hash). Prevent and audit writes to the security.ima xattr if the hash algorithm used in the new value is not available in the current kernel. Signed-off-by: THOBY Simon Reviewed-by: Lakshmi Ramasubramanian Signed-off-by: Mimi Zohar --- security/integrity/ima/ima.h | 2 +- security/integrity/ima/ima_appraise.c | 49 ++++++++++++++++++++++++++++++++--- 2 files changed, 47 insertions(+), 4 deletions(-) (limited to 'security') diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index 2f4c20b16ad7..829478dabeeb 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -319,7 +319,7 @@ int ima_must_appraise(struct user_namespace *mnt_userns, struct inode *inode, void ima_update_xattr(struct integrity_iint_cache *iint, struct file *file); enum integrity_status ima_get_cache_status(struct integrity_iint_cache *iint, enum ima_hooks func); -enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value, +enum hash_algo ima_get_hash_algo(const struct evm_ima_xattr_data *xattr_value, int xattr_len); int ima_read_xattr(struct dentry *dentry, struct evm_ima_xattr_data **xattr_value); diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index 63bec42c353f..baeb10efbf51 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -171,7 +171,7 @@ static void ima_cache_flags(struct integrity_iint_cache *iint, } } -enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value, +enum hash_algo ima_get_hash_algo(const struct evm_ima_xattr_data *xattr_value, int xattr_len) { struct signature_v2_hdr *sig; @@ -575,6 +575,47 @@ static void ima_reset_appraise_flags(struct inode *inode, int digsig) clear_bit(IMA_DIGSIG, &iint->atomic_flags); } +/** + * validate_hash_algo() - Block setxattr with unsupported hash algorithms + * @dentry: object of the setxattr() + * @xattr_value: userland supplied xattr value + * @xattr_value_len: length of xattr_value + * + * The xattr value is mapped to its hash algorithm, and this algorithm + * must be built in the kernel for the setxattr to be allowed. + * + * Emit an audit message when the algorithm is invalid. + * + * Return: 0 on success, else an error. + */ +static int validate_hash_algo(struct dentry *dentry, + const struct evm_ima_xattr_data *xattr_value, + size_t xattr_value_len) +{ + char *path = NULL, *pathbuf = NULL; + enum hash_algo xattr_hash_algo; + + xattr_hash_algo = ima_get_hash_algo(xattr_value, xattr_value_len); + + if (likely(xattr_hash_algo == ima_hash_algo || + crypto_has_alg(hash_algo_name[xattr_hash_algo], 0, 0))) + return 0; + + pathbuf = kmalloc(PATH_MAX, GFP_KERNEL); + if (!pathbuf) + return -EACCES; + + path = dentry_path(dentry, pathbuf, PATH_MAX); + + integrity_audit_msg(AUDIT_INTEGRITY_DATA, d_inode(dentry), path, + "set_data", "unavailable-hash-algorithm", + -EACCES, 0); + + kfree(pathbuf); + + return -EACCES; +} + int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name, const void *xattr_value, size_t xattr_value_len) { @@ -592,9 +633,11 @@ int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name, digsig = (xvalue->type == EVM_XATTR_PORTABLE_DIGSIG); } if (result == 1 || evm_revalidate_status(xattr_name)) { + result = validate_hash_algo(dentry, xvalue, xattr_value_len); + if (result) + return result; + ima_reset_appraise_flags(d_backing_inode(dentry), digsig); - if (result == 1) - result = 0; } return result; } -- cgit v1.2.3-59-g8ed1b From 1624dc0086056c3a35fd34b0235bb1eb88c1c4d5 Mon Sep 17 00:00:00 2001 From: THOBY Simon Date: Mon, 16 Aug 2021 08:11:00 +0000 Subject: IMA: add support to restrict the hash algorithms used for file appraisal The kernel accepts any hash algorithm as a value for the security.ima xattr. Users may wish to restrict the accepted algorithms to only support strong cryptographic ones. Provide the plumbing to restrict the permitted set of hash algorithms used for verifying file hashes and signatures stored in security.ima xattr. Signed-off-by: THOBY Simon Reviewed-by: Lakshmi Ramasubramanian Signed-off-by: Mimi Zohar --- security/integrity/ima/ima.h | 6 +++--- security/integrity/ima/ima_api.c | 6 ++++-- security/integrity/ima/ima_appraise.c | 5 +++-- security/integrity/ima/ima_main.c | 18 +++++++++++++++--- security/integrity/ima/ima_policy.c | 18 ++++++++++++++++-- 5 files changed, 41 insertions(+), 12 deletions(-) (limited to 'security') diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index 829478dabeeb..bcaf818fb647 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -47,7 +47,7 @@ enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8, TPM_PCR10 = 10 }; extern int ima_policy_flag; /* set during initialization */ -extern int ima_hash_algo; +extern int ima_hash_algo __ro_after_init; extern int ima_sha1_idx __ro_after_init; extern int ima_hash_algo_idx __ro_after_init; extern int ima_extra_slots __ro_after_init; @@ -254,7 +254,7 @@ int ima_get_action(struct user_namespace *mnt_userns, struct inode *inode, const struct cred *cred, u32 secid, int mask, enum ima_hooks func, int *pcr, struct ima_template_desc **template_desc, - const char *func_data); + const char *func_data, unsigned int *allowed_algos); int ima_must_measure(struct inode *inode, int mask, enum ima_hooks func); int ima_collect_measurement(struct integrity_iint_cache *iint, struct file *file, void *buf, loff_t size, @@ -285,7 +285,7 @@ int ima_match_policy(struct user_namespace *mnt_userns, struct inode *inode, const struct cred *cred, u32 secid, enum ima_hooks func, int mask, int flags, int *pcr, struct ima_template_desc **template_desc, - const char *func_data); + const char *func_data, unsigned int *allowed_algos); void ima_init_policy(void); void ima_update_policy(void); void ima_update_policy_flag(void); diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c index d8e321cc6936..2c6c3a5228b5 100644 --- a/security/integrity/ima/ima_api.c +++ b/security/integrity/ima/ima_api.c @@ -172,6 +172,7 @@ err_out: * @pcr: pointer filled in if matched measure policy sets pcr= * @template_desc: pointer filled in if matched measure policy sets template= * @func_data: func specific data, may be NULL + * @allowed_algos: allowlist of hash algorithms for the IMA xattr * * The policy is defined in terms of keypairs: * subj=, obj=, type=, func=, mask=, fsmagic= @@ -188,14 +189,15 @@ int ima_get_action(struct user_namespace *mnt_userns, struct inode *inode, const struct cred *cred, u32 secid, int mask, enum ima_hooks func, int *pcr, struct ima_template_desc **template_desc, - const char *func_data) + const char *func_data, unsigned int *allowed_algos) { int flags = IMA_MEASURE | IMA_AUDIT | IMA_APPRAISE | IMA_HASH; flags &= ima_policy_flag; return ima_match_policy(mnt_userns, inode, cred, secid, func, mask, - flags, pcr, template_desc, func_data); + flags, pcr, template_desc, func_data, + allowed_algos); } /* diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index baeb10efbf51..e2edef8a9185 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -77,8 +77,9 @@ int ima_must_appraise(struct user_namespace *mnt_userns, struct inode *inode, return 0; security_task_getsecid_subj(current, &secid); - return ima_match_policy(mnt_userns, inode, current_cred(), secid, func, - mask, IMA_APPRAISE | IMA_HASH, NULL, NULL, NULL); + return ima_match_policy(mnt_userns, inode, current_cred(), secid, + func, mask, IMA_APPRAISE | IMA_HASH, NULL, + NULL, NULL, NULL); } static int ima_fix_xattr(struct dentry *dentry, diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 1cba6beb5a60..af6367ba34ee 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -215,6 +215,7 @@ static int process_measurement(struct file *file, const struct cred *cred, int xattr_len = 0; bool violation_check; enum hash_algo hash_algo; + unsigned int allowed_algos = 0; if (!ima_policy_flag || !S_ISREG(inode->i_mode)) return 0; @@ -224,7 +225,8 @@ static int process_measurement(struct file *file, const struct cred *cred, * Included is the appraise submask. */ action = ima_get_action(file_mnt_user_ns(file), inode, cred, secid, - mask, func, &pcr, &template_desc, NULL); + mask, func, &pcr, &template_desc, NULL, + &allowed_algos); violation_check = ((func == FILE_CHECK || func == MMAP_CHECK) && (ima_policy_flag & IMA_MEASURE)); if (!action && !violation_check) @@ -361,6 +363,16 @@ static int process_measurement(struct file *file, const struct cred *cred, if ((file->f_flags & O_DIRECT) && (iint->flags & IMA_PERMIT_DIRECTIO)) rc = 0; + + /* Ensure the digest was generated using an allowed algorithm */ + if (rc == 0 && must_appraise && allowed_algos != 0 && + (allowed_algos & (1U << hash_algo)) == 0) { + rc = -EACCES; + + integrity_audit_msg(AUDIT_INTEGRITY_DATA, file_inode(file), + pathname, "collect_data", + "denied-hash-algorithm", rc, 0); + } out_locked: if ((mask & MAY_WRITE) && test_bit(IMA_DIGSIG, &iint->atomic_flags) && !(iint->flags & IMA_NEW_FILE)) @@ -438,7 +450,7 @@ int ima_file_mprotect(struct vm_area_struct *vma, unsigned long prot) inode = file_inode(vma->vm_file); action = ima_get_action(file_mnt_user_ns(vma->vm_file), inode, current_cred(), secid, MAY_EXEC, MMAP_CHECK, - &pcr, &template, NULL); + &pcr, &template, NULL, NULL); /* Is the mmap'ed file in policy? */ if (!(action & (IMA_MEASURE | IMA_APPRAISE_SUBMASK))) @@ -896,7 +908,7 @@ int process_buffer_measurement(struct user_namespace *mnt_userns, security_task_getsecid_subj(current, &secid); action = ima_get_action(mnt_userns, inode, current_cred(), secid, 0, func, &pcr, &template, - func_data); + func_data, NULL); if (!(action & IMA_MEASURE) && !digest) return -ENOENT; } diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index fd5d46e511f1..1536e6f5eb22 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -35,6 +35,7 @@ #define IMA_FSNAME 0x0200 #define IMA_KEYRINGS 0x0400 #define IMA_LABEL 0x0800 +#define IMA_VALIDATE_ALGOS 0x1000 #define UNKNOWN 0 #define MEASURE 0x0001 /* same as IMA_MEASURE */ @@ -79,6 +80,7 @@ struct ima_rule_entry { bool (*uid_op)(kuid_t, kuid_t); /* Handlers for operators */ bool (*fowner_op)(kuid_t, kuid_t); /* uid_eq(), uid_gt(), uid_lt() */ int pcr; + unsigned int allowed_algos; /* bitfield of allowed hash algorithms */ struct { void *rule; /* LSM file metadata specific */ char *args_p; /* audit value */ @@ -90,6 +92,14 @@ struct ima_rule_entry { struct ima_template_desc *template; }; +/* + * sanity check in case the kernels gains more hash algorithms that can + * fit in an unsigned int + */ +static_assert( + 8 * sizeof(unsigned int) >= HASH_ALGO__LAST, + "The bitfield allowed_algos in ima_rule_entry is too small to contain all the supported hash algorithms, consider using a bigger type"); + /* * Without LSM specific knowledge, the default policy can only be * written in terms of .action, .func, .mask, .fsmagic, .uid, and .fowner @@ -646,6 +656,7 @@ static int get_subaction(struct ima_rule_entry *rule, enum ima_hooks func) * @pcr: set the pcr to extend * @template_desc: the template that should be used for this rule * @func_data: func specific data, may be NULL + * @allowed_algos: allowlist of hash algorithms for the IMA xattr * * Measure decision based on func/mask/fsmagic and LSM(subj/obj/type) * conditions. @@ -658,7 +669,7 @@ int ima_match_policy(struct user_namespace *mnt_userns, struct inode *inode, const struct cred *cred, u32 secid, enum ima_hooks func, int mask, int flags, int *pcr, struct ima_template_desc **template_desc, - const char *func_data) + const char *func_data, unsigned int *allowed_algos) { struct ima_rule_entry *entry; int action = 0, actmask = flags | (flags << 1); @@ -684,8 +695,11 @@ int ima_match_policy(struct user_namespace *mnt_userns, struct inode *inode, action &= ~IMA_HASH; if (ima_fail_unverifiable_sigs) action |= IMA_FAIL_UNVERIFIABLE_SIGS; - } + if (allowed_algos && + entry->flags & IMA_VALIDATE_ALGOS) + *allowed_algos = entry->allowed_algos; + } if (entry->action & IMA_DO_MASK) actmask &= ~(entry->action | entry->action << 1); -- cgit v1.2.3-59-g8ed1b From 583a80ae86b5ceb68119cfb9a37404cf22f6cc46 Mon Sep 17 00:00:00 2001 From: THOBY Simon Date: Mon, 16 Aug 2021 08:11:00 +0000 Subject: IMA: add a policy option to restrict xattr hash algorithms on appraisal The kernel has the ability to restrict the set of hash algorithms it accepts for the security.ima xattr when it appraises files. Define a new IMA policy rule option "appraise_algos=", using the mentioned mechanism to expose a user-toggable policy knob to opt-in to that restriction and select the desired set of algorithms that must be accepted. When a policy rule uses the 'appraise_algos' option, appraisal of a file referenced by that rule will now fail if the digest algorithm employed to hash the file was not one of those explicitly listed in the option. In its absence, any hash algorithm compiled in the kernel will be accepted. For example, on a system where SELinux is properly deployed, the rule appraise func=BPRM_CHECK obj_type=iptables_exec_t \ appraise_algos=sha256,sha384 will block the execution of iptables if the xattr security.ima of its executables were not hashed with either sha256 or sha384. Signed-off-by: THOBY Simon Reviewed-by: Lakshmi Ramasubramanian Signed-off-by: Mimi Zohar --- Documentation/ABI/testing/ima_policy | 6 ++- security/integrity/ima/ima_policy.c | 74 ++++++++++++++++++++++++++++++++++-- 2 files changed, 75 insertions(+), 5 deletions(-) (limited to 'security') diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy index 070779e8d836..b0e3d278e799 100644 --- a/Documentation/ABI/testing/ima_policy +++ b/Documentation/ABI/testing/ima_policy @@ -27,7 +27,7 @@ Description: lsm: [[subj_user=] [subj_role=] [subj_type=] [obj_user=] [obj_role=] [obj_type=]] option: [[appraise_type=]] [template=] [permit_directio] - [appraise_flag=] [keyrings=] + [appraise_flag=] [appraise_algos=] [keyrings=] base: func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK] [FIRMWARE_CHECK] @@ -55,6 +55,10 @@ Description: label:= [selinux]|[kernel_info]|[data_label] data_label:= a unique string used for grouping and limiting critical data. For example, "selinux" to measure critical data for SELinux. + appraise_algos:= comma-separated list of hash algorithms + For example, "sha256,sha512" to only accept to appraise + files where the security.ima xattr was hashed with one + of these two algorithms. default policy: # PROC_SUPER_MAGIC diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index 1536e6f5eb22..cb86da0e562b 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -960,7 +960,7 @@ enum { Opt_fsuuid, Opt_uid_eq, Opt_euid_eq, Opt_fowner_eq, Opt_uid_gt, Opt_euid_gt, Opt_fowner_gt, Opt_uid_lt, Opt_euid_lt, Opt_fowner_lt, - Opt_appraise_type, Opt_appraise_flag, + Opt_appraise_type, Opt_appraise_flag, Opt_appraise_algos, Opt_permit_directio, Opt_pcr, Opt_template, Opt_keyrings, Opt_label, Opt_err }; @@ -995,6 +995,7 @@ static const match_table_t policy_tokens = { {Opt_fowner_lt, "fowner<%s"}, {Opt_appraise_type, "appraise_type=%s"}, {Opt_appraise_flag, "appraise_flag=%s"}, + {Opt_appraise_algos, "appraise_algos=%s"}, {Opt_permit_directio, "permit_directio"}, {Opt_pcr, "pcr=%s"}, {Opt_template, "template=%s"}, @@ -1095,7 +1096,8 @@ static bool ima_validate_rule(struct ima_rule_entry *entry) return false; if (entry->action != APPRAISE && - entry->flags & (IMA_DIGSIG_REQUIRED | IMA_MODSIG_ALLOWED | IMA_CHECK_BLACKLIST)) + entry->flags & (IMA_DIGSIG_REQUIRED | IMA_MODSIG_ALLOWED | + IMA_CHECK_BLACKLIST | IMA_VALIDATE_ALGOS)) return false; /* @@ -1125,7 +1127,7 @@ static bool ima_validate_rule(struct ima_rule_entry *entry) IMA_UID | IMA_FOWNER | IMA_FSUUID | IMA_INMASK | IMA_EUID | IMA_PCR | IMA_FSNAME | IMA_DIGSIG_REQUIRED | - IMA_PERMIT_DIRECTIO)) + IMA_PERMIT_DIRECTIO | IMA_VALIDATE_ALGOS)) return false; break; @@ -1137,7 +1139,7 @@ static bool ima_validate_rule(struct ima_rule_entry *entry) IMA_INMASK | IMA_EUID | IMA_PCR | IMA_FSNAME | IMA_DIGSIG_REQUIRED | IMA_PERMIT_DIRECTIO | IMA_MODSIG_ALLOWED | - IMA_CHECK_BLACKLIST)) + IMA_CHECK_BLACKLIST | IMA_VALIDATE_ALGOS)) return false; break; @@ -1187,6 +1189,28 @@ static bool ima_validate_rule(struct ima_rule_entry *entry) return true; } +static unsigned int ima_parse_appraise_algos(char *arg) +{ + unsigned int res = 0; + int idx; + char *token; + + while ((token = strsep(&arg, ",")) != NULL) { + idx = match_string(hash_algo_name, HASH_ALGO__LAST, token); + + if (idx < 0) { + pr_err("unknown hash algorithm \"%s\"", + token); + return 0; + } + + /* Add the hash algorithm to the 'allowed' bitfield */ + res |= (1U << idx); + } + + return res; +} + static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) { struct audit_buffer *ab; @@ -1522,6 +1546,25 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) else result = -EINVAL; break; + case Opt_appraise_algos: + ima_log_string(ab, "appraise_algos", args[0].from); + + if (entry->allowed_algos) { + result = -EINVAL; + break; + } + + entry->allowed_algos = + ima_parse_appraise_algos(args[0].from); + /* invalid or empty list of algorithms */ + if (!entry->allowed_algos) { + result = -EINVAL; + break; + } + + entry->flags |= IMA_VALIDATE_ALGOS; + + break; case Opt_permit_directio: entry->flags |= IMA_PERMIT_DIRECTIO; break; @@ -1714,6 +1757,23 @@ static void ima_show_rule_opt_list(struct seq_file *m, seq_printf(m, "%s%s", i ? "|" : "", opt_list->items[i]); } +static void ima_policy_show_appraise_algos(struct seq_file *m, + unsigned int allowed_hashes) +{ + int idx, list_size = 0; + + for (idx = 0; idx < HASH_ALGO__LAST; idx++) { + if (!(allowed_hashes & (1U << idx))) + continue; + + /* only add commas if the list contains multiple entries */ + if (list_size++) + seq_puts(m, ","); + + seq_puts(m, hash_algo_name[idx]); + } +} + int ima_policy_show(struct seq_file *m, void *v) { struct ima_rule_entry *entry = v; @@ -1825,6 +1885,12 @@ int ima_policy_show(struct seq_file *m, void *v) seq_puts(m, " "); } + if (entry->flags & IMA_VALIDATE_ALGOS) { + seq_puts(m, "appraise_algos="); + ima_policy_show_appraise_algos(m, entry->allowed_algos); + seq_puts(m, " "); + } + for (i = 0; i < MAX_LSM_RULES; i++) { if (entry->lsm[i].rule) { switch (i) { -- cgit v1.2.3-59-g8ed1b From 4f2946aa0c45c78b4f4ef101bab9694e38c68db0 Mon Sep 17 00:00:00 2001 From: THOBY Simon Date: Mon, 16 Aug 2021 08:11:01 +0000 Subject: IMA: introduce a new policy option func=SETXATTR_CHECK While users can restrict the accepted hash algorithms for the security.ima xattr file signature when appraising said file, users cannot restrict the algorithms that can be set on that attribute: any algorithm built in the kernel is accepted on a write. Define a new value for the ima policy option 'func' that restricts globally the hash algorithms accepted when writing the security.ima xattr. When a policy contains a rule of the form appraise func=SETXATTR_CHECK appraise_algos=sha256,sha384,sha512 only values corresponding to one of these three digest algorithms will be accepted for writing the security.ima xattr. Attempting to write the attribute using another algorithm (or "free-form" data) will be denied with an audit log message. In the absence of such a policy rule, the default is still to only accept hash algorithms built in the kernel (with all the limitations that entails). Signed-off-by: THOBY Simon Reviewed-by: Lakshmi Ramasubramanian Signed-off-by: Mimi Zohar --- Documentation/ABI/testing/ima_policy | 9 ++++- security/integrity/ima/ima.h | 6 ++- security/integrity/ima/ima_appraise.c | 29 ++++++++++--- security/integrity/ima/ima_main.c | 2 +- security/integrity/ima/ima_policy.c | 76 ++++++++++++++++++++++++++++++----- 5 files changed, 104 insertions(+), 18 deletions(-) (limited to 'security') diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy index b0e3d278e799..5c2798534950 100644 --- a/Documentation/ABI/testing/ima_policy +++ b/Documentation/ABI/testing/ima_policy @@ -30,9 +30,10 @@ Description: [appraise_flag=] [appraise_algos=] [keyrings=] base: func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK] - [FIRMWARE_CHECK] + [FIRMWARE_CHECK] [KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK] [KEXEC_CMDLINE] [KEY_CHECK] [CRITICAL_DATA] + [SETXATTR_CHECK] mask:= [[^]MAY_READ] [[^]MAY_WRITE] [[^]MAY_APPEND] [[^]MAY_EXEC] fsmagic:= hex value @@ -138,3 +139,9 @@ Description: keys added to .builtin_trusted_keys or .ima keyring: measure func=KEY_CHECK keyrings=.builtin_trusted_keys|.ima + + Example of the special SETXATTR_CHECK appraise rule, that + restricts the hash algorithms allowed when writing to the + security.ima xattr of a file: + + appraise func=SETXATTR_CHECK appraise_algos=sha256,sha384,sha512 diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index bcaf818fb647..be965a8715e4 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -46,6 +46,9 @@ enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8, TPM_PCR10 = 10 }; /* current content of the policy */ extern int ima_policy_flag; +/* bitset of digests algorithms allowed in the setxattr hook */ +extern atomic_t ima_setxattr_allowed_hash_algorithms; + /* set during initialization */ extern int ima_hash_algo __ro_after_init; extern int ima_sha1_idx __ro_after_init; @@ -198,6 +201,7 @@ static inline unsigned int ima_hash_key(u8 *digest) hook(KEXEC_CMDLINE, kexec_cmdline) \ hook(KEY_CHECK, key) \ hook(CRITICAL_DATA, critical_data) \ + hook(SETXATTR_CHECK, setxattr_check) \ hook(MAX_CHECK, none) #define __ima_hook_enumify(ENUM, str) ENUM, @@ -288,7 +292,7 @@ int ima_match_policy(struct user_namespace *mnt_userns, struct inode *inode, const char *func_data, unsigned int *allowed_algos); void ima_init_policy(void); void ima_update_policy(void); -void ima_update_policy_flag(void); +void ima_update_policy_flags(void); ssize_t ima_parse_add_rule(char *); void ima_delete_rules(void); int ima_check_policy(void); diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index e2edef8a9185..8f1eb7ef041e 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -595,12 +595,32 @@ static int validate_hash_algo(struct dentry *dentry, { char *path = NULL, *pathbuf = NULL; enum hash_algo xattr_hash_algo; + const char *errmsg = "unavailable-hash-algorithm"; + unsigned int allowed_hashes; xattr_hash_algo = ima_get_hash_algo(xattr_value, xattr_value_len); - if (likely(xattr_hash_algo == ima_hash_algo || - crypto_has_alg(hash_algo_name[xattr_hash_algo], 0, 0))) - return 0; + allowed_hashes = atomic_read(&ima_setxattr_allowed_hash_algorithms); + + if (allowed_hashes) { + /* success if the algorithm is allowed in the ima policy */ + if (allowed_hashes & (1U << xattr_hash_algo)) + return 0; + + /* + * We use a different audit message when the hash algorithm + * is denied by a policy rule, instead of not being built + * in the kernel image + */ + errmsg = "denied-hash-algorithm"; + } else { + if (likely(xattr_hash_algo == ima_hash_algo)) + return 0; + + /* allow any xattr using an algorithm built in the kernel */ + if (crypto_has_alg(hash_algo_name[xattr_hash_algo], 0, 0)) + return 0; + } pathbuf = kmalloc(PATH_MAX, GFP_KERNEL); if (!pathbuf) @@ -609,8 +629,7 @@ static int validate_hash_algo(struct dentry *dentry, path = dentry_path(dentry, pathbuf, PATH_MAX); integrity_audit_msg(AUDIT_INTEGRITY_DATA, d_inode(dentry), path, - "set_data", "unavailable-hash-algorithm", - -EACCES, 0); + "set_data", errmsg, -EACCES, 0); kfree(pathbuf); diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index af6367ba34ee..a734f7d5292c 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -1051,7 +1051,7 @@ static int __init init_ima(void) pr_warn("Couldn't register LSM notifier, error %d\n", error); if (!error) - ima_update_policy_flag(); + ima_update_policy_flags(); return error; } diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index cb86da0e562b..9eaa509f487a 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -53,6 +53,8 @@ int ima_policy_flag; static int temp_ima_appraise; static int build_ima_appraise __ro_after_init; +atomic_t ima_setxattr_allowed_hash_algorithms; + #define MAX_LSM_RULES 6 enum lsm_rule_types { LSM_OBJ_USER, LSM_OBJ_ROLE, LSM_OBJ_TYPE, LSM_SUBJ_USER, LSM_SUBJ_ROLE, LSM_SUBJ_TYPE @@ -720,24 +722,57 @@ int ima_match_policy(struct user_namespace *mnt_userns, struct inode *inode, return action; } -/* - * Initialize the ima_policy_flag variable based on the currently - * loaded policy. Based on this flag, the decision to short circuit - * out of a function or not call the function in the first place - * can be made earlier. +/** + * ima_update_policy_flags() - Update global IMA variables + * + * Update ima_policy_flag and ima_setxattr_allowed_hash_algorithms + * based on the currently loaded policy. + * + * With ima_policy_flag, the decision to short circuit out of a function + * or not call the function in the first place can be made earlier. + * + * With ima_setxattr_allowed_hash_algorithms, the policy can restrict the + * set of hash algorithms accepted when updating the security.ima xattr of + * a file. + * + * Context: called after a policy update and at system initialization. */ -void ima_update_policy_flag(void) +void ima_update_policy_flags(void) { struct ima_rule_entry *entry; + int new_policy_flag = 0; + rcu_read_lock(); list_for_each_entry(entry, ima_rules, list) { + /* + * SETXATTR_CHECK rules do not implement a full policy check + * because rule checking would probably have an important + * performance impact on setxattr(). As a consequence, only one + * SETXATTR_CHECK can be active at a given time. + * Because we want to preserve that property, we set out to use + * atomic_cmpxchg. Either: + * - the atomic was non-zero: a setxattr hash policy is + * already enforced, we do nothing + * - the atomic was zero: no setxattr policy was set, enable + * the setxattr hash policy + */ + if (entry->func == SETXATTR_CHECK) { + atomic_cmpxchg(&ima_setxattr_allowed_hash_algorithms, + 0, entry->allowed_algos); + /* SETXATTR_CHECK doesn't impact ima_policy_flag */ + continue; + } + if (entry->action & IMA_DO_MASK) - ima_policy_flag |= entry->action; + new_policy_flag |= entry->action; } + rcu_read_unlock(); ima_appraise |= (build_ima_appraise | temp_ima_appraise); if (!ima_appraise) - ima_policy_flag &= ~IMA_APPRAISE; + new_policy_flag &= ~IMA_APPRAISE; + + ima_policy_flag = new_policy_flag; } static int ima_appraise_flag(enum ima_hooks func) @@ -903,7 +938,9 @@ void __init ima_init_policy(void) ARRAY_SIZE(critical_data_rules), IMA_DEFAULT_POLICY); - ima_update_policy_flag(); + atomic_set(&ima_setxattr_allowed_hash_algorithms, 0); + + ima_update_policy_flags(); } /* Make sure we have a valid policy, at least containing some rules. */ @@ -943,7 +980,7 @@ void ima_update_policy(void) */ kfree(arch_policy_entry); } - ima_update_policy_flag(); + ima_update_policy_flags(); /* Custom IMA policy has been loaded */ ima_process_queued_keys(); @@ -1176,6 +1213,23 @@ static bool ima_validate_rule(struct ima_rule_entry *entry) if (ima_rule_contains_lsm_cond(entry)) return false; + break; + case SETXATTR_CHECK: + /* any action other than APPRAISE is unsupported */ + if (entry->action != APPRAISE) + return false; + + /* SETXATTR_CHECK requires an appraise_algos parameter */ + if (!(entry->flags & IMA_VALIDATE_ALGOS)) + return false; + + /* + * full policies are not supported, they would have too + * much of a performance impact + */ + if (entry->flags & ~(IMA_FUNC | IMA_VALIDATE_ALGOS)) + return false; + break; default: return false; @@ -1332,6 +1386,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) entry->func = KEY_CHECK; else if (strcmp(args[0].from, "CRITICAL_DATA") == 0) entry->func = CRITICAL_DATA; + else if (strcmp(args[0].from, "SETXATTR_CHECK") == 0) + entry->func = SETXATTR_CHECK; else result = -EINVAL; if (!result) -- cgit v1.2.3-59-g8ed1b From 8ecd39cb61d9e302716d73be329c4d4822c24931 Mon Sep 17 00:00:00 2001 From: THOBY Simon Date: Mon, 16 Aug 2021 08:11:01 +0000 Subject: IMA: prevent SETXATTR_CHECK policy rules with unavailable algorithms SETXATTR_CHECK policy rules assume that any algorithm listed in the 'appraise_algos' flag must be accepted when performing setxattr() on the security.ima xattr. However nothing checks that they are available in the current kernel. A userland application could hash a file with a digest that the kernel wouldn't be able to verify. However, if SETXATTR_CHECK is not in use, the kernel already forbids that xattr write. Verify that algorithms listed in appraise_algos are available to the current kernel and reject the policy update otherwise. This will fix the inconsistency between SETXATTR_CHECK and non-SETXATTR_CHECK behaviors. That filtering is only performed in ima_parse_appraise_algos() when updating policies so that we do not have to pay the price of allocating a hash object every time validate_hash_algo() is called in ima_inode_setxattr(). Signed-off-by: THOBY Simon Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_policy.c | 6 ++++++ 1 file changed, 6 insertions(+) (limited to 'security') diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index 9eaa509f487a..87b9b71cb820 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -1258,6 +1258,12 @@ static unsigned int ima_parse_appraise_algos(char *arg) return 0; } + if (!crypto_has_alg(hash_algo_name[idx], 0, 0)) { + pr_err("unavailable hash algorithm \"%s\", check your kernel configuration", + token); + return 0; + } + /* Add the hash algorithm to the 'allowed' bitfield */ res |= (1U << idx); } -- cgit v1.2.3-59-g8ed1b From cb181da161963eddc9de0000de6ab2c7942be219 Mon Sep 17 00:00:00 2001 From: THOBY Simon Date: Sun, 22 Aug 2021 08:55:26 +0000 Subject: IMA: reject unknown hash algorithms in ima_get_hash_algo The new function validate_hash_algo() assumed that ima_get_hash_algo() always return a valid 'enum hash_algo', but it returned the user-supplied value present in the digital signature without any bounds checks. Update ima_get_hash_algo() to always return a valid hash algorithm, defaulting on 'ima_hash_algo' when the user-supplied value inside the xattr is invalid. Signed-off-by: THOBY Simon Reported-by: syzbot+e8bafe7b82c739eaf153@syzkaller.appspotmail.com Fixes: 50f742dd9147 ("IMA: block writes of the security.ima xattr with unsupported algorithms") Reviewed-by: Lakshmi Ramasubramanian Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_appraise.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'security') diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index 8f1eb7ef041e..dbba51583e7c 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -185,7 +185,8 @@ enum hash_algo ima_get_hash_algo(const struct evm_ima_xattr_data *xattr_value, switch (xattr_value->type) { case EVM_IMA_XATTR_DIGSIG: sig = (typeof(sig))xattr_value; - if (sig->version != 2 || xattr_len <= sizeof(*sig)) + if (sig->version != 2 || xattr_len <= sizeof(*sig) + || sig->hash_algo >= HASH_ALGO__LAST) return ima_hash_algo; return sig->hash_algo; break; -- cgit v1.2.3-59-g8ed1b