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

[Casey Schaufler]

On 12/28/2020 5:53 PM, Mimi Zohar wrote:
> On Mon, 2020-12-28 at 15:20 -0800, Casey Schaufler wrote:
>> 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.
> From an IMA perspective, allowing multiple LSMs to define the same
> policy label is worse than requiring the label be constrained to a
> particular LSM.

Just to be sure we're talking about the same thing,
the case I'm referring to is something like a file with
two extended attributes:

	security.apparmor MacAndCheese
	security.SMACK64 MacAndCheese

and an IMA rule that says

	dont_measure obj_type=MacAndCheese

In this case the dont_measure will be applied to both.
On the other hand,

	security.apparmor MacAndCheese
	security.SMACK64 FranksAndBeans

would also apply the rule to both, which is not
what you want. Unfortunately, there is no way to
differentiate which LSM hit the rule.

So now I'm a little confused. The case where both LSMs
use the same label looks like it works right, where the
case where they're different doesn't.

I'm beginning to think that identifying which LSMs matched
a rule (it may be none, either or both) is the right solution.
I don't think that audit is as sensitive to this.


>
>>>>> 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?
> Unfortunately, unless audit supports per LSM labels, the infrastructure
> needs to detect and prevent the label collision.

That would be a massive performance hit.

>
>>> 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?
> LSM label collision is probably an example of kernel integrity critical
> data (yet to be upstreamed).
>
>>>>>> +       }
>>>>>> +       return 0;
>>>>>>  }
>>>>>>  #endif /* CONFIG_AUDIT */
>