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

Jakub Hrozek jhrozek at redhat.com
Tue Aug 19 13:53:21 UTC 2014


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



More information about the sssd-devel mailing list