[PATCH 0/3] ima: addressing mmap/mprotect concerns

[PATCH 0/3] ima: addressing mmap/mprotect concerns

[Mimi Zohar]

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/cce2c4c7-5333-41c3-aeef-34d43e63acb0@omprussia.ru/
[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

[PATCH 1/3] ima: verify mprotect change is consistent with mmap policy

[Mimi Zohar]

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

[PATCH 2/3] ima: prevent a file already mmap'ed write to be mmap'ed execute

[Mimi Zohar]

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 <zohar@linux.ibm.com>
---
 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

[PATCH 3/3] ima: prevent a file already mmap'ed read|execute to be mmap'ed write

[Mimi Zohar]

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 patch prevents allowing a file in policy, already mmap'ed
read|execute or read, from being mmap'ed shared write.  It should
differentiate between read|execute and read.

Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 include/linux/ima.h               |  6 ++++--
 security/integrity/ima/ima_main.c | 21 ++++++++++++++++++++-
 security/security.c               |  4 ++--
 3 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/include/linux/ima.h b/include/linux/ima.h
index dc12fbcf484c..04444895b4f2 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -20,7 +20,8 @@ extern int ima_bprm_check(struct linux_binprm *bprm);
 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_mmap(struct file *file, unsigned long prot,
+		unsigned long flags);
 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,
@@ -66,7 +67,8 @@ static inline void ima_file_free(struct file *file)
 	return;
 }
 
-static inline int ima_file_mmap(struct file *file, unsigned long prot)
+static inline int ima_file_mmap(struct file *file, unsigned long prot,
+		unsigned long flags)
 {
 	return 0;
 }
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index ae77d13cb43c..d13e4efa8599 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -354,6 +354,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
  * ima_file_mmap - based on policy, collect/store measurement.
  * @file: pointer to the file to be measured (May be NULL)
  * @prot: contains the protection that will be applied by the kernel.
+ * @flags:
  *
  * Measure files being mmapped executable based on the ima_must_measure()
  * policy decision.
@@ -361,8 +362,9 @@ static int process_measurement(struct file *file, const struct cred *cred,
  * On success return 0.  On integrity appraisal error, assuming the file
  * is in policy and IMA-appraisal is in enforcing mode, return -EACCES.
  */
-int ima_file_mmap(struct file *file, unsigned long prot)
+int ima_file_mmap(struct file *file, unsigned long prot, unsigned long flags)
 {
+	struct inode *inode;
 	u32 secid;
 
 	if (file && (prot & PROT_EXEC)) {
@@ -371,6 +373,23 @@ int ima_file_mmap(struct file *file, unsigned long prot)
 					   0, MAY_EXEC, MMAP_CHECK);
 	}
 
+	/*
+	 * Prevent a file, in policy, mapped read|execute, from being mapped
+	 * write shared. (Should differentiate between read and read|execute.)
+	 */
+	if (file && (prot & PROT_WRITE) && ((flags & MAP_TYPE) == MAP_SHARED) &&
+	    mapping_mapped(file->f_mapping) &&
+	    !mapping_writably_mapped(file->f_mapping)) {
+		inode = file_inode(file);
+
+		if (!ima_must_appraise(inode, MAY_ACCESS, MMAP_CHECK))
+			return 0;
+
+		integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode,
+				    file_dentry(file)->d_iname,
+				    "mmap_file", "mmapped_readers", -EACCES, 0);
+		return -EACCES;
+	}
 	return 0;
 }
 
diff --git a/security/security.c b/security/security.c
index 98ce27933e72..e64d9c5b2e1a 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1400,7 +1400,7 @@ int security_mmap_file(struct file *file, unsigned long prot,
 					mmap_prot(file, prot), flags);
 	if (ret)
 		return ret;
-	return ima_file_mmap(file, prot);
+	return ima_file_mmap(file, prot, flags);
 }
 
 int security_mmap_addr(unsigned long addr)
@@ -1416,7 +1416,7 @@ int security_file_mprotect(struct vm_area_struct *vma, unsigned long reqprot,
 	ret = call_int_hook(file_mprotect, 0, vma, reqprot, prot);
 	if (ret)
 		return ret;
-	return ima_file_mmap(vma->vm_file, prot);
+	return ima_file_mmap(vma->vm_file, prot, 0);
 }
 
 int security_file_lock(struct file *file, unsigned int cmd)
-- 
2.7.5