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

NK Storage: Counter value is not written for HOTP #60

Closed
szszszsz opened this issue May 14, 2016 · 7 comments
Closed

NK Storage: Counter value is not written for HOTP #60

szszszsz opened this issue May 14, 2016 · 7 comments
Assignees

Comments

@szszszsz
Copy link
Member

szszszsz commented May 14, 2016

Ubuntu 16.04
occurency: always
Nitrokey Storage

HOTP counter value is not set as in configuration form during HOTP password setup. Issue is not occuring on NK Pro.

It is set to 0. Checked with RFC document example.

@jans23
Copy link
Member

jans23 commented May 14, 2016

@szszszsz Can you tell if this is a firmware or App issue?

@szszszsz
Copy link
Member Author

@jans23 I will investigate it further and let you know. After a quick gaze it looks like both sticks are handled by same HOTP setting function thus pointing the problem might be in the Nitrokey Storage firmware.

@szszszsz
Copy link
Member Author

I will check it further on Monday.

@szszszsz
Copy link
Member Author

Hi @jans23

I have found that the firmware in Nitrokey Storage accepts counter value being written in plaintext whereas Nitrokey Pro accepts counter as binary 64-bit int. The reason in sending an integral value as text is probably to avoid conversion problems between little and big endian architectures. While this is an universal solution, it probably will not be of use here. Having only 8 bytes for counter we could have either 8-digits number as text or almost 20-digits number as 64 bit long int.
This should not be widespread case but please consider following. In case where higher counter value would be needed (other end proposed counter start position would be randomly set during initialization with number of more than 8 digits) or counter value would be near 8-digits max (in short time making overflow), use of HOTP protocol with Nitrokey Storage would not be possible as counter values desynchronize between endpoints. For end-user it could be visible as creating invalid HOTP codes, which are not authorizing ordered operations.
In reference implementation from RFC document https://tools.ietf.org/html/rfc4226 the counter is also implemented as 64-bit int.
Sending counter value as a text specifically for Nitrokey Storage can be handled within nitrokey-app but it should be noted somewhere to avoid such issues later. As a matter of fact, it was handled this way but it has also broken support for Nitrokey Pro (issue #27).
Checking git history has shown that in the beginning it was implemented in Nitrokey Storage firmware as in RFC (as 64-bit int), but was changed to text representation. I was unable to read from commit messages what was the cause of this change.

I will support both firmwares for now in nitrokey-app if possible.

Related code blocks:
Handling HOTP in Nitrokey Pro Firmware
Nitrokey Storage Firmware is, was
Fix for issue 27 - HOTP not working for Nitrokey Pro - Fixed counter for Nitrokey Pro but broken for Nitrokey Storage

@szszszsz szszszsz self-assigned this May 16, 2016
@szszszsz
Copy link
Member Author

Actually it's 7 digits max for plaintext since in current implementation in firmware atoi is runned directly on the packet buffer (last byte has to be '\0').

@szszszsz
Copy link
Member Author

One more correction - since counter value would be transferred only on checking slot in GUI and it is stored in binary form on stick, the only problems are actually setting higher counter value than 9999999 and editing it in GUI - counter value is read back and shown in form.

szszszsz added a commit that referenced this issue May 16, 2016
…#64)

* Check HOTP counter text length and save only 7 digits or issue warning
* Read back HOTP counter value for Nitrokey Pro
* Fix: check ulong for HOTP counter, not long
* Limit counter digits count to 7 for Nitrokey Storage
* Handle HOTP counter for both Nitrokey Pro (ulong) and Storage (text)
Closes issue #60
@szszszsz
Copy link
Member Author

With fix both sticks HOTP counters are handled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants