rlm_krb5 hardware preauth

[Frank Cusack]

On November 24, 2005 2:54:43 AM -0500 Benjamin Bennett <ben at 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