bin/62885: pam_radius doesn't maintain multiple state fields
Phil Pennock
pdp+freebsd at nl.demon.net
Sun Feb 15 12:20:05 PST 2004
>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:
More information about the freebsd-bugs
mailing list