starred/google2fa

Fork 0

mirror of https://github.com/antonioribeiro/google2fa.git synced 2026-04-25 08:05:49 +03:00

[GH-ISSUE #69] verifyKeyNewer checks "newer or equal" instead of "strictly newer" #499

New issue

Closed

opened 2026-03-14 11:58:40 +03:00 by kerem · 1 comment

kerem commented

2026-03-14 11:58:40 +03:00

Owner

Originally created by @SebastianS90 on GitHub (Jun 21, 2017).
Original GitHub issue: https://github.com/antonioribeiro/google2fa/issues/69

First of all, thank you so much for merging and improving my PR (#50)!

But are you sure that this change in the specification is correct?

 public function it_verifies_keys_newer()
 {
   $this->verifyKeyNewer($this->secret, '512396', 26213401, 2, 26213400)->shouldBe(false);    // 26213400
-  $this->verifyKeyNewer($this->secret, '410272', 26213401, 2, 26213400)->shouldBe(false);    // 26213401
+  $this->verifyKeyNewer($this->secret, '410272', 26213401, 2, 26213400)->shouldBe(26213401);    // 26213401
   $this->verifyKeyNewer($this->secret, '239815', 26213401, 2, 26213400)->shouldBe(26213402); // 26213402
   $this->verifyKeyNewer($this->secret, '313366', 26213401, 2, 26213400)->shouldBe(false);    // 26213403
 }

For me it looks wrong, especially when looking at the example from the readme file:

$secret = $request->input('secret');

$timestamp = $google2fa->verifyKeyNewer($user->google2fa_secret, $secret, $user->google2fa_ts);

if ($timestamp !== false) {
    $user->update(['google2fa_ts' => $timestamp]);
    // successful
} else {
    // failed
}

There we store the returned timestamp into the user model, and take it from there for the next verifyKeyNewer call. The comparison therefore must be "strictly newer" and not "newer or equal".
With your change, the situation is as follows (assuming that timestamp 26213401 is in the window):

The user provides the token 410272 which is for the timestamp 26213401. Assume that this timestamp is new, i.e. the check will pass.
The verifyKeyNewer call returns 26213401, we store that timestamp into the user model.
An attacker has witnessed the user entering credentials and otp token. He therefore knows the token. While still being in the window, he tries to login with correct credentials and the same token 410272.
We check: $google2fa->verifyKeyNewer($user->google2fa_secret, 410272, 26213401), which returns 26213401 as specified by your changed test.
The attacker therefore can login, reusing the already used token.

To resolve this, we either can:

change the test to ->shouldBe(false) again and make sure that the implementation passes this test, or
change the readme and instruct to increment the timestamp at some point, e.g.
$user->update(['google2fa_ts' => $timestamp + 1]); (but this sounds less convenient)

I might be wrong, as I haven't yet found the time to thoroughly test your new version, but the changed test feels wrong for me.

Anyways, thanks a lot for the effort you put into maintaining this package and further improving it for Laravel 5.5.

Originally created by @SebastianS90 on GitHub (Jun 21, 2017). Original GitHub issue: https://github.com/antonioribeiro/google2fa/issues/69 First of all, thank you so much for merging and improving my PR (#50)! But are you sure that [this change](https://github.com/antonioribeiro/google2fa/commit/9392c633cf12dacce353dec76a6f728526569d86?diff=unified#diff-846ea7c292e99ddae9a9dbe731587825) in the specification is correct? ```diff public function it_verifies_keys_newer() { $this->verifyKeyNewer($this->secret, '512396', 26213401, 2, 26213400)->shouldBe(false); // 26213400 - $this->verifyKeyNewer($this->secret, '410272', 26213401, 2, 26213400)->shouldBe(false); // 26213401 + $this->verifyKeyNewer($this->secret, '410272', 26213401, 2, 26213400)->shouldBe(26213401); // 26213401 $this->verifyKeyNewer($this->secret, '239815', 26213401, 2, 26213400)->shouldBe(26213402); // 26213402 $this->verifyKeyNewer($this->secret, '313366', 26213401, 2, 26213400)->shouldBe(false); // 26213403 } ``` For me it looks wrong, especially when looking at the [example from the readme file](https://github.com/antonioribeiro/google2fa/blob/v2.0.0/readme.md#validation-window): ```php $secret = $request->input('secret'); $timestamp = $google2fa->verifyKeyNewer($user->google2fa_secret, $secret, $user->google2fa_ts); if ($timestamp !== false) { $user->update(['google2fa_ts' => $timestamp]); // successful } else { // failed } ``` There we store the returned timestamp into the user model, and take it from there for the next `verifyKeyNewer` call. The comparison therefore must be "strictly newer" and not "newer or equal". With your change, the situation is as follows (assuming that timestamp `26213401` is in the window): * The user provides the token `410272` which is for the timestamp `26213401`. Assume that this timestamp is new, i.e. the check will pass. * The `verifyKeyNewer` call returns `26213401`, we store that timestamp into the user model. * An attacker has witnessed the user entering credentials and otp token. He therefore knows the token. While still being in the window, he tries to login with correct credentials and the same token `410272`. * We check: `$google2fa->verifyKeyNewer($user->google2fa_secret, 410272, 26213401)`, which returns `26213401` as specified by your changed test. * The attacker therefore can login, reusing the already used token. To resolve this, we either can: * change the test to `->shouldBe(false)` again and make sure that the implementation passes this test, or * change the readme and instruct to increment the timestamp at some point, e.g. `$user->update(['google2fa_ts' => $timestamp + 1]);` *(but this sounds less convenient)* I might be wrong, as I haven't yet found the time to thoroughly test your new version, but the changed test feels wrong for me. Anyways, thanks a lot for the effort you put into maintaining this package and further improving it for Laravel 5.5.

kerem closed this issue

2026-03-14 11:58:45 +03:00

kerem commented

2026-03-14 11:58:51 +03:00

Author

Owner

@antonioribeiro commented on GitHub (Jun 21, 2017):

Yeah, looking at the point of view of an attacker, you are right. So I just moved it back to +1 and tagged 2.0.1.

Thank you.

@antonioribeiro commented on GitHub (Jun 21, 2017): Yeah, looking at the point of view of an attacker, you are right. So I just moved it back to +1 and tagged 2.0.1. Thank you.

No labels

bug

pull-request

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

starred/google2fa#499

No description provided.

Rows
Columns