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.
