From f7a859ff7395c0ffe60f9563df5354473e5f9244 Mon Sep 17 00:00:00 2001 From: Roberto Sassu Date: Fri, 12 Sep 2014 19:35:55 +0200 Subject: ima: fix race condition on ima_rdwr_violation_check and process_measurement This patch fixes a race condition between two functions that try to access the same inode. Since the i_mutex lock is held and released separately in the two functions, there may be the possibility that a violation is not correctly detected. Suppose there are two processes, A (reader) and B (writer), if the following sequence happens: A: ima_rdwr_violation_check() B: ima_rdwr_violation_check() B: process_measurement() B: starts writing the inode A: process_measurement() the ToMToU violation (a reader may be accessing a content different from that measured, due to a concurrent modification by a writer) will not be detected. To avoid this issue, the violation check and the measurement must be done atomically. This patch fixes the problem by moving the violation check inside process_measurement() when the i_mutex lock is held. Differently from the old code, the violation check is executed also for the MMAP_CHECK hook (other than for FILE_CHECK). This allows to detect ToMToU violations that are possible because shared libraries can be opened for writing while they are in use (according to the output of 'man mmap', the mmap() flag MAP_DENYWRITE is ignored). Changes in v5 (Roberto Sassu): * get iint if action is not zero * exit process_measurement() after the violation check if action is zero * reverse order process_measurement() exit cleanup (Mimi) Changes in v4 (Dmitry Kasatkin): * iint allocation is done before calling ima_rdrw_violation_check() (Suggested-by Mimi) * do not check for violations if the policy does not contain 'measure' rules (done by Roberto Sassu) Changes in v3 (Dmitry Kasatkin): * no violation checking for MMAP_CHECK function in this patch * remove use of filename from violation * removes checking if ima is enabled from ima_rdrw_violation_check * slight style change Suggested-by: Dmitry Kasatkin Signed-off-by: Roberto Sassu Signed-off-by: Dmitry Kasatkin Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_main.c | 54 ++++++++++++++++++++++++--------------- 1 file changed, 33 insertions(+), 21 deletions(-) (limited to 'security/integrity/ima') diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 2191b36ad1da..03bb52ecf490 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -77,21 +77,19 @@ __setup("ima_hash=", hash_setup); * could result in a file measurement error. * */ -static void ima_rdwr_violation_check(struct file *file) +static void ima_rdwr_violation_check(struct file *file, + struct integrity_iint_cache *iint, + char **pathbuf, + const char **pathname) { struct inode *inode = file_inode(file); fmode_t mode = file->f_mode; bool send_tomtou = false, send_writers = false; - char *pathbuf = NULL; - const char *pathname; - - if (!S_ISREG(inode->i_mode) || !(ima_policy_flag & IMA_MEASURE)) - return; if (mode & FMODE_WRITE) { if (atomic_read(&inode->i_readcount) && IS_IMA(inode)) { - struct integrity_iint_cache *iint; - iint = integrity_iint_find(inode); + if (!iint) + iint = integrity_iint_find(inode); /* IMA_MEASURE is set from reader side */ if (iint && (iint->flags & IMA_MEASURE)) send_tomtou = true; @@ -105,14 +103,13 @@ static void ima_rdwr_violation_check(struct file *file) if (!send_tomtou && !send_writers) return; - pathname = ima_d_path(&file->f_path, &pathbuf); + *pathname = ima_d_path(&file->f_path, pathbuf); if (send_tomtou) - ima_add_violation(file, pathname, "invalid_pcr", "ToMToU"); + ima_add_violation(file, *pathname, "invalid_pcr", "ToMToU"); if (send_writers) - ima_add_violation(file, pathname, + ima_add_violation(file, *pathname, "invalid_pcr", "open_writers"); - kfree(pathbuf); } static void ima_check_last_writer(struct integrity_iint_cache *iint, @@ -160,13 +157,14 @@ static int process_measurement(struct file *file, int mask, int function, int opened) { struct inode *inode = file_inode(file); - struct integrity_iint_cache *iint; + struct integrity_iint_cache *iint = NULL; struct ima_template_desc *template_desc; char *pathbuf = NULL; const char *pathname = NULL; int rc = -ENOMEM, action, must_appraise; struct evm_ima_xattr_data *xattr_value = NULL, **xattr_ptr = NULL; int xattr_len = 0; + bool violation_check; if (!ima_policy_flag || !S_ISREG(inode->i_mode)) return 0; @@ -176,7 +174,9 @@ static int process_measurement(struct file *file, int mask, int function, * Included is the appraise submask. */ action = ima_get_action(inode, mask, function); - if (!action) + violation_check = (function == FILE_CHECK && + (ima_policy_flag & IMA_MEASURE)); + if (!action && !violation_check) return 0; must_appraise = action & IMA_APPRAISE; @@ -187,9 +187,19 @@ static int process_measurement(struct file *file, int mask, int function, mutex_lock(&inode->i_mutex); - iint = integrity_inode_get(inode); - if (!iint) - goto out; + if (action) { + iint = integrity_inode_get(inode); + if (!iint) + goto out; + } + + if (violation_check) { + ima_rdwr_violation_check(file, iint, &pathbuf, &pathname); + if (!action) { + rc = 0; + goto out_free; + } + } /* Determine if already appraised/measured based on bitmask * (IMA_MEASURE, IMA_MEASURED, IMA_XXXX_APPRAISE, IMA_XXXX_APPRAISED, @@ -218,7 +228,8 @@ static int process_measurement(struct file *file, int mask, int function, goto out_digsig; } - pathname = ima_d_path(&file->f_path, &pathbuf); + if (!pathname) /* ima_rdwr_violation possibly pre-fetched */ + pathname = ima_d_path(&file->f_path, &pathbuf); if (action & IMA_MEASURE) ima_store_measurement(iint, file, pathname, @@ -228,13 +239,15 @@ static int process_measurement(struct file *file, int mask, int function, xattr_value, xattr_len, opened); if (action & IMA_AUDIT) ima_audit_measurement(iint, pathname); - kfree(pathbuf); + out_digsig: if ((mask & MAY_WRITE) && (iint->flags & IMA_DIGSIG)) rc = -EACCES; + kfree(xattr_value); +out_free: + kfree(pathbuf); out: mutex_unlock(&inode->i_mutex); - kfree(xattr_value); if ((rc && must_appraise) && (ima_appraise & IMA_APPRAISE_ENFORCE)) return -EACCES; return 0; @@ -288,7 +301,6 @@ int ima_bprm_check(struct linux_binprm *bprm) */ int ima_file_check(struct file *file, int mask, int opened) { - ima_rdwr_violation_check(file); return process_measurement(file, mask & (MAY_READ | MAY_WRITE | MAY_EXEC), FILE_CHECK, opened); -- cgit v1.2.3