On Wed, 2024-12-04 at 11:15 +0100, Mickaël Salaün wrote:
> On Tue, Dec 03, 2024 at 06:34:24PM -0500, Mimi Zohar wrote:
> > Like direct file execution (e.g. ./script.sh), indirect file exection
> > (e.g. sh script.sh) needs 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.
> >
> > Differentiate kernel and userspace enforced integrity audit messages.
> >
>
> I guess there is a missing tag:
>
> Co-developed-by: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
Having a different author with multiple "Signed-off-by" implies the patch
history, but adding the "Co-developed-by" is explicit. I'll add the Co-
developed-by tag.
>
> > Signed-off-by: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
> > Signed-off-by: Mimi Zohar <zohar@xxxxxxxxxxxxx>
>
> With some minor comments, this looks good to me. I'll include this patch
> or the next one in my patch series. Thanks!
Thank you.
>
> > ---
> > Changelog v2:
> > - Mickael: Use same audit messages with new audit message number
> > - Stefan Berger: Return boolean from is_bprm_creds_for_exec()
> >
> > include/uapi/linux/audit.h | 1 +
> > security/integrity/ima/ima_appraise.c | 28 +++++++++++++++++++++++++--
> > security/integrity/ima/ima_main.c | 22 +++++++++++++++++++++
> > 3 files changed, 49 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> > index 75e21a135483..826337905466 100644
> > --- a/include/uapi/linux/audit.h
> > +++ b/include/uapi/linux/audit.h
> > @@ -161,6 +161,7 @@
> > #define AUDIT_INTEGRITY_RULE 1805 /* policy rule */
> > #define AUDIT_INTEGRITY_EVM_XATTR 1806 /* New EVM-covered xattr */
> > #define AUDIT_INTEGRITY_POLICY_RULE 1807 /* IMA policy rules */
> > +#define AUDIT_INTEGRITY_DATA_CHECK 1808 /* Userspace enforced data integrity */
> >
> > #define AUDIT_KERNEL 2000 /* Asynchronous audit record. NOT A REQUEST. */
> >
> > diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> > index 656c709b974f..144e0b39fbcd 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>
> > @@ -469,6 +470,18 @@ int ima_check_blacklist(struct ima_iint_cache *iint,
> > return rc;
> > }
> >
> > +static bool is_bprm_creds_for_exec(enum ima_hooks func, struct file *file)
> > +{
> > + struct linux_binprm *bprm = NULL;
> > +
> > + if (func == BPRM_CHECK) {
>
> struct linux_binprm *bprm;
Local variables are normally defined at the beginning of the function.
>
> > + bprm = container_of(&file, struct linux_binprm, file);
> > + if (bprm->is_check)
> > + return true;
>
> return bprm->is_check;
Yes, that's better.
>
> > + }
> > + return false;
> > +}
> > +
> > /*
> > * ima_appraise_measurement - appraise file measurement
> > *
> > @@ -483,6 +496,7 @@ int ima_appraise_measurement(enum ima_hooks func, struct ima_iint_cache *iint,
> > int xattr_len, const struct modsig *modsig)
> > {
> > static const char op[] = "appraise_data";
> > + int audit_msgno = AUDIT_INTEGRITY_DATA;
> > const char *cause = "unknown";
> > struct dentry *dentry = file_dentry(file);
> > struct inode *inode = d_backing_inode(dentry);
> > @@ -494,6 +508,16 @@ int ima_appraise_measurement(enum ima_hooks func, struct ima_iint_cache *iint,
> > if (!(inode->i_opflags & IOP_XATTR) && !try_modsig)
> > return INTEGRITY_UNKNOWN;
> >
> > + /*
> > + * Unlike any of the other LSM hooks where the kernel enforces file
> > + * integrity, enforcing file integrity for the bprm_creds_for_exec()
> > + * LSM hook with the AT_EXECVE_CHECK flag is left up to the discretion
> > + * of the script interpreter(userspace). Differentiate kernel and
> > + * userspace enforced integrity audit messages.
> > + */
> > + if (is_bprm_creds_for_exec(func, file))
> > + audit_msgno = AUDIT_INTEGRITY_DATA_CHECK;
> > +
> > /* If reading the xattr failed and there's no modsig, error out. */
> > if (rc <= 0 && !try_modsig) {
> > if (rc && rc != -ENODATA)
> > @@ -569,7 +593,7 @@ int ima_appraise_measurement(enum ima_hooks func, struct ima_iint_cache *iint,
> > (iint->flags & IMA_FAIL_UNVERIFIABLE_SIGS))) {
> > status = INTEGRITY_FAIL;
> > cause = "unverifiable-signature";
> > - integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
> > + integrity_audit_msg(audit_msgno, inode, filename,
> > op, cause, rc, 0);
> > } else if (status != INTEGRITY_PASS) {
> > /* Fix mode, but don't replace file signatures. */
> > @@ -589,7 +613,7 @@ int ima_appraise_measurement(enum ima_hooks func, struct ima_iint_cache *iint,
> > status = INTEGRITY_PASS;
> > }
> >
> > - integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
> > + integrity_audit_msg(audit_msgno, inode, filename,
> > op, cause, rc, 0);
> > } else {
> > ima_cache_flags(iint, func);
> > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> > index 06132cf47016..f0830e6d0cda 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 - collect/store/appraise measurement.
> > + * @bprm: contains the linux_binprm structure
> > + *
> > + * Based on the IMA policy and the execvat(2) AT_CHECK flag, measure and
>
> AT_EXECVE_CHECK
Thanks, good catch.
>
> > + * 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)
> > +{
>
> We could have a comment explaining that ima_bprm_check() will not be
> called a second time bi the bprm_check_security hook if bprm->is_check
> is true because this hook would then not be called. This would not be a
> security issue anyway, just a useless call.
Proposed comment:
+ /*
+ * As security_bprm_check() is called multiple times, both
+ * the script and the shebang interpreter are measured, appraised,
+ * and audited. Limit usage of this LSM hook to just measuring,
+ * appraising, and auditing the indirect script execution
+ * (e.g. ./sh example.sh).
+ */
>
> > + 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
> >
> >
