Document the need to prevent OTP reuse and maybe provide suggested solutions

[vsajip]

According to Section 5.2 of RFC6238,

Note that a prover may send the same OTP inside a given time-step window multiple times to a verifier. The verifier MUST NOT accept the second attempt of the OTP after the successful validation has been issued for the first OTP, which ensures one-time only use of an OTP.

However, the existing implementation (v2.2.6) appears to violate this:

$ python
Python 3.5.2 (default, Nov 23 2017, 16:37:01) 
[GCC 5.4.0 20160609] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import pyotp; key = pyotp.random_base32(length=20)
>>> t = pyotp.TOTP(key)
>>> t.now()
'925243'
>>> t.verify('925243')
True
>>> t.verify('925243')
True
>>> t.verify('925243')
True
>>> t.verify('925243')
True
>>> 

This could be a security issue: an attacker could conceivably MITM the connection between the verifier and provider, get the credentials & TOTP value, and use those to log in within the 30-second window. This would appear to defeat the verification because the TOTP can be replayed.

Alternatively, an attacker could shoulder-surf the victim and then log in separately within 30 seconds.

[tilkinsc]

It would be confusing for this library to have the feature. Store your own keys and bounce it off a hashtable, the library just does its job of functionality.

[vsajip]

Well, the README references the RFCs which suggests that the library is supposed to comply with them. I'm not sure what would be confusing about complying with the RFCs. I note that ROTP (which this library was inspired by) once had this problem, but they appear to have fixed it (not using a hashtable - that's not a solution that would e.g. survive a server restart, unless the hashtable is made persistent).

[kislyuk]

Thanks for bringing this up @vsajip. To be clear, the implementation does comply with all applicable aspects of the RFC. I agree there is a need to document the fact that the implementer is responsible for storing used OTPs during their valid window. ROTP did not incorporate this functionality into the codebase, but instead provided a suggestion for how to do that in their documentation. We could do the same.

[vsajip]

See this part of the ROTP code, which allows passing of a prior_time value to test against and disallow. There doesn't seem to an equivalent in pyotp - have I missed it?

[kislyuk]

Right. There isn't. But ROTP does not keep track of this information - it is the caller's responsibility to do so, and to configure ROTP accordingly. That one kwarg does not by itself guard against reuse - the doc section does.

Using the package does not free the implementer from having to understand the standard and its threat model. Our responsibility, though, is to document the caveat in the place the implementer is most likely to hit it.

[hudecof]

seems that this this conversation is not following. I have to agree, that the library do not solve the REPLAY attacks, but the solution is easy.

Solution 1 as @vsajip wrote, we could provide last entered code and the library needs to check the verified code is later in time windows. It's upon the developer to store the last values.

Solution 2 or return the timestamp or 0 instead if the True/False and accept one more parameter as lower bound for time window.

I could provide MR for both, but the second solution will be mostly bot backward compatible, if some test the result to True/False precisely.

[kislyuk]

While I can see some possible ways to make replay rejection more obviously necessary and easier to implement, none of the suggestions here are satisfactory. One proper way to do this might be to add an optional (but recommended) serializer constructor argument, which would be an object subclassed from an abstract serializer class provided by this library (i.e. implementing an interface). The interface would have methods like save_last_used_totp_timestamp and get_last_used_totp_timestamp.

I welcome PRs implementing this. In the meantime, I've documented the security considerations in the readme.

[tilkinsc]

Why not just implement this repo in a new repo with features? It would be much better than clogging this repo which does its job.