Re: [RFC PATCH] ima: instantiate the bprm_creds_for_exec() hook

["Mickaël Salaün" <mic@digikod.net>]

CCing Jeff, Kees, Paul, and audit@

I guess this RFC is superseded by
https://lore.kernel.org/r/20241127210234.121546-1-zohar@linux.ibm.com
(and then doesn't need a reply) but for reference, here was may main
concern anyway.

On Wed, Nov 27, 2024 at 10:05:26AM -0500, Mimi Zohar wrote:
> Like direct file execution (e.g. ./script.sh), indirect file exection
> (e.g. sh script.sh) need to be measured and appraised.  Instantiate
> the new security_bprm_creds_for_exec() hook to measure and verify the
> indirect file's integrity.  Unlike direct file execution, indirect file
> execution is optionally enforced by the interpreter.
> 
> Define two new audit messages:
> - Userspace-enforcing-IMA-signature-required
> - Userspace-not-enforcing-IMA-signature-required
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
>  security/integrity/ima/ima_appraise.c | 24 +++++++++++++++++++++++-
>  security/integrity/ima/ima_main.c     | 22 ++++++++++++++++++++++
>  2 files changed, 45 insertions(+), 1 deletion(-)
> 
> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> index 656c709b974f..5a3b5cdecb51 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -8,6 +8,7 @@
>  #include <linux/module.h>
>  #include <linux/init.h>
>  #include <linux/file.h>
> +#include <linux/binfmts.h>
>  #include <linux/fs.h>
>  #include <linux/xattr.h>
>  #include <linux/magic.h>
> @@ -16,6 +17,7 @@
>  #include <linux/fsverity.h>
>  #include <keys/system_keyring.h>
>  #include <uapi/linux/fsverity.h>
> +#include <linux/securebits.h>
>  
>  #include "ima.h"
>  
> @@ -469,6 +471,26 @@ int ima_check_blacklist(struct ima_iint_cache *iint,
>  	return rc;
>  }
>  
> +static int is_bprm_creds_for_exec(enum ima_hooks func, struct file *file,
> +				  const char **cause)
> +{
> +	const struct cred *cred = current_cred();
> +	struct linux_binprm *bprm = NULL;
> +
> +	if (func == BPRM_CHECK) {
> +		bprm = container_of(&file, struct linux_binprm, file);
> +		if (!bprm->is_check)
> +			return 0;
> +
> +		if (cred->securebits & SECBIT_EXEC_RESTRICT_FILE)

The is_bprm_creds_for_exec() implementation from the next patch series
doesn't check securebits anymore, but for reference, LSMs should not
rely on caller's securebits to infer a behavior because user space could
just not check these bits.  For instance, on tailored systems such as
chromeOS, the libc could call execveat+AT_EXECVE_CHECK whatever
SECBIT_EXEC_RESTRICT_FILE is set or not:
https://lore.kernel.org/r/20241127.aizae7eeHohn@digikod.net

> +			*cause = "Userspace-enforcing-IMA-signature-required";
> +		else
> +			*cause = "Userspace-not-enforcing-IMA-signature-required";
> +		return 1;
> +	}
> +	return 0;
> +}
> +
>  /*
>   * ima_appraise_measurement - appraise file measurement
>   *
> @@ -502,7 +524,7 @@ int ima_appraise_measurement(enum ima_hooks func, struct ima_iint_cache *iint,
>  		if (iint->flags & IMA_DIGSIG_REQUIRED) {
>  			if (iint->flags & IMA_VERITY_REQUIRED)
>  				cause = "verity-signature-required";
> -			else
> +			else if (!is_bprm_creds_for_exec(func, file, &cause))
>  				cause = "IMA-signature-required";
>  		} else {
>  			cause = "missing-hash";
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 06132cf47016..2b5d6bae77a4 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -554,6 +554,27 @@ static int ima_bprm_check(struct linux_binprm *bprm)
>  				   MAY_EXEC, CREDS_CHECK);
>  }
>  
> +/**
> + * ima_bprm_creds_for_exec - based on policy, collect/store/appraise measurement.
> + * @bprm: contains the linux_binprm structure
> + *
> + * Based on the IMA policy and the execvat(2) AT_CHECK flag, measure and
> + * appraise the integrity of a file to be executed by script interpreters.
> + * Unlike any of the other LSM hooks where the kernel enforces file integrity,
> + * enforcing file integrity is left up to the discretion of the script
> + * interpreter (userspace).
> + *
> + * On success return 0.  On integrity appraisal error, assuming the file
> + * is in policy and IMA-appraisal is in enforcing mode, return -EACCES.
> + */
> +static int ima_bprm_creds_for_exec(struct linux_binprm *bprm)
> +{
> +	if (!bprm->is_check)
> +		return 0;
> +
> +	return ima_bprm_check(bprm);
> +}
> +
>  /**
>   * ima_file_check - based on policy, collect/store measurement.
>   * @file: pointer to the file to be measured
> @@ -1177,6 +1198,7 @@ static int __init init_ima(void)
>  
>  static struct security_hook_list ima_hooks[] __ro_after_init = {
>  	LSM_HOOK_INIT(bprm_check_security, ima_bprm_check),
> +	LSM_HOOK_INIT(bprm_creds_for_exec, ima_bprm_creds_for_exec),
>  	LSM_HOOK_INIT(file_post_open, ima_file_check),
>  	LSM_HOOK_INIT(inode_post_create_tmpfile, ima_post_create_tmpfile),
>  	LSM_HOOK_INIT(file_release, ima_file_free),
> -- 
> 2.47.0
> 
>