Re: Race conditioned discovered between ima_match_rules and ima_update_lsm_update_rules

["Guozihua (Scott)" <guozihua@huawei.com>]

On 2022/8/8 11:02, Guozihua (Scott) wrote:
> 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?
> 

Including SELinux in the loop.

-- 
Best
GUO Zihua