linux-security-module.vger.kernel.org archive mirror
 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 v26 22/25] Audit: Add new record for multiple process LSM attributes
Date: Fri, 21 May 2021 22:20:44 -0400	[thread overview]
Message-ID: <CAHC9VhR9OPbNCLaKpCEt9mES8yWXpNoTBrgnKW2ER+vEkuNQwQ@mail.gmail.com> (raw)
In-Reply-To: <d753115f-6cbd-0886-473c-b10485cb7c52@schaufler-ca.com>

On Fri, May 21, 2021 at 6:05 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 5/21/2021 1:19 PM, Paul Moore wrote:
> > On Thu, May 13, 2021 at 4:32 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.
> >> This record is linked with the same timestamp and serial number
> >> using the audit_alloc_local() mechanism.
> > The record is linked with the other associated records into a single
> > event, it doesn't matter if it gets the timestamp/serial from
> > audit_alloc_local() or an existing audit event, e.g. ongoing syscall.
> >
> >> The record is produced only in cases where there is more than one
> >> security module with a process "context".
> >> In cases where this record is produced the subj= fields of
> >> other records in the audit event will be set to "subj=?".
> >>
> >> An example of the MAC_TASK_CONTEXTS (1420) record is:
> >>
> >>         type=UNKNOWN[1420]
> >>         msg=audit(1600880931.832:113)
> >>         subj_apparmor==unconfined
> > It should be just a single "=" in the line above.
>
> AppArmor provides the 2nd "=" as part of the subject context.
> What's here is correct. I won't argue that it won't case confusion
> or worse.

Oh, wow, okay.  That needs to change at some point but I agree it's
out of scope for this patchset.  In the meantime I might suggest using
something other than AppArmor as an example here.

> >>         subj_smack=_
> >>
> >> There will be a subj_$LSM= entry for each security module
> >> LSM that supports the secid_to_secctx and secctx_to_secid
> >> hooks. The BPF security module implements secid/secctx
> >> translation hooks, so it has to be considered to provide a
> >> secctx even though it may not actually do so.
> >>
> >> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> >> To: paul@paul-moore.com
> >> To: linux-audit@redhat.com
> >> To: rgb@redhat.com
> >> Cc: netdev@vger.kernel.org
> >> ---
> >>  drivers/android/binder.c                |  2 +-
> >>  include/linux/audit.h                   | 24 ++++++++
> >>  include/linux/security.h                | 16 ++++-
> >>  include/net/netlabel.h                  |  3 +-
> >>  include/net/scm.h                       |  2 +-
> >>  include/net/xfrm.h                      | 13 +++-
> >>  include/uapi/linux/audit.h              |  1 +
> >>  kernel/audit.c                          | 80 ++++++++++++++++++-------
> >>  kernel/audit.h                          |  3 +
> >>  kernel/auditfilter.c                    |  6 +-
> >>  kernel/auditsc.c                        | 75 ++++++++++++++++++++---
> >>  net/ipv4/ip_sockglue.c                  |  2 +-
> >>  net/netfilter/nf_conntrack_netlink.c    |  4 +-
> >>  net/netfilter/nf_conntrack_standalone.c |  2 +-
> >>  net/netfilter/nfnetlink_queue.c         |  2 +-
> >>  net/netlabel/netlabel_domainhash.c      |  4 +-
> >>  net/netlabel/netlabel_unlabeled.c       | 24 ++++----
> >>  net/netlabel/netlabel_user.c            | 20 ++++---
> >>  net/netlabel/netlabel_user.h            |  6 +-
> >>  net/xfrm/xfrm_policy.c                  | 10 ++--
> >>  net/xfrm/xfrm_state.c                   | 20 ++++---
> >>  security/integrity/ima/ima_api.c        |  7 ++-
> >>  security/integrity/integrity_audit.c    |  6 +-
> >>  security/security.c                     | 46 +++++++++-----
> >>  security/smack/smackfs.c                |  3 +-
> >>  25 files changed, 274 insertions(+), 107 deletions(-)
> > ...
> >
> >> diff --git a/include/linux/audit.h b/include/linux/audit.h
> >> index 97cd7471e572..229cd71fbf09 100644
> >> --- a/include/linux/audit.h
> >> +++ b/include/linux/audit.h
> >> @@ -386,6 +395,19 @@ static inline void audit_ptrace(struct task_struct *t)
> >>                 __audit_ptrace(t);
> >>  }
> >>
> >> +static inline struct audit_context *audit_alloc_for_lsm(gfp_t gfp)
> >> +{
> >> +       struct audit_context *context = audit_context();
> >> +
> >> +       if (context)
> >> +               return context;
> >> +
> >> +       if (lsm_multiple_contexts())
> >> +               return audit_alloc_local(gfp);
> >> +
> >> +       return NULL;
> >> +}
> > See my other comments, but this seems wrong at face value.  The
> > additional LSM record should happen as part of the existing audit log
> > functions.
>
> I'm good with that. But if you defer calling audit_alloc_local()
> until you know you need it you may be in a place where you can't
> associate the new context with the event. I think. I will have
> another go at it.

I can't think of a case where you would ever not know if you need to
allocate a local context at the start.  If you are unsure, get in
touch and we can work it out.

> > I think I was distracted with the local context issue and I've lost
> > track of the details here, perhaps it's best to fix the local context
> > issue first (that should be a big change to this patch) and then we
> > can take another look.
>
> I really need to move forward. I'll give allocation of local contexts
> as necessary in audit_log_task_context() another shot.

