Re: [PATCH v12 14/26] ima: Implement hierarchical processing of file accesses

[Serge E. Hallyn]

On Wed, Apr 20, 2022 at 10:06:21AM -0400, Stefan Berger wrote:
> Implement hierarchical processing of file accesses in IMA namespaces by
> walking the list of user namespaces towards the root. This way file
> accesses can be audited in an IMA namespace and also be evaluated against
> the IMA policies of parent IMA namespaces.
> 
> Pass the user_namespace into process_measurement since we will be walking
> the hierarchy of user_namespaces towards the init_user_ns and we can easily
> derive the ima_namespace from the user_namespace.
> 
> __process_measurement() returns either 0 or -EACCES. For hierarchical
> processing remember the -EACCES returned by this function but continue
> to the parent user namespace. At the end either return 0 or -EACCES
> if an error occurred in one of the IMA namespaces.
> 
> Currently the ima_ns pointer of the user_namespace is always NULL except
> at the init_user_ns, so test ima_ns for NULL pointer and skip the call to
> __process_measurement() if it is NULL. Once IMA namespacing is fully
> enabled, the pointer may still be NULL due to late initialization of the
> IMA namespace.
> 
> Signed-off-by: Stefan Berger <stefanb@xxxxxxxxxxxxx>
> Reviewed-by: Mimi Zohar <zohar@xxxxxxxxxxxxx>

Reviewed-by: Serge Hallyn <serge@xxxxxxxxxx>

