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

Michal Židek mzidek at redhat.com
Mon Aug 18 11:53:58 UTC 2014


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




More information about the sssd-devel mailing list