I appreciate the desire to move forward, and while I can't speak for
everyone, I'll do my best to work with you to find a good solution.
If you get stuck or aren't sure you know how to reach me :)

As a start, I might suggest looking at some of the recent audit
container ID patchsets from Richard; while they have had some issues,
they should serve as a basic example of what we mean when we talk
about "local contexts" and how they should be used.

-- 
paul moore
www.paul-moore.com

  reply	other threads:[~2021-05-22  2:21 UTC|newest]

Thread overview: 49+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20210513200807.15910-1-casey.ref@schaufler-ca.com>
2021-05-13 20:07 ` [PATCH v26 00/25] LSM: Module stacking for AppArmor Casey Schaufler
2021-05-13 20:07   ` [PATCH v26 01/25] LSM: Infrastructure management of the sock security Casey Schaufler
2021-05-13 20:07   ` [PATCH v26 02/25] LSM: Add the lsmblob data structure Casey Schaufler
2021-05-22  8:39     ` Mickaël Salaün
2021-05-25 23:52       ` Casey Schaufler
2021-05-26  9:53         ` Mickaël Salaün
2021-05-13 20:07   ` [PATCH v26 03/25] LSM: provide lsm name and id slot mappings Casey Schaufler
2021-05-14 19:00     ` Kees Cook
2021-05-21 20:18     ` Paul Moore
2021-05-13 20:07   ` [PATCH v26 04/25] IMA: avoid label collisions with stacked LSMs Casey Schaufler
2021-05-14 19:00     ` Kees Cook
2021-05-13 20:07   ` [PATCH v26 05/25] LSM: Use lsmblob in security_audit_rule_match Casey Schaufler
2021-05-13 20:07   ` [PATCH v26 06/25] LSM: Use lsmblob in security_kernel_act_as Casey Schaufler
2021-05-13 20:07   ` [PATCH v26 07/25] LSM: Use lsmblob in security_secctx_to_secid Casey Schaufler
2021-05-14 19:03     ` Kees Cook
2021-05-21 20:18     ` Paul Moore
2021-05-13 20:07   ` [PATCH v26 08/25] LSM: Use lsmblob in security_secid_to_secctx Casey Schaufler
2021-05-14 19:05     ` Kees Cook
2021-05-21 20:18     ` Paul Moore
2021-05-13 20:07   ` [PATCH v26 09/25] LSM: Use lsmblob in security_ipc_getsecid Casey Schaufler
2021-05-13 20:07   ` [PATCH v26 10/25] LSM: Use lsmblob in security_task_getsecid Casey Schaufler
2021-05-13 20:07   ` [PATCH v26 11/25] LSM: Use lsmblob in security_inode_getsecid Casey Schaufler
2021-05-13 20:07   ` [PATCH v26 12/25] LSM: Use lsmblob in security_cred_getsecid Casey Schaufler
2021-05-13 20:07   ` [PATCH v26 13/25] IMA: Change internal interfaces to use lsmblobs Casey Schaufler
2021-05-13 20:07   ` [PATCH v26 14/25] LSM: Specify which LSM to display Casey Schaufler
2021-05-14 19:23     ` Kees Cook
2021-05-17 19:52       ` Casey Schaufler
2021-05-21 20:19         ` Paul Moore
2021-05-13 20:07   ` [PATCH v26 15/25] LSM: Ensure the correct LSM context releaser Casey Schaufler
2021-05-21 20:19     ` Paul Moore
2021-05-13 20:07   ` [PATCH v26 16/25] LSM: Use lsmcontext in security_secid_to_secctx Casey Schaufler
2021-05-13 20:07   ` [PATCH v26 17/25] LSM: Use lsmcontext in security_inode_getsecctx Casey Schaufler
2021-05-14 19:24     ` Kees Cook
2021-05-13 20:08   ` [PATCH v26 18/25] LSM: security_secid_to_secctx in netlink netfilter Casey Schaufler
2021-05-21 20:19     ` Paul Moore
2021-05-13 20:08   ` [PATCH v26 19/25] NET: Store LSM netlabel data in a lsmblob Casey Schaufler
2021-05-13 20:08   ` [PATCH v26 20/25] LSM: Verify LSM display sanity in binder Casey Schaufler
2021-05-13 20:08   ` [PATCH v26 21/25] audit: add support for non-syscall auxiliary records Casey Schaufler
2021-05-21 20:19     ` Paul Moore
2021-05-13 20:08   ` [PATCH v26 22/25] Audit: Add new record for multiple process LSM attributes Casey Schaufler
2021-05-21 20:19     ` Paul Moore
2021-05-21 21:26       ` Richard Guy Briggs
2021-05-21 22:05       ` Casey Schaufler
2021-05-22  2:20         ` Paul Moore [this message]
2021-05-22 12:58           ` Richard Guy Briggs
2021-05-25 16:26       ` Casey Schaufler
2021-05-13 20:08   ` [PATCH v26 23/25] Audit: Add a new record for multiple object " Casey Schaufler
2021-05-13 20:08   ` [PATCH v26 24/25] LSM: Add /proc attr entry for full LSM context Casey Schaufler
2021-05-13 20:08   ` [PATCH v26 25/25] AppArmor: Remove the exclusive flag Casey Schaufler

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=CAHC9VhR9OPbNCLaKpCEt9mES8yWXpNoTBrgnKW2ER+vEkuNQwQ@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 \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).