Hello, Thank you for your awesome answer !
Le lundi 26 janvier 2015 à 03:39 +0200, Ilkka Virta a écrit : > [Not sure why I was in Cc, but ok, I'll comment on this.] I add you to the Cc because you posted on oath-toolkit mailing list not so long ago about the usersfile… so I thought you might be interested. > On 25.1.2015 16:26, Maxime de Roucy wrote: > > 0002-different-usersfile-field-5-if-HOTP-TOTP > > ============================================= > > > > As it is mansion in the userfile google specification, field 5 is different > > if the line is related to HOTP or TOTP. > > https://code.google.com/p/mod-authn-otp/wiki/UsersFile > > > > Currently that's not the case. This patch correct this issue and use the > > 5th field value to improve the TOTP replay verification. > > That's rather a non-backward compatible change, obviously. Yes indeed > --- > > I was going to wonder if that document is even supposed to represent > this code, but apparently the link has been added to the README in git > since I last looked. (I can't see it in the web though.) > > From a documentation viewpoint, I would suggest rewriting the doc > anyway, since a) there's the failure counter field that's not used (and > the IP address too?), b) the otp len (digit count) definition in the > first field is not used either (parse_type() parses it, but it's not > used anywhere). c) MOTP? > > --- > > About the file format itself, there are some things I do find rather > confusing: > > The case you just patched here, reusing the counter field for a counter > offset is one, since really TOTP uses a counter value much in the same > way as HOTP does. The only difference is that for TOTP the counter value > is derived from the current time instead of an event counter. The > underlying algorithm is _exactly the same_. The TOTP RFC even defines > TOTP as a function of HOTP. > > > So, really, for both cases one would only need to save the last used > counter value C_last, and to accept the given otp if there is some > counter value C > C_last that matches it. > One that is, for HOTP, within the check window, i.e. C <= C_last + > window, and for TOTP, close enough to the current time: > C_now - window <= C <= C_now + window. > > There's really no need to save the timestamp for this: the timestamp + > time offset of last login gives exactly the same information as the > counter value of the last login, right? Unless the logic behind this is > completely different from what I think it is? (and see the test below.) > > > And there's really no need to save the last used otp either. > > Consider the difference between the following lines: > HOTP testi - 00000000 0 > HOTP testi - 00000000 0 328482 2015-01-26T01:43:47L > > In the current implementation, the first one accepts the otp matching > counter value zero (328482), and the second one doesn't. > > This doesn't accept it either, but the error code is different: > HOTP testi - 00000000 1 > > I do find the difference between the first two rather confusing, and > actually the doc that was linked here does say that field 5 should be > the "Next expected counter value", while what is really saved is the > previously used one. > > > And finally, saving the timestamp in local time (for anything other than > logging) is a bit questionable, since the local time may well repeat if > the timezone changes, and often does once a year in any case when coming > back from daylight saving. The algorithm uses UTC so why not save that? So you think we should completely reformat the userfile. I agree. I will try to do it when I have some time. > A quick test with the patch: > > usersfile: > HOTP/T60 testi - 00000000 > > # oathtool --totp -s 60 -N now 0000 -w 4 > 528395 > 541518 > 785090 > 052058 > 477398 > > Try with su and pam_oath, from the above codes in this order: > 528395 -> OATH_OK (just set up) > 052058 -> OATH_OK (3 mins newer, ok) > 541518 -> OATH_OK (2 mins older than prev, shouldn't this fail?) > 052058 -> OATH_OK (replay, but newer than last accepted one) > > > I did get some OATH_REPLAYED_OTP errors too in some cases when going > back in time. I can't make any sense of the code to find out what is > going on there (parse_usersfile() is ~400 lines, I counted five levels > of nesting, it's impossible to read). I found the bug : diff --git a/liboath/usersfile.c b/liboath/usersfile.c index d9bd09c..8803614 100644 --- a/liboath/usersfile.c +++ b/liboath/usersfile.c @@ -369,7 +369,7 @@ parse_usersfile (const char *username, time (NULL), totpstepsize, 0, window, &new_totp_position, otp); - if (rc == OATH_OK) + if (rc >= OATH_OK) { // the supplied OTP is valide // but it may have been already be played But as I said, I will re-implement a complete parser to simplify the userfile format and update the documentation. Maybe also a "setuid root" helper as in pam_unix : https://www.kernel.org/pub/linux/libs/pam/FAQ (Q9) I will send my patches to this mailing list when I will have some time. -- Regards Maxime de Roucy
signature.asc
Description: This is a digitally signed message part
