Nitrokey / nitrokey-app

Nitrokey's Application (Win, Linux, Mac)
https://www.nitrokey.com/
287 stars 55 forks source link

Set time on device on each TOTP request #467

Closed szszszsz closed 2 years ago

szszszsz commented 3 years ago

TOTP code is not working after longer usage session

Environment

Expected behavior

A TOTP code is always current

Current behavior

A TOTP code is invalid, when device is connected longer, and Nitrokey App is not restarted before the TOTP code request

Details: https://support.nitrokey.com/t/otp-works-at-start-then-no/2966/2

Workaround

Restart application before a TOTP code request, when some bigger time has elapsed since the last one.

Possible solution

See https://github.com/Nitrokey/nitrokey-app/blob/master/src/ui/mainwindow.cpp#L1470

Steps for reproduction

Preconditions

Connect Nitrokey Pro device and start Application.

Steps

  1. Get TOTP code (expected to be valid)
  2. Wait some longer time (exact period unknown)
  3. Get TOTP code (expected to be invalid)
jans23 commented 3 years ago

Potentially relevant: I noticed that TOTP may fail after waking up the computer from standby while NK App is running. In those cases the OTP code validation fails. I suspect because NK App messes up the local system time. But this issue is not 100% reproducible. As a workaround I have to restart NK App.

szszszsz commented 3 years ago

@jans23 That might be the same cause. What USB Security Dongle model do you use?

Edit:

Nitrokey Pro

franz-strebel commented 3 years ago

Thanks for taking up the issue and I suppose the solution you mentioned makes sense, have the NK get the time from the computer every time it generates an OTP code.

Just to add some info, I've had the issue happen to me without the computer going to sleep as it has occurred on my desktop computer, which does not have any power saving mode enabled.

Thanks again.

franz-strebel commented 3 years ago

So I did a test by modifying the source and compiling the app. Basically I just put:

if (isTOTP){ libada::i()->set_current_time(); // rest of the code

I'm not a security expert so I am not sure about any adverse effects this may have but TOTP codes are now consistently right.

szszszsz commented 3 years ago

Hi @franz-strebel ! Exactly! That's almost the solution. I will do some review as well, but I do not foresee any other changes needed. Security should not decrease from time updates. Potentially this might make bigger impact on the MCU's flash, since the last time set is registered there to detect malfunction or attempt to access an old value. Making the time update no more frequent than once per 10 minutes should be the best solution.

PRs are welcomed :-)

franz-strebel commented 3 years ago

OK, I'll look into that and perhaps send you a PR. :)