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

[Ilkka Virta]

[Not sure why I was in Cc, but ok, I'll comment on this.]

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.

---

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?

---

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).


--
Ilkka Virta - itvirta at iki.fi