On Tue, Aug 16, 2016 at 01:13:02PM -0400, Stephen Smalley wrote:
On 08/16/2016 11:59 AM, Gary Tierney wrote:Hi Stephen, Replied inline below. On Mon, Aug 15, 2016 at 03:58:44PM -0400, Stephen Smalley wrote:On 07/27/2016 05:25 PM, Gary Tierney wrote:semanage-login supports login mappings using the %group syntax, but genhomedircon does not expand groups to the users belonging to them. This commit adds support for generating home directory contexts for login mappings using the group syntax and adds error reporting for handling cases where there is ambiguity due to a user belonging to multiple groups mapped by semanage-login. If a login mapping is added for the user which belongs to multiple groups it will take precedence and resolve the ambiguity issue.Sorry for the long delay in responding. One question/comment below.Signed-off-by: Gary Tierney <gary.tierney@xxxxxxx> --- libsemanage/src/genhomedircon.c | 319 +++++++++++++++++++++++++++++++--------- 1 file changed, 247 insertions(+), 72 deletions(-) diff --git a/libsemanage/src/genhomedircon.c b/libsemanage/src/genhomedircon.c index c5ea436..2955b19 100644 --- a/libsemanage/src/genhomedircon.c +++ b/libsemanage/src/genhomedircon.c @@ -838,94 +1064,43 @@ static genhomedircon_user_entry_t *get_users(genhomedircon_settings_t * s, nusers = 0; } + qsort(seuser_list, nseusers, sizeof(semanage_seuser_t *), + &seuser_sort_func); qsort(user_list, nusers, sizeof(semanage_user_t *), (int (*)(const void *, const void *))&user_sort_func); - /* Allocate space for the getpwnam_r buffer */ - rbuflen = sysconf(_SC_GETPW_R_SIZE_MAX); - if (rbuflen <= 0) - goto cleanup; - rbuf = malloc(rbuflen); - if (rbuf == NULL) - goto cleanup; - for (i = 0; i < nseusers; i++) { seuname = semanage_seuser_get_sename(seuser_list[i]); name = semanage_seuser_get_name(seuser_list[i]); - if (strcmp(name,"root") && strcmp(seuname, s->fallback->sename) == 0) - continue; -This appears to change the behavior of genhomedircon in general, not just with respect to %group handling. Was this intentional? I'm not necessarily opposed to this change, but I am unclear on the implications. It seems that previously genhomedircon would not generate file_contexts.homedirs entries for users who were mapped to the fallback seuser, with an exception for root for /root labeling. With this change, they will have entries added.Hmm, yes, you're right. This is a mistake. We do need to skip this conditional however, so we can check if a user already has a mapping when we're expanding the members of a group. To prevent this from happening it should suffice to check if the user that we're writing contexts for shares an sename with the __default__ mapping, and if so, simply skip writing contexts for that user. We can do this check inside write_gen_home_dir_contexts() like so: https://github.com/garyttierney/selinux/commit/ebdb7f225ddbd5f311b2db75f68e2896285a5090#diff-b298746a257be78548f69d5d296dcd09R1140 An alternative fix would be to prevent these users from being added to the `genhomedircon_user_entry_t` stack in the first place, and have get_group_users() search the seusers list for a login mapping which matches the group members username, skipping the group member if a match is found. Any thoughts on either of these solutions?I'm actually inclined to just take your patch as is, removing this test altogether, but I wanted to check whether this might have undesirable side effects. I am unclear on the correctness of the current code before your patch, e.g. what if the Linux user/login name is mapped to the same seuser as the fallback but has a more specific level within the authorized range for that seuser? The fact that we already have an exception for root suggests that something is wrong with this test. At worst, removing the test may lead to some unnecessary (but harmless) entries in file_contexts.homedirs, but only if there is an entry in seusers for that Linux user/login in the first place, and they can obviously address that by deleting the user's entry if they do not truly need to map them to a different seuser and/or level.
Yes, the level is something I hadn't given much consideration. As far as I can tell the only consequences would be a difference in generated homedir contexts for users who had expected adding a mapping to the fallback user with a more specific level like you mentioned to work.
There doesn't seem to be any useful VCS history attached to the line of code responsible for this test, but it seems to me like it exists solely to avoid unnecessarily creating homedir contexts for logins mapped to the fallback user (since the exception for root makes sure that root will always have contexts generated), but it creates the issue with the level mentioned above. It seems like it might also create an issue with users whose home directories aren't under /home (or LU_HOMEDIRECTORY), since they wont be matched by the fallback users homedir regexp. Since an incorrect level could be a security issue, I think this is definitely something which should be resolved.
Now that this patch addresses these 2 separate issues, would it be worth breaking this into 2 commits? If all is OK with the new fallback user behavior, of course.
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Selinux mailing list Selinux@xxxxxxxxxxxxx To unsubscribe, send email to Selinux-leave@xxxxxxxxxxxxx. To get help, send an email containing "help" to Selinux-request@xxxxxxxxxxxxx.
