Message-ID: <CAHC9VhR8PscKpA5BrgTNj8cq_eQ6svqru6UXidc=v5+Ha+PM7Q@mail.gmail.com>
Date:   Wed, 26 Aug 2020 09:42:54 -0400
From:   Paul Moore <paul@...l-moore.com>
To:     Peter Enderborg <peter.enderborg@...y.com>
Cc:     linux-kernel@...r.kernel.org,
        SElinux list <selinux@...r.kernel.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Stephen Smalley <stephen.smalley.work@...il.com>
Subject: Re: [RFC PATCH] selinux: Add denied trace with permssion filter

On Mon, Aug 24, 2020 at 9:23 AM Peter Enderborg
<peter.enderborg@...y.com> wrote:
>
> This adds tracing of all denies. They are grouped with trace_seq for
> each audit.
>
> A filter can be inserted with a write to it's filter section.
>
> echo "permission==\"entrypoint\"" > events/avc/selinux_denied/filter
>
> A output will be like:
>           runcon-1046  [002] .N..   156.351738: selinux_denied:
>           trace_seq=2 result=-13
>           scontext=system_u:system_r:cupsd_t:s0-s0:c0.
>           c1023 tcontext=system_u:object_r:bin_t:s0
>           tclass=file permission=entrypoint
>
> Signed-off-by: Peter Enderborg <peter.enderborg@...y.com>
> ---
>  include/trace/events/avc.h | 37 +++++++++++++++++++++++++++++++++++++
>  security/selinux/avc.c     | 27 +++++++++++++++++++++++++--
>  2 files changed, 62 insertions(+), 2 deletions(-)

My most significant comment is that I don't think we want, or need,
two trace points in the avc_audit_post_callback() function.  Yes, I
understand they are triggered slightly differently, but from my
perspective there isn't enough difference between the two tracepoints
to warrant including both.  However, while the tracepoints may be
redundant in my mind, this new event does do the permission lookup in
the kernel so that the contexts/class/permissions are all available as
a string which is a good thing.

Without going into the details, would the tracing folks be okay with
doing something similar with the existing selinux_audited tracepoint?
It's extra work in the kernel, but since it would only be triggered
when the tracepoint was active it seems bearable to me.

> diff --git a/include/trace/events/avc.h b/include/trace/events/avc.h
> index 94bca8bef8d2..9a28559956de 100644
> --- a/include/trace/events/avc.h
> +++ b/include/trace/events/avc.h
> @@ -54,6 +54,43 @@ TRACE_EVENT(selinux_audited,
>         )
>  );
>
> +TRACE_EVENT(selinux_denied,
> +
> +       TP_PROTO(struct selinux_audit_data *sad,
> +               char *scontext,
> +               char *tcontext,
> +               const char *tclass,
> +               const char *permission,
> +               int64_t seq
> +       ),
> +
> +       TP_ARGS(sad, scontext, tcontext, tclass, permission, seq),
> +
> +       TP_STRUCT__entry(
> +               __field(int, result)
> +               __string(scontext, scontext)
> +               __string(tcontext, tcontext)
> +               __string(permission, permission)
> +               __string(tclass, tclass)
> +               __field(u64, seq)
> +       ),
> +
> +       TP_fast_assign(
> +               __entry->result = sad->result;
> +               __entry->seq    = seq;
> +               __assign_str(tcontext, tcontext);
> +               __assign_str(scontext, scontext);
> +               __assign_str(permission, permission);
> +               __assign_str(tclass, tclass);
> +       ),
> +
> +       TP_printk("trace_seq=%lld result=%d scontext=%s tcontext=%s tclass=%s permission=%s",
> +                __entry->seq, __entry->result, __get_str(scontext), __get_str(tcontext),
> +                __get_str(tclass), __get_str(permission)
> +
> +       )
> +);
> +
>  #endif
>
>  /* This part must be outside protection */
> diff --git a/security/selinux/avc.c b/security/selinux/avc.c
> index 1debddfb5af9..ca53baca15e1 100644
> --- a/security/selinux/avc.c
> +++ b/security/selinux/avc.c
> @@ -92,6 +92,7 @@ struct selinux_avc {
>  };
>
>  static struct selinux_avc selinux_avc;
> +static atomic64_t trace_seqno;
>
>  void selinux_avc_init(struct selinux_avc **avc)
>  {
> @@ -731,9 +732,31 @@ static void avc_audit_post_callback(struct audit_buffer *ab, void *a)
>         tclass = secclass_map[sad->tclass-1].name;
>         audit_log_format(ab, " tclass=%s", tclass);
>
> -       if (sad->denied)
> +       if (sad->denied) {
>                 audit_log_format(ab, " permissive=%u", sad->result ? 0 : 1);
> -
> +               if (trace_selinux_denied_enabled()) {
> +                       int i, perm;
> +                       const char **perms;
> +                       u32 denied = sad->denied;
> +                       int64_t seq;
> +
> +                       seq = atomic_long_inc_return(&trace_seqno);
> +                       perms = secclass_map[sad->tclass-1].perms;
> +                       i = 0;
> +                       perm = 1;
> +                       while (i < (sizeof(denied) * 8)) {
> +                               if ((perm & denied & sad->requested) && perms[i]) {
> +                                       trace_selinux_denied(sad, scontext, tcontext,
> +                                                            tclass, perms[i], seq);
> +                                       denied &= ~perm;
> +                                       if (!denied)
> +                                               break;
> +                               }
> +                               i++;
> +                               perm <<= 1;
> +                       }
> +               }
> +       }
>         trace_selinux_audited(sad, scontext, tcontext, tclass);
>         kfree(tcontext);
>         kfree(scontext);
> --
> 2.17.1

-- 
paul moore
www.paul-moore.com

Powered by blists - more mailing lists