New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disallow authentication code once verified #30

Closed
vilda opened this Issue May 26, 2015 · 9 comments

Comments

4 participants
@vilda

vilda commented May 26, 2015

This is a suggestion for small improvement. RFC 6238 requires the implementation to disallow (MUST NOT) multiple submissions of the same authentication code. This is apparently to detect MITM and over the shoulder attacks.

@ShaneWilton

This comment has been minimized.

Member

ShaneWilton commented May 26, 2015

Thanks for pointing this out! I'll fix it when I have a chance.

@f3ndot

This comment has been minimized.

Contributor

f3ndot commented Jun 20, 2015

Submitted to ROTP gem as mdp/rotp#44 as it's their domain to comply with HOTP and TOTP specs as defined in the RFCs.

@vilda

This comment has been minimized.

vilda commented Jun 23, 2015

I'm not sure the ROTP library is a good candidate. You have a good point that it is supposed to follow RFC (and at least document clearly where it does not). However, since you have to store somewhere the last accepted code or timestamp, ROTP would need to implement access layer to a database or other storage. That seems out of scope for such simple (purely algorithm) library.

@f3ndot

This comment has been minimized.

Contributor

f3ndot commented Jun 23, 2015

@vilda I'm inclined to agree with you and the ROTP maintainer at this point. I'd hope that he'll provide extendable hooks and clear documentation in future work.

I'll probably dabble in implementing a solution in your library and submitting a PR, but I have little (read: zero) experience in working with Rails gems.

@f3ndot

This comment has been minimized.

Contributor

f3ndot commented Sep 5, 2015

Would an approach like this work?

  1. Add a column consumed_otp to the devise two-factor model table.
  2. Server, upon supplied a valid OTP, hashes a concat of the current timestep and valid OTP and compares it to the value stored in consumed_otp.
  3. If there is no match, permit login and update consumed_otp value for the row. If there is a match, deny login as it's a "burned" OTP code for that time step.

I feel like it's acceptable because:

  1. The concatenation of the time step ensures the burned code applies only for that given time step
  2. There's no master list of N-many "burned codes" since we only need to know the one code issued for a given timestep.
  3. A valid OTP is inherently unique to a given user, therefore the resulting hash is unique to a given user.
@ShaneWilton

This comment has been minimized.

Member

ShaneWilton commented Sep 5, 2015

I don't think the hashing step is necessary. Storing the timestep of the most recently successful authentication should be sufficient -- we don't care what the OTP was, we just care that a given OTP is only used once, and since each timestep has only one OTP, we can avoid needing to store the OTP entirely.

Other than that, your approach looks fine to me.

It's also important to ensure that we work with the timestep used to generate the OTP -- not the timestep associated with the request. Because of the drift period, these may not be the same.

@f3ndot

This comment has been minimized.

Contributor

f3ndot commented Sep 5, 2015

I'm happy with your suggestion.

But with your approach, if you want to be anal about it, a user could theoretically login (thereby burning that timestep) disable & renable OTP, log out and try to login with a new OTP for that same timestep and getting denied, despite providing a unique OTP. Now with the default ROTP timestep interval being 30 seconds, and tinfoil's gem offering no option to override it, this becomes a very unpractical & narrow issue.

@ShaneWilton

This comment has been minimized.

Member

ShaneWilton commented Sep 5, 2015

Ha, I hadn't thought of that!

That does sound like a valid bug, though it's enough of an edge case that I think I'd prefer sticking with the simpler implementation. If anyone raises their pitchforks in the future because of it, you can point them in my direction :P

@f3ndot

This comment has been minimized.

Contributor

f3ndot commented Sep 5, 2015

Fine by me 😉

f3ndot added a commit to f3ndot/devise-two-factor that referenced this issue Sep 5, 2015

Disallow subsequent OTPs once validated via timesteps
In order to be fully RFC 6238 compliant for TOTPs, no valid OTP be used more than once for a given timestep. By storing the timestep of the last successfully validated OTP, we achieve this desired behaviour.

TOTP#timecode is a private method, so we had to duplicate it in order to obtain the current server timestep. Thankfully it's simply time divided by interval.

I was having issues with using `resource.save! in the strategy class as a means of object persistence to the database. Ultimately, I opted to use the ActiveRecord `save()` method inside the devise model which is what Devise does in its own model definitions.

Burning the timestep bumps the `updated_at` value for the model. Not sure if this is a feature or bug.

Refs tinfoil#30

f3ndot added a commit to f3ndot/devise-two-factor that referenced this issue Sep 5, 2015

Disallow subsequent OTPs once validated via timesteps
In order to be fully RFC 6238 compliant for TOTPs, no valid OTP be used more than once for a given timestep. By storing the timestep of the last successfully validated OTP, we achieve this desired behaviour.

TOTP#timecode is a private method, so we had to duplicate it in order to obtain the current server timestep. Thankfully it's simply time divided by interval.

I was having issues with using `resource.save!` in the strategy class as a means of object persistence to the database. Ultimately, I opted to use the ActiveRecord `save()` method inside the devise model which is what Devise does in its own model definitions.

Burning the timestep bumps the `updated_at` value for the model. Not sure if this is a feature or bug.

Refs tinfoil#30

f3ndot added a commit to f3ndot/devise-two-factor that referenced this issue Sep 5, 2015

Disallow subsequent OTPs once validated via timesteps
In order to be fully RFC 6238 compliant for TOTPs, no valid OTP may be used more than once for a given timestep. By storing the timestep of the last successfully validated OTP, we achieve this desired behaviour.

TOTP#timecode is a private method, so we had to duplicate it in order to obtain the current server timestep. Thankfully it's simply time divided by interval.

I was having issues with using `resource.save!` in the strategy class as a means of object persistence to the database. Ultimately, I opted to use the ActiveRecord `save()` method inside the devise model which is what Devise does in its own model definitions.

Burning the timestep bumps the `updated_at` value for the model. Not sure if this is a feature or bug.

Refs tinfoil#30

@bsedat bsedat closed this Sep 15, 2015

pdkproitf pushed a commit to pdkproitf/devise-two-factor that referenced this issue Aug 28, 2018

Disallow subsequent OTPs once validated via timesteps
In order to be fully RFC 6238 compliant for TOTPs, no valid OTP may be used more than once for a given timestep. By storing the timestep of the last successfully validated OTP, we achieve this desired behaviour.

TOTP#timecode is a private method, so we had to duplicate it in order to obtain the current server timestep. Thankfully it's simply time divided by interval.

I was having issues with using `resource.save!` in the strategy class as a means of object persistence to the database. Ultimately, I opted to use the ActiveRecord `save()` method inside the devise model which is what Devise does in its own model definitions.

Burning the timestep bumps the `updated_at` value for the model. Not sure if this is a feature or bug.

Refs tinfoil#30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment