Re: [PATCH] libsepol: Skip duplicate filename_trans rules in state->out policy.

[Eric Paris <eparis@parisplace.org>]

Why didn't it fail in the kernel on policy load?

On Tue, Apr 15, 2014 at 3:07 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On 04/15/2014 02:43 PM, Eric Paris wrote:
>> On Tue, 2014-04-15 at 14:34 -0400, Stephen Smalley wrote:
>>> On 04/15/2014 02:27 PM, Eric Paris wrote:
>>>> On Tue, 2014-04-15 at 14:21 -0400, Stephen Smalley wrote:
>>>>> On 04/15/2014 12:40 PM, Stephen Smalley wrote:
>>>>>> On 04/15/2014 12:27 PM, Richard Haines wrote:
>>>>>>> The current detection of duplicate rules does not cover the state->out
>>>>>>> policy and therefore will duplicate filename transition rules if already
>>>>>>> present.
>>>>>>>
>>>>>>> Signed-off-by: Richard Haines <richard_c_haines@btinternet.com>
>>>>>>> ---
>>>>>>>  libsepol/src/expand.c | 14 ++++++++++++++
>>>>>>>  1 file changed, 14 insertions(+)
>>>>>>>
>>>>>>> diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c
>>>>>>> index acb6906..e908fdb 100644
>>>>>>> --- a/libsepol/src/expand.c
>>>>>>> +++ b/libsepol/src/expand.c
>>>>>>> @@ -1534,6 +1534,20 @@ static int expand_filename_trans(expand_state_t *state, filename_trans_rule_t *r
>>>>>>>                                  if (cur_trans)
>>>>>>>                                          continue;
>>>>>>>
>>>>>>> +                                /* Now check if duplicate rule in state->out policy */
>>>>>>> +                                cur_trans = state->out->filename_trans;
>>>>>>> +
>>>>>>> +                                while (cur_trans) {
>>>>>>> +                                        if (cur_trans->stype == (i + 1) &&
>>>>>>> +                                            cur_trans->ttype == (j + 1) &&
>>>>>>> +                                            cur_trans->tclass == cur_rule->tclass &&
>>>>>>> +                                            !strcmp(cur_trans->name, cur_rule->name))
>>>>>>> +                                                break;
>>>>>>> +                                        cur_trans = cur_trans->next;
>>>>>>> +                                }
>>>>>>> +                                if (cur_trans)
>>>>>>> +                                        continue;
>>>>>>> +
>>>>>>>                                  new_trans = malloc(sizeof(*new_trans));
>>>>>>>                                  if (!new_trans) {
>>>>>>>                                          ERR(state->handle, "Out of memory!");
>>>>>>
>>>>>> Isn't this effectively a revert of:
>>>>>>
>>>>>> commit a29f6820c52b60b9028298cde9962dd140bbf9ea
>>>>>> Author: Adam Tkac <atkac@redhat.com>
>>>>>> Date:   Fri May 25 17:55:08 2012 +0200
>>>>>>
>>>>>>     libsepol: filename_trans: use some better sorting to compare and merge
>>>>>>
>>>>>> The kernel switched to using a hashtab for filename_trans rules in
>>>>>> commit 2463c26d50adc282d19317013ba0ff473823ca47
>>>>>> Author: Eric Paris <eparis@redhat.com>
>>>>>> Date:   Thu Apr 28 15:11:21 2011 -0400
>>>>>>
>>>>>>     SELinux: put name based create rules in a hashtable
>>>>>>
>>>>>> Is there a reason we don't do this in libsepol too?
>>>>>
>>>>> So if I am reading a29f68 correctly, it is completely wrong and should
>>>>> just be reverted.  That will fix the duplicate filename transition rules
>>>>> if I am not mistaken.  Then separately, we can look at bringing over the
>>>>> switch to using a hashtab that was already done in the kernel and use
>>>>> that to speed up this checking?  Comments?
>>>>
>>>> I think that's a good idea.  The kernel hashtab and this 'fix' were done
>>>> about the same time.  I intended to bring the kernel hashtab over and
>>>> got distracted and then forgot about it...
>>>>
>>>> Shouldn't be a hard thing, and I believe should get us back to having
>>>> bitwise the same policy in /sys/fs/selinux/policy as we have on disk...
>>>
>>> Looks like we have some other areas of divergence, e.g. range_tr hashtab
>>> conversion, ebitmap optimizations, possibly others.  The latter would
>>> require a policy binary format change (new version) to make it fully
>>> identical.
>>>
>>> Not sure how well even the hashtab is doing at this point as Fedora has
>>>> 150,000 file name transition rules in its policy per sedispol output?
>>>  That seems a bit excessive.
>>
>> My understanding is that MANY of these are because of the expansion of
>> attributes in the source and the parent object.  If we could leave these
>> as attributes it would shrink things and incredible amount...
>
> Hmmm...Fedora policy doesn't build with that change reverted, since it
> was failing to check all filename transition rules for conflicts:
> # semodule -B
> libsepol.expand_filename_trans: Conflicting filename trans rules
> keyrings staff_gkeyringd_t gnome_home_t : dir otype1:data_home_t
> otype2:gkeyringd_gnome_home_t
> libsepol.expand_module: Error during expand
> libsemanage.semanage_expand_sandbox: Expand module failed
> semodule:  Failed!
>
> That's a bug in policy not in libsepol; it was just hidden by the bug in
> libsepol.