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 subsequent OTPs once validated via timesteps #43

Merged
merged 3 commits into from Sep 15, 2015

Conversation

3 participants
@f3ndot
Contributor

f3ndot commented Sep 5, 2015

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.

Shitty .gif of it working in the demo app:
burned-otps

Refs #30

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 #30
@f3ndot

This comment has been minimized.

Contributor

f3ndot commented Sep 5, 2015

This change technically prevents a replay attack by those who can shoulder-surf or MiTM the victim. Going to request a CVE ID

CVE requested.

@@ -44,11 +44,25 @@
expect(subject.valid_otp?(otp)).to be true
end
it 'does not validate a previously precisely correct OTP' do

This comment has been minimized.

@ShaneWilton

ShaneWilton Sep 8, 2015

Member

I would probably move these new specs into a separate context, then move the setup logic into a before block. Something like:

context 'with a stored consumed_timestep' do
  context 'given a precisely correct OTP'
    let(:consumed_otp) { ROTP::TOTP.new(otp_secret).at(Time.now) }

    before do
      subject.valid_otp?(consumed_otp)
    end

    it 'fails to validate' do
      expect(subject.valid_otp?(consumed_otp)).to be false
    end
  end

  context 'given a previously valid OTP within the allowed drift' do
    let(:consumed_otp) { ROTP::TOTP.new(otp_secret).at(Time.now + subject.class.otp_allowed_drift, true) }

    before do
      subject.valid_otp?(consumed_otp)
    end

    it 'fails to validate' do
      expect(subject.valid_otp?(consumed_otp).to be false
    end
  end
end

It make it clearer, at a glance, what part of the spec is the setup, and what part is the expectation.

@ShaneWilton

This comment has been minimized.

Member

ShaneWilton commented Sep 8, 2015

Your changes make sense to me. I'm a little worried about adding side effects to valid_otp? though -- the name suggests that it should be a pure method. I wonder if the new logic should be moved to consume_otp, then we can wrap the entirety of the logic in validate_and_consume_otp.

I also wonder what the best approach is for telling our existing users that they'll need to add a new column. New users are fine, since the generator will handle that, but anyone else will need a new migration. I suspect we'll be able to get away with just making a new generator for the new column.

Thanks for the contribution!

@f3ndot

This comment has been minimized.

Contributor

f3ndot commented Sep 10, 2015

For those who are just upgrading, possible to provide a post-upgrade message in bundler? I think I've seen that done before.

@f3ndot

This comment has been minimized.

Contributor

f3ndot commented Sep 10, 2015

I'll be addressing your comments and update the PR when I get some time Friday

@ShaneWilton

This comment has been minimized.

Member

ShaneWilton commented Sep 10, 2015

Awesome, thanks! I would be okay with a post-upgrade message.

@bsedat bsedat referenced this pull request Sep 10, 2015

Closed

Security Bug #45

Justin Bull added some commits Sep 11, 2015

Justin Bull
Extract OTP consuming logic into separate method
Renaming the methods for better semantics and Rubyisms
@f3ndot

This comment has been minimized.

Contributor

f3ndot commented Sep 11, 2015

As for notifying users of the migration change, I'm still trying to find the "best practice" for doing so. Right now I think just a README section announcing the breaking change and the one-liner required to migrate is sufficient. Thoughts?

Post install message seems a little heavy-handed an ill-fitting, and there's no post-upgrade functionality in gemspec.

@bsedat

This comment has been minimized.

Member

bsedat commented Sep 11, 2015

Yeah, I agree that a post-install message may not be optimal, although it could caution that the installer should check the github for the latest upgrade recommendations. I've liked it when repositories that have an UPGRADING file with the major breaking changes between patch levels.

@bsedat

This comment has been minimized.

Member

bsedat commented Sep 11, 2015

I could also potentially turn on the Github Wiki for this repo, if that would be a better place.

@f3ndot

This comment has been minimized.

Contributor

f3ndot commented Sep 11, 2015

If I understand Devise's gem correctly, by virtue of adding consumed_timestep into the required_fields method array, the app would fail to boot if anyone uses the 2FA without that column existing in the app db?

That combined with a README announcing the breaking change should be sufficient.

I feel like this PR is ready to go, with y'all doing the necessary README (or wiki) updating and version bumping as required.

@ShaneWilton

This comment has been minimized.

Member

ShaneWilton commented Sep 14, 2015

Thanks Justin! This looks good to me. I'll just wait for one of my coworkers to take a look too, in case something slipped past me.

Sorry about the late response, I wasn't at my computer all weekend :)

bsedat added a commit that referenced this pull request Sep 15, 2015

Merge pull request #43 from f3ndot/issue-30-consume-valid-otps
Disallow subsequent OTPs once validated via timesteps

@bsedat bsedat merged commit cb025fb into tinfoil:master Sep 15, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bsedat

This comment has been minimized.

Member

bsedat commented Sep 15, 2015

Looks good to me. Thank you for the contribution! We'll push out a new version (probably a major version bump) as soon as we can.

ShaneWilton added a commit to ShaneWilton/devise-two-factor that referenced this pull request Sep 16, 2015

@f3ndot f3ndot deleted the f3ndot:issue-30-consume-valid-otps branch Sep 17, 2015

@f3ndot

This comment has been minimized.

Contributor

f3ndot commented Sep 17, 2015

Assigned CVE-2015-7225.

rspeicher added a commit to gitlabhq/gitlabhq that referenced this pull request Sep 20, 2015

DouweM added a commit to gitlabhq/gitlabhq that referenced this pull request Sep 20, 2015

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

Merge pull request tinfoil#43 from f3ndot/issue-30-consume-valid-otps
Disallow subsequent OTPs once validated via timesteps

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

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