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

On Tue, Aug 24, 2021 at 11:20 AM Casey Schaufler [off-list ref] wrote:

On 8/24/2021 7:45 AM, Paul Moore wrote:

quoted

On Fri, Aug 20, 2021 at 7:48 PM Casey Schaufler [off-list ref] wrote:

quoted

quoted

On 8/20/2021 12:06 PM, Paul Moore wrote:

quoted

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 ;)

quoted

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.

quoted

[*] 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