> 
> ---
> 
> v10:
>   - Fixed compilation issue
> 
> v9:
>   - Switch callers to pass user_namespace rather than ima_namespace with
>     potential NULL pointer
>   - Add default case to switch statement and warn if this happens
>   - Implement ima_ns_from_user_ns() in this patch now
> ---
>  security/integrity/ima/ima.h      |  8 ++++
>  security/integrity/ima/ima_main.c | 76 +++++++++++++++++++++++--------
>  2 files changed, 65 insertions(+), 19 deletions(-)
> 
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index 626a6ce2453c..2775a6d89e6d 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -497,4 +497,12 @@ struct user_namespace *ima_user_ns_from_file(const struct file *filp)
>  	return file_inode(filp)->i_sb->s_user_ns;
>  }
>  
> +static inline struct ima_namespace
> +*ima_ns_from_user_ns(struct user_namespace *user_ns)
> +{
> +	if (user_ns == &init_user_ns)
> +		return &init_ima_ns;
> +	return NULL;
> +}
> +
>  #endif /* __LINUX_IMA_H */
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index d44faf1c065d..17eb2971eb36 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -196,10 +196,10 @@ void ima_file_free(struct file *file)
>  	ima_check_last_writer(iint, inode, file);
>  }
>  
> -static int process_measurement(struct ima_namespace *ns,
> -			       struct file *file, const struct cred *cred,
> -			       u32 secid, char *buf, loff_t size, int mask,
> -			       enum ima_hooks func)
> +static int __process_measurement(struct ima_namespace *ns,
> +				 struct file *file, const struct cred *cred,
> +				 u32 secid, char *buf, loff_t size, int mask,
> +				 enum ima_hooks func)
>  {
>  	struct inode *inode = file_inode(file);
>  	struct integrity_iint_cache *iint = NULL;
> @@ -391,6 +391,41 @@ static int process_measurement(struct ima_namespace *ns,
>  	return 0;
>  }
>  
> +static int process_measurement(struct user_namespace *user_ns,
> +			       struct file *file, const struct cred *cred,
> +			       u32 secid, char *buf, loff_t size, int mask,
> +			       enum ima_hooks func)
> +{
> +	struct ima_namespace *ns;
> +	int ret = 0;
> +
> +	while (user_ns) {
> +		ns = ima_ns_from_user_ns(user_ns);
> +		if (ns) {
> +			int rc;
> +
> +			rc = __process_measurement(ns, file, cred, secid, buf,
> +						   size, mask, func);
> +			switch (rc) {
> +			case 0:
> +				break;
> +			case -EACCES:
> +				/* return this error at the end but continue */
> +				ret = -EACCES;
> +				break;
> +			default:
> +				/* should not happen */
> +				ret = -EACCES;
> +				WARN_ON_ONCE(true);
> +			}
> +		}
> +
> +		user_ns = user_ns->parent;
> +	}
> +
> +	return ret;
> +}
> +
>  /**
>   * ima_file_mmap - based on policy, collect/store measurement.
>   * @file: pointer to the file to be measured (May be NULL)
> @@ -404,13 +439,14 @@ static int process_measurement(struct ima_namespace *ns,
>   */
>  int ima_file_mmap(struct file *file, unsigned long prot)
>  {
> -	struct ima_namespace *ns = &init_ima_ns;
> +	struct user_namespace *user_ns = current_user_ns();
>  	u32 secid;
>  
>  	if (file && (prot & PROT_EXEC)) {
>  		security_current_getsecid_subj(&secid);
> -		return process_measurement(ns, file, current_cred(), secid,
> -					   NULL, 0, MAY_EXEC, MMAP_CHECK);
> +		return process_measurement(user_ns, file, current_cred(),
> +					   secid, NULL, 0, MAY_EXEC,
> +					   MMAP_CHECK);
>  	}
>  
>  	return 0;
> @@ -486,19 +522,19 @@ int ima_file_mprotect(struct vm_area_struct *vma, unsigned long prot)
>   */
>  int ima_bprm_check(struct linux_binprm *bprm)
>  {
> -	struct ima_namespace *ns = &init_ima_ns;
> +	struct user_namespace *user_ns = current_user_ns();
>  	int ret;
>  	u32 secid;
>  
>  	security_current_getsecid_subj(&secid);
> -	ret = process_measurement(ns, bprm->file, current_cred(), secid, NULL,
> -				  0, MAY_EXEC, BPRM_CHECK);
> +	ret = process_measurement(user_ns, bprm->file, current_cred(), secid,
> +				  NULL, 0, MAY_EXEC, BPRM_CHECK);
>  	if (ret)
>  		return ret;
>  
>  	security_cred_getsecid(bprm->cred, &secid);
> -	return process_measurement(ns, bprm->file, bprm->cred, secid, NULL, 0,
> -				   MAY_EXEC, CREDS_CHECK);
> +	return process_measurement(user_ns, bprm->file, bprm->cred, secid,
> +				   NULL, 0, MAY_EXEC, CREDS_CHECK);
>  }
>  
>  /**
> @@ -513,11 +549,12 @@ int ima_bprm_check(struct linux_binprm *bprm)
>   */
>  int ima_file_check(struct file *file, int mask)
>  {
> -	struct ima_namespace *ns = &init_ima_ns;
> +	struct user_namespace *user_ns = current_user_ns();
>  	u32 secid;
>  
>  	security_current_getsecid_subj(&secid);
> -	return process_measurement(ns, file, current_cred(), secid, NULL, 0,
> +	return process_measurement(user_ns, file, current_cred(), secid,
> +				   NULL, 0,
>  				   mask & (MAY_READ | MAY_WRITE | MAY_EXEC |
>  					   MAY_APPEND), FILE_CHECK);
>  }
> @@ -718,7 +755,7 @@ void ima_post_path_mknod(struct user_namespace *mnt_userns,
>  int ima_read_file(struct file *file, enum kernel_read_file_id read_id,
>  		  bool contents)
>  {
> -	struct ima_namespace *ns = &init_ima_ns;
> +	struct user_namespace *user_ns = current_user_ns();
>  	enum ima_hooks func;
>  	u32 secid;
>  
> @@ -741,7 +778,7 @@ int ima_read_file(struct file *file, enum kernel_read_file_id read_id,
>  	/* Read entire file for all partial reads. */
>  	func = read_idmap[read_id] ?: FILE_CHECK;
>  	security_current_getsecid_subj(&secid);
> -	return process_measurement(ns, file, current_cred(), secid, NULL,
> +	return process_measurement(user_ns, file, current_cred(), secid, NULL,
>  				   0, MAY_READ, func);
>  }
>  
> @@ -769,7 +806,8 @@ const int read_idmap[READING_MAX_ID] = {
>  int ima_post_read_file(struct file *file, void *buf, loff_t size,
>  		       enum kernel_read_file_id read_id)
>  {
> -	struct ima_namespace *ns = &init_ima_ns;
> +	struct user_namespace *user_ns = current_user_ns();
> +	struct ima_namespace *ns = ima_ns_from_user_ns(user_ns);
>  	enum ima_hooks func;
>  	u32 secid;
>  
> @@ -786,8 +824,8 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size,
>  
>  	func = read_idmap[read_id] ?: FILE_CHECK;
>  	security_current_getsecid_subj(&secid);
> -	return process_measurement(ns, file, current_cred(), secid, buf, size,
> -				   MAY_READ, func);
> +	return process_measurement(user_ns, file, current_cred(), secid,
> +				   buf, size, MAY_READ, func);
>  }
>  
>  /**
> -- 
> 2.34.1