Return time window value for successful verification

[lellis1936]

It's too bad there is not a method to return the matching time slot value when a verification is successful.

Our desired method of preventing code reuse would be to store that value, and ensure any new attempts use a slot that is greater.

You probably get the gist, but this whole area is something not well defined, and your library (as nice as it is), does not seem to help with this.

Maybe I'm missing something in the library.

[RobThree]

It's too bad there is not a method to return the matching time slot value when a verification is successful.

You're welcome to do a PR 😉

Maybe I'm missing something in the library.

It's not currently in it but I'm open to consider exposing this information; however there is a chance of race conditions (however small) when you ask for the timeslice just before/after validating the code so to prevent that we would have to change the VerifyCode method to return both the validation result (valid code true/false) and the timeslice it used to validate. However, another problem is that we allow for more than one timeslice since client devices may be off by some margin. This discrepancy argument allows someone to specify how many timeslices a client device is allowed to be off. That would further complicate the above return result.

An alternative would be to expose the GetTimeSlice(...) method by making it public so you can use it for your own calculations (again: with a small risk of race conditions but that would probably be acceptable).

I think the best thing to do is simply store the current DateTime (not timeslice, actual date + time) and use that to ensure that no new attempts are being made in whatever timespan you desire. To me that makes much more sense (and doesn't require one to know/understand/handle "internals" like timeslices) and doesn't require changes in TwoFactorAuth.Net.

[lellis1936]

I think the best thing to do is simply store the current DateTime...

The problem is this: let's say you allow a 5-minute discrepancy. Once logged in you are not able to log in again for five minutes. That can be a major nuisance. That's why storing the slice is better, though still subject to quirks.

I did think about providing an enhancement, but with all the affected overloads the only simple thing I could think of was a hack to store the slot value when a match occurs and provide a separate method to retrieve it. I found that too unpalatable to think about submitting.

[RobThree]

let's say you allow a 5-minute discrepancy

Which is 10 (default) timeslices... pretty much if you ask me.

That can be a major nuisance.

I wonder... You just logged in. I'm sure a small percentage will encounter this anyway but you could use a (Re)Captcha or something to be able to bypass the second-login-attempt.

That's why storing the slice is better

You could still allow for a 5 minute discrepancy but store the date/time + a 30 second timespan for the case you're describing? The options are not mutually exclusive?

though still subject to quirks.

I'm pretty sure there will always be 'quirks' in this setup. With clear instructions in the UI and an option to bypass the 'protection' by having to enter a (Re)Captcha or something (with also the 2FA code ofcourse) I'm pretty sure it'll be fine for most users.

I did think about providing an enhancement, but with all the affected overloads the only simple thing I could think of was a hack to store the slot value when a match occurs and provide a separate method to retrieve it. I found that too unpalatable to think about submitting.

Huh? Just changing Private to Public on the GetTimeSlice method would allow you (in your scenario) to store the timeslice (besides the small race-conditions). If you're not willing to take (again, a very small) risk on race-conditions then the VerifyCode would have to be modified as I described here to return a status and "affected" or "used" timeslice(s) when verifying the code. That would break backwards compatibility and is, IMHO, not an option for solving such a trivial problem.

[lellis1936]

Which is 10 (default) timeslices... pretty much if you ask me.

Sure is, it's just an example. I log out and in frequently when testing so having to wait more than 30 s would be a nuisance for me.

Huh? Just changing Private to Public on the GetTimeSlice method

But I don't want to store the current timeslice, I want to store the timeslice that actually corresponded with the entered TOTP code. The idea is if we save that, we can just disallow that timeslot, or any older timeslot from being used. Well anyway that's the idea but your methods don't easily support use of this scheme, which admittedly has its faults.

The idea of course is to prevent the need for storing all used codes while still allowing a quick logoff/login use case.

Thanks for your suggestions and about Captcha (which might be the best I can do). I have to think this through some more for the best compromise in our app.

[RobThree]

What if you store a timestamp and the code? That would allow for discrepancies either way (+ or - from current time) with a variable discrepancy but would still only result one actual timeslice that would match and thus be blocked to prevent replay attacks.

[lellis1936]

I'm intrigued but not sure what I'd do with the timestamp. You must see an algorithm I don't. Seems like all we can do here is prevent the same code from being used twice. I must be missing it.

[RobThree]

Codes are not unique; the code you get could, theoretically, pop-up again tomorrow. The chances may be slim but there's a chance. So if you keep track of the code used and a timestamp you'll know if it's a replay-attack / code reuse or just a repeated code. All codes older than, say, 24hrs, can be deleted from your database (nightly task for example) or you can keep keep them indefinitely. Whatever. Either way, if you want to prevent replay-attacks / code reuse you'll need the actual code used and a timestamp to figure out if it's a replay-attack / code reuse or just a duplicate code.

[lellis1936]

Now I understand. That works to block the most recent code but not any prior. Nitpicking, but the RFC is clear that a code for a given timeslot should only validate once.

In our homegrown library (which I'd hoped to retire), I do this by storing the timeslot value after success and disallowing that or any earlier timeslot. Looking around at other libraries, this seems to be a common approach others are using to solve this problem.

I'm sure I can do that somehow without modifying your library, but it needs me to use implementation details in my code I'd be better off not using.

A separate method to return the matching timeslot after successful verification would help, This wouldn't require a breaking change but the implementation would be a bit ugly. If I were to do such pull request it would probably be rejected ;)

On Tue, Jun 5, 2018 at 2:12 AM, Rob Janssen notifications@github.com wrote:

Codes are not unique; the code you get could, theoretically, pop-up again tomorrow. The chances may be slim but there's a chance. So if you keep track of the code used and a timestamp you'll know if it's a replay-attack or just a repeated code. All codes older than, say, 24hrs, can be deleted from your database (nightly task for example) or you can keep keep them indefinitely. Whatever. Either way, if you want to prevent replay-attacks you'll need the actual code used and a timestamp to figure out if it's a replay or just a duplicate code.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/RobThree/TwoFactorAuth.Net/issues/4#issuecomment-394605702, or mute the thread https://github.com/notifications/unsubscribe-auth/AOYUh-h0bfT5s7z4w4d7TfRBIQSIDcJhks5t5i93gaJpZM4UXWJP .

[RobThree]

That works to block the most recent code but not any prior.

In our homegrown library [...] I do this by storing the timeslot value after success and disallowing that or any earlier timeslot.

So then you store the current time / timestamp on login? I don't see the problem? You can keep track of earlier used codes just as I described (in a database or whatever).

Nitpicking, but the RFC is clear that a code for a given timeslot should only validate once.

There is only one code for a given timeslot so all you need to store is the time(slot/stamp/whatever) of a succesful login (and, optionally, the code used). Since you cannot get to the time>slot< (currenty, but as discussed we could make it public) you can store the timestamp (+ code). This is exactly the same (a timeslot is nothing else but a timestamp mod X seconds (30 by default)). If you see the same timeslot/stamp (for the same user) in a timespan of 30(...by default) seconds again (with an already used code) you can act upon it and deny access ("validate once").

[...] but the implementation would be a bit ugly. If I were to do such pull request it would probably be rejected ;)

I'm afraid it will be 😉

But I'll try to create an example for you tonight or soon to show you what I mean.

[RobThree]

Here's my idea in pseudo-code

var date = DateTime.Now;
var result = VerifyCode(secret, code, discrepancy, date);

// If result == true ("OK") you know exactly what date was used (no race-conditions). Now 
// you store the date (+ code) somewhere (database?)

For next logins you check when the last login was. If last login is within X timespan (which happens to be 30 seconds for example) and the code equals previous code then there's (a possibility of) a replay attack.

[lellis1936]

I see. But follow my thought:

code stream t-2 = 123 t-1 = 456 t-0 = 789

Assume variance (discrepancy) of 5 timeslots, your default.

All in one 30 second interval: Log in 1 happens at t-0 using code 789. We store 789 and the time.
Log in 2 happens at t+1 using code 456. We store 456 and the time. Log in 3 happens at t+2 using code 789 again. Still works.

This may be a low-risk scenario, it's true.

Google libpam solves this by supporting a list of recently used slot timestamps (not codes). Your scheme could be adapted but the list would need to include both the code and time element. The list would be awkward for the client app to maintain, for sure.

Just want to make sure my understanding is correct.

[RobThree]

Your scheme could be adapted but the list would need to include both the code and time element. The list would be awkward for the client app to maintain, for sure.

So then making the GetTimeSlice() method public (and offer an overload that accepts a DateTime) would solve it (in combination with this code to pass date to the GetTimeSlice() method(s)), right?

edit: ... ah, it won't since you won't know which timeslice was actually used because of the discrepancy allowed. Now I see. Let me think about that... We could expose a 'read-only' TimeSliceUsed property that holds the actual, used and correct, timeslice that is updated by the VerifyCode() method. But that would be problematic if the instance is used by more than one thread for example. Another option would be to add an overload(s) that use out parameters to return the timeslice.

Let me think about this for a while (or let me know your thoughts/suggestions) on how to best handle this.

[RobThree]

Currently we have:

public bool VerifyCode(string secret, string code) { ... }
public bool VerifyCode(string secret, string code, int discrepancy) { ... }
public bool VerifyCode(string secret, string code, int discrepancy, DateTime dateTime) { ... }
public bool VerifyCode(string secret, string code, int discrepancy, long timestamp) { ... }

I could add one or more or all of these overloads:

public bool VerifyCode(string secret, string code, out long timeSlice) { ... }
public bool VerifyCode(string secret, string code, int discrepancy, out long timeSlice) { ... }
public bool VerifyCode(string secret, string code, int discrepancy, DateTime dateTime, out long timeSlice) { ... }
public bool VerifyCode(string secret, string code, int discrepancy, long timestamp, out long timeSlice) { ... }

Which would return the timeslice used using an out argument. Currently that looks to be the most compatible method but I'll think about it for a while. Let me know what you think.

[lellis1936]

I think it would be lovely if you could do that.

[RobThree]

Added the overloads.

Will need to write some unittests and then publish a new package. Give me a day or two to iron out all the kinks (especially since the VerifyCode() was susceptible to timing attacks if not altered correctly; I think my changes are correct but I will need to give this some thorough thought).

(By the way, the build is failing because of a test failing because convert-unix-time.com appears to be down / gone. Will fix that too) Fixed that. Dropped the ConvertUnixTimeDotCom timeprovider (which was a mess to begin with) and replaced it with an NTPTimeProvider while I was at it.

[RobThree]

I present to you: 1.3.2 🎉

[lellis1936]

Fantastic. I am building a client for the library now.