[PATCH v2] ima: instantiate the bprm_creds_for_exec() hook

[PATCH v2] ima: instantiate the bprm_creds_for_exec() hook

[Mimi Zohar]

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.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
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) {
+		bprm = container_of(&file, struct linux_binprm, file);
+		if (bprm->is_check)
+			return true;
+	}
+	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
+ * 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

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

[Mickaël Salaün]

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@huawei.com>

> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>

With some minor comments, this looks good to me. I'll include this patch
or the next one in my patch series.  Thanks!

> ---
> 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;

> +		bprm = container_of(&file, struct linux_binprm, file);
> +		if (bprm->is_check)
> +			return true;

return bprm->is_check;

> +	}
> +	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

> + * 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.

> +	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
> 
> 

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

[Mimi Zohar]

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@huawei.com>

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@huawei.com>
> > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> 
> 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
> > 
> > 

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

[Mickaël Salaün]

On Wed, Dec 04, 2024 at 09:57:42AM -0500, Mimi Zohar wrote:
> 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@huawei.com>
> 
> 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@huawei.com>
> > > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> > 
> > 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).
> +        */

Looks good!  Feel free to send a new patch with these changes.

> 
> > 
> > > +	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
> > > 
> > > 
> 
> 

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

[Stefan Berger]

On 12/3/24 6:34 PM, Mimi Zohar wrote:
> Like direct file execution (e.g. ./script.sh), indirect file exection

typo: execution

> (e.g. sh script.sh) needs to be measured and appraised.  Instantiate

If I understand the underlying patches correctly then 'sh script.sh' 
would be evaluated with execveat(AT_CHECK) but this requires the execute 
flag to be set. To maintain backwards compatibility  sh cannot assume 
that script.sh has the execute flag set since it doesn't need today:

$ echo 'echo hi' > foo.sh
$ sh foo.sh
hi

the same is true for python:

$ echo 'print("hi")' > foo.py
$ python foo.py
hi

I am not sure which interpreters are going to be able to take advantage 
of this or whether they will behave differently if the x bit is set 
versus when it is not set...?

[PATCH v2] ima: instantiate the bprm_creds_for_exec() hook

[Mimi Zohar]

Like direct file execution (e.g. ./script.sh), indirect file execution
(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.

Co-developed-by: Roberto Sassu <roberto.sassu@huawei.com>
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
Changelog v3:
- Mickael: add comment ima_bprm_creds_for_exec(), minor code cleanup,
  add Co-developed-by tag.

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 | 27 +++++++++++++++++++++++--
 security/integrity/ima/ima_main.c     | 29 +++++++++++++++++++++++++++
 3 files changed, 55 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..fc0d1f3cceca 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,17 @@ 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;
+
+	if (func == BPRM_CHECK) {
+		bprm = container_of(&file, struct linux_binprm, file);
+		return bprm->is_check;
+	}
+	return false;
+}
+
 /*
  * ima_appraise_measurement - appraise file measurement
  *
@@ -483,6 +495,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 +507,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 +592,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 +612,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..5d4ac8aa2f1f 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -554,6 +554,34 @@ 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_EXECVE_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)
+{
+	/*
+	 * 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 +1205,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

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

[Mimi Zohar]

This should have been patch v3.

Mimi

On Wed, 2024-12-04 at 14:25 -0500, Mimi Zohar wrote:
> Like direct file execution (e.g. ./script.sh), indirect file execution
> (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.
> 
> Co-developed-by: Roberto Sassu <roberto.sassu@huawei.com>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
> Changelog v3:
> - Mickael: add comment ima_bprm_creds_for_exec(), minor code cleanup,
>   add Co-developed-by tag.
> 
> 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 | 27 +++++++++++++++++++++++--
>  security/integrity/ima/ima_main.c     | 29 +++++++++++++++++++++++++++
>  3 files changed, 55 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..fc0d1f3cceca 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,17 @@ 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;
> +
> +	if (func == BPRM_CHECK) {
> +		bprm = container_of(&file, struct linux_binprm, file);
> +		return bprm->is_check;
> +	}
> +	return false;
> +}
> +
>  /*
>   * ima_appraise_measurement - appraise file measurement
>   *
> @@ -483,6 +495,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 +507,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 +592,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 +612,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..5d4ac8aa2f1f 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -554,6 +554,34 @@ 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_EXECVE_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)
> +{
> +	/*
> +	 * 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 +1205,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),

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

[Mickaël Salaün]

On Wed, Dec 04, 2024 at 02:01:02PM -0500, Stefan Berger wrote:
> 
> 
> On 12/3/24 6:34 PM, Mimi Zohar wrote:
> > Like direct file execution (e.g. ./script.sh), indirect file exection
> 
> typo: execution
> 
> > (e.g. sh script.sh) needs to be measured and appraised.  Instantiate
> 
> If I understand the underlying patches correctly then 'sh script.sh' would
> be evaluated with execveat(AT_CHECK) but this requires the execute flag to
> be set. To maintain backwards compatibility  sh cannot assume that script.sh
> has the execute flag set since it doesn't need today:
> 
> $ echo 'echo hi' > foo.sh
> $ sh foo.sh
> hi
> 
> the same is true for python:
> 
> $ echo 'print("hi")' > foo.py
> $ python foo.py
> hi
> 
> I am not sure which interpreters are going to be able to take advantage of
> this or whether they will behave differently if the x bit is set versus when
> it is not set...?

This is a valid concern handled with two new securebits.  See the
related patch series and documentation:
https://lore.kernel.org/all/20241112191858.162021-3-mic@digikod.net/

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

[Stefan Berger]

On 12/3/24 6:34 PM, 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.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>

Tested-by: Stefan Berger <stefanb@linux.ibm.com>

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

[Paul Moore]

On Dec  4, 2024 Mimi Zohar <zohar@linux.ibm.com> wrote:
> 
> Like direct file execution (e.g. ./script.sh), indirect file execution
> (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.
> 
> Co-developed-by: Roberto Sassu <roberto.sassu@huawei.com>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
> Changelog v3:
> - Mickael: add comment ima_bprm_creds_for_exec(), minor code cleanup,
>   add Co-developed-by tag.
> 
> 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 | 27 +++++++++++++++++++++++--
>  security/integrity/ima/ima_main.c     | 29 +++++++++++++++++++++++++++
>  3 files changed, 55 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 */

