| Message ID | 20241203233424.287880-1-zohar@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | [v2] ima: instantiate the bprm_creds_for_exec() hook | expand |
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 > >
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 > > > >
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 > > > > > > > >
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...?
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/
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>
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),