On Nov 15, 2016 09:30, "Nick Kralevich" <nnk@xxxxxxxxxx> wrote:
>
> Perhaps merge Stephen's change now, and address any changes in a
> followup? I'd like to get it merged into the selinux tree so I can
> merge the changes into the Android tree.
That's fine with me too.
>
> -- Nick
>
> On Tue, Nov 15, 2016 at 9:10 AM, William Roberts
> <bill.c.roberts@xxxxxxxxx> wrote:
> > For bit setting in constant time, one could always clear the bit(s) and or
> > in what you want. I think that logic might be applicable here. I could take
> > a stab at looking at it today, if no one has anything better by tomorrow
> > well just merge yours as is. Does that sound reasonable?
> >
> >
> > On Nov 15, 2016 06:18, "Stephen Smalley" <sds@xxxxxxxxxxxxx> wrote:
> >>
> >> On 11/14/2016 02:41 PM, Roberts, William C wrote:
> >> >
> >> >
> >> >> -----Original Message-----
> >> >> From: Selinux [mailto:selinux-bounces@xxxxxxxxxxxxx] On Behalf Of
> >> >> Roberts,
> >> >> William C
> >> >> Sent: Monday, November 14, 2016 10:44 AM
> >> >> To: Stephen Smalley <sds@xxxxxxxxxxxxx>; selinux@xxxxxxxxxxxxx
> >> >> Subject: RE: [PATCH v2] libsepol: fix checkpolicy dontaudit compiler
> >> >> bug
> >> >>
> >> >>
> >> >>
> >> >>> -----Original Message-----
> >> >>> From: Selinux [mailto:selinux-bounces@xxxxxxxxxxxxx] On Behalf Of
> >> >>> Stephen Smalley
> >> >>> Sent: Monday, November 14, 2016 9:48 AM
> >> >>> To: selinux@xxxxxxxxxxxxx
> >> >>> Cc: Stephen Smalley <sds@xxxxxxxxxxxxx>
> >> >>> Subject: [PATCH v2] libsepol: fix checkpolicy dontaudit compiler bug
> >> >>>
> >> >>> The combining logic for dontaudit rules was wrong, causing a dontaudit
> >> >>> A B:C *; rule to be clobbered by a dontaudit A B:C p; rule.
> >> >>>
> >> >>> Reported-by: Nick Kralevich <nnk@xxxxxxxxxx>
> >> >>> Signed-off-by: Stephen Smalley <sds@xxxxxxxxxxxxx>
> >> >>> ---
> >> >>> libsepol/src/expand.c | 16 ++++++++++++----
> >> >>> 1 file changed, 12 insertions(+), 4 deletions(-)
> >> >>>
> >> >>> diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c index
> >> >>> 004a029..d7adbf8
> >> >>> 100644
> >> >>> --- a/libsepol/src/expand.c
> >> >>> +++ b/libsepol/src/expand.c
> >> >>> @@ -1604,7 +1604,8 @@ static int expand_range_trans(expand_state_t *
> >> >>> state, static avtab_ptr_t find_avtab_node(sepol_handle_t * handle,
> >> >>> avtab_t * avtab, avtab_key_t * key,
> >> >>> cond_av_list_t ** cond,
> >> >>> - av_extended_perms_t *xperms)
> >> >>> + av_extended_perms_t *xperms,
> >> >>> + char *alloced)
> >> >>> {
> >> >>> avtab_ptr_t node;
> >> >>> avtab_datum_t avdatum;
> >> >>> @@ -1658,6 +1659,11 @@ static avtab_ptr_t
> >> >>> find_avtab_node(sepol_handle_t * handle,
> >> >>> nl->next = *cond;
> >> >>> *cond = nl;
> >> >>> }
> >> >>> + if (alloced)
> >> >>> + *alloced = 1;
> >> >>> + } else {
> >> >>> + if (alloced)
> >> >>> + *alloced = 0;
> >> >>> }
> >> >>>
> >> >>> return node;
> >> >>> @@ -1750,7 +1756,7 @@ static int expand_terule_helper(sepol_handle_t *
> >> >>> handle,
> >> >>> return EXPAND_RULE_CONFLICT;
> >> >>> }
> >> >>>
> >> >>> - node = find_avtab_node(handle, avtab, &avkey, cond, NULL);
> >> >>> + node = find_avtab_node(handle, avtab, &avkey, cond, NULL,
> >> >>> NULL);
> >> >>> if (!node)
> >> >>> return -1;
> >> >>> if (enabled) {
> >> >>> @@ -1790,6 +1796,7 @@ static int expand_avrule_helper(sepol_handle_t *
> >> >>> handle,
> >> >>> class_perm_node_t *cur;
> >> >>> uint32_t spec = 0;
> >> >>> unsigned int i;
> >> >>> + char alloced;
> >> >>>
> >> >>> if (specified & AVRULE_ALLOWED) {
> >> >>> spec = AVTAB_ALLOWED;
> >> >>> @@ -1824,7 +1831,8 @@ static int expand_avrule_helper(sepol_handle_t *
> >> >>> handle,
> >> >>> avkey.target_class = cur->tclass;
> >> >>> avkey.specified = spec;
> >> >>>
> >> >>> - node = find_avtab_node(handle, avtab, &avkey, cond,
> >> >>> extended_perms);
> >> >>> + node = find_avtab_node(handle, avtab, &avkey, cond,
> >> >>> + extended_perms, &alloced);
> >> >>> if (!node)
> >> >>> return EXPAND_RULE_ERROR;
> >> >>> if (enabled) {
> >> >>> @@ -1850,7 +1858,7 @@ static int expand_avrule_helper(sepol_handle_t *
> >> >>> handle,
> >> >>> */
> >> >>> avdatump->data &= cur->data;
> >> >>> } else if (specified & AVRULE_DONTAUDIT) {
> >> >>> - if (avdatump->data)
> >> >>> + if (!alloced)
> >> >>> avdatump->data &= ~cur->data;
> >> >>> else
> >> >>> avdatump->data = "">
> >> >>
> >> >> This seems awkward to me. If the insertion created a new empty node why
> >> >> wouldn't !avdump->data be true (note the addition of the not operator)?
> >> >
> >> > I misstated that a bit, but the !avdump->data was the else case. I am
> >> > really
> >> > saying why didn't this work before? In my mind, we don't care if its
> >> > allocated
> >> > we care if it's set or not.
> >>
> >> The old logic wrongly assumed that !avdatump->data would only be true if
> >> this was the first dontaudit rule for the (source type, target type,
> >> target class) and the node had just been allocated by find_avtab_node()
> >> with a zero avdatump->data value.
> >> However, if you have a dontaudit A B:C *; rule, then the set complement
> >> of it will be 0, so we can have !avdatump->data be true in that case
> >> too. Thus, if we end up processing:
> >> dontaudit A B:C *;
> >> dontaudit A B:C { p1 p2 ... };
> >> we'll end up clobbering avdatump->data with ~{ p1 p2 ... }.
> >>
> >> The marlin policy contains:
> >> dontaudit su self:capability *;
> >> dontaudit domain self:capability sys_module;
> >> and self rules are expanded (the kernel has no notion of self), so we
> >> end up with:
> >> dontaudit su self:capability *;
> >> dontaudit su self:capability sys_module;
> >>
> >> We have never encountered this situation before because there are no
> >> dontaudit A B:C *; rules in refpolicy; that's a corner case that only
> >> shows up in Android's su policy, and only because it is a permissive
> >> domain with no explicit allow rules (other than those picked up via
> >> macros that set up attributes or transitions).
> >>
> >> The fix was to replace the avdatump->data test with an explicit
> >> indication that the node was freshly allocated i.e. this is the first
> >> such rule. I agree that it could be clearer, but I was going for the
> >> simplest, least invasive fix for now, both due to limited time and to
> >> ease back-porting.
> >>
> >> >>
> >> >> Or perhaps a mechanism to actual set the data on allocation, this way
> >> >> the logic is
> >> >> Just &=.
> >>
> >> _______________________________________________
> >> Selinux mailing list
> >> Selinux@xxxxxxxxxxxxx
> >> To unsubscribe, send email to Selinux-leave@xxxxxxxxxxxxx.
> >> To get help, send an email containing "help" to
> >> Selinux-request@xxxxxxxxxxxxx.
> >
> >
> > _______________________________________________
> > Selinux mailing list
> > Selinux@xxxxxxxxxxxxx
> > To unsubscribe, send email to Selinux-leave@xxxxxxxxxxxxx.
> > To get help, send an email containing "help" to
> > Selinux-request@xxxxxxxxxxxxx.
>
>
>
> --
> Nick Kralevich | Android Security | nnk@xxxxxxxxxx | 650.214.4037
_______________________________________________ Selinux mailing list Selinux@xxxxxxxxxxxxx To unsubscribe, send email to Selinux-leave@xxxxxxxxxxxxx. To get help, send an email containing "help" to Selinux-request@xxxxxxxxxxxxx.
