Re: [RFC PATCH] selinux: Add denied trace with permssion filter

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

On Thu, Aug 27, 2020 at 10:04 AM peter enderborg
<peter.enderborg@sony.com> wrote:
>
> On 8/27/20 3:30 PM, Paul Moore wrote:
> > On Wed, Aug 26, 2020 at 11:06 AM peter enderborg
> > <peter.enderborg@sony.com> wrote:
> >> On 8/26/20 4:45 PM, Paul Moore wrote:
> >>> On Wed, Aug 26, 2020 at 10:34 AM peter enderborg
> >>> <peter.enderborg@sony.com> wrote:
> >>>> On 8/26/20 3:42 PM, Paul Moore wrote:
> >>>>> On Mon, Aug 24, 2020 at 9:23 AM Peter Enderborg
> >>>>> <peter.enderborg@sony.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@sony.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
> >>>> We tried that but that was problematic too.
> >>> My apologies if I was on that thread, but can you remind me why it was
> >>> a problem?  Why can't we use a single tracepoint to capture the AVC
> >>> information?
> >> The problem is parsing the event.
> >>
> >> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2020_8_18_842&d=DwIBaQ&c=fP4tf--1dS0biCFlB0saz0I0kjO5v7-GLPtvShAo4cc&r=oO5HuGEGxznA2F3djiiYxmxxWQonw0h6Sks-BEoB4ys&m=qmi2ROWsLC_0mLLhHkpb71j1YoicydLh-7l4cOsLYcY&s=iS3eZr3TFrN5I7BbnvPFYOKd6DfW1FHTFcwI7joS_fk&e=
> >>
> >> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2020_8_21_526&d=DwIBaQ&c=fP4tf--1dS0biCFlB0saz0I0kjO5v7-GLPtvShAo4cc&r=oO5HuGEGxznA2F3djiiYxmxxWQonw0h6Sks-BEoB4ys&m=qmi2ROWsLC_0mLLhHkpb71j1YoicydLh-7l4cOsLYcY&s=9OsLN0Y5mUWxEAAqUE6K4PS57Pn1XyZz7GXak6uc_Ls&e=
> >>
> >> and the "single list" version
> >>
> >> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2020_8_17_1346&d=DwIBaQ&c=fP4tf--1dS0biCFlB0saz0I0kjO5v7-GLPtvShAo4cc&r=oO5HuGEGxznA2F3djiiYxmxxWQonw0h6Sks-BEoB4ys&m=qmi2ROWsLC_0mLLhHkpb71j1YoicydLh-7l4cOsLYcY&s=tWSY2ry2IT6RcT5BIUwMuqBL_yPObDE1VljbLqI1zrA&e=
> >>
> >> With this patch we follow standard message format so no plugin should be needed.
> > I'm evidently missing something very fundamental (likely), and/or I'm
> > just not communicating very clearly (also likely), because the above
> > links don't appear to make any sense with respect to my question.
> >
> > Let me try a reset ... Why can't we basically take the
> > "selinux_denied" TRACE_EVENT implementation in your patch and use it
> > to replace the "selinux_audited" TRACE_EVENT in the selinux/next tree
> > (of course with the necessary changes to the AVC callback code)?
> >
> > If the "selinux_denied" implementation is valid from a tracing point
> > of view, why can we not do this?  Of course if the "selinux_denied"
> > implementation is not a valid TRACE_EVENT then I'm not sure why this
> > was suggested for SELinux :)
>
> Im happly fine with replacing the selinux_audited with selinux_denied.  However it is the case where there are more than one denial at the same time. Im not sure how and when it might happen.

One thing I wondered about was why not build up a single string with
all of the permissions instead of generating multiple trace events?
In the previous discussion it was implied that this was due to
limitations in the tracing subsystem's filtering, and based on the
discussion thus far I'm guessing there is little desire for this
information if it can't be filtered on?

If that's the case then I think we are stuck with the tracing code
that currently lives in selinux/next, as I currently have little
desire to add more than one tracepoint in the SELinux permission
checking codepath.

> When that happen we got more than one event. I have no problems with that, but im not sure if the debug tools and perf can make sense of that.
>
> A other feature with the selinux_audited event it might be inserted on other places in the code too.  A denial is sort of final.

-- 
paul moore
www.paul-moore.com