Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Casey Schaufler <casey@schaufler-ca.com>
To: Paul Moore <paul@paul-moore.com>
Cc: casey.schaufler@intel.com, James Morris <jmorris@namei.org>,
	linux-security-module@vger.kernel.org, selinux@vger.kernel.org,
	linux-audit@redhat.com, keescook@chromium.org,
	john.johansen@canonical.com, penguin-kernel@i-love.sakura.ne.jp,
	Stephen Smalley <sds@tycho.nsa.gov>,
	linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
	Casey Schaufler <casey@schaufler-ca.com>
Subject: Re: [PATCH v28 22/25] Audit: Add record for multiple process LSM attributes
Date: Fri, 13 Aug 2021 11:48:26 -0700	[thread overview]
Message-ID: <fba1a123-d6e5-dcb0-3d49-f60b26f65b29@schaufler-ca.com> (raw)
In-Reply-To: <CAHC9VhSSASAL1mVwDo1VS3HcEF7Yb3LTTaoajEtq1HsA-8R+xQ@mail.gmail.com>

On 8/13/2021 8:31 AM, Paul Moore wrote:
> On Thu, Aug 12, 2021 at 6:38 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 8/12/2021 1:59 PM, Paul Moore wrote:
>>> On Wed, Jul 21, 2021 at 9:12 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>> Create a new audit record type to contain the subject information
>>>> when there are multiple security modules that require such data.
> ...
>
>>> The local
>>> audit context is a hack that is made necessary by the fact that we
>>> have to audit things which happen outside the scope of an executing
>>> task, e.g. the netfilter audit hooks, it should *never* be used when
>>> there is a valid task_struct.
>> In the existing audit code a "current context" is only needed for
>> syscall events, so that's the only case where it's allocated. Would
>> you suggest that I track down the non-syscall events that include
>> subj= fields and add allocate a "current context" for them? I looked
>> into doing that, and it wouldn't be simple.
> This is why the "local context" was created.  Prior to these stacking
> additions, and the audit container ID work, we never needed to group
> multiple audit records outside of a syscall context into a single
> audit event so passing a NULL context into audit_log_start() was
> reasonable.  The local context was designed as a way to generate a
> context for use in a local function scope to group multiple records,
> however, for reasons I'll get to below I'm now wondering if the local
> context approach is really workable ...

I haven't found a place where it didn't work. What is the concern?


>>> Hopefully that makes sense?
>> Yes, it makes sense. Methinks you may believe that the current context
>> is available more regularly than it actually is.
>>
>> I instrumented the audit event functions with:
>>
>>         WARN_ONCE(audit_context, "%s has context\n", __func__);
>>         WARN_ONCE(!audit_context, "%s lacks context\n", __func__);
>>
>> I only used local contexts where the 2nd WARN_ONCE was hit.
> What does your audit config look like?  Both the kernel command line
> and the output of 'auditctl -l' would be helpful.

On the fedora system:

BOOT_IMAGE=(hd0,gpt2)/vmlinuz-5.14.0-rc5stack+
root=/dev/mapper/fedora-root ro resume=/dev/mapper/fedora-swap
rd.lvm.lv=fedora/root rd.lvm.lv=fedora/swap lsm.debug

-a always,exit -F arch=b64 -S bpf -F key=testsuite-1628714321-EtlWIphW

On the Ubuntu system:

BOOT_IMAGE=/boot/vmlinuz-5.14.0-rc1stack+
root=UUID=39c25777-d413-4c2e-948c-dfa2bf259049 ro lsm.debug

No rules

> I'm beginning to suspect that you have the default
> we-build-audit-into-the-kernel-because-product-management-said-we-have-to-but-we-don't-actually-enable-it-at-runtime
> audit configuration that is de rigueur for many distros these days.

Yes, but I've also fiddled about with it so as to get better event coverage.
I've run the audit-testsuite, which has got to fiddle about with the audit
configuration.

> If that is the case, there are many cases where you would not see a
> NULL current->audit_context simply because the config never allocated
> one, see kernel/auditsc.c:audit_alloc().

