Re: [PATCH v8 10/19] ima: Implement hierarchical processing of file accesses

[Christian Brauner]

On Tue, Jan 04, 2022 at 12:04:07PM -0500, Stefan Berger wrote:
> From: Stefan Berger <stefanb@xxxxxxxxxxxxx>
> 
> 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.
> 
> __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 also be NULL due to late initialization of the
> IMA namespace.
> 
> Signed-off-by: Stefan Berger <stefanb@xxxxxxxxxxxxx>
> ---
>  include/linux/ima.h               |  6 +++++
>  security/integrity/ima/ima_main.c | 37 +++++++++++++++++++++++++++----
>  2 files changed, 39 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index b6ab66a546ae..fcee2a51bb87 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -65,6 +65,12 @@ static inline const char * const *arch_get_ima_policy(void)
>  }
>  #endif
>  
> +static inline struct user_namespace
> +*ima_ns_to_user_ns(struct ima_namespace *ns)
> +{
> +	return current_user_ns();
> +}
> +
>  #else
>  static inline enum hash_algo ima_get_current_hash_algo(void)
>  {
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 621685d4eb95..51b0ef1cebbe 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -200,10 +200,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;
> @@ -395,6 +395,35 @@ static int process_measurement(struct ima_namespace *ns,
>  	return 0;
>  }
>  
> +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 user_namespace *user_ns = ima_ns_to_user_ns(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 -EACCES:
> +				/* return this error at the end but continue */
> +				ret = -EACCES;
> +				break;

This seems risky. Every error not -EACCES will be counted as a success.
It doesn't look like __process_measurement() will return anything else
but I would still place a WARN_ON() or WARN_ON_ONCE() in there to make
that assumption explicit.

Right now it looks like your only error condition is -EACCES and non-ima
cracks like me need to read through __process_measurement() to figure
out that that's ok. With a WARN_ON* in there I'd not have needed to bother.

switch (rc) {
case -EACCES:
	/* return this error at the end but continue */
	ret = -EACCES;
	break
default:
	WARN_ON_ONCE(true);
}

or sm similar.