take supplementary groups into account #168
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
|
@googlebot I signed it! |
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
| // get number of groups for the user | ||
| GroupList user_groups; | ||
| user_groups.n_groups = 0; | ||
| int err = getgrouplist(username, gid, NULL, &user_groups.n_groups); |
There was a problem hiding this comment.
Also check for the error.
| } | ||
|
|
||
| // store the old list of groups of this process | ||
| old_groups->n_groups = getgroups(0, NULL); |
There was a problem hiding this comment.
check for error (-1 return value)
| @@ -385,6 +392,38 @@ static int drop_privileges(pam_handle_t *pamh, const char *username, int uid, | |||
| gid_t gid = pw->pw_gid; | |||
| free(buf); | |||
|
|
|||
| // to access the secret file, we may have to be member of the same groups as the | |||
There was a problem hiding this comment.
Break out all this grouplist fetching into its own function, so that it's easy to follow the failure path, that should presumably be to degrade to the current behaviour of no extra groups.
Bonus points for hiding this behind a HAVE_SETGROUPLIST since it's apparently nonstandard.
|
|
||
| // store the old list of groups of this process | ||
| old_groups->n_groups = getgroups(0, NULL); | ||
| old_groups->gids = malloc(sizeof(gid_t)*old_groups->n_groups); |
There was a problem hiding this comment.
check for malloc fail.
Also above.
| // store the old list of groups of this process | ||
| old_groups->n_groups = getgroups(0, NULL); | ||
| old_groups->gids = malloc(sizeof(gid_t)*old_groups->n_groups); | ||
| err = getgroups(old_groups->n_groups, old_groups->gids); |
There was a problem hiding this comment.
From the manpage:
It is unspecified whether the effective group ID of the calling process
is included in the returned list. (Thus, an application should also
call getegid(2) and add or remove the resulting value.)
Are we already capturing this using old_gid, so no need?
| } | ||
| // cleanup user group list | ||
| free(user_groups.gids); | ||
| user_groups.n_groups = -1; |
There was a problem hiding this comment.
why not 0?
Well, if this is in its own function then this auto variable will go out of scope anyway.
| @@ -2132,6 +2174,13 @@ static int google_authenticator(pam_handle_t *pamh, | |||
| "but can't switch back", old_uid, uid); | |||
| } | |||
| } | |||
| // restore old supplementary groups for process | |||
| if (old_groups.n_groups > 0) { | |||
There was a problem hiding this comment.
If the list is empty we still want to call setgroups(), no? That is, we want to drop whatever groups we added before.
There was a problem hiding this comment.
I think when we make sure that the effective group is taken into account, then the list can never be empty. Except for platforms where not all functions are available. I will make sure that this case is also handled correctly.
|
And you've confirmed that this fixes the issue for you? |
|
Thank you for your comments! I will move my changes into a separate function and hide it behind I can also confirm that the issue #167 is solved when supplementary groups are taken into account. |
Fix for issue #167.
The changes certainly needs to be properly reviewed.