[PATCH] IPA: incorrect expansion of group membership when encountering a non-POSIX group
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
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@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Hello,
please see updated patches.
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@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
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@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@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.
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@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
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@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
On Tue, Aug 19, 2014 at 06:34:32PM +0200, Michal Židek wrote:
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@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
Oh well, stuff happens.
I reverted them all.
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Michal Židek -
Pavel Reichl