I assume you mean that I *would* see a NULL current->audit_context
in the "event not enabled" case.
 

> If that is the case, I'm
> honestly a little surprised we didn't realize that earlier, especially
> given all the work/testing that Richard has done with the audit
> container ID bits, but then again he surely had a proper audit config
> during his testing so it wouldn't have appeared.
>
> Good times.

Indeed.

> Regardless, assuming that is the case we probably need to find an
> alternative to the local context approach as it currently works.  For
> reasons we already talked about, we don't want to use a local
> audit_context if there is the possibility for a proper
> current->audit_context, but we need to do *something* so that we can
> group these multiple events into a single record.

I tried a couple things, but neither was satisfactory.

> Since this is just occurring to me now I need a bit more time to think
> on possible solutions - all good ideas are welcome - but the first
> thing that pops into my head is that we need to augment
> audit_log_end() to potentially generated additional, associated
> records similar to what we do on syscall exit in audit_log_exit().

I looked into that. You need a place to save the timestamp
that doesn't disappear. That's the role the audit_context plays
now.

>   Of
> course the audit_log_end() changes would be much more limited than
> audit_log_exit(), just the LSM subject and audit container ID info,
> and even then we might want to limit that to cases where the ab->ctx
> value is NULL and let audit_log_exit() handle it otherwise.  We may
> need to store the event type in the audit_buffer during
> audit_log_start() so that we can later use that in audit_log_end() to
> determine what additional records are needed.
>
> Regardless, let's figure out why all your current->audit_context
> values are NULL

That's what's maddening, and why I implemented audit_alloc_for_lsm().
They aren't all NULL. Sometimes current->audit_context is NULL,
sometimes it isn't, for the same event. I thought it might be a
question of the netlink interface being treated specially, but
that doesn't explain all the cases.

>  first (report back on your audit config please), I may
> be worrying about a hypothetical that isn't real.
>


  reply	other threads:[~2021-08-13 18:48 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20210722004758.12371-1-casey@schaufler-ca.com>
2021-07-22  0:47 ` [PATCH v28 07/25] LSM: Use lsmblob in security_secctx_to_secid Casey Schaufler
2021-07-22  0:47 ` [PATCH v28 08/25] LSM: Use lsmblob in security_secid_to_secctx Casey Schaufler
2021-07-22  0:47 ` [PATCH v28 10/25] LSM: Use lsmblob in security_task_getsecid Casey Schaufler
2021-07-22  0:47 ` [PATCH v28 15/25] LSM: Ensure the correct LSM context releaser Casey Schaufler
2021-07-22  0:47 ` [PATCH v28 16/25] LSM: Use lsmcontext in security_secid_to_secctx Casey Schaufler
2021-07-22  0:47 ` [PATCH v28 18/25] LSM: security_secid_to_secctx in netlink netfilter Casey Schaufler
2021-07-22  0:47 ` [PATCH v28 19/25] NET: Store LSM netlabel data in a lsmblob Casey Schaufler
2021-07-22  0:47 ` [PATCH v28 22/25] Audit: Add record for multiple process LSM attributes Casey Schaufler
2021-08-12 20:59   ` Paul Moore
2021-08-12 22:38     ` Casey Schaufler
2021-08-13 15:31       ` Paul Moore
2021-08-13 18:48         ` Casey Schaufler [this message]
2021-08-13 20:43           ` Paul Moore
2021-08-13 21:47             ` Casey Schaufler
2021-08-16 18:57               ` Paul Moore

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=fba1a123-d6e5-dcb0-3d49-f60b26f65b29@schaufler-ca.com \
    --to=casey@schaufler-ca.com \
    --cc=casey.schaufler@intel.com \
    --cc=jmorris@namei.org \
    --cc=john.johansen@canonical.com \
    --cc=keescook@chromium.org \
    --cc=linux-audit@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=paul@paul-moore.com \
    --cc=penguin-kernel@i-love.sakura.ne.jp \
    --cc=sds@tycho.nsa.gov \
    --cc=selinux@vger.kernel.org \
    --subject='Re: [PATCH v28 22/25] Audit: Add record for multiple process LSM attributes' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).