On 12/28/2020 2:14 PM, Mimi Zohar wrote:

On Mon, 2020-12-28 at 12:06 -0800, Casey Schaufler wrote:

quoted

On 12/28/2020 11:24 AM, Mimi Zohar wrote:

quoted

Hi Casey,

On Fri, 2020-11-20 at 12:14 -0800, Casey Schaufler wrote:

quoted

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.

quoted

quoted

 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.

quoted

quoted

quoted

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

quoted

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.

quoted

quoted

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?

quoted

quoted

quoted

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