Re: [OATH-Toolkit-help] oath-toolkit patchs related to usersfile parsing & writing

[Maxime de Roucy]

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