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

[peter enderborg]

On 8/31/20 4:16 PM, Paul Moore wrote:
> On Thu, Aug 27, 2020 at 10:04 AM peter enderborg
> <peter.enderborg@xxxxxxxx> wrote:
>> On 8/27/20 3:30 PM, Paul Moore wrote:
>>> On Wed, Aug 26, 2020 at 11:06 AM peter enderborg
>>> <peter.enderborg@xxxxxxxx> wrote:
>>>> On 8/26/20 4:45 PM, Paul Moore wrote:
>>>>> On Wed, Aug 26, 2020 at 10:34 AM peter enderborg
>>>>> <peter.enderborg@xxxxxxxx> wrote:
>>>>>> On 8/26/20 3:42 PM, Paul Moore wrote:
>>>>>>> On Mon, Aug 24, 2020 at 9:23 AM Peter Enderborg
>>>>>>> <peter.enderborg@xxxxxxxx> 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@xxxxxxxx>
>>>>>>>> ---
>>>>>>>>  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?

The information is of course as essential as for audit messages.
I dont see much of the problem with having as the first suggestion with
a list. It works fine for trace_pipe. It is not failing due to that we can not
filter with that. It is cause in other tools in user-space
that needs a plugin to parse it. It need static
mapping for something that is not really static. Not in runtime, and it will
change over time.

A other idea based on the first one is to have multiple pairs like

class=file permission=read permission=write permission=open

but then you need to filter on numeric values that are not static and
I don't know if library can make anything useful from that.

I don't see why it should be a issue with a event for each denial, all
of the trace system is opt-in. It is usually only a NOP instruction,
but here  it is a conditional branch and it is in the end of long
process where of a very tiny percent a ending up as denial.

>From my view it is more annoying that we do similar things for
audit_log but not equal enough to be shared.


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