av_decision on audit callback

av_decision on audit callback

[Roberts, William C]

[-- Attachment #1: Type: text/plain, Size: 1058 bytes --]

I would like to be able to gather the result of permissive mode per domain from a check_access() call for the userspace object managers on Android.

>From what I can tell check_access() calls avc_has_perm with a NULL 5th argument. That argument is for the struct avc_entry_ref.

That structure has a pointer to an opaque type, avc_entry. Which contains struct av_decision.

Which contains flags that have a permissive flag:

struct av_decision {
        access_vector_t allowed;
        access_vector_t decided;
        access_vector_t auditallow;
        access_vector_t auditdeny;
        unsigned int seqno;
        unsigned int flags;
};

/* Definitions of av_decision.flags */
#define SELINUX_AVD_FLAGS_PERMISSIVE    0x0001

It looks like if check_access just passes this structure and then avc_has_perm() when it calls avc_audit, it could supply the av_decision structure to the avc_suppl_audit() call. We could then have an audit2 callback that takes this parameter.

Is this mostly right, seem sane? Better way to do this?



[-- Attachment #2: Type: text/html, Size: 5858 bytes --]

Re: av_decision on audit callback

[Stephen Smalley]

On 10/02/2015 02:48 PM, Roberts, William C wrote:
> I would like to be able to gather the result of permissive mode per
> domain from a check_access() call for the userspace object managers on
> Android.
>
>  From what I can tell check_access() calls avc_has_perm with a NULL 5th
> argument. That argument is for the struct avc_entry_ref.
>
> That structure has a pointer to an opaque type, avc_entry. Which
> contains struct av_decision.
>
> Which contains flags that have a permissive flag:
>
> struct av_decision {
>
>          access_vector_t allowed;
>
>          access_vector_t decided;
>
>          access_vector_t auditallow;
>
>          access_vector_t auditdeny;
>
>          unsigned int seqno;
>
>          unsigned int flags;
>
> };
>
> /* Definitions of av_decision.flags */
>
> #define SELINUX_AVD_FLAGS_PERMISSIVE    0x0001
>
> It looks like if check_access just passes this structure and then
> avc_has_perm() when it calls avc_audit, it could supply the av_decision
> structure to the avc_suppl_audit() call. We could then have an audit2
> callback that takes this parameter.
>
> Is this mostly right, seem sane? Better way to do this?

It doesn't need to be exposed at that level; the libselinux avc_audit() 
routine can log it, similar to what is done in the kernel.  It already 
has the av_decision structure available to it.

Re: av_decision on audit callback

[Stephen Smalley]

On 10/02/2015 02:54 PM, Stephen Smalley wrote:
> On 10/02/2015 02:48 PM, Roberts, William C wrote:
>> I would like to be able to gather the result of permissive mode per
>> domain from a check_access() call for the userspace object managers on
>> Android.
>>
>>  From what I can tell check_access() calls avc_has_perm with a NULL 5th
>> argument. That argument is for the struct avc_entry_ref.
>>
>> That structure has a pointer to an opaque type, avc_entry. Which
>> contains struct av_decision.
>>
>> Which contains flags that have a permissive flag:
>>
>> struct av_decision {
>>
>>          access_vector_t allowed;
>>
>>          access_vector_t decided;
>>
>>          access_vector_t auditallow;
>>
>>          access_vector_t auditdeny;
>>
>>          unsigned int seqno;
>>
>>          unsigned int flags;
>>
>> };
>>
>> /* Definitions of av_decision.flags */
>>
>> #define SELINUX_AVD_FLAGS_PERMISSIVE    0x0001
>>
>> It looks like if check_access just passes this structure and then
>> avc_has_perm() when it calls avc_audit, it could supply the av_decision
>> structure to the avc_suppl_audit() call. We could then have an audit2
>> callback that takes this parameter.
>>
>> Is this mostly right, seem sane? Better way to do this?
>
> It doesn't need to be exposed at that level; the libselinux avc_audit()
> routine can log it, similar to what is done in the kernel.  It already
> has the av_decision structure available to it.

To clarify, anything directly known to the AVC, like the permissive 
flag, can be directly logged by it.  The audit callback is for logging 
auxiliary audit information not known to the AVC (the pid of the client 
process being a good example).

RE: av_decision on audit callback

[Roberts, William C]

> -----Original Message-----
> From: Stephen Smalley [mailto:sds@tycho.nsa.gov]
> Sent: Friday, October 2, 2015 12:12 PM
> To: Roberts, William C; seandroid-list@tycho.nsa.gov; selinux@tycho.nsa.gov
> Subject: Re: av_decision on audit callback
> 
> On 10/02/2015 02:54 PM, Stephen Smalley wrote:
> > On 10/02/2015 02:48 PM, Roberts, William C wrote:
> >> I would like to be able to gather the result of permissive mode per
> >> domain from a check_access() call for the userspace object managers
> >> on Android.
> >>
> >>  From what I can tell check_access() calls avc_has_perm with a NULL
> >> 5th argument. That argument is for the struct avc_entry_ref.
> >>
> >> That structure has a pointer to an opaque type, avc_entry. Which
> >> contains struct av_decision.
> >>
> >> Which contains flags that have a permissive flag:
> >>
> >> struct av_decision {
> >>
> >>          access_vector_t allowed;
> >>
> >>          access_vector_t decided;
> >>
> >>          access_vector_t auditallow;
> >>
> >>          access_vector_t auditdeny;
> >>
> >>          unsigned int seqno;
> >>
> >>          unsigned int flags;
> >>
> >> };
> >>
> >> /* Definitions of av_decision.flags */
> >>
> >> #define SELINUX_AVD_FLAGS_PERMISSIVE    0x0001
> >>
> >> It looks like if check_access just passes this structure and then
> >> avc_has_perm() when it calls avc_audit, it could supply the
> >> av_decision structure to the avc_suppl_audit() call. We could then
> >> have an audit2 callback that takes this parameter.
> >>
> >> Is this mostly right, seem sane? Better way to do this?
> >
> > It doesn't need to be exposed at that level; the libselinux
> > avc_audit() routine can log it, similar to what is done in the kernel.
> > It already has the av_decision structure available to it.
> 
> To clarify, anything directly known to the AVC, like the permissive flag, can be
> directly logged by it.  The audit callback is for logging auxiliary audit information
> not known to the AVC (the pid of the client process being a good example).

I was wondering if we could just dump permissive=0|1 from the AVC logging routine, but that
would affect everyone.  I guess then you would be ok with that? Does order matter with
the fields wrt parsing? I don't want to break any desktop tooling I am aware of, would we upstream
this change as well?

Re: av_decision on audit callback

[Stephen Smalley]

On 10/02/2015 04:07 PM, Roberts, William C wrote:
>
>
>> -----Original Message-----
>> From: Stephen Smalley [mailto:sds@tycho.nsa.gov]
>> Sent: Friday, October 2, 2015 12:12 PM
>> To: Roberts, William C; seandroid-list@tycho.nsa.gov; selinux@tycho.nsa.gov
>> Subject: Re: av_decision on audit callback
>>
>> On 10/02/2015 02:54 PM, Stephen Smalley wrote:
>>> On 10/02/2015 02:48 PM, Roberts, William C wrote:
>>>> I would like to be able to gather the result of permissive mode per
>>>> domain from a check_access() call for the userspace object managers
>>>> on Android.
>>>>
>>>>   From what I can tell check_access() calls avc_has_perm with a NULL
>>>> 5th argument. That argument is for the struct avc_entry_ref.
>>>>
>>>> That structure has a pointer to an opaque type, avc_entry. Which
>>>> contains struct av_decision.
>>>>
>>>> Which contains flags that have a permissive flag:
>>>>
>>>> struct av_decision {
>>>>
>>>>           access_vector_t allowed;
>>>>
>>>>           access_vector_t decided;
>>>>
>>>>           access_vector_t auditallow;
>>>>
>>>>           access_vector_t auditdeny;
>>>>
>>>>           unsigned int seqno;
>>>>
>>>>           unsigned int flags;
>>>>
>>>> };
>>>>
>>>> /* Definitions of av_decision.flags */
>>>>
>>>> #define SELINUX_AVD_FLAGS_PERMISSIVE    0x0001
>>>>
>>>> It looks like if check_access just passes this structure and then
>>>> avc_has_perm() when it calls avc_audit, it could supply the
>>>> av_decision structure to the avc_suppl_audit() call. We could then
>>>> have an audit2 callback that takes this parameter.
>>>>
>>>> Is this mostly right, seem sane? Better way to do this?
>>>
>>> It doesn't need to be exposed at that level; the libselinux
>>> avc_audit() routine can log it, similar to what is done in the kernel.
>>> It already has the av_decision structure available to it.
>>
>> To clarify, anything directly known to the AVC, like the permissive flag, can be
>> directly logged by it.  The audit callback is for logging auxiliary audit information
>> not known to the AVC (the pid of the client process being a good example).
>
> I was wondering if we could just dump permissive=0|1 from the AVC logging routine, but that
> would affect everyone.  I guess then you would be ok with that? Does order matter with
> the fields wrt parsing? I don't want to break any desktop tooling I am aware of, would we upstream
> this change as well?

Just put it at the end (i.e. log_append() after the avc_dump_query() 
call), like we do in the kernel.  Shouldn't be a problem.  Technically 
order shouldn't matter but safer to just append it to the current fields.

Yes, we'd upstream it.

RE: av_decision on audit callback

[Roberts, William C]

> -----Original Message-----
> From: Stephen Smalley [mailto:sds@tycho.nsa.gov]
> Sent: Friday, October 2, 2015 1:13 PM
> To: Roberts, William C; seandroid-list@tycho.nsa.gov; selinux@tycho.nsa.gov
> Subject: Re: av_decision on audit callback
> 
> On 10/02/2015 04:07 PM, Roberts, William C wrote:
> >
> >
> >> -----Original Message-----
> >> From: Stephen Smalley [mailto:sds@tycho.nsa.gov]
> >> Sent: Friday, October 2, 2015 12:12 PM
> >> To: Roberts, William C; seandroid-list@tycho.nsa.gov;
> >> selinux@tycho.nsa.gov
> >> Subject: Re: av_decision on audit callback
> >>
> >> On 10/02/2015 02:54 PM, Stephen Smalley wrote:
> >>> On 10/02/2015 02:48 PM, Roberts, William C wrote:
> >>>> I would like to be able to gather the result of permissive mode per
> >>>> domain from a check_access() call for the userspace object managers
> >>>> on Android.
> >>>>
> >>>>   From what I can tell check_access() calls avc_has_perm with a
> >>>> NULL 5th argument. That argument is for the struct avc_entry_ref.
> >>>>
> >>>> That structure has a pointer to an opaque type, avc_entry. Which
> >>>> contains struct av_decision.
> >>>>
> >>>> Which contains flags that have a permissive flag:
> >>>>
> >>>> struct av_decision {
> >>>>
> >>>>           access_vector_t allowed;
> >>>>
> >>>>           access_vector_t decided;
> >>>>
> >>>>           access_vector_t auditallow;
> >>>>
> >>>>           access_vector_t auditdeny;
> >>>>
> >>>>           unsigned int seqno;
> >>>>
> >>>>           unsigned int flags;
> >>>>
> >>>> };
> >>>>
> >>>> /* Definitions of av_decision.flags */
> >>>>
> >>>> #define SELINUX_AVD_FLAGS_PERMISSIVE    0x0001
> >>>>
> >>>> It looks like if check_access just passes this structure and then
> >>>> avc_has_perm() when it calls avc_audit, it could supply the
> >>>> av_decision structure to the avc_suppl_audit() call. We could then
> >>>> have an audit2 callback that takes this parameter.
> >>>>
> >>>> Is this mostly right, seem sane? Better way to do this?
> >>>
> >>> It doesn't need to be exposed at that level; the libselinux
> >>> avc_audit() routine can log it, similar to what is done in the kernel.
> >>> It already has the av_decision structure available to it.
> >>
> >> To clarify, anything directly known to the AVC, like the permissive
> >> flag, can be directly logged by it.  The audit callback is for
> >> logging auxiliary audit information not known to the AVC (the pid of the client
> process being a good example).
> >
> > I was wondering if we could just dump permissive=0|1 from the AVC
> > logging routine, but that would affect everyone.  I guess then you
> > would be ok with that? Does order matter with the fields wrt parsing?
> > I don't want to break any desktop tooling I am aware of, would we upstream
> this change as well?
> 
> Just put it at the end (i.e. log_append() after the avc_dump_query() call), like we
> do in the kernel.  Shouldn't be a problem.  Technically order shouldn't matter but
> safer to just append it to the current fields.
> 
> Yes, we'd upstream it.
> 
Done. Ill post back with a patch on android-review. And once merged there I can send one to the
Mailing list or you can cherry-pick. Are you ok with that?

Re: av_decision on audit callback

[Stephen Smalley]

On 10/02/2015 04:22 PM, Roberts, William C wrote:
>
>
>> -----Original Message-----
>> From: Stephen Smalley [mailto:sds@tycho.nsa.gov]
>> Sent: Friday, October 2, 2015 1:13 PM
>> To: Roberts, William C; seandroid-list@tycho.nsa.gov; selinux@tycho.nsa.gov
>> Subject: Re: av_decision on audit callback
>>
>> On 10/02/2015 04:07 PM, Roberts, William C wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Stephen Smalley [mailto:sds@tycho.nsa.gov]
>>>> Sent: Friday, October 2, 2015 12:12 PM
>>>> To: Roberts, William C; seandroid-list@tycho.nsa.gov;
>>>> selinux@tycho.nsa.gov
>>>> Subject: Re: av_decision on audit callback
>>>>
>>>> On 10/02/2015 02:54 PM, Stephen Smalley wrote:
>>>>> On 10/02/2015 02:48 PM, Roberts, William C wrote:
>>>>>> I would like to be able to gather the result of permissive mode per
>>>>>> domain from a check_access() call for the userspace object managers
>>>>>> on Android.
>>>>>>
>>>>>>    From what I can tell check_access() calls avc_has_perm with a
>>>>>> NULL 5th argument. That argument is for the struct avc_entry_ref.
>>>>>>
>>>>>> That structure has a pointer to an opaque type, avc_entry. Which
>>>>>> contains struct av_decision.
>>>>>>
>>>>>> Which contains flags that have a permissive flag:
>>>>>>
>>>>>> struct av_decision {
>>>>>>
>>>>>>            access_vector_t allowed;
>>>>>>
>>>>>>            access_vector_t decided;
>>>>>>
>>>>>>            access_vector_t auditallow;
>>>>>>
>>>>>>            access_vector_t auditdeny;
>>>>>>
>>>>>>            unsigned int seqno;
>>>>>>
>>>>>>            unsigned int flags;
>>>>>>
>>>>>> };
>>>>>>
>>>>>> /* Definitions of av_decision.flags */
>>>>>>
>>>>>> #define SELINUX_AVD_FLAGS_PERMISSIVE    0x0001
>>>>>>
>>>>>> It looks like if check_access just passes this structure and then
>>>>>> avc_has_perm() when it calls avc_audit, it could supply the
>>>>>> av_decision structure to the avc_suppl_audit() call. We could then
>>>>>> have an audit2 callback that takes this parameter.
>>>>>>
>>>>>> Is this mostly right, seem sane? Better way to do this?
>>>>>
>>>>> It doesn't need to be exposed at that level; the libselinux
>>>>> avc_audit() routine can log it, similar to what is done in the kernel.
>>>>> It already has the av_decision structure available to it.
>>>>
>>>> To clarify, anything directly known to the AVC, like the permissive
>>>> flag, can be directly logged by it.  The audit callback is for
>>>> logging auxiliary audit information not known to the AVC (the pid of the client
>> process being a good example).
>>>
>>> I was wondering if we could just dump permissive=0|1 from the AVC
>>> logging routine, but that would affect everyone.  I guess then you
>>> would be ok with that? Does order matter with the fields wrt parsing?
>>> I don't want to break any desktop tooling I am aware of, would we upstream
>> this change as well?
>>
>> Just put it at the end (i.e. log_append() after the avc_dump_query() call), like we
>> do in the kernel.  Shouldn't be a problem.  Technically order shouldn't matter but
>> safer to just append it to the current fields.
>>
>> Yes, we'd upstream it.
>>
> Done. Ill post back with a patch on android-review. And once merged there I can send one to the
> Mailing list or you can cherry-pick. Are you ok with that?

Yes.  As with the kernel, only add permissive= to avc: denied messages, 
not avc: granted ones.  See the kernel logic (but note that the code and 
data structures aren't the same as the libselinux AVC anymore).

RE: av_decision on audit callback

[Roberts, William C]

> -----Original Message-----
> From: Stephen Smalley [mailto:sds@tycho.nsa.gov]
> Sent: Friday, October 2, 2015 1:26 PM
> To: Roberts, William C; seandroid-list@tycho.nsa.gov; selinux@tycho.nsa.gov
> Subject: Re: av_decision on audit callback
> 
> On 10/02/2015 04:22 PM, Roberts, William C wrote:
> >
> >
> >> -----Original Message-----
> >> From: Stephen Smalley [mailto:sds@tycho.nsa.gov]
> >> Sent: Friday, October 2, 2015 1:13 PM
> >> To: Roberts, William C; seandroid-list@tycho.nsa.gov;
> >> selinux@tycho.nsa.gov
> >> Subject: Re: av_decision on audit callback
> >>
> >> On 10/02/2015 04:07 PM, Roberts, William C wrote:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Stephen Smalley [mailto:sds@tycho.nsa.gov]
> >>>> Sent: Friday, October 2, 2015 12:12 PM
> >>>> To: Roberts, William C; seandroid-list@tycho.nsa.gov;
> >>>> selinux@tycho.nsa.gov
> >>>> Subject: Re: av_decision on audit callback
> >>>>
> >>>> On 10/02/2015 02:54 PM, Stephen Smalley wrote:
> >>>>> On 10/02/2015 02:48 PM, Roberts, William C wrote:
> >>>>>> I would like to be able to gather the result of permissive mode
> >>>>>> per domain from a check_access() call for the userspace object
> >>>>>> managers on Android.
> >>>>>>
> >>>>>>    From what I can tell check_access() calls avc_has_perm with a
> >>>>>> NULL 5th argument. That argument is for the struct avc_entry_ref.
> >>>>>>
> >>>>>> That structure has a pointer to an opaque type, avc_entry. Which
> >>>>>> contains struct av_decision.
> >>>>>>
> >>>>>> Which contains flags that have a permissive flag:
> >>>>>>
> >>>>>> struct av_decision {
> >>>>>>
> >>>>>>            access_vector_t allowed;
> >>>>>>
> >>>>>>            access_vector_t decided;
> >>>>>>
> >>>>>>            access_vector_t auditallow;
> >>>>>>
> >>>>>>            access_vector_t auditdeny;
> >>>>>>
> >>>>>>            unsigned int seqno;
> >>>>>>
> >>>>>>            unsigned int flags;
> >>>>>>
> >>>>>> };
> >>>>>>
> >>>>>> /* Definitions of av_decision.flags */
> >>>>>>
> >>>>>> #define SELINUX_AVD_FLAGS_PERMISSIVE    0x0001
> >>>>>>
> >>>>>> It looks like if check_access just passes this structure and then
> >>>>>> avc_has_perm() when it calls avc_audit, it could supply the
> >>>>>> av_decision structure to the avc_suppl_audit() call. We could
> >>>>>> then have an audit2 callback that takes this parameter.
> >>>>>>
> >>>>>> Is this mostly right, seem sane? Better way to do this?
> >>>>>
> >>>>> It doesn't need to be exposed at that level; the libselinux
> >>>>> avc_audit() routine can log it, similar to what is done in the kernel.
> >>>>> It already has the av_decision structure available to it.
> >>>>
> >>>> To clarify, anything directly known to the AVC, like the permissive
> >>>> flag, can be directly logged by it.  The audit callback is for
> >>>> logging auxiliary audit information not known to the AVC (the pid
> >>>> of the client
> >> process being a good example).
> >>>
> >>> I was wondering if we could just dump permissive=0|1 from the AVC
> >>> logging routine, but that would affect everyone.  I guess then you
> >>> would be ok with that? Does order matter with the fields wrt parsing?
> >>> I don't want to break any desktop tooling I am aware of, would we
> >>> upstream
> >> this change as well?
> >>
> >> Just put it at the end (i.e. log_append() after the avc_dump_query()
> >> call), like we do in the kernel.  Shouldn't be a problem.
> >> Technically order shouldn't matter but safer to just append it to the current
> fields.
> >>
> >> Yes, we'd upstream it.
> >>
> > Done. Ill post back with a patch on android-review. And once merged
> > there I can send one to the Mailing list or you can cherry-pick. Are you ok with
> that?
> 
> Yes.  As with the kernel, only add permissive= to avc: denied messages, not avc:
> granted ones.  See the kernel logic (but note that the code and data structures
> aren't the same as the libselinux AVC anymore).

Ok. Yeah it wouldn't make sense for granted. Its good thing you said something.