opened
mjjohnson
username_0
Implemented a TODO related to a token's counter. The counter is [stored by the token, starts at 0](http://fidoalliance.org/specs/fido-u2f-v1.0-ps-20141009/fido-u2f-implementation-considerations-ps-20141009.html#token-counters), and should always increase. If not, it [may be an indication that the original device has been cloned](http://fidoalliance.org/specs/fido-u2f-v1.0-ps-20141009/fido-u2f-overview-ps-20141009.html#counters-as-a-signal-for-detecting-cloned-u2f-devices). (However, as far as I can tell, the device *may* also have wrapped around to 0 after reaching its maximum value.)

This patch adds a field to the U2FKey model for the last seen counter value. Upon successful authentication, it updates the counter value to the current value. However, if the counter value is not larger than the last seen value, it doesn't sign the user in (nor update the last seen value). A signal is provided to allow a website to perform their own actions (e.g., sending a warning email or locking the user's account pending some further action).

There are some things that should probably be decided before this is pulled. First of all, since this is a model change, it might be good to provide a migration. I didn't see any existing migrations, so perhaps it would be worth adding one? (It wouldn't help for this change, but would for any future changes.) If you think this is a good idea, I'm happy to create one and add it to the pull request.

Secondly, is refusing to authenticate the user the correct behavior? Or should it only send the signal, and allow the website to decide what to do? (I like the idea of being flexible, but I also like the idea of good defaults...not sure what the right balance is here.)

Finally, I'd like to add a test to verify that the signal gets sent, but I'm not sure of the best way to test signals. It looks like we might need to add a mock library such as [mock](http://www.voidspace.org.uk/python/mock/) (part of the standard library in Python 3), but I wanted to hold off adding a new dependency unless you thought it was a good idea.
Store login_counter and verify it's increasing
issue
created
gavinwahl
username_1
Thanks for implementing this, let me think about those questions.

The specs are maddeningly vague about handling rollover -- first it says "If the counter value has moved backward, it signals that there is more than one U2F device with the same key pair for the origin", which seems like there isn't overflow, but then "...if they suspect token cloning due to non-increasing counter values (other than wrap-around)." 

How are you supposed to tell if a nonincreasing counter value is due to wraparound or cloning? Require counters increase, unless they decrease by 'a lot', for some arbitrary value of 'a lot'? That doesn't sound very nice.
comment
created
gavinwahl
username_1
I think he means does roll over, but it'll never happen in practice. The 31-bit counter will roll over after 68 years if it's used once per second.
comment
created
mjjohnson
username_0
Heh, that does put it in perspective. Hopefully other manufacturers provide similarly generous counters. I'm thinking that perhaps we should just make it clear in the documentation that if a non-increasing counter value is seen, it's either cloning or rollover, and then let site owners decide what to do about it if the value is ever non-increasing? (Potentially they could evaluate their policy based on the manufacturer: for instance, in the case of Yubico, rollover will never happen, so it must be an attempt at cloning.)

This is kind of punting, but the standard also seems to avoid the issue. (Too bad it didn't specify a minimum number of bits for the counter or something...)
comment
created
gavinwahl
username_1
I'm thinking the right behavior might just be to generate a log message
comment
created
gordonzola
username_2
Is there something to do with this PR before merging, aside a rebase + conflict fix?
I’m willing to help if needed.
comment
created
gordonzola
username_2
Given that there is three distinct strategies regarding a non-incrementing counter, this shouldn’t be in the scope of this lib to make a choice for the developers that will use it.
However, we should give the choice, as a settings value for example. If we implement the 3 strategies as functions/classes, it allows to implement even other strategies.

So I suggest the implementation of the 3 following strategies:

- forbid login with a U2F challenge containing an incorrect counter (and contact the user to tell her that we detected a problem with this attempt)
- automatically disable the U2F device after a suspicious login (and contact the user). The user will then have to use either a TOTP token, or a backup code
- automatically block the user after a suspicious login (and contact the user). This would be quite extreme, as it’s then difficult for the user to recover the account

Contacting the user should use the known contact methods: mail and/or SMS, if we have the user’s phone number.
comment