Re: [PATCH v26 22/25] Audit: Add new record for multiple process LSM attributes

[Casey Schaufler]

On 5/21/2021 1:19 PM, Paul Moore wrote:
> On Thu, May 13, 2021 at 4:32 PM Casey Schaufler <casey@xxxxxxxxxxxxxxxx> wrote:
>> Create a new audit record type to contain the subject information
>> when there are multiple security modules that require such data.
>> This record is linked with the same timestamp and serial number
>> using the audit_alloc_local() mechanism.
> The record is linked with the other associated records into a single
> event, it doesn't matter if it gets the timestamp/serial from
> audit_alloc_local() or an existing audit event, e.g. ongoing syscall.
>
>> The record is produced only in cases where there is more than one
>> security module with a process "context".
>> In cases where this record is produced the subj= fields of
>> other records in the audit event will be set to "subj=?".
>>
>> An example of the MAC_TASK_CONTEXTS (1420) record is:
>>
>>         type=UNKNOWN[1420]
>>         msg=audit(1600880931.832:113)
>>         subj_apparmor==unconfined
> It should be just a single "=" in the line above.

AppArmor provides the 2nd "=" as part of the subject context.
What's here is correct. I won't argue that it won't case confusion
or worse.


>>         subj_smack=_
>>
>> There will be a subj_$LSM= entry for each security module
>> LSM that supports the secid_to_secctx and secctx_to_secid
>> hooks. The BPF security module implements secid/secctx
>> translation hooks, so it has to be considered to provide a
>> secctx even though it may not actually do so.
>>
>> Signed-off-by: Casey Schaufler <casey@xxxxxxxxxxxxxxxx>
>> To: paul@xxxxxxxxxxxxxx
>> To: linux-audit@xxxxxxxxxx
>> To: rgb@xxxxxxxxxx
>> Cc: netdev@xxxxxxxxxxxxxxx
>> ---
>>  drivers/android/binder.c                |  2 +-
>>  include/linux/audit.h                   | 24 ++++++++
>>  include/linux/security.h                | 16 ++++-
>>  include/net/netlabel.h                  |  3 +-
>>  include/net/scm.h                       |  2 +-
>>  include/net/xfrm.h                      | 13 +++-
>>  include/uapi/linux/audit.h              |  1 +
>>  kernel/audit.c                          | 80 ++++++++++++++++++-------
>>  kernel/audit.h                          |  3 +
>>  kernel/auditfilter.c                    |  6 +-
>>  kernel/auditsc.c                        | 75 ++++++++++++++++++++---
>>  net/ipv4/ip_sockglue.c                  |  2 +-
>>  net/netfilter/nf_conntrack_netlink.c    |  4 +-
>>  net/netfilter/nf_conntrack_standalone.c |  2 +-
>>  net/netfilter/nfnetlink_queue.c         |  2 +-
>>  net/netlabel/netlabel_domainhash.c      |  4 +-
>>  net/netlabel/netlabel_unlabeled.c       | 24 ++++----
>>  net/netlabel/netlabel_user.c            | 20 ++++---
>>  net/netlabel/netlabel_user.h            |  6 +-
>>  net/xfrm/xfrm_policy.c                  | 10 ++--
>>  net/xfrm/xfrm_state.c                   | 20 ++++---
>>  security/integrity/ima/ima_api.c        |  7 ++-
>>  security/integrity/integrity_audit.c    |  6 +-
>>  security/security.c                     | 46 +++++++++-----
>>  security/smack/smackfs.c                |  3 +-
>>  25 files changed, 274 insertions(+), 107 deletions(-)
> ...
>
>> diff --git a/include/linux/audit.h b/include/linux/audit.h
>> index 97cd7471e572..229cd71fbf09 100644
>> --- a/include/linux/audit.h
>> +++ b/include/linux/audit.h
>> @@ -386,6 +395,19 @@ static inline void audit_ptrace(struct task_struct *t)
>>                 __audit_ptrace(t);
>>  }
>>
>> +static inline struct audit_context *audit_alloc_for_lsm(gfp_t gfp)
>> +{
>> +       struct audit_context *context = audit_context();
>> +
>> +       if (context)
>> +               return context;
>> +
>> +       if (lsm_multiple_contexts())
>> +               return audit_alloc_local(gfp);
>> +
>> +       return NULL;
>> +}
> See my other comments, but this seems wrong at face value.  The
> additional LSM record should happen as part of the existing audit log
> functions.

I'm good with that. But if you defer calling audit_alloc_local()
until you know you need it you may be in a place where you can't
associate the new context with the event. I think. I will have
another go at it.

<snip> 

> I think I was distracted with the local context issue and I've lost
> track of the details here, perhaps it's best to fix the local context
> issue first (that should be a big change to this patch) and then we
> can take another look.

I really need to move forward. I'll give allocation of local contexts
as necessary in audit_log_task_context() another shot. 

>
>
> --
> paul moore
> www.paul-moore.com