From 22f8cc4e6c149ceb272ecad2caea9f33ec86dfaa Mon Sep 17 00:00:00 2001 From: Samuel Varley Date: Tue, 24 Nov 2015 16:25:13 +1300 Subject: Ensure config.h is included first. pam_radius_auth.c was including config.h via pam_radius_auth.h but only after other library header files had been included. This meant you could have _GNU_SOURCE, for example, defined in config.h but it did not have any effect on which library functions were provided. --- src/pam_radius_auth.c | 4 ---- src/pam_radius_auth.h | 3 +++ 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/pam_radius_auth.c b/src/pam_radius_auth.c index 90d074a..d184e1a 100644 --- a/src/pam_radius_auth.c +++ b/src/pam_radius_auth.c @@ -55,10 +55,6 @@ #define PAM_SM_PASSWORD #define PAM_SM_SESSION -#include -#include -#include - #include "pam_radius_auth.h" #define DPRINT if (opt_debug & PAM_DEBUG_ARG) _pam_log diff --git a/src/pam_radius_auth.h b/src/pam_radius_auth.h index 95f262c..defec5c 100644 --- a/src/pam_radius_auth.h +++ b/src/pam_radius_auth.h @@ -3,6 +3,9 @@ #include "config.h" +#include +#include +#include #include #include #include -- cgit v1.2.3 From c2c6b4cbb3906f1171e5d18426509489a15dc7a0 Mon Sep 17 00:00:00 2001 From: Samuel Varley Date: Thu, 19 Nov 2015 10:09:46 +1300 Subject: Thread safety: Store session start time as PAM data. Previously, it was stored as file-scope variable. I also deleted the file-scope variable, "live_server", because it was not being used. --- src/pam_radius_auth.c | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/src/pam_radius_auth.c b/src/pam_radius_auth.c index d184e1a..a0a37e8 100644 --- a/src/pam_radius_auth.c +++ b/src/pam_radius_auth.c @@ -64,13 +64,6 @@ static CONST char *pam_module_name = "pam_radius_auth"; static char conf_file[BUFFER_SIZE]; /* configuration file */ static int opt_debug = FALSE; /* print debug info */ -/* we need to save these from open_session to close_session, since - * when close_session will be called we won't be root anymore and - * won't be able to access again the radius server configuration file - * -- cristiang */ -static radius_server_t *live_server = NULL; -static time_t session_time; - /* logging */ static void _pam_log(int err, CONST char *format, ...) { @@ -987,7 +980,6 @@ static int talk_radius(radius_conf_t *conf, AUTH_HDR *request, AUTH_HDR *respons /* we've found one that does respond, forget about the other servers */ cleanup(server->next); server->next = NULL; - live_server = server; /* we've got a live one! */ break; } } @@ -1354,9 +1346,15 @@ static int pam_private_session(pam_handle_t *pamh, int flags, int argc, CONST ch add_int_attribute(request, PW_ACCT_AUTHENTIC, PW_AUTH_RADIUS); if (status == PW_STATUS_START) { - session_time = time(NULL); + time_t *session_time = malloc(sizeof(time_t)); + time(session_time); + pam_set_data(pamh, "rad_session_time", (void *) session_time, _int_free); } else { - add_int_attribute(request, PW_ACCT_SESSION_TIME, time(NULL) - session_time); + time_t *session_time; + retval = pam_get_data(pamh, "rad_session_time", (CONST void **) &session_time); + PAM_FAIL_CHECK; + + add_int_attribute(request, PW_ACCT_SESSION_TIME, time(NULL) - *session_time); } retval = talk_radius(&config, request, response, NULL, NULL, 1); -- cgit v1.2.3 From e2af858a6e8de859af6ca4198fd60ea4405df3d8 Mon Sep 17 00:00:00 2001 From: Samuel Varley Date: Thu, 19 Nov 2015 17:39:02 +1300 Subject: Thread safety: Use getaddrinfo() instead of gethostbyname(). --- src/pam_radius_auth.c | 103 ++++++++------------------------------------------ 1 file changed, 16 insertions(+), 87 deletions(-) diff --git a/src/pam_radius_auth.c b/src/pam_radius_auth.c index a0a37e8..7c86315 100644 --- a/src/pam_radius_auth.c +++ b/src/pam_radius_auth.c @@ -178,93 +178,28 @@ void _int_free(pam_handle_t * pamh, void *x, int error_status) * SMALL HELPER FUNCTIONS *************************************************************************/ -/* - * Return an IP address in host long notation from - * one supplied in standard dot notation. - */ -static uint32_t ipstr2long(char *ip_str) { - char buf[6]; - char *ptr; - int i; - int count; - uint32_t ipaddr; - int cur_byte; - - ipaddr = (uint32_t)0; - - for(i = 0;i < 4;i++) { - ptr = buf; - count = 0; - *ptr = '\0'; - - while(*ip_str != '.' && *ip_str != '\0' && count < 4) { - if (!isdigit((unsigned char)*ip_str)) { - return (uint32_t)0; - } - *ptr++ = *ip_str++; - count++; - } - - if (count >= 4 || count == 0) { - return (uint32_t)0; - } - - *ptr = '\0'; - cur_byte = atoi(buf); - if (cur_byte < 0 || cur_byte > 255) { - return (uint32_t)0; - } - - ip_str++; - ipaddr = ipaddr << 8 | (uint32_t)cur_byte; - } - return ipaddr; -} - -/* - * Check for valid IP address in standard dot notation. - */ -static int good_ipaddr(char *addr) { - int dot_count; - int digit_count; - - dot_count = 0; - digit_count = 0; - while(*addr != '\0' && *addr != ' ') { - if (*addr == '.') { - dot_count++; - digit_count = 0; - } else if (!isdigit((unsigned char)*addr)) { - dot_count = 5; - } else { - digit_count++; - if (digit_count > 3) { - dot_count = 5; - } - } - addr++; - } - if (dot_count != 3) { - return -1; - } else { - return 0; - } -} - /* * Return an IP address in host long notation from a host * name or address in dot notation. */ static uint32_t get_ipaddr(char *host) { - struct hostent *hp; - - if (good_ipaddr(host) == 0) { - return ipstr2long(host); - } else if ((hp = gethostbyname(host)) == (struct hostent *)NULL) { - return (uint32_t)0; + struct addrinfo hints; + struct addrinfo *results; + uint32_t addr; + + memset(&hints, 0, sizeof(hints)); + hints.ai_family = AF_INET; + hints.ai_socktype = SOCK_DGRAM; + + if (getaddrinfo(host, NULL, &hints, &results) == 0) { + struct sockaddr_in *sockaddr = (struct sockaddr_in *)results->ai_addr; + addr = ntohl(sockaddr->sin_addr.s_addr); + freeaddrinfo(results); + } else { + addr = (uint32_t)0; } - return ntohl(*(uint32_t *)hp->h_addr); + return addr; } /* @@ -726,13 +661,7 @@ static void build_radius_packet(AUTH_HDR *request, CONST char *user, CONST char if ((conf->server->ip.s_addr == ntohl(0x7f000001)) || (!hostname[0])) { ipaddr = 0x7f000001; } else { - struct hostent *hp; - - if ((hp = gethostbyname(hostname)) == (struct hostent *) NULL) { - ipaddr = 0x00000000; /* no client IP address */ - } else { - ipaddr = ntohl(*(uint32_t *) hp->h_addr); /* use the first one available */ - } + ipaddr = get_ipaddr(hostname); } /* If we can't find an IP address, then don't add one */ -- cgit v1.2.3 From c65921779fc834e7cde4ac7d3a5d779414dd043e Mon Sep 17 00:00:00 2001 From: Samuel Varley Date: Fri, 20 Nov 2015 12:53:10 +1300 Subject: Thread safety: Use getaddrinfo() instead of getservbyname(). --- src/pam_radius_auth.c | 42 +++++++++++++++++++++++++++++++----------- 1 file changed, 31 insertions(+), 11 deletions(-) diff --git a/src/pam_radius_auth.c b/src/pam_radius_auth.c index 7c86315..38a0f80 100644 --- a/src/pam_radius_auth.c +++ b/src/pam_radius_auth.c @@ -202,6 +202,30 @@ static uint32_t get_ipaddr(char *host) { return addr; } +/* + * Gets the UDP port number associated with a service name. + * The port number is returned in network byte order. + */ +static uint16_t get_udp_port(char *service) { + struct addrinfo hints; + struct addrinfo *results; + uint16_t port; + + memset(&hints, 0, sizeof(hints)); + hints.ai_family = AF_INET; + hints.ai_socktype = SOCK_DGRAM; + + if (getaddrinfo(NULL, service, &hints, &results) == 0) { + struct sockaddr_in *sockaddr = (struct sockaddr_in *)results->ai_addr; + port = sockaddr->sin_port; + freeaddrinfo(results); + } else { + port = (uint16_t)0; + } + + return port; +} + /* * take server->hostname, and convert it to server->ip and server->port */ @@ -231,29 +255,25 @@ static int host2server(radius_server_t *server) server->port = htons((uint16_t) (i + 1)); } } else { /* the port looks like it's a name */ - struct servent *svp; - if (p) { /* maybe it's not "radius" */ - svp = getservbyname (p, "udp"); + server->port = get_udp_port(p); /* quotes allow distinction from above, lest p be radius or radacct */ - DPRINT(LOG_DEBUG, "DEBUG: getservbyname('%s', udp) returned %p.\n", p, svp); + DPRINT(LOG_DEBUG, "DEBUG: get_udp_port('%s') returned %u.\n", p, server->port); *(--p) = ':'; /* be sure to put the delimiter back */ } else { if (!server->accounting) { - svp = getservbyname ("radius", "udp"); - DPRINT(LOG_DEBUG, "DEBUG: getservbyname(radius, udp) returned %p.\n", svp); + server->port = get_udp_port("radius"); + DPRINT(LOG_DEBUG, "DEBUG: get_udp_port(radius) returned %u.\n", server->port); } else { - svp = getservbyname ("radacct", "udp"); - DPRINT(LOG_DEBUG, "DEBUG: getservbyname(radacct, udp) returned %p.\n", svp); + server->port = get_udp_port("radacct"); + DPRINT(LOG_DEBUG, "DEBUG: get_udp_port(radacct) returned %u.\n", server->port); } } - if (svp == (struct servent *) 0) { + if (!server->port) { /* debugging above... */ return PAM_AUTHINFO_UNAVAIL; } - - server->port = svp->s_port; } } -- cgit v1.2.3 From 14664dab2d129a7f975648930a1594bdcc1b374a Mon Sep 17 00:00:00 2001 From: Samuel Varley Date: Mon, 23 Nov 2015 16:01:01 +1300 Subject: Thread safety: Use strerror_r() instead of strerror(). --- src/pam_radius_auth.c | 46 ++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 40 insertions(+), 6 deletions(-) diff --git a/src/pam_radius_auth.c b/src/pam_radius_auth.c index 38a0f80..29b0322 100644 --- a/src/pam_radius_auth.c +++ b/src/pam_radius_auth.c @@ -178,6 +178,28 @@ void _int_free(pam_handle_t * pamh, void *x, int error_status) * SMALL HELPER FUNCTIONS *************************************************************************/ +/* + * A strerror_r() wrapper function to deal with its nuisances. + */ +static void get_error_string(int errnum, char *buf, size_t buflen) { +#if !defined(__GLIBC__) || ((_POSIX_C_SOURCE >= 200112L || _XOPEN_SOURCE >= 600) && ! _GNU_SOURCE) + /* XSI version of strerror_r(). */ + int retval = strerror_r(errnum, buf, buflen); + + /* POSIX does not state what will happen to the buffer if the function fails. + * Put it into a known state rather than leave it possibly uninitialized. */ + if (retval != 0 && buflen > (size_t)0) { + buf[0] = '\0'; + } +#else + /* GNU version of strerror_r(). */ + char tmp_buf[BUFFER_SIZE]; + char *retval = strerror_r(errnum, tmp_buf, sizeof(tmp_buf)); + + snprintf(buf, buflen, "%s", retval); +#endif +} + /* * Return an IP address in host long notation from a host * name or address in dot notation. @@ -553,8 +575,10 @@ static int initialize(radius_conf_t *conf, int accounting) /* the first time around, read the configuration file */ if ((fserver = fopen (conf_file, "r")) == (FILE*)NULL) { + char error_string[BUFFER_SIZE]; + get_error_string(errno, error_string, sizeof(error_string)); _pam_log(LOG_ERR, "Could not open configuration file %s: %s\n", - conf_file, strerror(errno)); + conf_file, error_string); return PAM_ABORT; } @@ -619,7 +643,9 @@ static int initialize(radius_conf_t *conf, int accounting) /* open a socket. Dies if it fails */ conf->sockfd = socket(AF_INET, SOCK_DGRAM, 0); if (conf->sockfd < 0) { - _pam_log(LOG_ERR, "Failed to open RADIUS socket: %s\n", strerror(errno)); + char error_string[BUFFER_SIZE]; + get_error_string(errno, error_string, sizeof(error_string)); + _pam_log(LOG_ERR, "Failed to open RADIUS socket: %s\n", error_string); return PAM_AUTHINFO_UNAVAIL; } @@ -636,7 +662,9 @@ static int initialize(radius_conf_t *conf, int accounting) if (bind(conf->sockfd, &salocal, sizeof (struct sockaddr_in)) < 0) { - _pam_log(LOG_ERR, "Failed binding to port: %s", strerror(errno)); + char error_string[BUFFER_SIZE]; + get_error_string(errno, error_string, sizeof(error_string)); + _pam_log(LOG_ERR, "Failed binding to port: %s", error_string); close(conf->sockfd); return PAM_AUTHINFO_UNAVAIL; } @@ -766,8 +794,10 @@ static int talk_radius(radius_conf_t *conf, AUTH_HDR *request, AUTH_HDR *respons /* send the packet */ if (sendto(conf->sockfd, (char *) request, total_length, 0, &saremote, sizeof(struct sockaddr_in)) < 0) { + char error_string[BUFFER_SIZE]; + get_error_string(errno, error_string, sizeof(error_string)); _pam_log(LOG_ERR, "Error sending RADIUS packet to server %s: %s", - server->hostname, strerror(errno)); + server->hostname, error_string); ok = FALSE; goto next; /* skip to the next server */ } @@ -816,8 +846,10 @@ static int talk_radius(radius_conf_t *conf, AUTH_HDR *request, AUTH_HDR *respons } } else { /* not an interrupt, it was a real error */ + char error_string[BUFFER_SIZE]; + get_error_string(errno, error_string, sizeof(error_string)); _pam_log(LOG_ERR, "Error waiting for response from RADIUS server %s: %s", - server->hostname, strerror(errno)); + server->hostname, error_string); ok = FALSE; break; } @@ -828,8 +860,10 @@ static int talk_radius(radius_conf_t *conf, AUTH_HDR *request, AUTH_HDR *respons /* try to receive some data */ if ((total_length = recvfrom(conf->sockfd, (void *) response, BUFFER_SIZE, 0, &saremote, &salen)) < 0) { + char error_string[BUFFER_SIZE]; + get_error_string(errno, error_string, sizeof(error_string)); _pam_log(LOG_ERR, "error reading RADIUS packet from server %s: %s", - server->hostname, strerror(errno)); + server->hostname, error_string); ok = FALSE; break; -- cgit v1.2.3 From 8cadbd70f4e8323f7e2901c774d5206cd456643f Mon Sep 17 00:00:00 2001 From: Samuel Varley Date: Thu, 26 Nov 2015 11:04:25 +1300 Subject: Thread safety: Store the name of conf file in radius_conf_t. I needed to move the position of the structures so I could use the CONST macro with the new member. --- src/pam_radius_auth.c | 19 +++++--------- src/pam_radius_auth.h | 70 ++++++++++++++++++++++++++------------------------- 2 files changed, 42 insertions(+), 47 deletions(-) diff --git a/src/pam_radius_auth.c b/src/pam_radius_auth.c index 29b0322..0ee145e 100644 --- a/src/pam_radius_auth.c +++ b/src/pam_radius_auth.c @@ -61,7 +61,6 @@ /* internal data */ static CONST char *pam_module_name = "pam_radius_auth"; -static char conf_file[BUFFER_SIZE]; /* configuration file */ static int opt_debug = FALSE; /* print debug info */ /* logging */ @@ -84,7 +83,7 @@ static int _pam_parse(int argc, CONST char **argv, radius_conf_t *conf) memset(conf, 0, sizeof(radius_conf_t)); /* ensure it's initialized */ - strcpy(conf_file, CONF_FILE); + conf->conf_file = CONF_FILE; /* set the default prompt */ snprintf(conf->prompt, MAXPROMPT, "%s: ", DEFAULT_PROMPT); @@ -101,13 +100,7 @@ static int _pam_parse(int argc, CONST char **argv, radius_conf_t *conf) /* generic options */ if (!strncmp(*argv,"conf=",5)) { - /* protect against buffer overflow */ - if (strlen(*argv+5) >= sizeof(conf_file)) { - _pam_log(LOG_ERR, "conf= argument too long"); - conf_file[0] = 0; - return 0; - } - strcpy(conf_file,*argv+5); + conf->conf_file = *argv+5; } else if (!strcmp(*argv, "use_first_pass")) { ctrl |= PAM_USE_FIRST_PASS; @@ -574,11 +567,11 @@ static int initialize(radius_conf_t *conf, int accounting) char src_ip[MAX_IP_LEN]; /* the first time around, read the configuration file */ - if ((fserver = fopen (conf_file, "r")) == (FILE*)NULL) { + if ((fserver = fopen (conf->conf_file, "r")) == (FILE*)NULL) { char error_string[BUFFER_SIZE]; get_error_string(errno, error_string, sizeof(error_string)); _pam_log(LOG_ERR, "Could not open configuration file %s: %s\n", - conf_file, error_string); + conf->conf_file, error_string); return PAM_ABORT; } @@ -604,7 +597,7 @@ static int initialize(radius_conf_t *conf, int accounting) src_ip[0] = 0; if (sscanf(p, "%s %s %d %s", hostname, secret, &timeout, src_ip) < 2) { _pam_log(LOG_ERR, "ERROR reading %s, line %d: Could not read hostname or secret\n", - conf_file, line); + conf->conf_file, line); continue; /* invalid line */ } else { /* read it in and save the data */ radius_server_t *tmp; @@ -636,7 +629,7 @@ static int initialize(radius_conf_t *conf, int accounting) if (!server) { /* no server found, die a horrible death */ _pam_log(LOG_ERR, "No RADIUS server found in configuration file %s\n", - conf_file); + conf->conf_file); return PAM_AUTHINFO_UNAVAIL; } diff --git a/src/pam_radius_auth.h b/src/pam_radius_auth.h index defec5c..0882c53 100644 --- a/src/pam_radius_auth.h +++ b/src/pam_radius_auth.h @@ -47,40 +47,6 @@ #define MAXPROMPT 33 /* max prompt length, including '\0' */ #define DEFAULT_PROMPT "Password" /* default prompt, without the ': ' */ -/************************************************************************* - * Additional RADIUS definitions - *************************************************************************/ - -/* Per-attribute structure */ -typedef struct attribute_t { - unsigned char attribute; - unsigned char length; - unsigned char data[1]; -} attribute_t; - -typedef struct radius_server_t { - struct radius_server_t *next; - struct in_addr ip; - uint16_t port; - char *hostname; - char *secret; - int timeout; - int accounting; -} radius_server_t; - -typedef struct radius_conf_t { - radius_server_t *server; - int retries; - int localifdown; - char *client_id; - int accounting_bug; - int force_prompt; - int max_challenge; - int sockfd; - int debug; - char prompt[MAXPROMPT]; -} radius_conf_t; - /************************************************************************* * Platform specific defines @@ -146,4 +112,40 @@ typedef struct radius_conf_t { #define TRUE !FALSE #endif + +/************************************************************************* + * Additional RADIUS definitions + *************************************************************************/ + +/* Per-attribute structure */ +typedef struct attribute_t { + unsigned char attribute; + unsigned char length; + unsigned char data[1]; +} attribute_t; + +typedef struct radius_server_t { + struct radius_server_t *next; + struct in_addr ip; + uint16_t port; + char *hostname; + char *secret; + int timeout; + int accounting; +} radius_server_t; + +typedef struct radius_conf_t { + radius_server_t *server; + int retries; + int localifdown; + char *client_id; + int accounting_bug; + int force_prompt; + int max_challenge; + int sockfd; + int debug; + CONST char *conf_file; + char prompt[MAXPROMPT]; +} radius_conf_t; + #endif /* PAM_RADIUS_H */ -- cgit v1.2.3 From 306401ef5fdbaa4b623e7947cc83df680a783964 Mon Sep 17 00:00:00 2001 From: Samuel Varley Date: Thu, 26 Nov 2015 18:13:02 +1300 Subject: Thread safety: Control debug with local variable. --- src/pam_radius_auth.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/pam_radius_auth.c b/src/pam_radius_auth.c index 0ee145e..8350eb1 100644 --- a/src/pam_radius_auth.c +++ b/src/pam_radius_auth.c @@ -57,11 +57,10 @@ #include "pam_radius_auth.h" -#define DPRINT if (opt_debug & PAM_DEBUG_ARG) _pam_log +#define DPRINT if (debug) _pam_log /* internal data */ static CONST char *pam_module_name = "pam_radius_auth"; -static int opt_debug = FALSE; /* print debug info */ /* logging */ static void _pam_log(int err, CONST char *format, ...) @@ -131,8 +130,7 @@ static int _pam_parse(int argc, CONST char **argv, radius_conf_t *conf) } else if (!strcmp(*argv, "debug")) { ctrl |= PAM_DEBUG_ARG; - conf->debug = 1; - opt_debug = TRUE; + conf->debug = TRUE; } else if (!strncmp(*argv, "prompt=", 7)) { if (!strncmp(conf->prompt, (char*)*argv+7, MAXPROMPT)) { @@ -244,7 +242,7 @@ static uint16_t get_udp_port(char *service) { /* * take server->hostname, and convert it to server->ip and server->port */ -static int host2server(radius_server_t *server) +static int host2server(int debug, radius_server_t *server) { char *p; @@ -763,7 +761,7 @@ static int talk_radius(radius_conf_t *conf, AUTH_HDR *request, AUTH_HDR *respons memset(response, 0, sizeof(AUTH_HDR)); /* only look up IP information as necessary */ - if ((retval = host2server(server)) != PAM_SUCCESS) { + if ((retval = host2server(conf->debug, server)) != PAM_SUCCESS) { _pam_log(LOG_ERR, "Failed looking up IP address for RADIUS server %s (errcode=%d)", server->hostname, retval); @@ -1036,6 +1034,7 @@ PAM_EXTERN int pam_sm_authenticate(pam_handle_t *pamh,int flags,int argc,CONST c CONST char *rhost; char *resp2challenge = NULL; int ctrl; + int debug; int retval = PAM_AUTH_ERR; int num_challenge = 0; @@ -1046,6 +1045,7 @@ PAM_EXTERN int pam_sm_authenticate(pam_handle_t *pamh,int flags,int argc,CONST c radius_conf_t config; ctrl = _pam_parse(argc, argv, &config); + debug = config.debug; /* grab the user name */ retval = pam_get_user(pamh, &user, NULL); -- cgit v1.2.3