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

Added authgroup PAM option for challenge-response mode #94

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

Added authgroup PAM option for challenge-response mode #94

Show file tree
Hide file tree
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
4 changes: 4 additions & 0 deletions AUTHORS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,3 +42,7 @@ Drop permissions before opening user files.

Fredrik Thulin <[email protected]>
Maintainer alumni, helped on drop_privs.c, utils.c, and more.

Jonathan D. Hall <[email protected]>
Added authgroup PAM module option for challenge-response

17 changes: 16 additions & 1 deletion pam_yubico.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ struct cfg
int try_first_pass;
int use_first_pass;
const char *auth_file;
const char *auth_group;
const char *capath;
const char *cainfo;
const char *proxy;
Expand Down Expand Up @@ -439,7 +440,7 @@ do_challenge_response(pam_handle_t *pamh, struct cfg *cfg, const char *username)
char *userfile = NULL, *tmpfile = NULL;
FILE *f = NULL;
char buf[CR_RESPONSE_SIZE + 16], response_hex[CR_RESPONSE_SIZE * 2 + 1];
int ret, fd;
int ret, fd, result;

unsigned int response_len = 0;
YK_KEY *yk = NULL;
Expand All @@ -449,6 +450,18 @@ do_challenge_response(pam_handle_t *pamh, struct cfg *cfg, const char *username)

struct passwd *p;
struct stat st;

if(cfg->auth_group) {
result = check_user_group((char*)username,(char*)cfg->auth_group);

if(result == 1) {
errstr = NULL;
errno = 0;
ret = PAM_SUCCESS;
Copy link
Member

Choose a reason for hiding this comment

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

I'm not convinced PAM_SUCCESS is the appropriate return value here, PAM_IGNORE or PAM_USER_UNKNOWN might be more appropriate (but might also require more complex config).

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 you're right. I believe PAM_IGNORE would be more appropriate, as PAM_USER_UNKNOWN is going to have an adverse affect if the particular auth module is required by the PAM config. If PAM_IGNORE is returned, it should force PAM to skip it - whether it's required or optional... I'll do some playing with it.

DBG(("User %s is not in group %s - skipping YubiKey requirement for login!", username, cfg->auth_group));
goto out;
}
}

/* we must declare two sepparate privs structures as they can't be reused */
PAM_MODUTIL_DEF_PRIVS(privs);
Expand Down Expand Up @@ -711,6 +724,8 @@ parse_cfg (int flags, int argc, const char **argv, struct cfg *cfg)
cfg->use_first_pass = 1;
if (strncmp (argv[i], "authfile=", 9) == 0)
cfg->auth_file = argv[i] + 9;
if (strncmp (argv[i], "authgroup=", 10) == 0)
cfg->auth_group = argv[i] + 10;
Copy link
Member

Choose a reason for hiding this comment

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

very minor: the indentation is miss-aligned here.

Copy link
Author

Choose a reason for hiding this comment

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

This is an odd one that I keep fighting with my IDE over... When I open the source, all of the if statements are indented twice and the blocks under them are inverse-indented two back... It could be due to me using (I'm going to duck for cover as I type this) Xcode... I'll pop it open in vim and fix it. :)

if (strncmp (argv[i], "capath=", 7) == 0)
cfg->capath = argv[i] + 7;
if (strncmp (argv[i], "cainfo=", 7) == 0)
Expand Down
22 changes: 22 additions & 0 deletions util.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,28 @@ check_firmware_version(YK_KEY *yk, bool verbose, bool quiet)
return 1;
}

int
check_user_group(char *username, char *group)
{
struct group *group_ptr;
char **group_member;

if((group_ptr = getgrnam(group)) == NULL) {
Copy link
Member

Choose a reason for hiding this comment

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

Please use getgrnam_r() instead of getgrnam() here. One issue here is that group membership defined in the user record is not checked by the getgrnam functions, that has to be fetched with getpwnam_r() and getgrgid_r().

Copy link
Author

Choose a reason for hiding this comment

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

I'm not certain about whether or not getgrnam_r provides any additional checking over getgrnam, as they both essentially return a list of members of a specified group. But I believe the biggest difference between the two is that getgrnam stores the data in a single buffer and returns a pointer to it, but the buffer is overwritten on subsequent calls. However, I don't mind making the modifications to use getgrnam_r instead as it would provide a starting grounds for expansion with regards to remote authentication and threading... I have some ideas for developing a means of making challenge-response capable of remotely authenticating via a centralized server and using SSL to keep the challenge/response safe between the client/server.

We can chat more about that outside of here some time. If you could drop me an email at [email protected], I'd love to send you over the details of what I'd like to take on and contribute to the project.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it's the thread safety of getgrnam_r() that I'm after since the calling application might be threaded.

// Group was not found... Return a fail
Copy link
Member

Choose a reason for hiding this comment

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

Please use c style comments (/* */) as the rest of the project uses only that

D(("Group %s does not exist... Skipping group verification and returning valid!\n",group));
return 1;
}

for (group_member = group_ptr->gr_mem; *group_member != NULL; group_member++) {
if(strcmp(*group_member,username) == 0) {
// We found the user...
D(("User: %s is part of the Group: %s\n", username,group));
return 0;
}
}
return 1;
}

int
init_yubikey(YK_KEY **yk)
{
Expand Down
3 changes: 3 additions & 0 deletions util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@

#include <stdio.h>
#include <stdint.h>
#include <grp.h>
#include <string.h>
#include <pwd.h>

#if defined(DEBUG_PAM)
Expand Down Expand Up @@ -90,6 +92,7 @@ int write_chalresp_state(FILE *f, CR_STATE *state);

int init_yubikey(YK_KEY **yk);
int check_firmware_version(YK_KEY *yk, bool verbose, bool quiet);
int check_user_group(char *username, char *group);
int challenge_response(YK_KEY *yk, int slot,
char *challenge, unsigned int len,
bool hmac, bool may_block, bool verbose,
Expand Down