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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <886fcd04-6a08-d78c-dc82-301c991e5ad8@schaufler-ca.com>
Date:   Mon, 28 Dec 2020 12:06:24 -0800
From:   Casey Schaufler <casey@...aufler-ca.com>
To:     Mimi Zohar <zohar@...ux.ibm.com>, casey.schaufler@...el.com,
        jmorris@...ei.org, linux-security-module@...r.kernel.org,
        selinux@...r.kernel.org
Cc:     linux-audit@...hat.com, keescook@...omium.org,
        john.johansen@...onical.com, penguin-kernel@...ove.sakura.ne.jp,
        paul@...l-moore.com, sds@...ho.nsa.gov,
        linux-kernel@...r.kernel.org, bpf@...r.kernel.org
Subject: Re: [PATCH v23 02/23] LSM: Create and manage the lsmblob data
 structure.

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.

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

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