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