Re: [PATCH v15 20/23] Audit: Add subj_LSM fields when necessary

[Paul Moore <paul@paul-moore.com>]

On Mon, Mar 9, 2020 at 9:25 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 3/6/2020 6:24 PM, Paul Moore wrote:
> > On Fri, Feb 21, 2020 at 7:06 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >> Add record entries to identify subject data for all of the
> >> security modules when there is more than one.
> >>
> >> Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
> >> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> >> Cc: netdev@vger.kernel.org
> >> Cc: linux-audit@redhat.com
> >> ---
> >>  drivers/android/binder.c                |  2 +-
> >>  include/linux/audit.h                   |  1 +
> >>  include/linux/security.h                |  9 ++++-
> >>  include/net/scm.h                       |  3 +-
> >>  kernel/audit.c                          | 40 ++++++++++++++++++-
> >>  kernel/audit_fsnotify.c                 |  1 +
> >>  kernel/auditfilter.c                    |  1 +
> >>  kernel/auditsc.c                        | 10 +++--
> >>  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_unlabeled.c       | 11 ++++--
> >>  net/netlabel/netlabel_user.c            |  2 +-
> >>  net/xfrm/xfrm_policy.c                  |  2 +
> >>  net/xfrm/xfrm_state.c                   |  2 +
> >>  security/integrity/ima/ima_api.c        |  1 +
> >>  security/integrity/integrity_audit.c    |  1 +
> >>  security/security.c                     | 51 +++++++++++++++++++++++--
> >>  19 files changed, 124 insertions(+), 23 deletions(-)
> > ...
> >
> >> diff --git a/kernel/audit.c b/kernel/audit.c
> >> index a25097cfe623..c3a1d8d2d33c 100644
> >> --- a/kernel/audit.c
> >> +++ b/kernel/audit.c
> >> @@ -2054,6 +2061,33 @@ void audit_log_key(struct audit_buffer *ab, char *key)
> >>                 audit_log_format(ab, "(null)");
> >>  }
> >>
> >> +void audit_log_task_lsms(struct audit_buffer *ab)
> >> +{
> >> +       int i;
> >> +       const char *lsm;
> >> +       struct lsmblob blob;
> >> +       struct lsmcontext context;
> >> +
> >> +       /*
> >> +        * Don't do anything unless there is more than one LSM
> >> +        * with a security context to report.
> >> +        */
> >> +       if (security_lsm_slot_name(1) == NULL)
> >> +               return;
> >> +
> >> +       security_task_getsecid(current, &blob);
> >> +
> >> +       for (i = 0; i < LSMBLOB_ENTRIES; i++) {
> >> +               lsm = security_lsm_slot_name(i);
> >> +               if (lsm == NULL)
> >> +                       break;
> >> +               if (security_secid_to_secctx(&blob, &context, i))
> >> +                       continue;
> >> +               audit_log_format(ab, " subj_%s=%s", lsm, context.context);
> >> +               security_release_secctx(&context);
> >> +       }
> >> +}
> >> +
> >>  int audit_log_task_context(struct audit_buffer *ab)
> >>  {
> >>         int error;
> >> @@ -2064,7 +2098,7 @@ int audit_log_task_context(struct audit_buffer *ab)
> >>         if (!lsmblob_is_set(&blob))
> >>                 return 0;
> >>
> >> -       error = security_secid_to_secctx(&blob, &context);
> >> +       error = security_secid_to_secctx(&blob, &context, LSMBLOB_FIRST);
> >>         if (error) {
> >>                 if (error != -EINVAL)
> >>                         goto error_path;
> > Sorry, please disregard my previous ACK.
>
> :(
>
> > We should treat "subj=" similar to how we treat "obj="; if there is
> > more than one LSM loaded the "subj=" should be set to "?" with the
> > "subj_XXX=" set to the appropriate label for the named LSM.  This
> > patch looks like it is always using LSMBLOB_FIRST and not "?" when
> > multiple LSMs are present.
>
> I'm fine with that, although I could see someone suggesting that
> would constitute breaking backward compatibility.

The argument is the same for both the subject and object fields.  I
maintain that in the brave new world of LSM stacking if you are using
a newly stacked kernel with old userspace, having a "?" for a
subject/object label is much safer than only showing the first LSM's
information and assuming that was the problem.  With a "?" for a
subject/object label you have some indication that something is "not
quite right" and you can dig into it further.

-- 
paul moore
www.paul-moore.com


--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit