Hi, I posted a patch to bugzilla (http://bugs.freeradius.org/show_bug.cgi?id=296) which adds kerberos hardware preauthentication support to rlm_krb5 when using MIT krb5 libs. Also pasted below. Any feedback from people involved with rlm_krb5 or other modules which use challenge-response would be appreciated. This patch takes advantage of the krb5 libs (and hence rlm_krb5) being thread-unsafe already, but could be changed if that bothers people. --ben --- src/modules/rlm_krb5/rlm_krb5.c.dist 2005-11-18 18:06:03.000000000 -0500 +++ src/modules/rlm_krb5/rlm_krb5.c 2005-11-18 21:44:49.000000000 -0500 @@ -20,6 +20,7 @@ * Copyright 2000 The FreeRADIUS server project * Copyright 2000 Nathan Neulinger <nneul@umr.edu> * Copyright 2000 Alan DeKok <aland@ox.org> + * Copyright 2005 Benjamin Bennett <ben@psc.edu> */ @@ -39,6 +40,8 @@ #include <krb5.h> #include <com_err.h> +REQUEST *global_request; + typedef struct rlm_krb5_t { const char *keytab; const char *service_princ; @@ -205,12 +208,83 @@ return 0; } +#ifndef HEIMDAL_KRB5 +static krb5_error_code KRB5_CALLCONV +krb5_radius_prompter(krb5_context context, void *data, const char *name, + const char *banner, int num_prompts, + krb5_prompt prompts[]) +{ + VALUE_PAIR *state; + VALUE_PAIR *reply; + char *challenge; + char *newstate; + int clen; + int r; + + /* Squelch unused param warnings */ + context = context; + data = data; + + if(num_prompts != 1) { + radlog(L_DBG, "rlm_krb5: [%s] krb5_radius_prompter failed: num_prompts (%d) != 1", global_request->username->strvalue, num_prompts); + global_request->reply->code = PW_AUTHENTICATION_REJECT; + return KRB5_LIBOS_CANTREADPWD; + } + + state = pairfind(global_request->packet->vps, PW_STATE); + + if(!state) { /* NEW Request */ + radlog(L_DBG, "rlm_krb5: [%s] krb5_radius_prompter: additional token required", global_request->username->strvalue); + + /* Setup challenge prompt */ + clen = strlen(name)+strlen(banner)+strlen(prompts[0].prompt)+5; + challenge = rad_malloc(clen); + snprintf(challenge, clen, "%s\n%s\n%s: ", name, banner, + prompts[0].prompt); + + /* Re-encode User-Password to send back */ + rad_pwencode((char *)global_request->password->strvalue, + &global_request->password->length, + global_request->secret, + (char *)global_request->packet->vector); + + /* Setup State with current Authenticator and User-Password */ + newstate = rad_malloc(AUTH_VECTOR_LEN+ + global_request->password->length+1); + memcpy(newstate, global_request->packet->vector, AUTH_VECTOR_LEN); + memcpy(newstate+AUTH_VECTOR_LEN, global_request->password->strvalue, + global_request->password->length+1); + + /* Put State into a challenge reply */ + reply = pairmake("Reply-Message", challenge, T_OP_EQ); + pairadd(&global_request->reply->vps, reply); + state = pairmake("State", newstate, T_OP_EQ); + pairadd(&global_request->reply->vps, state); + global_request->reply->code = PW_ACCESS_CHALLENGE; + free(challenge); + free(newstate); + r = KRB5_LIBOS_CANTREADPWD; + } else { /* Challenge Response */ + radlog(L_DBG, "rlm_krb5: [%s] krb5_radius_prompter: using additional token", global_request->username->strvalue); + + strncpy(prompts[0].reply->data, + (char *)global_request->password->strvalue, + prompts[0].reply->length); + prompts[0].reply->data[(prompts[0].reply->length)-1] = '\0'; + prompts[0].reply->length = strlen(prompts[0].reply->data); + r = 0; + } + return r; +} +#endif + /* validate userid/passwd */ /* MIT case */ #ifndef HEIMDAL_KRB5 static int krb5_auth(void *instance, REQUEST *request) { int r; + VALUE_PAIR *state; krb5_data tgtname = { 0, @@ -218,11 +292,13 @@ KRB5_TGS_NAME }; krb5_creds kcreds; + krb5_get_init_creds_opt opts; krb5_ccache ccache; char cache_name[L_tmpnam + 8]; krb5_context context = *((rlm_krb5_t *)instance)->context; /* copy data */ - const char *user, *pass; + const char *user; + char *pass; /* * We can only authenticate user requests which HAVE @@ -271,6 +347,8 @@ /* * Actually perform the authentication */ + krb5_get_init_creds_opt_init(&opts); + memset((char *)&kcreds, 0, sizeof(kcreds)); if ( (r = krb5_parse_name(context, user, &kcreds.client)) ) { @@ -302,22 +380,40 @@ return RLM_MODULE_REJECT; } - if ( (r = krb5_get_in_tkt_with_password(context, - 0, NULL, NULL, NULL, pass, ccache, &kcreds, 0)) ) { - radlog(L_AUTH, "rlm_krb5: [%s] krb5_g_i_t_w_p failed: %s", - user, error_message(r)); - krb5_free_cred_contents(context, &kcreds); - krb5_cc_destroy(context, ccache); - return RLM_MODULE_REJECT; - } else { + global_request = request; + state = pairfind(request->packet->vps, PW_STATE); + if(state) { + radlog(L_DBG, "rlm_krb5: [%s] challenge response", user); + if(state->length <= AUTH_VECTOR_LEN) { + radlog(L_AUTH, "rlm_krb5: [%s] BAD State data", user); + return RLM_MODULE_REJECT; + } + pass = (char *)state->strvalue+AUTH_VECTOR_LEN; + rad_pwdecode(pass, state->length-AUTH_VECTOR_LEN, + request->secret, (char *)state->strvalue); + } + + if(!(r = krb5_get_init_creds_password(context, &kcreds, + kcreds.client, pass, krb5_radius_prompter, NULL, + (krb5_deltat)0, NULL, &opts))) { + if( (r = krb5_cc_store_cred(context, ccache, &kcreds)) ) { + radlog(L_AUTH, "rlm_krb5: [%s] krb5_cc_store_cred failed: %s", + user, error_message(r)); + return RLM_MODULE_REJECT; + } /* Now verify the KDC's identity. */ r = verify_krb5_tgt(context, (rlm_krb5_t *)instance, user, ccache); - krb5_free_cred_contents(context, &kcreds); - krb5_cc_destroy(context, ccache); - return r; + } else if(r == KRB5_LIBOS_CANTREADPWD) { + r = RLM_MODULE_HANDLED; + } else { + radlog(L_AUTH, "rlm_krb5: [%s] krb5_g_i_c_p failed: %s", user, + error_message(r)); + r = RLM_MODULE_REJECT; } - return RLM_MODULE_REJECT; + krb5_free_cred_contents(context, &kcreds); + krb5_cc_destroy(context, ccache); + return r; } #else /* HEIMDAL_KRB5 */
On November 20, 2005 2:37:44 AM -0500 Benjamin Bennett <ben@psc.edu> wrote:
Hi,
I posted a patch to bugzilla (http://bugs.freeradius.org/show_bug.cgi?id=296) which adds kerberos hardware preauthentication support to rlm_krb5 when using MIT krb5 libs. Also pasted below.
Any feedback from people involved with rlm_krb5 or other modules which use challenge-response would be appreciated. This patch takes advantage of the krb5 libs (and hence rlm_krb5) being thread-unsafe already, but could be changed if that bothers people.
Yes, very bothersome. Steady progress has been made on making krb5 libs thread-safe (and on Solaris they will be thread-safe when they are exposed). Use the data arg to the prompter, that's what it's there for, and it's trivial to use. Fix the style -- line length (there's a radlog() call in the prompter that needs to have the last arg on a new line; if (foo) not if(foo); etc). There's other stuff, but the existing code doesn't have the greatest style to begin with so I won't belabor it. The part where you copy the password into prompts[0].reply looks broken. Maybe I'm wrong; it's been a long time since I've written krb5 code. Does the library allocate reply->data for you? Looks that way. In which case your copy of the password is incorrect, you are copying as much space as is allocated in the reply, which could be greater (or less) than the length of the password. If reply->length is greater than strlen(password->strvalue), you'll run off of the password buffer and possibly segfault. If lesser, you'll truncate the password. Null termination of reply->data seems incorrect as well. In the new request case, you should set reply->code just before you set r, for readability (move the 2 free() statements up). You should have a lot more comments, eg, why 'pass' and password get switched up. More important, why you save the password in the State AVP. I guess it's because you fail the krb5 part of the auth because it's too difficult to issue the challenge to the user via RADIUS while staying inside the prompte function. Which brings me to my last point, the state isn't adequately protected. If I can sit between the NAS and the RADIUS server, and insert packets, I can authenticate as another user by waiting for him to authenticate and then stealing the State AVP. Now, I login to the NAS as that user, get a challenge from krb5 via RADIUS and when the NAS sends the response back, I remove the state and insert the one I stole (from long ago). You simply decode the state and accept it as valid. This allows me to login as a user and only have to guess the SAD, without having to know the long term password. Since the SAD data can be trivial to guess, (e.g. it is commonly a 6 digit decimal number without any PIN), if the krb5 server (or at least this specific user) operates in the modes where the password is used along with the SAD (say, because the SAD is exceptionally weak), then you've significantly broken things. If the use-sad-as-key mode is used, well then you're fine, but since rlm_krb5 should be usable for any configuration, it should be fixed. Oh, also you'll want to note that the failing of the krb5 auth in order to escape the prompter function alters the kdc logs. There will always be at least 2 auth requests for a single TGT return. -frank
On Wed, 2005-11-23 at 21:33 -0800, Frank Cusack wrote:
Yes, very bothersome. Steady progress has been made on making krb5 libs thread-safe (and on Solaris they will be thread-safe when they are exposed). Use the data arg to the prompter, that's what it's there for, and it's trivial to use.
doh! forgot about that little bugger, will do.
Fix the style -- line length (there's a radlog() call in the prompter that needs to have the last arg on a new line; if (foo) not if(foo); etc). There's other stuff, but the existing code doesn't have the greatest style to begin with so I won't belabor it.
heh, I think there's half the problem, I was trying to follow the style of rlm_krb5. I'll add some spaces and newlines.
The part where you copy the password into prompts[0].reply looks broken. Maybe I'm wrong; it's been a long time since I've written krb5 code. Does the library allocate reply->data for you? Looks that way. In which case your copy of the password is incorrect, you are copying as much space as is allocated in the reply, which could be greater (or less) than the length of the password. If reply->length is greater than strlen(password->strvalue), you'll run off of the password buffer and possibly segfault. If lesser, you'll truncate the password.
reply->data is a buffer on pa_sam_2's stack (at least that's where it is in krb5-1.4.2). reply->length is set to the sizeof that buffer before calling the prompter function. I'm copying until the null at the end of user-password, up to a max of reply->length, to prevent running off the buffer.
Null termination of reply->data seems incorrect as well.
That null termination ensures that we return with a null terminated string in reply->data. Usually it would already be null terminated, except if we hit reply->length (100 bytes in krb5-1.4.2) before the end of user-password.
In the new request case, you should set reply->code just before you set r, for readability (move the 2 free() statements up).
will do.
You should have a lot more comments, eg, why 'pass' and password get switched up. More important, why you save the password in the State AVP.
will do.
Which brings me to my last point, the state isn't adequately protected. If I can sit between the NAS and the RADIUS server, and insert packets, I can authenticate as another user by waiting for him to authenticate and then stealing the State AVP. Now, I login to the NAS as that user, get a challenge from krb5 via RADIUS and when the NAS sends the response back, I remove the state and insert the one I stole (from long ago). You simply decode the state and accept it as valid. This allows me to login as a user and only have to guess the SAD, without having to know the long term password. Since the SAD data can be trivial to guess, (e.g. it is commonly a 6 digit decimal number without any PIN), if the krb5 server (or at least this specific user) operates in the modes where the password is used along with the SAD (say, because the SAD is exceptionally weak), then you've significantly broken things. If the use-sad-as-key mode is used, well then you're fine, but since rlm_krb5 should be usable for any configuration, it should be fixed.
suggestions? I'm working with a 4-bytes of SAD w/8 digit pin so I'll admit that I didn't think about this too much. Are people still using those little SecurID thingies? ;-)
Oh, also you'll want to note that the failing of the krb5 auth in order to escape the prompter function alters the kdc logs. There will always be at least 2 auth requests for a single TGT return.
There are 2 kdc requests under normal circumstances, such as kinit with posix prompter. On the first request the kdc returns hw preauth required, which causes the prompter function to be called. If the prompter function returns success, a second request will use hw preauth. Since the prompter function returns error on initial radius request, we end up with a total of 3 kdc requests. The only ways I can think to work around this would require adding state of these requests to radiusd, which seems like a generally bad idea, or explicitly telling radiusd which principals require hw auth, which seems like an admin nightmare. Other ideas? thanks for your suggestions! --ben
Benjamin Bennett wrote:
Null termination of reply->data seems incorrect as well.
That null termination ensures that we return with a null terminated string in reply->data. Usually it would already be null terminated, except if we hit reply->length (100 bytes in krb5-1.4.2) before the end of user-password.
I'd suggest to use the functions strlcpy and strlcat in the new code. If they aren't available on the system, we have replacement functions in the libradius in CVS head. http://www.usenix.org/events/usenix99/full_papers/millert/millert_html/index... -- Nicolas Baradakis
On November 24, 2005 2:54:43 AM -0500 Benjamin Bennett <ben@psc.edu> wrote:
On Wed, 2005-11-23 at 21:33 -0800, Frank Cusack wrote:
The part where you copy the password into prompts[0].reply looks broken. Maybe I'm wrong; it's been a long time since I've written krb5 code. Does the library allocate reply->data for you? Looks that way. In which case your copy of the password is incorrect, you are copying as much space as is allocated in the reply, which could be greater (or less) than the length of the password. If reply->length is greater than strlen(password->strvalue), you'll run off of the password buffer and possibly segfault. If lesser, you'll truncate the password.
reply->data is a buffer on pa_sam_2's stack (at least that's where it is in krb5-1.4.2). reply->length is set to the sizeof that buffer before calling the prompter function.
I'm copying until the null at the end of user-password, up to a max of reply->length, to prevent running off the buffer.
Yeah, you're right, sorry about that. I either had a brain fart and thought you'd copy until reply->length, regardless of the length of passwd->strvalue, or I thought password->strvalue might not be null terminated. Not sure which, but anyway, my mistake.
Null termination of reply->data seems incorrect as well.
That null termination ensures that we return with a null terminated string in reply->data. Usually it would already be null terminated, except if we hit reply->length (100 bytes in krb5-1.4.2) before the end of user-password.
OK, I just wasn't sure whether or not krb5 libs expect data to be null terminated or not. That always confused me (since there is an explicit length). Someone (Joel of Joel On Software, I think) calls these fucked strings -- combination of pascal and c strings. Which reminds me, if there is a passwd->length to go along with passwd->strvalue, you should use that rather than running down the string again with strlen(). You'll have to adjust for whether or not it includes null termination, of course. I also second Nicolas' suggestion of using strlcpy().
Which brings me to my last point, the state isn't adequately protected. ...
suggestions?
See src/modules/rlm_otp/otp_radstate.c. I HMAC the State with a key generated at FR startup time. The State includes the time, and I verify that the time the State is received is sufficiently close to the time the State was sent. This limits State replay to that time interval, which isn't perfect but for my use it was good enough. The HMAC is required to verify the integrity of the time data. I'd say you need at least that level of protection. You might want to go a step further and prevent replay altogether by using a monotonic counter instead of the time, and keep state (on the State, heh) of the smallest valid counter value, and the holes (invalid counter values) from smallest->current counter which are no longer valid. You only have to track state for the time interval you select, so this won't grow unbounded. I suggest noting the holes rather than having a list of all valid values, but anyway you need some way to track out-of-order replies.
I'm working with a 4-bytes of SAD w/8 digit pin so I'll admit that I didn't think about this too much. Are people still using those little SecurID thingies? ;-)
Interesting, is that actually a hardware device? I'd be interested to know about your implementation, maybe off-list.
Oh, also you'll want to note that the failing of the krb5 auth in order to escape the prompter function alters the kdc logs. There will always be at least 2 auth requests for a single TGT return.
There are 2 kdc requests under normal circumstances, such as kinit with posix prompter. On the first request the kdc returns hw preauth required, which causes the prompter function to be called. If the prompter function returns success, a second request will use hw preauth.
Since the prompter function returns error on initial radius request, we end up with a total of 3 kdc requests.
The only ways I can think to work around this would require adding state of these requests to radiusd, which seems like a generally bad idea, or explicitly telling radiusd which principals require hw auth, which seems like an admin nightmare. Other ideas?
I suggest simply documenting it. -frank
Frank Cusack <fcusack@fcusack.com> wrote:
See src/modules/rlm_otp/otp_radstate.c. I HMAC the State with a key generated at FR startup time. The State includes the time, and I verify that the time the State is received is sufficiently close to the time the State was sent. This limits State replay to that time interval, which isn't perfect but for my use it was good enough. The HMAC is required to verify the integrity of the time data.
This code is useful enough that it should go into the server core, to avoid repetition in multiple modules (eap, otp, krb5 ...) I'll take a look at doing it. The EAP module solves this problem by changing the State attribute for every Access-Challenge, and expiring old ones. Alan DeKok.
On November 20, 2005 2:37:44 AM -0500 Benjamin Bennett <ben@psc.edu> wrote:
Any feedback from people involved with rlm_krb5 or other modules which use challenge-response would be appreciated. This patch takes advantage of the krb5 libs (and hence rlm_krb5) being thread-unsafe already, but could be changed if that bothers people.
Just an interesting note, MIT krb5-1.4.3, released 11/16, is thread-safe. -frank
participants (4)
-
Alan DeKok -
Benjamin Bennett -
Frank Cusack -
Nicolas Baradakis