marcelkliemannel / kotlin-onetimepassword

A Kotlin one-time password library to generate "Google Authenticator", "Time-based One-time Password" (TOTP) and "HMAC-based One-time Password" (HOTP) codes based on RFC 4226 and 6238.
Apache License 2.0
154 stars 25 forks source link

Time Based OTP counter calculation - a bug maybe #5

Closed coud closed 4 years ago

coud commented 4 years ago

I'm having a weird result when validating the code that has not been expired yet. Sometime it's true and sometime it's false. So I dig into it and I think that there is a bug in counter calculation.

For example, a code that will be valid for 3 seconds or 3000 milliseconds:

The first timestamp that generates the code is 1593777274247 is divided into 3000 milliseconds which is 531259091 and used as counter.

Now 2005 milliseconds passed and the timestamp is 1593777276252 and then divided to 3000 milliseconds which is 531259092 and used as counter.

The result of validation will be invalid because the counters are different.

Another example, suppose we use seconds instead of milliseconds and a very small number: Validity - 7 seconds

Starting timestamp - 10 So code is valid until 17 Counter = (10 / 7 = 1)

4 seconds passed and the timestamp now is 14 Counter = (14 / 7 = 2)

The resulting counters are different but the time does not yet passed the validity period.

marcelkliemannel commented 4 years ago

Hi,

thanks for taking the time to investigate the problem and create an issue.

The problem was to wrongly use truncated division for calculating how many times the time step fits into the timestamp. This can sometimes lead to an incorrect counter value due to a rounding error. The solution is to use floored division, as required by the RFC 6238.

I published a new version 2.0.1, which will be available at Maven Central shortly.

Kind regards, Marcel

coud commented 4 years ago

Thank you for taking time to look about this. I realized that this is not really a bug but just my misunderstanding about the algorithm. What you did is already correct since integer division is same as floor and your update does not made a change. I looked at python version of the rfc (pyotp) and it's the same formula. Now I understand what a time window or time step is. It's just my first impression is just like jwt's expiration time.

marcelkliemannel commented 4 years ago

The old code behaves wrong for negative timestamp values. For a timestamp -1234567and a time step 30000 the counter value would be -41 with integer division, but -42 with floor division. This is because integer division only behaves like floor division for a positive values. For negative values integer division round towards zero and floor division in the other direction to negative infinity.

Whereby this is probably rather a theoretical error case, which does not occur in practice...

ndawar94 commented 3 years ago

@coud Vide your first comment, I had the exact same issue and did the exact same digging and figured the same thing, that there's an issue w/ the way counters are calculated. But, in your last comment you did concur that it was your misunderstanding. Would be great if you could explain to me how you arrived at the conclusion that the counter calculation is actually correct?

coud commented 3 years ago

@ndawar94 If you look at python's pyotp, it has the same behavior. If you tried one of the otp generators app on mobile such as google authenticator, you'll see that the code is generated each interval and this interval timing is the same for all of the otp generators that follows the rfc.

ndawar94 commented 3 years ago

@coud Thanks for the prompt reply. So, that means if I'm not relying on an authentication app and instead use sms/email to send the otp to the user, there's no way for me to pinpoint the exact validity of a TOTP token. E.g. if the step is defined as 30 secs and window as 2, the validity of a token might not always be exact 60 secs. It could even be less than that in some instances which can be attributed to the way counter is being calculated. Is my understanding correct?

coud commented 3 years ago

@ndawar94 Yes that's correct.