| Message ID | 20230126163812.1870942-2-roberto.sassu@huaweicloud.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | [v3,1/2] ima: Align ima_file_mmap() parameters with mmap_file LSM hook | expand |
On Thu, 2023-01-26 at 17:38 +0100, Roberto Sassu wrote: > From: Roberto Sassu <roberto.sassu@huawei.com> > > Commit 98de59bfe4b2f ("take calculation of final prot in > security_mmap_file() into a helper") caused ima_file_mmap() to receive the > protections requested by the application and not those applied by the > kernel. > > After restoring the original MMAP_CHECK behavior with a patch, existing > systems might be broken due to not being ready to handle new entries > (previously missing) in the IMA measurement list. Is this a broken system or a broken attestation server? The attestation server might not be able to handle the additional measurements, but the system, itself, is not broken. "with a patch" is unnecessary. > > Restore the original correct MMAP_CHECK behavior instead of keeping the ^ add missing comma after "behavior" > current buggy one and introducing a new hook with the correct behavior. The > second option ^ The second option -> Otherwise, > would have had the risk of IMA users not noticing the problem > at all, as they would actively have to update the IMA policy, to switch to > the correct behavior. > > Also, introduce the new MMAP_CHECK_REQPROT hook to keep the current > behavior, so that IMA users could easily fix a broken system, although this > approach is discouraged due to potentially missing measurements. Again, is this a broken system or a broken attestation server? > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> Otherwise, the patch looks good.
On Sun, 2023-01-29 at 09:52 -0500, Mimi Zohar wrote: > On Thu, 2023-01-26 at 17:38 +0100, Roberto Sassu wrote: > > From: Roberto Sassu <roberto.sassu@huawei.com> > > > > Commit 98de59bfe4b2f ("take calculation of final prot in > > security_mmap_file() into a helper") caused ima_file_mmap() to receive the > > protections requested by the application and not those applied by the > > kernel. > > > > After restoring the original MMAP_CHECK behavior with a patch, existing > > systems might be broken due to not being ready to handle new entries > > (previously missing) in the IMA measurement list. > > Is this a broken system or a broken attestation server? The > attestation server might not be able to handle the additional > measurements, but the system, itself, is not broken. Ok, wasn't clear. I meant attestation server. The system itself is not broken. > "with a patch" is unnecessary. Ok. > > Restore the original correct MMAP_CHECK behavior instead of keeping the > > ^ add missing comma after "behavior" > > > current buggy one and introducing a new hook with the correct behavior. The > > second option > > ^ The second option -> Otherwise, > > > would have had the risk of IMA users not noticing the problem > > at all, as they would actively have to update the IMA policy, to switch to > > the correct behavior. > > > > Also, introduce the new MMAP_CHECK_REQPROT hook to keep the current > > behavior, so that IMA users could easily fix a broken system, although this > > approach is discouraged due to potentially missing measurements. > > Again, is this a broken system or a broken attestation server? > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > > Otherwise, the patch looks good. Ok, will make the changes. Thanks Roberto
diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy index db17fc8a0c9f..49db0ff288e5 100644 --- a/Documentation/ABI/testing/ima_policy +++ b/Documentation/ABI/testing/ima_policy @@ -35,7 +35,7 @@ Description: [FIRMWARE_CHECK] [KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK] [KEXEC_CMDLINE] [KEY_CHECK] [CRITICAL_DATA] - [SETXATTR_CHECK] + [SETXATTR_CHECK][MMAP_CHECK_REQPROT] mask:= [[^]MAY_READ] [[^]MAY_WRITE] [[^]MAY_APPEND] [[^]MAY_EXEC] fsmagic:= hex value diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index 03b440921e61..7186769d5e13 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -190,6 +190,7 @@ static inline unsigned int ima_hash_key(u8 *digest) hook(NONE, none) \ hook(FILE_CHECK, file) \ hook(MMAP_CHECK, mmap) \ + hook(MMAP_CHECK_REQPROT, mmap_reqprot) \ hook(BPRM_CHECK, bprm) \ hook(CREDS_CHECK, creds) \ hook(POST_SETATTR, post_setattr) \ diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c index c1e76282b5ee..3e134c900f0c 100644 --- a/security/integrity/ima/ima_api.c +++ b/security/integrity/ima/ima_api.c @@ -179,7 +179,8 @@ void ima_add_violation(struct file *file, const unsigned char *filename, * subj=, obj=, type=, func=, mask=, fsmagic= * subj,obj, and type: are LSM specific. * func: FILE_CHECK | BPRM_CHECK | CREDS_CHECK | MMAP_CHECK | MODULE_CHECK - * | KEXEC_CMDLINE | KEY_CHECK | CRITICAL_DATA + * | KEXEC_CMDLINE | KEY_CHECK | CRITICAL_DATA | SETXATTR_CHECK + * | MMAP_CHECK_REQPROT * mask: contains the permission mask * fsmagic: hex value * diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index ee6f7e237f2e..97c7d247315c 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -111,6 +111,7 @@ enum integrity_status ima_get_cache_status(struct integrity_iint_cache *iint, { switch (func) { case MMAP_CHECK: + case MMAP_CHECK_REQPROT: return iint->ima_mmap_status; case BPRM_CHECK: return iint->ima_bprm_status; @@ -131,6 +132,7 @@ static void ima_set_cache_status(struct integrity_iint_cache *iint, { switch (func) { case MMAP_CHECK: + case MMAP_CHECK_REQPROT: iint->ima_mmap_status = status; break; case BPRM_CHECK: @@ -155,6 +157,7 @@ static void ima_cache_flags(struct integrity_iint_cache *iint, { switch (func) { case MMAP_CHECK: + case MMAP_CHECK_REQPROT: iint->flags |= (IMA_MMAP_APPRAISED | IMA_APPRAISED); break; case BPRM_CHECK: diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index f48f4e694921..58c2fd5fe22c 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -89,7 +89,8 @@ static int mmap_violation_check(enum ima_hooks func, struct file *file, struct inode *inode; int rc = 0; - if ((func == MMAP_CHECK) && mapping_writably_mapped(file->f_mapping)) { + if ((func == MMAP_CHECK || func == MMAP_CHECK_REQPROT) && + mapping_writably_mapped(file->f_mapping)) { rc = -ETXTBSY; inode = file_inode(file); @@ -227,7 +228,8 @@ static int process_measurement(struct file *file, const struct cred *cred, action = ima_get_action(file_mnt_user_ns(file), inode, cred, secid, mask, func, &pcr, &template_desc, NULL, &allowed_algos); - violation_check = ((func == FILE_CHECK || func == MMAP_CHECK) && + violation_check = ((func == FILE_CHECK || func == MMAP_CHECK || + func == MMAP_CHECK_REQPROT) && (ima_policy_flag & IMA_MEASURE)); if (!action && !violation_check) return 0; @@ -411,12 +413,23 @@ int ima_file_mmap(struct file *file, unsigned long reqprot, unsigned long prot, unsigned long flags) { u32 secid; + int ret; - if (file && (prot & PROT_EXEC)) { - security_current_getsecid_subj(&secid); + if (!file) + return 0; + + security_current_getsecid_subj(&secid); + + if (reqprot & PROT_EXEC) { + ret = process_measurement(file, current_cred(), secid, NULL, + 0, MAY_EXEC, MMAP_CHECK_REQPROT); + if (ret) + return ret; + } + + if (prot & PROT_EXEC) return process_measurement(file, current_cred(), secid, NULL, 0, MAY_EXEC, MMAP_CHECK); - } return 0; } @@ -457,6 +470,10 @@ int ima_file_mprotect(struct vm_area_struct *vma, unsigned long prot) action = ima_get_action(file_mnt_user_ns(vma->vm_file), inode, current_cred(), secid, MAY_EXEC, MMAP_CHECK, &pcr, &template, NULL, NULL); + action |= ima_get_action(file_mnt_user_ns(vma->vm_file), inode, + current_cred(), secid, MAY_EXEC, + MMAP_CHECK_REQPROT, &pcr, &template, NULL, + NULL); /* Is the mmap'ed file in policy? */ if (!(action & (IMA_MEASURE | IMA_APPRAISE_SUBMASK))) diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index 6a68ec270822..419db81c4f67 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -697,6 +697,7 @@ static int get_subaction(struct ima_rule_entry *rule, enum ima_hooks func) switch (func) { case MMAP_CHECK: + case MMAP_CHECK_REQPROT: return IMA_MMAP_APPRAISE; case BPRM_CHECK: return IMA_BPRM_APPRAISE; @@ -1266,6 +1267,7 @@ static bool ima_validate_rule(struct ima_rule_entry *entry) case NONE: case FILE_CHECK: case MMAP_CHECK: + case MMAP_CHECK_REQPROT: case BPRM_CHECK: case CREDS_CHECK: case POST_SETATTR: @@ -1504,6 +1506,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) else if ((strcmp(args[0].from, "FILE_MMAP") == 0) || (strcmp(args[0].from, "MMAP_CHECK") == 0)) entry->func = MMAP_CHECK; + else if ((strcmp(args[0].from, "MMAP_CHECK_REQPROT") == 0)) + entry->func = MMAP_CHECK_REQPROT; else if (strcmp(args[0].from, "BPRM_CHECK") == 0) entry->func = BPRM_CHECK; else if (strcmp(args[0].from, "CREDS_CHECK") == 0)