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. -- 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 = ~cur->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.