[SSSD] [PATCHES] SELinux processing related patches

Jakub Hrozek jhrozek at redhat.com
Wed Mar 6 12:56:46 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

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.



More information about the sssd-devel mailing list