Igor Zhbanov's "Should mprotect(..., PROT_EXEC) be checked by IMA?"
thread raised concerns about IMA's handling of mmap/mprotect. [1] The
kernel calls deny_write_access() to prevent a file already opened for
write from being executed and also prevents files being executed from
being opened for write. For some reason this does not extend to files
being mmap'ed execute. This is a known, well described problem.[2]
Jordan Glover commented that the proposed minor LSM "SARA" addresses
this issue.[3]
This patch set attempts to address some the IMA mmap/mprotect concerns
without locking the mmap'ed files.
Mimi
[1] https://lore.kernel.org/linux-integrity/[email protected]/
[2] ]https://pax.grsecurity.net/docs/mprotect.txt
[3] https://sara.smeso.it/en/latest/
Mimi Zohar (3):
ima: verify mprotect change is consistent with mmap policy
ima: prevent a file already mmap'ed write to be mmap'ed execute
ima: prevent a file already mmap'ed read|execute to be mmap'ed write
include/linux/ima.h | 6 +++--
security/integrity/ima/ima_main.c | 53 ++++++++++++++++++++++++++++++++++++---
security/security.c | 9 +++++--
3 files changed, 61 insertions(+), 7 deletions(-)
--
2.7.5
IMA can be configured to measure and appraise a file's integrity being
mmap'ed execute. Files can be mmap'ed read/write and later changed to
execute to circumvent IMA's mmap measurement and appraisal policy rules.
To prevent this from happening, this patch similarly calls
ima_file_mmap() for mprotect changes.
Suggested-by: Stephen Smalley <[email protected]>
Signed-off-by: Mimi Zohar <[email protected]>
---
security/security.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/security/security.c b/security/security.c
index 23cbb1a295a3..98ce27933e72 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1411,7 +1411,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_mmap(vma->vm_file, prot);
}
int security_file_lock(struct file *file, unsigned int cmd)
--
2.7.5
The kernel calls deny_write_access() to prevent a file already opened
for write from being executed and also prevents files being executed
from being opened for write. For some reason this does not extend to
files being mmap'ed execute.
From an IMA perspective, measuring/appraising the integrity of a file
being mmap'ed execute, without first making sure the file cannot be
modified, makes no sense. This patch prevents files, in policy, already
mmap'ed write, from being mmap'ed execute.
Signed-off-by: Mimi Zohar <[email protected]>
---
security/integrity/ima/ima_main.c | 32 ++++++++++++++++++++++++++++++--
1 file changed, 30 insertions(+), 2 deletions(-)
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 357edd140c09..ae77d13cb43c 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -72,6 +72,27 @@ static int __init hash_setup(char *str)
}
__setup("ima_hash=", hash_setup);
+/* Prevent mmap'ing a file execute that is already mmap'ed write */
+static int mmap_violation_check(enum ima_hooks func, struct file *file,
+ char **pathbuf, const char **pathname,
+ char *filename)
+{
+ struct inode *inode;
+ int rc = 0;
+
+ if ((func == MMAP_CHECK) && mapping_writably_mapped(file->f_mapping)) {
+ rc = -ETXTBSY;
+ inode = file_inode(file);
+
+ if (!*pathbuf) /* ima_rdwr_violation possibly pre-fetched */
+ *pathname = ima_d_path(&file->f_path, pathbuf,
+ filename);
+ integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, *pathname,
+ "mmap_file", "mmapped_writers", rc, 0);
+ }
+ return rc;
+}
+
/*
* ima_rdwr_violation_check
*
@@ -270,8 +291,12 @@ static int process_measurement(struct file *file, const struct cred *cred,
/* Nothing to do, just return existing appraised status */
if (!action) {
- if (must_appraise)
- rc = ima_get_cache_status(iint, func);
+ if (must_appraise) {
+ rc = mmap_violation_check(func, file, &pathbuf,
+ &pathname, filename);
+ if (!rc)
+ rc = ima_get_cache_status(iint, func);
+ }
goto out_locked;
}
@@ -298,6 +323,9 @@ static int process_measurement(struct file *file, const struct cred *cred,
rc = ima_appraise_measurement(func, iint, file, pathname,
xattr_value, xattr_len);
inode_unlock(inode);
+
+ rc = mmap_violation_check(func, file, &pathbuf, &pathname,
+ filename);
}
if (action & IMA_AUDIT)
ima_audit_measurement(iint, pathname);
--
2.7.5