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

[Richard Guy Briggs]

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.

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

> > - RGB

- RGB

--
Richard Guy Briggs <rgb at 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