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

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(a)redhat.com&gt; 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