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

[Paul Moore <paul@paul-moore.com>]

On Fri, Aug 20, 2021 at 7:48 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > On 8/20/2021 12:06 PM, Paul Moore wrote:
> >> Unless you explicitly enable audit on the kernel cmdline, e.g.
> >> "audit=1", processes started before userspace enables audit will not
> >> have a properly allocated audit_context; see the "if
> >> (likely(!audit_ever_enabled))" check at the top of audit_alloc() for
> >> the reason why.
>
> I found a hack-around that no one will like. I changed that check to be
>
> (likely(!audit_ever_enabled) && !lsm_multiple_contexts())
>
> It probably introduces a memory leak and/or performance degradation,
> but it has the desired affect.

I can't speak for everyone, but I know I don't like that as a solution
;)  I imagine such a change would also draw the ire of the never-audit
crowd and the distros themselves.  However, I understand the need to
just get *something* in place so you can continue to test/develop;
it's fine to keep that for now, but I'm going to be very disappointed
if that line finds its way into the next posted patchset revision.

I'm very much open to ideas but my gut feeling is that the end
solution is going to be changes to audit_log_start() and
audit_log_end().  In my mind the primary reason for this hunch is that
support for multiple LSMs[*] needs to be transparent to the various
callers in the kernel; this means the existing audit pattern of ...

  audit_log_start(...);
  audit_log_format(...);
  audit_log_end(...);

... should be preserved and be unchanged from what it is now.  We've
already talked in some general terms about what such changes might
look like, but to summarize the previous discussions, I think we would
need to think about the following things:

* Adding a timestamp/serial field to the audit_buffer struct, it could
even be in a union with the audit_context pointer as we would never
need both at the same time: if the audit_context ptr is non-NULL you
use that, otherwise you use the timestamp.  The audit_buffer timestamp
would not eliminate the need for the timestamp info in the
audit_context struct for what I hope are obvious reasons.

* Additional logic in audit_log_end() to generate additional ancillary
records for LSM labels.  This will likely mean storing the message
"type" passed to audit_log_start() in the audit_record struct and
using that information in audit_log_end() to decide if a LSM ancillary
record is needed.  Logistically this would likely mean converting the
existing audit_log_end() function into a static/private
__audit_log_end() and converting audit_log_end() into something like
this:

  void audit_log_end(ab)
  {
    __audit_log_end(ab); // rm the ab cleanup from __audit_log_end
    if (ab->anc_mlsm) {
      // generate the multiple lsm record
    }
    audit_buffer_free(ab);
  }

* Something else that I'm surely forgetting :)

At the end of all this we may find that the "local" audit_context
concept is no longer needed.  It was originally created to deal with
grouping ancillary records with non-syscall records into a single
event; while what we are talking about above is different, I believe
that our likely solution will also work to solve the original "local"
audit_context use case as well.  We'll have to see how this goes.

[*] I expect that the audit container ID work will have similar issues
and need a similar solution, I'm surprised it hasn't come up yet.

-- 
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit