I filed an issue with the library but my sense is that this is not possible to fix there. Copying:

As far as I can tell, GoogleAuthenticator library doesn't prevent a replay attack.

For this to be relevant we have to assume that either the victim is using an insecure network or has malware on their computer - both of which are not ideal, but happen more often than we'd like.

Would it be possible for GoogleAuthenticator library itself to prevent a replay? My guess is that the library doesn't have any way to persist data so it can't really prevent this kind of attack, but maybe you have ideas on that.

If it's not possible to solve in the library, then I suggest adding a note to the README to encourage people who use the library to add a feature to their code to prevent replay.

So, this protection is likely to have to happen in the totp code in tfa_basic.

CommentFileSizeAuthor
#7 2329867-tfa-basic-totp-replay-7.patch9.82 KBcoltrane
#6 2329867-tfa-basic-totp-replay-6.patch9.82 KBcoltrane
#4 2329867-tfa-basic-totp-replay-4.patch8.73 KBcoltrane
#2 2329867-tfa-basic-totp-replay.patch8.5 KBcoltrane

Comments

lva’s picture

lva commented

I posted part of a possible solution to the library maintainer at https://github.com/PHPGangsta/GoogleAuthenticator/issues/9

Instead of passing around only seeds, you should pass an array by reference. This array contains the seeds and the timeslice of last successful authentication. The array can then also contain other information like number of successful and failed authentications (which can then also be used to lock the token).

The tfa_basic module would have to modify the database table in which it stores the seeds so that it can handle the storage of this array. I recommend to serialize it and then to encrypt/encode it as you already do.

Furthermore, after calling the verifyCode function of the GoogleAuthenticator library, the tfa_basic module should save the array back to the database since it will be changed (hence the passing by reference).

coltrane’s picture

coltrane
he/him
commented
Status: Active » Needs review
StatusFileSize
new 2329867-tfa-basic-totp-replay.patch8.5 KB

Feature + test.

New table stores salted hash of accepted TOTP codes to prevent replay attack. TOTP plugin rejects codes that are stored. Stored codes are cleared on cron after 1 day, variable controlled.

I think this is a candidate feature for moving to TFA module itself, but will test here first.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented 
  1. +++ b/includes/tfa_totp.inc
@@ -64,9 +70,14 @@ class TfaTotp extends TfaBasePlugin implements TfaValidationPluginInterface {
+        $this->errorMessages['code'] = t('Invalid code, it was recently used for a login. Please wait for application to generate a new code.');

    "wait for application" -> "wait for the application" 

  2. +++ b/tfa_basic.module
@@ -128,6 +128,12 @@ function tfa_basic_cron() {
+  // Delete accepted codes older than expiration.
+  $accepted_expiration = variable_get('tfa_basic_accepted_code_expiration', 3600 * 24);
+  db_delete('tfa_accepted_code')
+    ->condition('time_accepted', REQUEST_TIME - $accepted_expiration, '<')
+    ->execute();

    Nice & simple :)

coltrane’s picture

coltrane
he/him
commented
StatusFileSize
new 2329867-tfa-basic-totp-replay-4.patch8.73 KB

Thanks for review @Wim Leers!

Updated message and used else conditional so as to have a single return statement.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented 
  1. +++ b/tfa_basic.install
@@ -189,6 +221,44 @@ function tfa_basic_update_7001() {
+    'indexes' => array(

+++ b/tfa_basic.module
@@ -128,6 +128,12 @@ function tfa_basic_cron() {
 /**

    I think an index on the time_accepted would help the cron runs go a good bit faster.

+1 from me, with or without that new index.

coltrane’s picture

coltrane
he/him
commented
StatusFileSize
new 2329867-tfa-basic-totp-replay-6.patch9.82 KB

Add index in time_accepted and also expanded test to confirm cron deletes accepted codes.

coltrane’s picture

coltrane
he/him
commented
StatusFileSize
new 2329867-tfa-basic-totp-replay-7.patch9.82 KB

A reroll to cleanly apply since tfa_basic_update_7002() conflicted.

  • coltrane committed dc865ff on 7.x-1.x 
    Issue #2329867 by coltrane: Prevent the re-use of TOTP codes
coltrane’s picture

coltrane
he/him
commented
Status: Needs review » Fixed

Committed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.