I worry that "DATA_CHECK" is a bit vague, should we change the name so
that there is some hint of either userspace enforcement or
AT_EXECVE_CHECK?

What about AUDIT_INTEGRITY_DATA_USER?

--
paul-moore.com

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

[Mimi Zohar]

On Thu, 2024-12-05 at 19:30 -0500, Paul Moore wrote:
> On Dec  4, 2024 Mimi Zohar <zohar@linux.ibm.com> wrote:
> > 
> > Like direct file execution (e.g. ./script.sh), indirect file execution
> > (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.
> > 
> > Co-developed-by: Roberto Sassu <roberto.sassu@huawei.com>
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> > ---
> > Changelog v3:
> > - Mickael: add comment ima_bprm_creds_for_exec(), minor code cleanup,
> >   add Co-developed-by tag.
> > 
> > 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 | 27 +++++++++++++++++++++++--
> >  security/integrity/ima/ima_main.c     | 29 +++++++++++++++++++++++++++
> >  3 files changed, 55 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 */
> 
> I worry that "DATA_CHECK" is a bit vague, should we change the name so
> that there is some hint of either userspace enforcement or
> AT_EXECVE_CHECK?
> 
> What about AUDIT_INTEGRITY_DATA_USER?

The emphasis should be on userspace - AUDIT_INTEGRITY_USERSPACE.

Mimi

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

[Mickaël Salaün]

On Thu, Dec 05, 2024 at 10:10:36PM -0500, Mimi Zohar wrote:
> On Thu, 2024-12-05 at 19:30 -0500, Paul Moore wrote:
> > On Dec  4, 2024 Mimi Zohar <zohar@linux.ibm.com> wrote:
> > > 
> > > Like direct file execution (e.g. ./script.sh), indirect file execution
> > > (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.
> > > 
> > > Co-developed-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> > > ---
> > > Changelog v3:
> > > - Mickael: add comment ima_bprm_creds_for_exec(), minor code cleanup,
> > >   add Co-developed-by tag.
> > > 
> > > 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 | 27 +++++++++++++++++++++++--
> > >  security/integrity/ima/ima_main.c     | 29 +++++++++++++++++++++++++++
> > >  3 files changed, 55 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 */
> > 
> > I worry that "DATA_CHECK" is a bit vague, should we change the name so
> > that there is some hint of either userspace enforcement or
> > AT_EXECVE_CHECK?
> > 
> > What about AUDIT_INTEGRITY_DATA_USER?
> 
> The emphasis should be on userspace - AUDIT_INTEGRITY_USERSPACE.

Looks good, I'll send a new patch series with this change, following
https://lore.kernel.org/all/20241205160925.230119-9-mic@digikod.net/

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

[Mimi Zohar]

On Tue, 2024-12-10 at 17:34 +0100, Mickaël Salaün wrote:
> > > > +++ 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 */
> > > 
> > > I worry that "DATA_CHECK" is a bit vague, should we change the name so
> > > that there is some hint of either userspace enforcement or
> > > AT_EXECVE_CHECK?
> > > 
> > > What about AUDIT_INTEGRITY_DATA_USER?
> > 
> > The emphasis should be on userspace - AUDIT_INTEGRITY_USERSPACE.
> 
> Looks good, I'll send a new patch series with this change, following
> https://lore.kernel.org/all/20241205160925.230119-9-mic@digikod.net/

Sound good!  Thank you.

Mimi