Skip to content
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

TOTPGenerator with ".withPeriod()" using less than a second parameter? #42

Closed
ohaya opened this issue Jul 30, 2021 · 9 comments
Closed
Assignees
Labels
bug Something isn't working question Further information is requested

Comments

@ohaya
Copy link

ohaya commented Jul 30, 2021

If we create a TOTPGenerator with ".withPeriod()" of less than a second (e.g., ".withPeriod(Duration.ofMillis(1))" ), we get an exception:

java.lang.ArithmeticException: / by zero
at com.bastiaanjansen.otp.TOTPGenerator.calculateCounter(TOTPGenerator.java:164)
at com.bastiaanjansen.otp.TOTPGenerator.generate(TOTPGenerator.java:49)
.
.

The reason we are trying this is that, from our testing, it appears that when the TOTPGenerator .generate() method is called, it will generate the same/identical code if the calls are within that Duration.ofSeconds(), i.e., if the the generator is created with .withPeriod(DurationofSeconds(10)), all calls to .generate(), within a 10 second period, return the same code.

We are looking at OTP-Java to generate OTP email codes, so that would mean that if we had requests (from different users) for several OTP email codes within a period of time, then the OTP-Java .generate() method would potentially provide us with the same code for several users.

Am I misinterpreting how this works?

Thanks,
Jim

@BastiaanJansen BastiaanJansen self-assigned this Jul 30, 2021
@BastiaanJansen
Copy link
Owner

Hi Jim,

Using a time step of 0, is not possible because that would mean that a token is valid for 0 seconds. That doesn't make much sense. You are right that the thrown error isn't handled properly. I will address that in the next release.

You should consider using a different secret for different users. This way the generated token will be different for each user independent of the time step.

I hope I helped you with your problem. If you have any other questions, don't hesitate to ask.

Bas

@BastiaanJansen BastiaanJansen added bug Something isn't working question Further information is requested labels Jul 30, 2021
@ohaya
Copy link
Author

ohaya commented Jul 30, 2021

Hi Bas,

I will try to do some testing with using different secrets - FYI, the reason I wasn't trying that till now is that I/we have been concerned about overhead/speed of getting the codes.

Re. the time step: Once you fix that (when is next release?), would it allow millisecs Duration?

The reason I ask that is that, even if using different secret per user yields different sequence of codes per user when we call the .generate(), if, say the Duration had to be minimum 1 sec, then if we called the .generate() for the same user/secret within < 1 sec, I think we would get the same code generated multiple times for a given user?

I need to talk to some colleagues if that (potentially getting the same code for a given user, when we call .generate()) will be acceptable.

On a kind of related question: If we did maintain an individual secret per user, would the time step/duration for each individual user's codes be independent?

What I mean if for example, if we had users A and B, and we had secretA for user A and secretB for userB, then we create the builders for users A and B, so we have builderA and builderB, respectively.

Then we use those to create the TOTPGenerator instances for users, e.g., generatorA and generatorB, both with the same Duration (e.g., 30 seconds), but say, 10 seconds apart.

Now, if we call generatorA.generate() to get codeA, and maybe a five secs later, call generatorB.generate() to get codeB, and then we use the .verify() method for each of those, would the time during which the generatorA.verify() returns true vs. the time during which the generatorB.verify() returns true be offset by 5 secs?

The reason for this question is that when we first found OTP-Java, we thought we could let OTP-Java manage the code lifetimes and use the OTP-Java .verify().

However, after initially reviewing/testing the TOTPGenerator, we were thinking that we would only use the .generate() to create the codes, but not use the generator.verify() method, but rather, after we generated a code using OTP-Java, we would store the timestamp and the generated code in our userstore, and manage the code verify and expiration ourselves, in our own code.

BUT now, if we go to individual secret per user, and if the answer to the above scenario is "Yes, the durations for each secret/code can be managed by OTP-Java individually", then we might then be able to use the OTP-Java .verify(), and not have to maintain the timestamps and manage the durations in our code (which would be pretty cool, actually).

I am really sorry for the somewhat complicated explanation/scenario :(!!

Thanks,
Jim

@BastiaanJansen
Copy link
Owner

BastiaanJansen commented Jul 30, 2021

Hi Jim,

Re. the time step: Once you fix that (when is next release?), would it allow milliseconds Duration?

I think it will be ready next week, at the latest. This would not allow milliseconds, simply because it wouldn't make much sense for a token to be valid for a for, say, 100 milliseconds. This also wouldn't be necessary as I explain later, so you can continue using the current version.

The reason I ask that is that, even if using different secret per user yields different sequence of codes per user when we call the .generate(), if, say the Duration had to be minimum 1 sec, then if we called the .generate() for the same user/secret within < 1 sec, I think we would get the same code generated multiple times for a given user?

You are right to think that when you use the same secret with a period of, say, 30 seconds the generator yields the same tokens within 30 seconds. This is because the period defines how long a token should be valid. When you use the .verify method, it checks if the token is correct for the current time and a specific period. So, when multiple users request an email verification within 30 seconds and the same secret is used, they all get the same token. This must be avoided, because a possible attacker could just request a token and get the same token needed to verify a different user as long as the attacker is requesting the code within the same time window.

On a kind of related question: If we did maintain an individual secret per user, would the time step/duration for each individual user's codes be independent?

Yes

Now, if we call generatorA.generate() to get codeA, and maybe a five secs later, call generatorB.generate() to get codeB, and then we use the .verify() method for each of those, would the time during which the generatorA.verify() returns true vs. the time during which the generatorB.verify() returns true be offset by 5 secs?

No, the current time window has nothing to do with the time the .verify method is called at. All generators with a period of 30 seconds fall into the same time window. So if you call generatorA.generate() to get code A, and five seconds later call generatorB.generate() and verify both tokens, they both will return true until the time window is over, which is the same for both generators.

The reason for this question is that when we first found OTP-Java, we thought we could let OTP-Java manage the code lifetimes and use the OTP-Java .verify().

You are right, this is indeed possible, or as you said, taking care of it yourself. I recommend using the verify method instead of using your own solution:

Use different secrets for each user. Say, you want the code to be valid for 1 hour. This means the user has 1 hour to click the verification link. When creating the verification request:

// Create verification request
byte[] secret = // Generate new secret and save it, to later verify the token. This secret should be different for each user, so users requesting a verification mail don't get the same token within the same time windo

TOTPGenerator generator = new TOTPGenerator.Builder(secret)
    .withPeriod(Duration.ofHours(1))
    .build();

String token = generator.generate();

// Send email with token

When the user clicked on the verification link:

// User clicked on verification link
byte[] secret = // retrieve user secret
String token = // token to verify

TOTPGenerator generator = new TOTPGenerator.Builder(secret)
    .withPeriod(Duration.ofHours(1))
    .build();

generator.verify(token); // returns true, if the token is valid at this moment and thus falls into the current time window with a period of 1 hour

This way, as you said, you don't have to maintain the timestamps and manage the durations in your code. Also keep in mind that the longer the time period is, brute forcing becomes more of a problem. So consider reducing the period, or add some sort of a maximum try system so you can only try 3 tokens before a new token is generated.

I hope it is becoming more clear! If you have any other questions, don't hesitate to ask.

Bas

@ohaya
Copy link
Author

ohaya commented Jul 30, 2021

Hi,

I was about to post some code that I have been using to test but then I saw your post above. I think this part of what you said is what is really unclear to me (and is causing me to not understand how this is all working):

"No, the current time window has nothing to do with the time the .verify method is called at. All generators with a period of 30 seconds fall into the same time window. So if you call generatorA.generate() to get code A, and five seconds later call generatorB.generate() and verify both tokens, they both will return true until the time window is over, which is the same for both generators."

Maybe a specific example might help me understand that above?

For example, say (assume: different secrets for both generatorA and generatorB and the .withPeriod is for 30 seconds for both):

10:00:00 - call generatorA.generate() and get codeA
10:00:05 - call generatorB.generate() and get codeB

Assume no more calls to generate()

Now, following what you said in the above quotation, when (at what time) would generatorA.verify(codeA) start returning false?

Also, when (at what time) would generatorB.verify(codeB) start returning false?

And, can you explain "Why" at those times?

@BastiaanJansen
Copy link
Owner

BastiaanJansen commented Jul 30, 2021

Hi Jim,

I see the confusion now.

For each generate method call, a counter is generated from the period with the following code:

System.currentTimeMillis() / period.toMillis();

So this counter is the same for every 30 seconds. This counter with the same secret will always generate the same token. So after the 30 second time window is over, the counter will be 1 bigger than the previous. Because it depends on the system time, it is not always the case that when a token generated on 10:00:00 will be invalid after 10:00:30.

Concrete example:
The milliseconds since epoch at 10:00:00 today is 1627632000000, which means the counter is 1627632000000 / 30000 = 54254400.
At 10:00:30, milliseconds since epoch is 1627632030000, which means the counter is 1627632030000 / 30000 = 54254401

As you can see, the counter will grow by 1 every 30 seconds. So, at 10:00:30, the token is invalid and the verify method will return false. Exactly after your defined period of 30 seconds.

However, the token generated by generatorB will also be invalid at 10:00:30:
The milliseconds since epoch at 10:00:05 today is 1627632005000, which means the counter is 1627632005000 / 30000 = 54254400.
At 10:00:30, milliseconds since epoch is 1627632030000, which means the counter is 1627632030000 / 30000 = 54254401 and thus a different token will be generated.

This token is only valid for 25 seconds. So a period of 30 seconds, does not mean each token is valid for 30 seconds, but rather that every 30 seconds, the previous tokens aren't valid anymore.

Bas

@ohaya
Copy link
Author

ohaya commented Jul 30, 2021

We have a saying, "This is hurting my head!" :)!

So, if I am understanding what you are saying, the .withPeriod() applies to "absolute wall clock time"?

Example:

So if I create a generator ("generatorC") whose builder has ".withPeriod(Duration.ofSeconds(20))", and I call that generatorC.generate() to create a code (call it "codeC") at 10:27:18, then:

  • if I call verify(codeC) at 10:27:19, it will return true
  • if I call verify(codeC) at 10:27:20, it will return true
  • if I call verify(codeC) at 10:27:21, it will return false

Is that correct?

@BastiaanJansen
Copy link
Owner

Almost! :). 10:27:20 will generate a different counter than 10:27:19, so it will return false:

10:27:19: milliseconds since epoch: 1627633639000 -> 1627633639000 / 20000 = 81381681
10:27:20: milliseconds since epoch: 1627633640000 -> 1627633640000 / 20000 = 81381682

That means:

  • if I call verify(codeC) at 10:27:19, it will return true
  • if I call verify(codeC) at 10:27:20, it will return false
  • if I call verify(codeC) at 10:27:21, it will return false

@ohaya
Copy link
Author

ohaya commented Jul 31, 2021

Hi Bas,

Ok, thanks for your patience with all of my questions, and for all of your responses!! I think I understand now. I will start a new issue if we have any new/different questions!

Thanks again, and have a good weekend!
Jim

@ohaya ohaya closed this as completed Jul 31, 2021
@BastiaanJansen
Copy link
Owner

FYI: I just released a new version with some bugfixes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants