Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Paul Moore <paul@paul-moore.com>
To: Casey Schaufler <casey@schaufler-ca.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
Subject: Re: [PATCH v28 22/25] Audit: Add record for multiple process LSM attributes
Date: Fri, 13 Aug 2021 11:31:30 -0400	[thread overview]
Message-ID: <CAHC9VhSSASAL1mVwDo1VS3HcEF7Yb3LTTaoajEtq1HsA-8R+xQ@mail.gmail.com> (raw)
In-Reply-To: <ace9d273-3560-3631-33fa-7421a165b038@schaufler-ca.com>

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 ...

> > 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.

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.
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().  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.

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.

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().  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 first (report back on your audit config please), I may
be worrying about a hypothetical that isn't real.

-- 
paul moore
www.paul-moore.com

  reply	other threads:[~2021-08-13 15:31 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 [this message]
2021-08-13 18:48         ` Casey Schaufler
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=CAHC9VhSSASAL1mVwDo1VS3HcEF7Yb3LTTaoajEtq1HsA-8R+xQ@mail.gmail.com \
    --to=paul@paul-moore.com \
    --cc=casey.schaufler@intel.com \
    --cc=casey@schaufler-ca.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=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).