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

From: Casey Schaufler <casey@schaufler-ca.com>
To: Richard Guy Briggs <rgb@redhat.com>
Cc: john.johansen@canonical.com, selinux@vger.kernel.org,
	linux-audit@redhat.com, casey.schaufler@intel.com,
	Stephen Smalley <sds@tycho.nsa.gov>,
	Casey Schaufler <casey@schaufler-ca.com>
Subject: Re: [PATCH v26 22/25] Audit: Add new record for multiple process LSM attributes
Date: Tue, 25 May 2021 15:46:08 -0700	[thread overview]
Message-ID: <a90a58be-5eb0-f447-a511-3f58e0326678@schaufler-ca.com> (raw)
In-Reply-To: <20210525200832.GI447005@madcap2.tricolour.ca>

On 5/25/2021 1:08 PM, Richard Guy Briggs wrote:
> On 2021-05-25 12:06, Casey Schaufler wrote:
>> On 5/25/2021 11:23 AM, Richard Guy Briggs wrote:
>>> On 2021-05-25 10:28, Casey Schaufler wrote:
>>>> On 5/21/2021 1:19 PM, Paul Moore wrote:
>>>>
>>>> <snip> and trim the CC list.
>>>>
>>>>> Okay, we've got a disconnect here regarding "audit contexts" and
>>>>> "local contexts", skip down below where I attempt to explain things a
>>>>> little more but basically if there is a place that uses this pattern:
>>>>>
>>>>>   audit_log_start(audit_context(), ...);
>>>> This pattern isn't helpful. audit_context() returns NULL in about 1 of 4 calls.
>>> Ok, this rings a bell...  I think we talked about this on an earlier
>>> revision...
>>>
>>>> All of these callers of audit_context() get a NULL result some of the time.
>>>>
>>>> getname_kernel
>>>> debugfs_create_dir
>>>> tracefs_create_file
>>>> vfs_fchown
>>>> do_settimeofday64
>>>> bprm_execve
>>>> ksys_mmap_pgoff
>>>> move_addr_to_kernel
>>>> __do_pipe_flags
>>>> __do_sys_capset
>>>> syscall_trace_enter
>>>> cap_bprm_creds_from_file
>>>> load_module
>>>> __x64_sys_fsetxattr
>>>> bpf_prog_load
>>>> audit_signal_info_syscall
>>>> sel_write_enforce
>>>> common_lsm_audit
>>>> audit_set_loginuid
>>>> __dev_set_promiscuity
>>>> ipcperms
>>>> devpts_pty_new
>>>>
>>>>> ... you don't need, or want, a "local context".  You might need a
>>>>> local context if you see the following pattern:
>>>>>
>>>>>   audit_log_start(NULL, ...);
>>>>>
>>>>> The "local context" idea is a hack and should be avoided whenever
>>>>> possible; if you have an existing audit context from a syscall, or
>>>>> something else, you *really* should use it ... or have a *really* good
>>>>> explanation as to why you can not.
>>>> Almost all audit events want to record the subject context by calling
>>>> audit_log_task_context(). If multiple contexts need to be recorded
>>>> there has to be a separate record, which means there has to be an
>>>> audit context. The only case where an audit context is reliably available
>>>> is in syscalls. Hence, a "local context" for many of the cases where the
>>>> first argument to audit_log_start() would otherwise be NULL, either
>>>> explicitly or because audit_context() returns NULL.
>>> Ok, so in that case, I think I'd test audit_context() and if it is
>>> indeed NULL then create a local context, otherwise use the one that is
>>> available.
>> audit_alloc_for_lsm() returns the value of audit_context() if it is
>> not NULL. Otherwise it checks to see if a separate record will
>> be required. If it is the value from audit_alloc_local() is returned.
>> Otherwise, it returns NULL.
>>
>>>   I should really go back and look carefully again at your
>>> code to see if it is in fact doing this, but unilaterally creating a
>>> local context if a context already exists is going to cause confusion
>>> because there will be two events generated for one event.
>> Indeed. I had discovered that.
>>
>>> Is it possible these functions are not actually generating records if
>>> the context is NULL?
>> There are definitely cases where records are generated when audit_context()
>> is NULL.
>>
>>> I'd be digging to find out why the context is NULL in these cases and if
>>> any record is even being produced in those cases?
>> Context is NULL because they're not coming out of syscalls.
> Where are they coming from then?  I'm guessing that they are in fact
> coming from syscalls, but that it wasn't a syscall rule that triggered
> the need to record the event.

audit_receive_msg() is one place. If you look at my patch you'll
see others, I only put audit_alloc_for_lsm() calls in where they were
needed. There are plenty of places.

>
>>>   Perhaps there is an
>>> active filter that indicates that logging is not of interest for that
>>> process/task/file/syscall/perm/etc...
>>>
>>>> Is there some other way to synchronize the "timestamp" without use of
>>>> an audit context? My understanding is that this is the primary purpose
>>>> of the audit context. 
>>> What has been done is to call it with a NULL context and it would assign
>>> a timestamp and serial number, but those are all single records that
>>> don't need synchronization (obviously).  This was why I'd come up with
>>> this mechanism to solve the need to attach a contid record to a single
>>> record associated with a network event (or user record that should not
>>> be associated with a syscall).  Those were the only two use cases I had
>>> up until now.
>> Right. Adding the contid record is a special case. Adding the lsmcontext
>> record is the common case. Thus Paul's lament.
> Yeah, this is a similar sort of accompanying record that needs to be
> tied into an event which is why I had suggested the similarity behind
> your local context generation patch and the one in the contid patchset.

Just so. I think the $2 point is that the audit system seems oriented
to have multiple records per event be unusual. I need to make it normal.

>
>>> - RGB
> - RGB
>
> --
> Richard Guy Briggs <rgb@redhat.com>
> Sr. S/W Engineer, Kernel Security, Base Operating Systems
> Remote, Ottawa, Red Hat Canada
> IRC: rgb, SunRaycer
> Voice: +1.647.777.2635, Internal: (81) 32635
>