[SSSD] [PATCH] IPA: incorrect expansion of group membership when encountering a non-POSIX group

Michal Židek mzidek at redhat.com
Tue Aug 19 16:34:32 UTC 2014


On 08/19/2014 03:53 PM, Jakub Hrozek wrote:
> On Mon, Aug 18, 2014 at 01:53:58PM +0200, Michal Židek wrote:
>> On 08/14/2014 01:20 PM, Pavel Reichl wrote:
>>>
>>> On 08/05/2014 05:18 PM, Michal Židek wrote:
>>>> On 07/16/2014 05:18 PM, Pavel Reichl wrote:
>>>>> Hello,
>>>>>
>>>>> please see attached patches.
>>>>>
>>>>> Regards,
>>>>>
>>>>> Pavel Reichl
>>>>>
>>>>
>>>> Hi,
>>>>
>>>> Patch 1: LGTM
>>>>
>>>> Patch 2:
>>>>
>>>>> @@ -962,9 +977,40 @@ static void sdap_initgr_nested_search(struct
>>>>> tevent_req *subreq)
>>>>> groups[0]);
>>>>>           state->groups_cur++;
>>>>>       } else {
>>>>> -        DEBUG(SSSDBG_OP_FAILURE,
>>>>> -              "Search for group %s, returned %zu results. Skipping\n",
>>>>> -               state->group_dns[state->cur], count);
>>>>> +        if (state->try_as_non_posix || count != 0) {
>>>>                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>>> This condition was a little difficult for me to understand
>>>> without additional explanation. Could you add some more descriptive
>>>> comments to this (and the else) branch?
>>>>
>>>>> +            /* We can't get the group as posix nor non-posix.
>>>>> Succumb on
>>>>> +             * this one and try to continue with another.
>>>>> +             */
>>>>> +            DEBUG(SSSDBG_OP_FAILURE,
>>>>> +                  "Search for group %s, returned %zu results.
>>>>> Skipping\n",
>>>>> +                  state->group_dns[state->cur], count);
>>>>> +        } else {
>>>>> +            /* Try to get the group as non-posix. */
>>>>> +            state->try_as_non_posix = true;
>>>>> +
>>>>> +            DEBUG(SSSDBG_OP_FAILURE,
>>>>> +                  "Search for group %s, returned %zu results. "
>>>>> +                  "Trying to get it as non-posix\n",
>>>>> +                  state->group_dns[state->cur], count);
>>>>> +
>>>>> +            subreq = sdap_get_generic_send(state, state->ev,
>>>>> +                                           state->opts, state->sh,
>>>>> + state->group_dns[state->cur],
>>>>> +                                           LDAP_SCOPE_BASE,
>>>>> + state->np_grp_filter,
>>>>> + state->np_grp_attrs,
>>>>> + state->opts->np_group_map,
>>>>> +                                           SDAP_OPTS_NP_GROUP,
>>>>> + dp_opt_get_int(state->opts->basic,
>>>>> + SDAP_SEARCH_TIMEOUT),
>>>>> +                                           false);
>>>>> +            if (!subreq) {
>>>>> +                tevent_req_error(req, ENOMEM);
>>>>> +                return;
>>>>> +            }
>>>>> +            tevent_req_set_callback(subreq,
>>>>> sdap_initgr_nested_search, req);
>>>>> +            return;
>>>>> +        }
>>>>>       }
>>>>>
>>>>>       state->cur++;
>>>>> @@ -972,6 +1018,7 @@ static void sdap_initgr_nested_search(struct
>>>>> tevent_req *subreq)
>>>>>        * memberOf which might not be only groups, but permissions, etc.
>>>>>        * Use state->groups_cur for group index cap */
>>>>>       if (state->cur < state->memberof->num_values) {
>>>>> +        state->try_as_non_posix = false;
>>>>>           subreq = sdap_get_generic_send(state, state->ev,
>>>>>                                          state->opts, state->sh,
>>>>> state->group_dns[state->cur],
>>>>> @@ -2656,6 +2703,7 @@ struct tevent_req
>>>>> *sdap_get_initgr_send(TALLOC_CTX *memctx,
>>>>>       state->conn = conn;
>>>>>       state->name = name;
>>>>>       state->grp_attrs = grp_attrs;
>>>>> +    state->np_grp_attrs = np_grp_attrs;
>>>>>       state->orig_user = NULL;
>>>>>       state->timeout = dp_opt_get_int(state->opts->basic,
>>>>> SDAP_SEARCH_TIMEOUT);
>>>>>       state->user_base_iter = 0;
>>>>> -- 1.9.3
>>>>>
>>>>>
>>>>
>>>> I found some problems while testing these patches.
>>>> Let us have hierarchy like this:
>>>>
>>>> GROUP    | MEMBERS (users and groups)
>>>> -------------------------------------
>>>> pos1     | ipauser1, nonpos2
>>>> nonpos2  | ipauser2, pos3
>>>> pos3     | ipauser3,
>>>>
>>>> Groups pos1 and pos3 are posix groups, nonpos2 is normal group.
>>>>
>>>> (note: output is sanitized  uid, gid and non relevant groups are deleted)
>>>>
>>>> This is correct (these command are given in order as shown):
>>>>
>>>> # id ipauser3
>>>> groups=234600001(pos1),234600003(pos3)
>>>>
>>>> # id ipauser2
>>>> groups=234600001(pos1)
>>>>
>>>> # getent group pos1
>>>> pos1:*:234600001:ipauser3,ipauser2,ipauser1
>>>>
>>>> Now I clear the cache and do:
>>>>
>>>> # getent group pos1
>>>> pos1:*:234600001:ipauser1
>>>>
>>>> Users ipauser2 and ipauser3 are not shown, which is
>>>> wrong.
>>>>
>>>> When I try hierarchy like this:
>>>>
>>>> GROUP     | MEMBERS (users and groups)
>>>> -------------------------------------
>>>> ppos1     | pnonpos, ppos2
>>>> pnonpos   | ipauser4,
>>>> ppos2     | ipauser5,
>>>>
>>>> Groups ppos1 and ppos2 are posix groups, pnonpos is normal group.
>>>>
>>>> # getent group ppos1
>>>> ppos1:*:234600007:ipauser5
>>>>
>>>> So ipauser4 was not returned and it should be. It is only
>>>> returned when I do 'id ipauser4' first.
>>>>
>>>> So when searching for users of groups (getgrnam) the
>>>> non-posix group cuts the hierarchy if the user data is
>>>> not cached already. This is still potential
>>>> security issue IMO. If you deny access to group
>>>> pos1, the user ipauser2 and ipauser3 will be able
>>>> to enter.
>>>>
>>>> So the bug is still there. On top of that we have
>>>> inconsistent behaviour when commands like 'id'
>>>> precede 'getent group', (in other words, we have
>>>> different results of getgrnam with clear and
>>>> filled cache) which is weird.
>>>>
>>>> Michal
>>>> _______________________________________________
>>>> sssd-devel mailing list
>>>> sssd-devel at lists.fedorahosted.org
>>>> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>>>
>>> Hello,
>>>
>>> please see updated patches.
>>>
>>
>> Thanks for adding the comments to the 2nd patch. It
>> helped a lot.
>>
>> Ack to all 3 patches.
>>
>> I just have one more comment, but this should not block these patches.
>> When looking at the functions in 3rd patch, the new added functions
>> are mostly copies of their posix alternativces. I wonder if it would
>> be possible to create one generic set of functions to deal with
>> this particular problem.
>>
>> But this does not need to be done now, and frankly I am not sure if it
>> is worth the effort, but removing duplicities might make us a little
>> more happy in the long term.
>>
>> But again, it does not need to be done now. These patches passed
>> my testing and work good. So ack again.
>>
>> Michal
>
> Pushed to master:
>      * 08145755f66e83c304e11228c2b610a09576dd81
>      * 5197ac634572a2e0f8c7cacad68d5e5336064744
>      * 4c560e7b98e7ab71d22be24d2fbc468396cb634f
> sssd-1-11:
>     * 1fe677614603cb57784dbc03f60dbe4c1ba2db44
>     * 4417c874595600cd93e12822fab54aa5753df74a
>     * b7afe5caaaeae1e92479284a7f555aee4ba23422

Sorry this was premature ACK from my side. The patches actually broke
the LDAP provider. I think (I am actually sure) that we should
revert these patches and use a different approach to solve this
problem (not so IPA specific).

Jakub, will you please revert these patches from both 1-11 and
master?

Sorry for the inconvenience.

Michal




More information about the sssd-devel mailing list