On Mon, Mar 18, 2013 at 03:32:45PM +0100, Michal Židek wrote:
> On 03/18/2013 12:03 PM, Jakub Hrozek wrote:
>> On Fri, Mar 08, 2013 at 09:10:24PM +0100, Michal Židek wrote:
>>>> 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
>>
>> Nack,
>>
>>> +/* Choose best selinux user based on given order and write
>>> + * the user to selinux login file. */
>>> +static errno_t choose_best_seuser(struct sysdb_attrs **usermaps,
>>> + struct pam_data *pd,
>>> + char **order_array, int order_count,
>>> + const char *default_user)
>>> +{
>>> + TALLOC_CTX *tmp_ctx;
>>> + char *file_content = NULL;
>>> + const char *tmp_str;
>>> + errno_t ret, err;
>>> + int i, j;
>>> +
>>> + /* If no maps match, we'll use the default SELinux user from the
>>> + * config */
>>> + file_content = talloc_strdup(tmp_ctx, default_user);
>>
>> This is clearly wrong, tmp_ctx is never assigned to. The code may work
>> only by chance without optimizations becaues tmp_ctx would have been NULL..
>>
>
> Good catch. Sorry for that.
>
>>> + if (file_content == NULL) {
>>> + ret = ENOMEM;
>>> + goto done;
>>> + }
>
> I also changed the behaviour if HAVE_SELINUX_LOGIN_DIR is not defined.
> In that case, the processing is skipped and the ipa_selinux_handler just
> returns success.
>
> New patches attached.
>
> Thanks
> Michal
All my tests passed now. Ack.
Please open a bug in Fedora against selinux-policy-targeted asking them
to allow sssd_be to write to the logins directory. It would be best to
paste them the complete AVC.