[SSSD] [PATCHES] SELinux processing related patches

Michal Židek mzidek at redhat.com
Fri Mar 8 20:10:24 UTC 2013


> On Mon, Mar 04, 2013 at 04:13:31PM +0100, Michal Židek wrote:
>> On 03/04/2013 02:53 PM, Pavel Březina wrote:
>> >On 03/01/2013 04:13 PM, Michal Židek wrote:
>> >>Patch 1: Right now, we have SELinux procesing split between the provider
>> >>and responder. This patch moves it to provider only. So no partial
>> >>results are sent to responder for further processing, which makes the
>> >>processing steps easier to follow. (Originally the idea was to move the
>> >>processing to the responder, but completely removing the procesing from
>> >>provider and let it just update db turned to be more invasive than
>> >>completely removing the procesing from responder.) If this patch is
>> >>accepted, SELinux policy needs to be modified to allow sssd_be to write
>> >>the selinux login files.
>> >>https://fedorahosted.org/sssd/ticket/1743
>> >>
>
> Even though we would have to tailor the SELinux policy, I'm not opposed
> to moving the logic to provider. Originally I proposed responder because
> I simply wanted all the processing to happen on one place.
>
> The whole intent of the refactor is to get rid of passing some
> information from provider by writing data to sysdb in provider and
> reading them in responder. If we get rid of that architectural bug, I'm
> fine with any solution.
>
> FWIW, the other alternative that we were discussing with Michal was to
> done the processing in provider and then send the resulting string to
> responder via DBus. But it seemed kind of pointless to send the result
> with DBus just to avoid moving the write of the file.
>
>> >>
>> >>Patch 2, 3: Reuse cached selinux maps. Patch 2 shows how it is done with
>> >>refresh interval fixed to 120 seconds. Patch 3 adds ipa option to
>> >>configure this interval (I didn't want to merge these patches for better
>> >>readability). Default value is 30 seconds.
>> >>https://fedorahosted.org/sssd/ticket/1744
>> >>
>> >>
>> >>Patches are attached.
>> >>
>> >>Thanks
>> >>Michal
>> >
>> >Hi,
>> >switch the order of the second and third patch. So the flow is: add new
>> >option, then use it.
>>
>> I don't think this is good idea. If the option is present, the logic
>> to implement it's effect should be present too. It is also easier to
>> test the patches one by one this way (first test the mechanism with
>> fixed time interval, then make it configurable and test if interval
>> is changing). For now I only changed the 120 seconds in the 2nd
>> patch to 5 seconds (so it is not that aggressive).
>> >
>> >Do we want to use it anyway? Shouldn't be SELinux maps always updated?
>> >
>>
>> Well, we can still make 0 seconds the default value (or 5 seconds,
>> which is used by IPA_HBAC_REFRESH). But selinux maps are not changed
>> frequently on ipa side, so having this option is IMO good thing. In
>> the new attached patches the default value is changed to 5.
>>
>> Michal
>
> Good start, but a nack, sorry.
>
> I'm seeing a segfaut and valgrind says it's becuase of a user-after-free
> bug:
> ==28566== Invalid read of size 4
> ==28566==    at 0x5264CFE: sysdb_attrs_get_el_ext (sysdb.c:313)
> ==28566==    by 0x5264EC6: sysdb_attrs_get_string (sysdb.c:356)
> ==28566==    by 0x5280A75: sysdb_store_selinux_entity (sysdb_selinux.c:102)
> ==28566==    by 0x528153D: sysdb_store_selinux_usermap (sysdb_selinux.c:181)
> ==28566==    by 0x122AABE0: ipa_save_user_maps (ipa_selinux_common.c:48)
> ==28566==    by 0x122A55E1: ipa_selinux_handler_done (ipa_selinux.c:260)
> ==28566==    by 0x122A9943: ipa_get_selinux_maps_done (ipa_selinux.c:1149)
> ==28566==    by 0x122AA8DB: ipa_selinux_get_maps_done (ipa_selinux_maps.c:203)
> ==28566==    by 0x12E2995C: sdap_get_generic_done (sdap_async.c:1558)
> ==28566==    by 0x12E29132: sdap_get_generic_ext_done (sdap_async.c:1449)
> ==28566==    by 0x12E2204B: sdap_process_message (sdap_async.c:366)
> ==28566==    by 0x12E21593: sdap_process_result (sdap_async.c:209)
> ==28566==  Address 0x17e416a0 is 80 bytes inside a block of size 96 free'd
> ==28566==    at 0x4C297A6: free (vg_replace_malloc.c:446)
> ==28566==    by 0x56E2880: _talloc_free_internal (talloc.c:876)
> ==28566==    by 0x122A5BB6: ipa_selinux_process_maps (ipa_selinux.c:355)
> ==28566==    by 0x122A4FFF: ipa_selinux_handler_done (ipa_selinux.c:224)
> ==28566==    by 0x122A9943: ipa_get_selinux_maps_done (ipa_selinux.c:1149)
> ==28566==    by 0x122AA8DB: ipa_selinux_get_maps_done (ipa_selinux_maps.c:203)
> ==28566==    by 0x12E2995C: sdap_get_generic_done (sdap_async.c:1558)
> ==28566==    by 0x12E29132: sdap_get_generic_ext_done (sdap_async.c:1449)
> ==28566==    by 0x12E2204B: sdap_process_message (sdap_async.c:366)
> ==28566==    by 0x12E21593: sdap_process_result (sdap_async.c:209)
> ==28566==    by 0x12E20BE5: sdap_ldap_next_result (sdap_async.c:159)
> ==28566==    by 0x54DAD3F: tevent_common_loop_timer_delay (in /usr/lib64/libtevent.so.0.9.17)
>
> Other tests passed, the use-after-free happens when there is a specific
> mapping rule only:
>
>     Rule name: test_user1_specific_host
>     SELinux User: staff_u:s0-s0:c0.c1023
>     Enabled: TRUE
>     Users: tuser3
>     Hosts: client.example.com
>

Fixed.

> The code looks OK to me, I would just change two things:
> 1) Split the function prepare_selinux_file_content() into one that
> creates the order array and one that evaluates the matched rules based
> on the order. Currently the function does two different tasks and is too
> long.
>
> 2) Rename the prepare_selinux_file_content() function so that it is
> clear it picks a single rule from a set of rules with same priority
> based on the configured order.
>

I split the function into two new functions (create_order_array,
choose_best_seuser) and added some comments.

New patches attached.

Thanks
Michal
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Move-SELinux-processing-to-provider.patch
Type: text/x-patch
Size: 35111 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20130308/52ff374f/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Reuse-cached-SELinux-mappings.patch
Type: text/x-patch
Size: 5157 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20130308/52ff374f/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Make-the-SELinux-refresh-time-configurable.patch
Type: text/x-patch
Size: 5433 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20130308/52ff374f/attachment-0002.bin>


More information about the sssd-devel mailing list