Re: [PATCH v23 02/23] LSM: Create and manage the lsmblob data structure.

[Casey Schaufler]

On 12/28/2020 2:14 PM, Mimi Zohar wrote:
> On Mon, 2020-12-28 at 12:06 -0800, Casey Schaufler wrote:
>> On 12/28/2020 11:24 AM, Mimi Zohar wrote:
>>> Hi Casey,
>>>
>>> On Fri, 2020-11-20 at 12:14 -0800, Casey Schaufler wrote:
>>>> diff --git a/security/security.c b/security/security.c
>>>> index 5da8b3643680..d01363cb0082 100644
>>>> --- a/security/security.c
>>>> +++ b/security/security.c
>>>>
>>>> @@ -2510,7 +2526,24 @@ int security_key_getsecurity(struct key *key, char **_buffer)
>>>>
>>>>  int security_audit_rule_init(u32 field, u32 op, char *rulestr, void **lsmrule)
>>>>  {
>>>> -       return call_int_hook(audit_rule_init, 0, field, op, rulestr, lsmrule);
>>>> +       struct security_hook_list *hp;
>>>> +       bool one_is_good = false;
>>>> +       int rc = 0;
>>>> +       int trc;
>>>> +
>>>> +       hlist_for_each_entry(hp, &security_hook_heads.audit_rule_init, list) {
>>>> +               if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
>>>> +                       continue;
>>>> +               trc = hp->hook.audit_rule_init(field, op, rulestr,
>>>> +                                              &lsmrule[hp->lsmid->slot]);
>>>> +               if (trc == 0)
>>>> +                       one_is_good = true;
>>>> +               else
>>>> +                       rc = trc;
>>>> +       }
>>>> +       if (one_is_good)
>>>> +               return 0;
>>>> +       return rc;
>>>>  }
>>> So the same string may be defined by multiple LSMs.
>> Yes. Any legal AppArmor label would also be a legal Smack label.
>>
>>>>  int security_audit_rule_known(struct audit_krule *krule)
>>>> @@ -2518,14 +2551,31 @@ int security_audit_rule_known(struct audit_krule *krule)
>>>>         return call_int_hook(audit_rule_known, 0, krule);
>>>>  }
>>>>
>>>> -void security_audit_rule_free(void *lsmrule)
>>>> +void security_audit_rule_free(void **lsmrule)
>>>>  {
>>>> -       call_void_hook(audit_rule_free, lsmrule);
>>>> +       struct security_hook_list *hp;
>>>> +
>>>> +       hlist_for_each_entry(hp, &security_hook_heads.audit_rule_free, list) {
>>>> +               if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
>>>> +                       continue;
>>>> +               hp->hook.audit_rule_free(lsmrule[hp->lsmid->slot]);
>>>> +       }
>>>>  }
>>>>
>>> If one LSM frees the string, then the string is deleted from all LSMs. 
>>> I don't understand how this safe.
>> The audit system doesn't have a way to specify which LSM
>> a watched label is associated with. Even if we added one,
>> we'd still have to address the current behavior. Assigning
>> the watch to all modules means that seeing the string
>> in any module is sufficient to generate the event.
> I originally thought loading a new LSM policy could not delete existing
> LSM labels, but that isn't true.  If LSM labels can come and go based
> on policy, with this code, could loading a new policy for one LSM
> result in deleting labels of another LSM?

No. I could imagine a situation where changing policy on
a system where audit rules have been set could result in
confusion, but that would be true in the single LSM case.
It would require that secids used in the old policy be
used for different labels in the new policy. That would
not be sane behavior. I know it's impossible for Smack.

This is one of the reasons I'm switching from a single secid
to a collection of secids. You don't want unnatural behavior
of one LSM to impact the behavior of another.


>
>>>> -int security_audit_rule_match(u32 secid, u32 field, u32 op, void *lsmrule)
>>>> +int security_audit_rule_match(u32 secid, u32 field, u32 op, void **lsmrule)
>>>>  {
>>>> -       return call_int_hook(audit_rule_match, 0, secid, field, op, lsmrule);
>>>> +       struct security_hook_list *hp;
>>>> +       int rc;
>>>> +
>>>> +       hlist_for_each_entry(hp, &security_hook_heads.audit_rule_match, list) {
>>>> +               if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
>>>> +                       continue;
>>>> +               rc = hp->hook.audit_rule_match(secid, field, op,
>>>> +                                              &lsmrule[hp->lsmid->slot]);
>>>> +               if (rc)
>>>> +                       return rc;
>>> Suppose that there is an IMA dont_measure or dont_appraise rule, if one
>>> LSM matches, then this returns true, causing any measurement or
>>> integrity verification to be skipped.
>> Yes, that is correct. Like the audit system, you're doing a string based
>> lookup, which pretty well has to work this way. I have proposed compound
>> label specifications in the past, but even if we accepted something like
>> "apparmor=dates,selinux=figs" we'd still have to be compatible with the
>> old style inputs.
>>
>>> Sample policy rules:
>>> dont_measure obj_type=foo_log
>>> dont_appraise obj_type=foo_log
> IMA could extend the existing policy rules like "lsm=[selinux] |
> [smack] | [apparmor]", but that assumes that the underlying
> infrastructure supports it.

Yes, but you would still need rational behavior in the
case where someone has old IMA policy rules.

>
>>> Are there any plans to prevent label collisions or at least notify of a
>>> label collision?
>> What would that look like? You can't say that Smack isn't allowed
>> to use valid AppArmor labels. How would Smack know? If the label is
>> valid to both, how would you decide which LSM gets to use it?
> As this is a runtime issue, when loading a new policy at least flag the
> collision.  When removing the label, when it is defined by multiple
> LSMs, at least flag the removal.

To what end would the collision be flagged? What would you do with
the information?

>
>>>> +       }
>>>> +       return 0;
>>>>  }
>>>>  #endif /* CONFIG_AUDIT */