On 11/15/2016 12:30 PM, Nick Kralevich 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. Yes, it is already merged. > > -- 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. > > > _______________________________________________ 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.
