Date: Sun, 15 Feb 2004 20:11:42 +0000 From: Phil Pennock <pdp+freebsd@nl.demon.net> To: FreeBSD-gnats-submit@FreeBSD.org Subject: bin/62885: pam_radius doesn't maintain multiple state fields Message-ID: <E1AsScA-0007BP-Dv@scimitar.noc.nl.demon.net> Resent-Message-ID: <200402152020.i1FKK5qY042494@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
>Number: 62885 >Category: bin >Synopsis: pam_radius doesn't maintain multiple state fields >Confidential: no >Severity: non-critical >Priority: low >Responsible: freebsd-bugs >State: open >Quarter: >Keywords: >Date-Required: >Class: sw-bug >Submitter-Id: current-users >Arrival-Date: Sun Feb 15 12:20:05 PST 2004 >Closed-Date: >Last-Modified: >Originator: Phil Pennock >Release: FreeBSD 4.7-RELEASE-p25 and 5.2-RELEASE-p2 i386 >Organization: Demon Internet Netherlands >Environment: FreeBSD/x86 both 4.x and 5.x Problem present in, and patch for: src/lib/libpam/modules/pam_radius/pam_radius.c,v 1.2.6.1 Problem still present in: src/lib/libpam/modules/pam_radius/pam_radius.c,v 1.19 >Description: RADIUS packets can include multiple state fields, which are an order-dependent collection of state information. All state fields should be maintained. The current pam_radius implementation supports just one state field. This is a bug. There's also no support for an initial state field or for a usercode suffix; the patch below fixes this too, as a feature-request level enhancement rather than a bug-fix. >How-To-Repeat: Use an authentication system which uses challenge/response over RADIUS with multiple state fields. Probably difficult to find if you don't have such an environment set up already. >Fix: This patch supports: * up to 10 state fields which need to be maintained * an initial state field which should be set with the first packet * a usercode suffix to be added to the user string (eg. "@domain" or ".domain.suffix") This patch adds one one-shot memory-leak, where the usercode has the user_suffix added. I'm not familiar enough with the PAM programming environment to know what is a safe, reliable and correct fix for this. Sorry. --- pam_radius.c.pre-pdp Sun Feb 15 19:19:38 2004 +++ pam_radius.c Sun Feb 15 20:50:35 2004 @@ -53,20 +53,30 @@ enum { PAM_OPT_CONF = PAM_OPT_STD_MAX, - PAM_OPT_TEMPLATE_USER + PAM_OPT_TEMPLATE_USER, + PAM_OPT_USER_SUFFIX, + PAM_OPT_INITIAL_STATE, }; static struct opttab other_options[] = { { "conf", PAM_OPT_CONF }, { "template_user", PAM_OPT_TEMPLATE_USER }, + { "user_suffix", PAM_OPT_USER_SUFFIX }, + { "initial_state", PAM_OPT_INITIAL_STATE }, { NULL, 0 } }; +struct state_items { + void *item; + size_t len; +}; + +#define MAX_STATE_ITEMS 10 #define MAX_CHALLENGE_MSGS 10 #define PASSWORD_PROMPT "RADIUS Password:" static int build_access_request(struct rad_handle *, const char *, - const char *, const void *, size_t); + const char *, const struct state_items *); static int do_accept(pam_handle_t *, struct rad_handle *); static int do_challenge(pam_handle_t *, struct rad_handle *, const char *); @@ -77,9 +87,10 @@ */ static int build_access_request(struct rad_handle *radh, const char *user, - const char *pass, const void *state, size_t state_len) + const char *pass, const struct state_items *states) { char host[MAXHOSTNAMELEN]; + int state_index = 0; if (rad_create_request(radh, RAD_ACCESS_REQUEST) == -1) { syslog(LOG_CRIT, "rad_create_request: %s", rad_strerror(radh)); @@ -94,10 +105,17 @@ syslog(LOG_CRIT, "rad_put_string: %s", rad_strerror(radh)); return (-1); } - if (state != NULL && rad_put_attr(radh, RAD_STATE, state, - state_len) == -1) { - syslog(LOG_CRIT, "rad_put_attr: %s", rad_strerror(radh)); - return (-1); + if (states != NULL) { + while (states[state_index].item != NULL) { + if (rad_put_attr(radh, RAD_STATE, + states[state_index].item, + states[state_index].len) == -1) { + syslog(LOG_CRIT, "rad_put_attr: %s", + rad_strerror(radh)); + return (-1); + } + ++state_index; + } } if (rad_put_int(radh, RAD_SERVICE_TYPE, RAD_AUTHENTICATE_ONLY) == -1) { syslog(LOG_CRIT, "rad_put_int: %s", rad_strerror(radh)); @@ -115,7 +133,8 @@ char *s; while ((attrtype = rad_get_attr(radh, &attrval, &attrlen)) > 0) { - if (attrtype == RAD_USER_NAME) { + switch (attrtype) { + case RAD_USER_NAME: s = rad_cvt_string(attrval, attrlen); if (s == NULL) { syslog(LOG_CRIT, @@ -124,6 +143,7 @@ } pam_set_item(pamh, PAM_USER, s); free(s); + break; } } if (attrtype == -1) { @@ -140,8 +160,8 @@ int attrtype; const void *attrval; size_t attrlen; - const void *state; - size_t statelen; + struct state_items states[MAX_STATE_ITEMS+1]; + int num_states; struct pam_message msgs[MAX_CHALLENGE_MSGS]; const struct pam_message *msg_ptrs[MAX_CHALLENGE_MSGS]; struct pam_response *resp; @@ -149,15 +169,21 @@ const void *item; const struct pam_conv *conv; - state = NULL; - statelen = 0; + memset(states, 0, sizeof(states)); + num_states = 0; num_msgs = 0; while ((attrtype = rad_get_attr(radh, &attrval, &attrlen)) > 0) { switch (attrtype) { case RAD_STATE: - state = attrval; - statelen = attrlen; + if (num_states >= MAX_STATE_ITEMS) { + syslog(LOG_ERR, + "Too many RADIUS state fields in response"); + return (PAM_SERVICE_ERR); + } + states[num_states].item = (void *)attrval; + states[num_states].len = (size_t)attrlen; + ++num_states; break; case RAD_REPLY_MESSAGE: @@ -201,8 +227,7 @@ if ((retval = conv->conv(num_msgs, msg_ptrs, &resp, conv->appdata_ptr)) != PAM_SUCCESS) return (retval); - if (build_access_request(radh, user, resp[num_msgs-1].resp, state, - statelen) == -1) + if (build_access_request(radh, user, resp[num_msgs-1].resp, states) == -1) return (PAM_SERVICE_ERR); memset(resp[num_msgs-1].resp, 0, strlen(resp[num_msgs-1].resp)); free(resp[num_msgs-1].resp); @@ -218,8 +243,9 @@ { struct options options; struct rad_handle *radh; + struct state_items *states; const char *user, *tmpuser, *pass; - char *conf_file, *template_user; + char *conf_file, *template_user, *user_suffix, *initial_state; int retval; int e; @@ -231,6 +257,20 @@ pam_test_option(&options, PAM_OPT_CONF, &conf_file); template_user = NULL; pam_test_option(&options, PAM_OPT_TEMPLATE_USER, &template_user); + user_suffix = NULL; + pam_test_option(&options, PAM_OPT_USER_SUFFIX, &user_suffix); + initial_state = NULL; + pam_test_option(&options, PAM_OPT_INITIAL_STATE, &initial_state); + + if (initial_state) { + states = calloc(2, sizeof(struct state_items)); + states[0].item = initial_state; + states[0].len = strlen(initial_state); + states[1].item = NULL; + states[1].len = 0; + } else { + states = NULL; + } retval = pam_get_user(pamh, &user, NULL); if (retval != PAM_SUCCESS) @@ -238,6 +278,21 @@ PAM_LOG("Got user: %s", user); + /* slight memory leak; one-shot, minimal and tolerable */ + if (user_suffix && strlen(user_suffix)) { + size_t sz; + char *p; + + sz = strlen(user) + strlen(user_suffix) + 1; + p = malloc(sz); + if (!p) { + syslog(LOG_CRIT, "malloc failed: %m"); + return (PAM_SERVICE_ERR); + } + sprintf(p, "%s%s", user, user_suffix); + user = p; + } + retval = pam_get_pass(pamh, &pass, PASSWORD_PROMPT, &options); if (retval != PAM_SUCCESS) return (retval); @@ -260,7 +315,7 @@ PAM_LOG("Radius config file read"); - if (build_access_request(radh, user, pass, NULL, 0) == -1) { + if (build_access_request(radh, user, pass, states) == -1) { rad_close(radh); return (PAM_SERVICE_ERR); } @@ -305,6 +360,7 @@ return (PAM_AUTH_ERR); case RAD_ACCESS_CHALLENGE: + PAM_LOG("Have RAD_ACCESS_CHALLENGE packet"); retval = do_challenge(pamh, radh, user); if (retval != PAM_SUCCESS) { rad_close(radh); >Release-Note: >Audit-Trail: >Unformatted:
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?E1AsScA-0007BP-Dv>