| Message ID | 1588627060-7399-1-git-send-email-zohar@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] ima: verify mprotect change is consistent with mmap policy | expand |
On 5/4/20 2:17 PM, Mimi Zohar wrote: Hi Mimi, > +int ima_file_mprotect(struct vm_area_struct *vma, unsigned long prot) > +{ > + struct ima_template_desc *template; > + struct inode *inode; > + int result = 0; > + int action; > + u32 secid; > + int pcr; > + > + if (vma->vm_file && (prot & PROT_EXEC) && !(vma->vm_flags & VM_EXEC)) { Just a suggestion: Maybe you could do the negative of the above check and return, so that the block within the if statement doesn't have to be indented. > + inode = file_inode(vma->vm_file); > + > + security_task_getsecid(current, &secid); > + action = ima_get_action(inode, current_cred(), secid, MAY_EXEC, > + MMAP_CHECK, &pcr, &template, 0); > + > + if (action & IMA_APPRAISE_SUBMASK) > + result = -EPERM; > + > + if ((action & IMA_APPRAISE_SUBMASK) || (action & IMA_MEASURE)) { action is checked for IMA_APPRAISE_SUBMASK bits in the previous if statement. Does it need to be checked again in the above if statement? > + struct file *file = vma->vm_file; > + char *pathbuf = NULL; > + const char *pathname; > + char filename[NAME_MAX]; > + > + pathname = ima_d_path(&file->f_path, &pathbuf, > + filename); > + integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, > + pathname, "collect_data", > + "failed-mprotect", result, 0); > + > + if (pathbuf) > + __putname(pathbuf); > + } > + } > + return result; > +} thanks, -lakshmi
On Mon, May 4, 2020 at 11:18 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > Files can be mmap'ed read/write and later changed to execute to circumvent > IMA's mmap appraise policy rules. Due to locking issues (mmap semaphore > would be taken prior to i_mutex), files can not be measured or appraised at > this point. Eliminate this integrity gap, by denying the mprotect > PROT_EXECUTE change, if an mmap appraise policy rule exists. Just keep in mind that there are other ways to create executable mappings containing controlled code; e.g. PROT_EXEC with MAP_ANONYMOUS, or writing to /proc/self/mem (which is a debugging API that works entirely without ever making the VMA writable - I had an old series to provide LSM hooks for that stuff at <https://lore.kernel.org/lkml/1478142286-18427-3-git-send-email-jann@thejh.net/>, but I guess I must have forgotten about it or something...).
Hi Jann, On Tue, 2020-05-05 at 02:15 +0200, Jann Horn wrote: > On Mon, May 4, 2020 at 11:18 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > > Files can be mmap'ed read/write and later changed to execute to circumvent > > IMA's mmap appraise policy rules. Due to locking issues (mmap semaphore > > would be taken prior to i_mutex), files can not be measured or appraised at > > this point. Eliminate this integrity gap, by denying the mprotect > > PROT_EXECUTE change, if an mmap appraise policy rule exists. > > Just keep in mind that there are other ways to create executable > mappings containing controlled code; e.g. PROT_EXEC with > MAP_ANONYMOUS, or writing to /proc/self/mem (which is a debugging API > that works entirely without ever making the VMA writable - I had an > old series to provide LSM hooks for that stuff at > <https://lore.kernel.org/lkml/1478142286-18427-3-git-send-email-jann@thejh.net/>, > but I guess I must have forgotten about it or something...). Sure. These sound like memory attacks, which should be closed, but are probably out of scope for IMA. thanks, Mimi
On Mon, 2020-05-04 at 15:51 -0700, Lakshmi Ramasubramanian wrote: > On 5/4/20 2:17 PM, Mimi Zohar wrote: > > Hi Mimi, > > > +int ima_file_mprotect(struct vm_area_struct *vma, unsigned long prot) > > +{ > > + struct ima_template_desc *template; > > + struct inode *inode; > > + int result = 0; > > + int action; > > + u32 secid; > > + int pcr; > > + > > + if (vma->vm_file && (prot & PROT_EXEC) && !(vma->vm_flags & VM_EXEC)) { > > Just a suggestion: > Maybe you could do the negative of the above check and return, so that > the block within the if statement doesn't have to be indented. Good suggestion. > > > + inode = file_inode(vma->vm_file); > > + > > + security_task_getsecid(current, &secid); > > + action = ima_get_action(inode, current_cred(), secid, MAY_EXEC, > > + MMAP_CHECK, &pcr, &template, 0); > > + > > + if (action & IMA_APPRAISE_SUBMASK) > > + result = -EPERM; > > + > > + if ((action & IMA_APPRAISE_SUBMASK) || (action & IMA_MEASURE)) { > > action is checked for IMA_APPRAISE_SUBMASK bits in the previous if > statement. Does it need to be checked again in the above if statement? Agreed, the code should be cleaned up here too. In either the measurement or the appraisal case, mprotect modifying the execute mmap flag should be audited, but only in the appraisal case is the request denied. Mimi > > > + struct file *file = vma->vm_file; > > + char *pathbuf = NULL; > > + const char *pathname; > > + char filename[NAME_MAX]; > > + > > + pathname = ima_d_path(&file->f_path, &pathbuf, > > + filename); > > + integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, > > + pathname, "collect_data", > > + "failed-mprotect", result, 0); > > + > > + if (pathbuf) > > + __putname(pathbuf); > > + } > > + } > > + return result; > > +} > > thanks, > -lakshmi >
diff --git a/include/linux/ima.h b/include/linux/ima.h index aefe758f4466..9164e1534ec9 100644 --- a/include/linux/ima.h +++ b/include/linux/ima.h @@ -18,6 +18,7 @@ extern int ima_file_check(struct file *file, int mask); extern void ima_post_create_tmpfile(struct inode *inode); extern void ima_file_free(struct file *file); extern int ima_file_mmap(struct file *file, unsigned long prot); +extern int ima_file_mprotect(struct vm_area_struct *vma, unsigned long prot); extern int ima_load_data(enum kernel_load_data_id id); extern int ima_read_file(struct file *file, enum kernel_read_file_id id); extern int ima_post_read_file(struct file *file, void *buf, loff_t size, @@ -70,6 +71,12 @@ static inline int ima_file_mmap(struct file *file, unsigned long prot) return 0; } +static inline int ima_file_mprotect(struct vm_area_struct *vma, + unsigned long prot) +{ + return 0; +} + static inline int ima_load_data(enum kernel_load_data_id id) { return 0; diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index f96f151294e6..a8706bf7ca25 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -394,6 +394,56 @@ int ima_file_mmap(struct file *file, unsigned long prot) } /** + * ima_file_mprotect - based on policy, limit mprotect change + * @prot: contains the protection that will be applied by the kernel. + * + * Files can be mmap'ed read/write and later changed to execute to circumvent + * IMA's mmap appraisal policy rules. Due to locking issues (mmap semaphore + * would be taken before i_mutex), files can not be measured or appraised at + * this point. Eliminate this integrity gap by denying the mprotect + * PROT_EXECUTE change, if an mmap appraise policy rule exists. + * + * On mprotect change success, return 0. On failure, return -EACESS. + */ +int ima_file_mprotect(struct vm_area_struct *vma, unsigned long prot) +{ + struct ima_template_desc *template; + struct inode *inode; + int result = 0; + int action; + u32 secid; + int pcr; + + if (vma->vm_file && (prot & PROT_EXEC) && !(vma->vm_flags & VM_EXEC)) { + inode = file_inode(vma->vm_file); + + security_task_getsecid(current, &secid); + action = ima_get_action(inode, current_cred(), secid, MAY_EXEC, + MMAP_CHECK, &pcr, &template, 0); + + if (action & IMA_APPRAISE_SUBMASK) + result = -EPERM; + + if ((action & IMA_APPRAISE_SUBMASK) || (action & IMA_MEASURE)) { + struct file *file = vma->vm_file; + char *pathbuf = NULL; + const char *pathname; + char filename[NAME_MAX]; + + pathname = ima_d_path(&file->f_path, &pathbuf, + filename); + integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, + pathname, "collect_data", + "failed-mprotect", result, 0); + + if (pathbuf) + __putname(pathbuf); + } + } + return result; +} + +/** * ima_bprm_check - based on policy, collect/store measurement. * @bprm: contains the linux_binprm structure * diff --git a/security/security.c b/security/security.c index 7fed24b9d57e..dd0917c5bfe9 100644 --- a/security/security.c +++ b/security/security.c @@ -1512,7 +1512,12 @@ int security_mmap_addr(unsigned long addr) int security_file_mprotect(struct vm_area_struct *vma, unsigned long reqprot, unsigned long prot) { - return call_int_hook(file_mprotect, 0, vma, reqprot, prot); + int ret; + + ret = call_int_hook(file_mprotect, 0, vma, reqprot, prot); + if (ret) + return ret; + return ima_file_mprotect(vma, prot); } int security_file_lock(struct file *file, unsigned int cmd)
Files can be mmap'ed read/write and later changed to execute to circumvent IMA's mmap appraise policy rules. Due to locking issues (mmap semaphore would be taken prior to i_mutex), files can not be measured or appraised at this point. Eliminate this integrity gap, by denying the mprotect PROT_EXECUTE change, if an mmap appraise policy rule exists. On mprotect change success, return 0. On failure, return -EACESS. Signed-off-by: Mimi Zohar <zohar@linux.ibm.com> --- include/linux/ima.h | 7 ++++++ security/integrity/ima/ima_main.c | 50 +++++++++++++++++++++++++++++++++++++++ security/security.c | 7 +++++- 3 files changed, 63 insertions(+), 1 deletion(-)