Bitwarden_rs: Cannot enable 2FA with Android Auth App

[crytectobi]

Hi,
I cannot activate 2FA with my Android phone and the Microsoft Authenticator app.
The log says:
[2019-04-23 19:35:27][rocket::rocket][INFO] PUT /api/two-factor/authenticator application/json; charset=utf-8:
[2019-04-23 19:35:27][_][INFO] Matched: PUT /api/two-factor/authenticator (activate_authenticator_put)
[2019-04-23 19:35:27][bitwarden_rs::error][ERROR] Invalid TOTP code. Invalid TOTP code

Bitwarden runs in a docker hosted on a openmediavault server.
That server is in time sync, the android phone has also the correct time.

Can I set the correct timezone in the bitwarden container? Is TZ env working or is this not implemented?
The openmediavault server is running in the Europe/Berlin timezone, phone as well. When the docker container runs in another timezone it might not work, right? Or any other ideas? :)

[shauder]

Log into the docker shell and take a look at the time there. I do set my timezone for my docker container. Examples from my docker-compose.

    environment:
      - TZ=${TIME_ZONE}

    volumes:
      - /etc/localtime:/etc/localtime:ro

calvin% docker exec -ti bitwardenrs /bin/sh 
/ # date
Wed Apr 24 19:18:32 UTC 2019

[crytectobi]

Time in the docker container seems correct:
docker exec -it Bitwarden date
Wed Apr 24 21:24:32 CEST 2019
It is about a minute off between my computer and the server. But that offset is okay for TOTP right?
Timezone is also correct.
EDIT:
Is port 3012 neccessary for this to work?

[shauder]

I believe a minute can cause issues. Port 312 is just for web sockets.

[dani-garcia]

The TOTP codes are generated per 30 second block, and at this time we only check the current time block, so any time deviation of over 30 seconds will not work.

Some implementations also check X time blocks before and after the current one, which would be a bit more insecure, but more convenient if the clients or the server aren't syncing their time with an NTP server.

We could potentially improve the situation by also checking the 2 blocks immediate to the current one, which would give a margin of 1.5 minutes, and it seems like an acceptable compromise between security and convenience.

[crytectobi]

I fixed the offset, now it worked. I didn't know the time window is so small, most of the 2FA services I used also allow, as you mentioned, the block before and after.
Btw the offset was not a minute, it was only about 35 seconds.
Thank you for pointing this out. :)

[SebastianS90]

Right now it is possible to use the same token multiple times within the 30 seconds window. That opens an attack vector where an attacker gets password and current 2FA token (keylogger, hidden camera pointed on keyboard, just watch the victim typing, ...) and uses these to immediately login to the victim's account. By widening the window to 1.5 minutes this attack would gain more importance.

You can rule out that attack vector by saving the timestamp of the last used token on the server and for the next 2FA challenge enforce that a token for a later timestamp must be used. This prevents reuse of 2FA tokens.

lower = max (last_used_timestamp + 1, current_timestamp() - window);
upper = current_timestamp() + window;
success = false;
for (t = lower; t <= upper; t++) {
    if (check(provided_token, secret, t)) {
      success = true;
      save_timestamp(t);
      break;
    }
}

[BlackDex]

Is this still an issue?
As @dani-garcia posted on the 24th of April about allowing previous and next also.
I think this is kinda the default according to the wiki page:
https://en.wikipedia.org/wiki/Time-based_One-time_Password_algorithm#Practical_considerations

It also is mentioned in the RFC6238: https://tools.ietf.org/html/rfc6238#section-6
There they talk about even going 2 time-steps back and forward. That would be to much if you ask me, since that kind of time drift will probably only occur with hardware-totp tokens which are older.
And not with the software/apps that will show the tokens most people use.

And if we implement the token-used-cache as explained by @SebastianS90, that would rule-out some hack-attempts. I even think that this could be a default item to include. Since, we just want it to be used ONCE, and not Twice.

[crytectobi]

@BlackDex from my side the issue is fixed/closed, but there should be enhancements I guess.

[dani-garcia]

TOTP has been checking both the previous and next slots for some time now so this can be closed.