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

[Paul Moore]

On Tue, Aug 24, 2021 at 11:20 AM Casey Schaufler <casey at schaufler-ca.com> wrote:
> On 8/24/2021 7:45 AM, Paul Moore wrote:
> > On Fri, Aug 20, 2021 at 7:48 PM Casey Schaufler <casey at 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.
>
> As I said, it's a hack-around that demonstrates the scope of the
> problem. Had you expressed enthusiastic approval for it I'd have
> been very surprised.

That's okay, you can admit you were trying to catch me not paying attention ;)

> > 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:
>
> I will give this a shot.

Thanks.  I'm sure I'm probably missing some detail, but if you get
stuck let me know and I'll try to lend a hand.

> > [*] 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.
>
> Hmm. That effort has been quiet lately. Too quiet.

The current delay is intentional and is related to the io_uring work.

When Richard and I first became aware of the io_uring issue Richard
was in the process of readying his latest revision to the audit
container ID patchset and some of the changes he was incorporating, in
my opinion, hinted at the io_uring issue, or at least drew more
attention to that than I was comfortable seeing posted publicly.
Richard discussed this with his management and security response team,
and they felt differently.  I told Richard that I didn't want to block
him posting an update to the patchset, but that I felt it would be The
Wrong Thing To Do and if he did post the patchset I would likely
ignore it until after the io_uring fix had been posted so as to not
draw additional attention to his changes.  I can't speak for Richard's
mindset, but he appeared anxious to post his changes regardless of my
concerns, and he did so shortly afterwards.

That's why you haven't seen much progress on this for a while, and why
you will see me comment on the latest patchset after the io_uring
patches land in -next after the next merge window closes.

-- 
paul moore
www.paul-moore.com