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

[Lakshmi Ramasubramanian <nramas@linux.microsoft.com>]

On 8/4/2021 2:20 AM, THOBY Simon wrote:
> While users can restrict the accepted hash algorithms for the
> security.ima xattr file signature when appraising said file, users
> cannot restrict the algorithms that can be set on that attribute:
> any algorithm built in the kernel is accepted on a write.
> 
> Define a new value for the ima policy option 'func' that restricts
> globally the hash algorithms accepted when writing the security.ima
> xattr.
> 
> When a policy contains a rule of the form
> 	appraise func=SETXATTR_CHECK appraise_hash=sha256,sha384,sha512
> only values corresponding to one of these three digest algorithms
> will be accepted for writing the security.ima xattr.
> Attempting to write the attribute using another algorithm (or "free-form"
> data) will be denied with an audit log message.
> In the absence of such a policy rule, the default is still to only
> accept hash algorithms built in the kernel (with all the limitations
> that entails).
> 
> Signed-off-by: Simon Thoby <simon.thoby@viveris.fr>
> ---
>   Documentation/ABI/testing/ima_policy  |  9 +++-
>   security/integrity/ima/ima.h          |  6 ++-
>   security/integrity/ima/ima_appraise.c | 29 ++++++++--
>   security/integrity/ima/ima_main.c     |  2 +-
>   security/integrity/ima/ima_policy.c   | 76 +++++++++++++++++++++++----
>   5 files changed, 104 insertions(+), 18 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
> index aeb622698047..537be0e1720e 100644
> --- a/Documentation/ABI/testing/ima_policy
> +++ b/Documentation/ABI/testing/ima_policy
> @@ -30,9 +30,10 @@ Description:
>   				[appraise_flag=] [appraise_hash=] [keyrings=]
>   		  base:
>   			func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK]
> -			        [FIRMWARE_CHECK]
> +				[FIRMWARE_CHECK]
Is there a white space in the above line that is showing up as a change 
in this patch?

Other changes look good.

Reviewed-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>

  -lakshmi

>   				[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 8db0238a0fd6..7f2aa875fd72 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_hashes);
>   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 9aa0334b8e54..4949aabce36c 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -597,12 +597,32 @@ static int validate_hash_algo(struct dentry *dentry,
>   	int result = 0;
>   	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 result;
> +	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 result;
> +
> +		/*
> +		 * 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 result;
> +
> +		/* allow any xattr using an algorithm built in the kernel */
> +		if (crypto_has_alg(hash_algo_name[xattr_hash_algo], 0, 0))
> +			return result;
> +	}
>   
>   	result = -EACCES;
>   
> @@ -613,8 +633,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,
> -			    "collect_data", "unavailable-hash-algorithm",
> -			    result, 0);
> +			    "collect_data", errmsg, result, 0);
>   
>   	kfree(pathbuf);
>   
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 205138e7016d..f6389f0ff81b 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -1052,7 +1052,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 831ce0c690ec..c83f7c199610 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_hashes);
> +			/* 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_hash parameter */
> +		if (!(entry->flags & IMA_VALIDATE_HASH))
> +			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 +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)
>