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

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.

>>>> 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.

>> 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 */