Re: [PATCH 2/2] selinux: add attributes to avc tracepoint

[Stephen Smalley]

On Thu, Aug 6, 2020 at 10:51 AM peter enderborg
<peter.enderborg@xxxxxxxx> wrote:
>
> On 8/6/20 3:49 PM, Stephen Smalley wrote:
> > On Thu, Aug 6, 2020 at 9:45 AM Stephen Smalley
> > <stephen.smalley.work@xxxxxxxxx> wrote:
> >> On 8/6/20 8:32 AM, Stephen Smalley wrote:
> >>
> >>> On 8/6/20 8:24 AM, peter enderborg wrote:
> >>>
> >>>> On 8/6/20 2:11 PM, Stephen Smalley wrote:
> >>>>> On 8/6/20 4:03 AM, Thiébaud Weksteen wrote:
> >>>>>
> >>>>>> From: Peter Enderborg <peter.enderborg@xxxxxxxx>
> >>>>>>
> >>>>>> Add further attributes to filter the trace events from AVC.
> >>>>> Please include sample usage and output in the description.
> >>>>>
> >>>>>
> >>>> Im not sure where you want it to be.
> >>>>
> >>>> In the commit message or in a Documentation/trace/events-avc.rst ?
> >>> I was just asking for it in the commit message / patch description.  I
> >>> don't know what is typical for Documentation/trace.
> >> For example, I just took the patches for a spin, running the
> >> selinux-testsuite under perf like so:
> >>
> >> sudo perf record -e avc:selinux_audited -g make test
> >>
> >> and then ran:
> >>
> >> sudo perf report -g
> >>
> >> and a snippet of sample output included:
> >>
> >>       6.40%     6.40%  requested=0x800000 denied=0x800000
> >> audited=0x800000 result=-13 ssid=922 tsid=922
> >> scontext=unconfined_u:unconfined_r:test_binder_mgr_t:s0-s0:c0.c1023
> >> tcontext=unconfined_u:unconfined_r:test_binder_mgr_t:s0-s0:c0.c1023
> >> tclass=capability
> > So then the question becomes how do you use the above information,
> > e.g. is that sufficient to correlate it to an actual avc: denied
> > message, how do you decode the requested/denied/audited fields (or
> > should the code do that for you and just report the string name(s) of
> > the permission(s), do you need all three of those fields separately,
> > is it useful to log the ssid/tsid at all given that you have the
> > contexts and sids are dynamically assigned, etc.
> >
> >>              |
> >>              ---0x495641000028933d
> >>                 __libc_start_main
> >>                 |
> >>                 |--4.60%--__GI___ioctl
> >>                 |          entry_SYSCALL_64
> >>                 |          do_syscall_64
> >>                 |          __x64_sys_ioctl
> >>                 |          ksys_ioctl
> >>                 |          binder_ioctl
> >>                 |          binder_set_nice
> >>                 |          can_nice
> >>                 |          capable
> >>                 |          security_capable
> >>                 |          cred_has_capability.isra.0
> >>                 |          slow_avc_audit
> >>                 |          common_lsm_audit
> >>                 |          avc_audit_post_callback
> >>                 |          avc_audit_post_callback
>
> The real cool thing happen when you enable "user-stack-trace" too.
>
>            <...>-4820  [007] .... 85878.897553: selinux_audited: requested=0x4000000 denied=0x4000000 audited=0x4000000 result=-13 ssid=341 tsid=61 scontext=system_u:system_r:ntpd_t:s0 tcontext=system_u:object_r:bin_t:s0 tclass=file
>            <...>-4820  [007] .... 85878.897572: <user stack trace>
>  =>  <00007f07d99bb45b>
>  =>  <0000555ecd89ca57>
>
> The fields are useful for filter what you what to see and what you can ignore.  Having the ssid and text was from the part where it is called.
> The numeric can be used for two things. When you dont have any context. Same same reason as in post_callback. We need to be static
> in format so it need  be there if it ever can happen. And it is also useful for faster filtering.
>
> You can do "ssid!=42 && ssid!=43 && tsid==666".  From my view it would be good to have all fields there. But they need to right typed to be able
> to use the filter mechanism. There must me some trade-off too where the argument filtering get bigger than the processing, but I think we can
> add a lot more before we reach that threshold.

I don't think the SIDs are useful because they are dynamically
assigned (aside from the initial SIDs for bootstrapping before policy
load) and are not exported to userspace (userspace has no way to look
them up).  It is probably a mistake that we even fall back to logging
them in the existing code and may just be a legacy of when SIDs were
exported to userspace (ancient history, before mainline merge of
SELinux).

In any event, if you were to include the sample usage and output I
provided as part of the commit message / patch description (or replace
with your own example), that would be helpful I think.  Even better
would be to also provide some hint as to how people are expected to
decode the requested/denied/audited fields (I know how to do that but
not everyone will, and ideally one would have a script or something
for doing so).