Linux Integrity Measurement development
 help / color / mirror / Atom feed
* Race conditioned discovered between ima_match_rules and ima_update_lsm_update_rules
@ 2022-08-08  3:02 Guozihua (Scott)
  2022-08-08  3:19 ` Guozihua (Scott)
  0 siblings, 1 reply; 10+ messages in thread
From: Guozihua (Scott) @ 2022-08-08  3:02 UTC (permalink / raw)
  To: linux-integrity@vger.kernel.org
  Cc: Mimi Zohar, xiujianfeng@huawei.com, luhuaxin

Hi Community,

Recently we discovered a race condition while updating SELinux policy 
with IMA lsm rule enabled. Which would lead to extra files being measured.

While SELinux policy is updated, the IDs for object types and such would 
be changed, and ima_lsm_update_rules would be called.

There are no lock applied in ima_lsm_update_rules. If user accesses a 
file during this time, ima_match_rules will be matching rules based on 
old SELinux au_seqno resulting in selinux_audit_rule_match returning 
-ESTALE.

However, in ima_match_rules, this error number is not handled, causing 
IMA to think the LSM rule is also a match, leading to measuring extra files.

Relevant codes are as follows:

> static void ima_lsm_update_rules(void)
> {
> 	struct ima_rule_entry *entry, *e;
> 	int result;
> 
> 	list_for_each_entry_safe(entry, e, &ima_policy_rules, list) {
> 		if (!ima_rule_contains_lsm_cond(entry))
> 			continue;
> 
> 		result = ima_lsm_update_rule(entry);

A RCU style update is used with no lock applied. Reading to rules would 
return rules with staled au_seqno.

> 		if (result) {
> 			pr_err("lsm rule update error %d\n", result);
> 			return;
> 		}
> 	}
> }


> int selinux_audit_rule_match(u32 sid, u32 field, u32 op, void *vrule)
> {
> 	struct selinux_state *state = &selinux_state;
> 	struct selinux_policy *policy;
> 	struct context *ctxt;
> 	struct mls_level *level;
> 	struct selinux_audit_rule *rule = vrule;
> 	int match = 0;
> 
> 	if (unlikely(!rule)) {
> 		WARN_ONCE(1, "selinux_audit_rule_match: missing rule\n");
> 		return -ENOENT;
> 	}
> 
> 	if (!selinux_initialized(state))
> 		return 0;
> 
> 	rcu_read_lock();
> 
> 	policy = rcu_dereference(state->policy);
> 
> 	if (rule->au_seqno < policy->latest_granting) {
> 		match = -ESTALE;
> 		goto out;
> 	}

SELinux would return -ESTALE here.

> static bool ima_match_rules(struct ima_rule_entry *rule,
> 			    struct user_namespace *mnt_userns,
> 			    struct inode *inode, const struct cred *cred,
> 			    u32 secid, enum ima_hooks func, int mask,
> 			    const char *func_data)
> {
> ...
> 
> 	for (i = 0; i < MAX_LSM_RULES; i++) {
> 		int rc = 0;
> 		u32 osid;
> 
> 		if (!rule->lsm[i].rule) {
> 			if (!rule->lsm[i].args_p)
> 				continue;
> 			else
> 				return false;
> 		}
> 		switch (i) {
> 		case LSM_OBJ_USER:
> 		case LSM_OBJ_ROLE:
> 		case LSM_OBJ_TYPE:
> 			security_inode_getsecid(inode, &osid);
> 			rc = ima_filter_rule_match(osid, rule->lsm[i].type,
> 						   Audit_equal,
> 						   rule->lsm[i].rule);

rc here will be -ESTALE.

> 			break;
> 		case LSM_SUBJ_USER:
> 		case LSM_SUBJ_ROLE:
> 		case LSM_SUBJ_TYPE:
> 			rc = ima_filter_rule_match(secid, rule->lsm[i].type,
> 						   Audit_equal,
> 						   rule->lsm[i].rule);
> 			break;
> 		default:
> 			break;
> 		}
> 		if (!rc)
> 			return false;

-ERRNO is not handled, this func will return true.

> 	}
> 	return true;
> }

It seems that IMA would not "leak" any files, but widening the 
measurement range.

Is this the intended behavior? Or is it a good idea to add a lock for 
LSM rules during update?

-- 
Best
GUO Zihua

^ permalink raw reply	[flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-08-18  1:12 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-08-08  3:02 Race conditioned discovered between ima_match_rules and ima_update_lsm_update_rules Guozihua (Scott)
2022-08-08  3:19 ` Guozihua (Scott)
2022-08-09 16:24   ` Paul Moore
2022-08-14 18:30     ` Mimi Zohar
2022-08-15 22:23       ` Paul Moore
2022-08-17  7:17         ` Guozihua (Scott)
2022-08-17  7:20           ` Guozihua (Scott)
2022-08-17 19:26             ` Mimi Zohar
2022-08-17 20:49               ` Paul Moore
2022-08-18  1:12                 ` Guozihua (Scott)

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox