Missing time instant verification

[j-skalicky]

I got asked by a colleague if this is a good library to use, and I'm sorry to conclude it is not secure due to its missing replay protections. This library only verifies if the code is correct (within allowable time window), but does not care if the same (or a later) code has been already provided or not.

Typically, I'd expect the high-level API to be

public long isValidCode(String secret, String code, long lastSuccessfulBucket) {}

returning the ID of the successful bucket or some special value if the code is invalid. (Maybe there's a better way to do that in Java, I'm not a Java guy :) But you get the idea.)

The key is that the backend DB should store the bucket for the last successful TOTP code - and never accept any from the same or previous buckets (since they could be replayed etc.). Currently, there's no way of knowing if the coming TOTP code is being replayed or not.

Unfortunately, due to this I cannot recommend the library for production. Which is a pity since I like its features.

[jarretttaylor]

Persisting records is something that should be implementation specific which puts it outside the scope of a TOTP library. One project might choose to store records in a database. Another project might choose to hold such temporary records an in-memory cache. As such, this project shouldn't (and doesn't) dictate how that is done.

If should be noted that previously used TOTP codes don't have to be stored forever. By nature, they are only good for a specific amount of time. You control that amount of time when configuring your dev.samstevens.totp.code.CodeVerifier.

CodeVerifier.setTimePeriod(int timePeriod);
CodeVerifier.setAllowedTimePeriodDiscrepancy(int allowedTimePeriodDiscrepancy);

I completely agree that code reuse should be restricted, and long-term persistence is especially important when you consider disallowing reuse of recovery codes. That being said, it is still a implementation-specific decision. This library doesn't have external dependencies on a database and should not.

[j-skalicky]

Persisting records is something that should be implementation specific which puts it outside the scope of a TOTP library.

I completely agree. But a TOTP library should at least expose the record - because let's suppose I want to persist the intervalID of a successful code. How do I get it? There's currently no way, I'd have to compute it myself outside the library, by effectively re-running the internal implementation. Could be n, n-1 or n+1.

Also, when I know that the user used a code at interval n (that's the current one) and a few seconds later, there comes a new, different code that's accepted, how do I know which intervalID it belongs to? Could be n-1 (which is still technically valid, but should be rejected), or `n+1 (which would be fine).

If should be noted that previously used TOTP codes don't have to be stored forever.

I don't want to store previously used TOTP. That makes no sense, as above: if the current interval is n and the user just uses the corresponding code and in the next second there comes another valid code corresponding to n-1, that's not a code reuse, but should still be rejected. This library will happily accept.

The only secure way to implement is to persist the intervalID and reject everything that's not later than that one. But you need an API that works with that.

[jarretttaylor]

Ok, I am tracking with you now. It could work something like below (which is a modification of DefaultCodeVerifier).

If null is returned, the code was invalid. If a non-null value is returned, that is the intervalID.

public Long isValidCode(String secret, String code, long lastSuccessfulBucket)
{
	Long successfulBucket = null;
	long currentBucket = Math.floorDiv(timeProvider.getTime(), timePeriod);
	// Calculate and compare the codes for all the "valid" time periods,
	// even if we get an early match, to avoid timing attacks
	boolean success = false;
	for (int i = -allowedTimePeriodDiscrepancy; i <= allowedTimePeriodDiscrepancy; i++)
	{
		long checkBucket = currentBucket + i;
		boolean res = checkCode(secret, checkBucket, code) && (checkBucket > lastSuccessfulBucket);
		if (res)
		{
			//should this use the constructor to avoid factory method optimizations (for timing purposes)?
			successfulBucket = Long.valueOf(checkBucket);
		}

		success = res || success;
	}

	return successfulBucket;
}

Admittedly, it could return a more complex object comprised of a success flag and the bucket, but this would get the job done. Also, the method name is a bit of a misnomer since it returns a Long.

[dahuber-github]

Don't return a null Long. Instead return an Optional<Long>. https://docs.oracle.com/javase/8/docs/api/java/util/Optional.html Best regards, Doug Huber