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

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.

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.

Is it possible these functions are not actually generating records if the 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?

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.

- 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