Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

take supplementary groups into account #168

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

take supplementary groups into account #168

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 51 additions & 2 deletions src/pam_google_authenticator.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

#include <errno.h>
#include <fcntl.h>
#include <grp.h>
#include <limits.h>
#include <pwd.h>
#include <stdarg.h>
Expand Down Expand Up @@ -76,6 +77,12 @@ typedef struct Params {
int allow_readonly;
} Params;

// structure to keep track of supplementary groups of the calling process
typedef struct GroupList {
int n_groups;
gid_t *gids;
} GroupList;

static char oom;

static const char* nobody = "nobody";
Expand Down Expand Up @@ -358,7 +365,7 @@ static int setgroup(int gid) {

// Drop privileges and return 0 on success.
static int drop_privileges(pam_handle_t *pamh, const char *username, int uid,
int *old_uid, int *old_gid) {
int *old_uid, int *old_gid, GroupList *old_groups) {
// Try to become the new user. This might be necessary for NFS mounted home
// directories.

Expand All @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// user that tries to authenticate.
// 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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also check for the error.


// now get a full list of all groups
user_groups.gids = malloc(sizeof(gid_t) * user_groups.n_groups);
err = getgrouplist(username, gid, user_groups.gids, &user_groups.n_groups);
if (err == -1) {
log_message(LOG_ERR, pamh, "Failed to get group memberships of user \"%s\"", username);
}

// store the old list of groups of this process
old_groups->n_groups = getgroups(0, NULL);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check for error (-1 return value)

old_groups->gids = malloc(sizeof(gid_t)*old_groups->n_groups);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check for malloc fail.

Also above.

err = getgroups(old_groups->n_groups, old_groups->gids);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

if (err == -1) {
log_message(LOG_ERR, pamh, "Failed to get supplementary groups for the process");
}

// change the list of groups to the groups the user is member of
err = setgroups(user_groups.n_groups, user_groups.gids);
if (err == -1) {
log_message(LOG_ERR, pamh, "Failed to set %d supplementary group IDs for the process", user_groups.n_groups);
}
// cleanup user group list
free(user_groups.gids);
user_groups.n_groups = -1;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not 0?

Well, if this is in its own function then this auto variable will go out of scope anyway.


// set primary uid und gid for the process
int gid_o = setgroup(gid);
int uid_o = setuser(uid);
if (uid_o < 0) {
Expand Down Expand Up @@ -1808,6 +1847,7 @@ static int google_authenticator(pam_handle_t *pamh,
int rc = PAM_AUTH_ERR;
uid_t uid = -1;
int old_uid = -1, old_gid = -1, fd = -1;
GroupList old_groups;
char *buf = NULL;
struct stat orig_stat = { 0 };
uint8_t *secret = NULL;
Expand All @@ -1833,6 +1873,8 @@ static int google_authenticator(pam_handle_t *pamh,
int stopped_by_rate_limit = 0;

// Drop privileges.
old_groups.n_groups = 0;
old_groups.gids = NULL;
{
const char* drop_username = username;

Expand All @@ -1845,7 +1887,7 @@ static int google_authenticator(pam_handle_t *pamh,
}
}

if (drop_privileges(pamh, drop_username, uid, &old_uid, &old_gid)) {
if (drop_privileges(pamh, drop_username, uid, &old_uid, &old_gid, &old_groups)) {
// Don't allow to continue without dropping privs.
goto out;
}
Expand Down Expand Up @@ -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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the list is empty we still want to call setgroups(), no? That is, we want to drop whatever groups we added before.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (setgroups(old_groups.n_groups, old_groups.gids) != 0) {
log_message(LOG_ERR, pamh, "Failed to restore %d old supplementary groups.", old_groups.n_groups);
}
free(old_groups.gids);
}
free(secret_filename);

// Clean up
Expand Down