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@tycho.nsa.gov ] 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@tycho.nsa.gov ] 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.
