Empty string is verified as correct due to Integer coercion

[mkdynamic]

Run this to illustrate what happens:

require 'rotp'

loop do
  totp = ROTP::TOTP.new(ROTP::Base32.random_base32, digits: 4)
  raise totp.now.inspect if totp.verify("")
end

I cannot think of why this would pose any security risk, but it was surprising initially. Although now I understand why it behaves like this after scoping the source, it still seems odd.

Thoughts?

[mdp]

So this comes down to the fact that ROTP allows for integers to be submitted and verified.

In this loop, with a 4 digit OTP, there's a 1:1000 chance that the OTP will be "0000", at which point, "".to_i yields 0 and is compared to the OTP.

This is not ideal. Although I don't think it's a serious security risk for most users, it does increase the number of "right" OTP's for a user. For example, "052189", also verifies with "52189", so now you have two "correct" answers. Realistically, it should default to requiring a padded string to verify, and if developers want to verify numbers, they'll have to do that themselves. I'd rather default to a more secure method.

This will be out in 1.7.0 today.

[mdp]

Thanks Mark, this is out in 1.7.0

[mkdynamic]

Cool, thanks @mdp

[rkh]

Breaking the public API in a minor release. I'd like to voice that I'm very displeased with this. This change could have been introduced without breaking the API.

[mdp]

Totally valid. I even looked at is as breaking API change in the git commit log and still only bumped it a minor level.

I've yanked 1.7.0 - The latest is in 2.0.0

I'm hoping this was quick enough to avoid any other people having issues. If you've already made the changes, just bump the version to 2.0.0 and you'll be good to go. Otherwise lock in at 1.6.1

Sorry for trouble this caused.

Edit: just bumped to 2.0.0 and yanked 1.7.0

[mdp]

Update to my comment above. I'm just yanking the 1.7.x branch entirely. Lock to 1.6.x for the old API. Bump to 2.0.0 for the latest.

[rkh]

Thanks a lot. <3