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

Pavel Reichl preichl at redhat.com
Mon Aug 18 11:59:25 UTC 2014


On 08/18/2014 01:53 PM, 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
>
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Thanks Michal, I know what you mean. I'm aware of that and I have it on 
my todo list, but right now I'm little out of time for that.




More information about the sssd-devel mailing list