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@xxxxxxxxxx> > 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 >
