From 31b70f66328e85517b159c786ab31f3fd9a7293c Mon Sep 17 00:00:00 2001 From: Dmitry Kasatkin Date: Fri, 27 Jun 2014 13:01:32 +0300 Subject: ima: move keyring initialization to ima_init() ima_init() is used as a single place for all initializations. Experimental keyring patches used the 'late_initcall' which was co-located with the late_initcall(init_ima). When the late_initcall for the keyring initialization was abandoned, initialization moved to init_ima, though it would be more logical to move it to ima_init, where the rest of the initialization is done. This patch moves the keyring initialization to ima_init() as a preparatory step for loading the keys which will be added to ima_init() in following patches. Signed-off-by: Dmitry Kasatkin Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_init.c | 4 ++++ security/integrity/ima/ima_main.c | 10 ++-------- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c index e8f9d70a465d..8cf0f39c8cd2 100644 --- a/security/integrity/ima/ima_init.c +++ b/security/integrity/ima/ima_init.c @@ -98,6 +98,10 @@ int __init ima_init(void) if (!ima_used_chip) pr_info("No TPM chip found, activating TPM-bypass!\n"); + rc = ima_init_keyring(INTEGRITY_KEYRING_IMA); + if (rc) + return rc; + rc = ima_init_crypto(); if (rc) return rc; diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 673a37e92ba3..ed7d9fa4f536 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -334,14 +334,8 @@ static int __init init_ima(void) hash_setup(CONFIG_IMA_DEFAULT_HASH); error = ima_init(); - if (error) - goto out; - - error = ima_init_keyring(INTEGRITY_KEYRING_IMA); - if (error) - goto out; - ima_initialized = 1; -out: + if (!error) + ima_initialized = 1; return error; } -- cgit v1.2.3-55-g7522 From 2faa6ef3b21152cc05b69a84113625dcee63176f Mon Sep 17 00:00:00 2001 From: Dmitry Kasatkin Date: Thu, 8 May 2014 13:11:29 +0300 Subject: ima: provide 'ima_appraise=log' kernel option The kernel boot parameter "ima_appraise" currently defines 'off', 'enforce' and 'fix' modes. When designing a policy and labeling the system, access to files are either blocked in the default 'enforce' mode or automatically fixed in the 'fix' mode. It is beneficial to be able to run the system in a logging only mode, without fixing it, in order to properly analyze the system. This patch adds a 'log' mode to run the system in a permissive mode and log the appraisal results. Signed-off-by: Dmitry Kasatkin Signed-off-by: Mimi Zohar --- Documentation/kernel-parameters.txt | 2 +- security/integrity/ima/ima.h | 5 +++-- security/integrity/ima/ima_appraise.c | 2 ++ 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt index 90c12c591168..2aa1b6e74aca 100644 --- a/Documentation/kernel-parameters.txt +++ b/Documentation/kernel-parameters.txt @@ -1292,7 +1292,7 @@ bytes respectively. Such letter suffixes can also be entirely omitted. Set number of hash buckets for inode cache. ima_appraise= [IMA] appraise integrity measurements - Format: { "off" | "enforce" | "fix" } + Format: { "off" | "enforce" | "fix" | "log" } default: "enforce" ima_appraise_tcb [IMA] diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index 8e4bb883fc13..d61680dcd365 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -159,8 +159,9 @@ void ima_delete_rules(void); /* Appraise integrity measurements */ #define IMA_APPRAISE_ENFORCE 0x01 #define IMA_APPRAISE_FIX 0x02 -#define IMA_APPRAISE_MODULES 0x04 -#define IMA_APPRAISE_FIRMWARE 0x08 +#define IMA_APPRAISE_LOG 0x04 +#define IMA_APPRAISE_MODULES 0x08 +#define IMA_APPRAISE_FIRMWARE 0x10 #ifdef CONFIG_IMA_APPRAISE int ima_appraise_measurement(int func, struct integrity_iint_cache *iint, diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index 013ec3f0e42d..2dc13fbb7e91 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -23,6 +23,8 @@ static int __init default_appraise_setup(char *str) { if (strncmp(str, "off", 3) == 0) ima_appraise = 0; + else if (strncmp(str, "log", 3) == 0) + ima_appraise = IMA_APPRAISE_LOG; else if (strncmp(str, "fix", 3) == 0) ima_appraise = IMA_APPRAISE_FIX; return 1; -- cgit v1.2.3-55-g7522 From be39ffc2fec78ff80d50e4b7970e94a8b1583862 Mon Sep 17 00:00:00 2001 From: Roberto Sassu Date: Fri, 12 Sep 2014 19:35:53 +0200 Subject: ima: return an error code from ima_add_boot_aggregate() This patch modifies ima_add_boot_aggregate() to return an error code. This way we can determine if all the initialization procedures have been executed successfully. Signed-off-by: Roberto Sassu Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_init.c | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c index 8cf0f39c8cd2..9164fc8cac84 100644 --- a/security/integrity/ima/ima_init.c +++ b/security/integrity/ima/ima_init.c @@ -43,7 +43,7 @@ int ima_used_chip; * a different value.) Violations add a zero entry to the measurement * list and extend the aggregate PCR value with ff...ff's. */ -static void __init ima_add_boot_aggregate(void) +static int __init ima_add_boot_aggregate(void) { static const char op[] = "add_boot_aggregate"; const char *audit_cause = "ENOMEM"; @@ -72,17 +72,23 @@ static void __init ima_add_boot_aggregate(void) result = ima_alloc_init_template(iint, NULL, boot_aggregate_name, NULL, 0, &entry); - if (result < 0) - return; + if (result < 0) { + audit_cause = "alloc_entry"; + goto err_out; + } result = ima_store_template(entry, violation, NULL, boot_aggregate_name); - if (result < 0) + if (result < 0) { ima_free_template_entry(entry); - return; + audit_cause = "store_entry"; + goto err_out; + } + return 0; err_out: integrity_audit_msg(AUDIT_INTEGRITY_PCR, NULL, boot_aggregate_name, op, audit_cause, result, 0); + return result; } int __init ima_init(void) @@ -109,7 +115,10 @@ int __init ima_init(void) if (rc != 0) return rc; - ima_add_boot_aggregate(); /* boot aggregate must be first entry */ + rc = ima_add_boot_aggregate(); /* boot aggregate must be first entry */ + if (rc != 0) + return rc; + ima_init_policy(); return ima_fs_init(); -- cgit v1.2.3-55-g7522 From a756024efea259282e65f3a00f512b094e805d76 Mon Sep 17 00:00:00 2001 From: Roberto Sassu Date: Fri, 12 Sep 2014 19:35:54 +0200 Subject: ima: added ima_policy_flag variable This patch introduces the new variable 'ima_policy_flag', whose bits are set depending on the action of the current policy rules. Only the flags IMA_MEASURE, IMA_APPRAISE and IMA_AUDIT are set. The new variable will be used to improve performance by skipping the unnecessary execution of IMA code if the policy does not contain rules with the above actions. Changes in v6 (Roberto Sassu) * do not check 'ima_initialized' before calling ima_update_policy_flag() in ima_update_policy() (suggested by Dmitry) * calling ima_update_policy_flag() moved to init_ima to co-locate with ima_initialized (Dmitry) * add/revise comments (Mimi) Changes in v5 (Roberto Sassu) * reset IMA_APPRAISE flag in 'ima_policy_flag' if 'ima_appraise' is set to zero (reported by Dmitry) * update 'ima_policy_flag' only if IMA initialization is successful (suggested by Mimi and Dmitry) * check 'ima_policy_flag' instead of 'ima_initialized' (suggested by Mimi and Dmitry) Signed-off-by: Roberto Sassu Signed-off-by: Dmitry Kasatkin Signed-off-by: Mimi Zohar --- security/integrity/ima/ima.h | 4 ++++ security/integrity/ima/ima_appraise.c | 4 ++-- security/integrity/ima/ima_main.c | 8 +++++--- security/integrity/ima/ima_policy.c | 23 +++++++++++++++++++++++ 4 files changed, 34 insertions(+), 5 deletions(-) diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index d61680dcd365..8ee997dff139 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -43,6 +43,9 @@ enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8 }; #define IMA_TEMPLATE_IMA_NAME "ima" #define IMA_TEMPLATE_IMA_FMT "d|n" +/* current content of the policy */ +extern int ima_policy_flag; + /* set during initialization */ extern int ima_initialized; extern int ima_used_chip; @@ -153,6 +156,7 @@ int ima_match_policy(struct inode *inode, enum ima_hooks func, int mask, int flags); void ima_init_policy(void); void ima_update_policy(void); +void ima_update_policy_flag(void); ssize_t ima_parse_add_rule(char *); void ima_delete_rules(void); diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index 2dc13fbb7e91..922685483bd3 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -318,7 +318,7 @@ void ima_inode_post_setattr(struct dentry *dentry) struct integrity_iint_cache *iint; int must_appraise, rc; - if (!ima_initialized || !ima_appraise || !S_ISREG(inode->i_mode) + if (!(ima_policy_flag & IMA_APPRAISE) || !S_ISREG(inode->i_mode) || !inode->i_op->removexattr) return; @@ -356,7 +356,7 @@ static void ima_reset_appraise_flags(struct inode *inode, int digsig) { struct integrity_iint_cache *iint; - if (!ima_initialized || !ima_appraise || !S_ISREG(inode->i_mode)) + if (!(ima_policy_flag & IMA_APPRAISE) || !S_ISREG(inode->i_mode)) return; iint = integrity_iint_find(inode); diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index ed7d9fa4f536..2191b36ad1da 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -85,7 +85,7 @@ static void ima_rdwr_violation_check(struct file *file) char *pathbuf = NULL; const char *pathname; - if (!S_ISREG(inode->i_mode) || !ima_initialized) + if (!S_ISREG(inode->i_mode) || !(ima_policy_flag & IMA_MEASURE)) return; if (mode & FMODE_WRITE) { @@ -168,7 +168,7 @@ static int process_measurement(struct file *file, int mask, int function, struct evm_ima_xattr_data *xattr_value = NULL, **xattr_ptr = NULL; int xattr_len = 0; - if (!ima_initialized || !S_ISREG(inode->i_mode)) + if (!ima_policy_flag || !S_ISREG(inode->i_mode)) return 0; /* Return an IMA_MEASURE, IMA_APPRAISE, IMA_AUDIT action @@ -334,8 +334,10 @@ static int __init init_ima(void) hash_setup(CONFIG_IMA_DEFAULT_HASH); error = ima_init(); - if (!error) + if (!error) { ima_initialized = 1; + ima_update_policy_flag(); + } return error; } diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index 07099a8bc283..cdc620b2152f 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -35,6 +35,8 @@ #define DONT_APPRAISE 0x0008 #define AUDIT 0x0040 +int ima_policy_flag; + #define MAX_LSM_RULES 6 enum lsm_rule_types { LSM_OBJ_USER, LSM_OBJ_ROLE, LSM_OBJ_TYPE, LSM_SUBJ_USER, LSM_SUBJ_ROLE, LSM_SUBJ_TYPE @@ -295,6 +297,26 @@ int ima_match_policy(struct inode *inode, enum ima_hooks func, int mask, return action; } +/* + * Initialize the ima_policy_flag variable based on the currently + * loaded policy. Based on this flag, the decision to short circuit + * out of a function or not call the function in the first place + * can be made earlier. + */ +void ima_update_policy_flag(void) +{ + struct ima_rule_entry *entry; + + ima_policy_flag = 0; + list_for_each_entry(entry, ima_rules, list) { + if (entry->action & IMA_DO_MASK) + ima_policy_flag |= entry->action; + } + + if (!ima_appraise) + ima_policy_flag &= ~IMA_APPRAISE; +} + /** * ima_init_policy - initialize the default measure rules. * @@ -341,6 +363,7 @@ void ima_update_policy(void) if (ima_rules == &ima_default_rules) { ima_rules = &ima_policy_rules; + ima_update_policy_flag(); cause = "complete"; result = 0; } -- cgit v1.2.3-55-g7522 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(-) 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-55-g7522 From 1b68bdf9cded82d37e443a20c5ed47bbb084d5dc Mon Sep 17 00:00:00 2001 From: Roberto Sassu Date: Fri, 12 Sep 2014 19:35:56 +0200 Subject: ima: detect violations for mmaped files This patch fixes the detection of the 'open_writers' violation for mmaped files. before) an 'open_writers' violation is detected if the policy contains a rule with the criteria: func=FILE_CHECK mask=MAY_READ after) an 'open_writers' violation is detected if the current event matches one of the policy rules. With the old behaviour, the 'open_writers' violation is not detected in the following case: policy: measure func=FILE_MMAP mask=MAY_EXEC steps: 1) open a shared library for writing 2) execute a binary that links that shared library 3) during the binary execution, modify the shared library and save the change result: the 'open_writers' violation measurement is not present in the IMA list. Only binaries executed are protected from writes. For libraries mapped in memory there is the flag MAP_DENYWRITE for this purpose, but according to the output of 'man mmap', the mmap flag is ignored. Since ima_rdwr_violation_check() is now called by process_measurement() the information about if the inode must be measured is already provided by ima_get_action(). Thus the unnecessary function ima_must_measure() has been removed. Changes in v3 (Dmitry Kasatkin): - Violation for MMAP_CHECK function are verified since this patch - Changed patch description a bit Signed-off-by: Roberto Sassu Signed-off-by: Dmitry Kasatkin Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_api.c | 5 ----- security/integrity/ima/ima_main.c | 9 +++++---- 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c index 65c41a968cc1..86885979918c 100644 --- a/security/integrity/ima/ima_api.c +++ b/security/integrity/ima/ima_api.c @@ -179,11 +179,6 @@ int ima_get_action(struct inode *inode, int mask, int function) return ima_match_policy(inode, function, mask, flags); } -int ima_must_measure(struct inode *inode, int mask, int function) -{ - return ima_match_policy(inode, function, mask, IMA_MEASURE); -} - /* * ima_collect_measurement - collect file measurement * diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 03bb52ecf490..62f59eca32d3 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -79,6 +79,7 @@ __setup("ima_hash=", hash_setup); */ static void ima_rdwr_violation_check(struct file *file, struct integrity_iint_cache *iint, + int must_measure, char **pathbuf, const char **pathname) { @@ -95,8 +96,7 @@ static void ima_rdwr_violation_check(struct file *file, send_tomtou = true; } } else { - if ((atomic_read(&inode->i_writecount) > 0) && - ima_must_measure(inode, MAY_READ, FILE_CHECK)) + if ((atomic_read(&inode->i_writecount) > 0) && must_measure) send_writers = true; } @@ -174,7 +174,7 @@ static int process_measurement(struct file *file, int mask, int function, * Included is the appraise submask. */ action = ima_get_action(inode, mask, function); - violation_check = (function == FILE_CHECK && + violation_check = ((function == FILE_CHECK || function == MMAP_CHECK) && (ima_policy_flag & IMA_MEASURE)); if (!action && !violation_check) return 0; @@ -194,7 +194,8 @@ static int process_measurement(struct file *file, int mask, int function, } if (violation_check) { - ima_rdwr_violation_check(file, iint, &pathbuf, &pathname); + ima_rdwr_violation_check(file, iint, action & IMA_MEASURE, + &pathbuf, &pathname); if (!action) { rc = 0; goto out_free; -- cgit v1.2.3-55-g7522