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

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.

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

> - RGB