Skip to content

Commit 032bf07

Browse files
DaneEverittDaneEveritt
authored
Ensure that TOTP tokens cannot be reused (#5481)
1 parent 1570ff2 commit 032bf07

File tree

5 files changed

+231
-14
lines changed

5 files changed

+231
-14
lines changed

‎app/Http/Controllers/Auth/LoginCheckpointController.php‎

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
namespace Pterodactyl\Http\Controllers\Auth;
44

5+
use Carbon\Carbon;
56
use Carbon\CarbonImmutable;
67
use Carbon\CarbonInterface;
78
use Pterodactyl\Models\User;
@@ -70,8 +71,20 @@ public function __invoke(LoginCheckpointRequest $request): JsonResponse
7071
}
7172
} else {
7273
$decrypted = $this->encrypter->decrypt($user->totp_secret);
74+
$oldTimestamp = $user->totp_authenticated_at
75+
? (int) floor($user->totp_authenticated_at->unix() / $this->google2FA->getKeyRegeneration())
76+
: null;
77+
78+
$verified = $this->google2FA->verifyKeyNewer(
79+
$decrypted,
80+
$request->input('authentication_code') ?? '',
81+
$oldTimestamp,
82+
config('pterodactyl.auth.2fa.window') ?? 1,
83+
);
84+
85+
if ($verified !== false) {
86+
$user->update(['totp_authenticated_at' => Carbon::now()]);
7387

74-
if ($this->google2FA->verifyKey($decrypted, (string) ($request->input('authentication_code') ?? ''), config('pterodactyl.auth.2fa.window'))) {
7588
Event::dispatch(new ProvidedAuthenticationToken($user));
7689

7790
return $this->sendLoginResponse($user, $request);
@@ -105,9 +118,9 @@ protected function isValidRecoveryToken(User $user, string $value): bool
105118
* will return false if the data is invalid, or if more time has passed than
106119
* was configured when the session was written.
107120
*/
108-
protected function hasValidSessionData(array $data): bool
121+
protected function hasValidSessionData(?array $data): bool
109122
{
110-
$validator = $this->validation->make($data, [
123+
$validator = $this->validation->make($data ?? [], [
111124
'user_id' => 'required|integer|min:1',
112125
'token_value' => 'required|string',
113126
'expires_at' => 'required',

‎app/Services/Users/ToggleTwoFactorService.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ public function handle(User $user, string $token, ?bool $toggleState = null): ar
7979
}
8080

8181
$this->repository->withoutFreshModel()->update($user->id, [
82-
'totp_authenticated_at' => Carbon::now(),
82+
'totp_authenticated_at' => null,
8383
'use_totp' => (is_null($toggleState) ? !$user->use_totp : $toggleState),
8484
]);
8585

‎database/Factories/UserFactory.php‎

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -5,18 +5,13 @@
55
use Carbon\Carbon;
66
use Ramsey\Uuid\Uuid;
77
use Illuminate\Support\Str;
8-
use Pterodactyl\Models\User;
98
use Illuminate\Database\Eloquent\Factories\Factory;
109

10+
/**
11+
* @extends \Illuminate\Database\Eloquent\Factories\Factory<\Pterodactyl\Models\User>
12+
*/
1113
class UserFactory extends Factory
1214
{
13-
/**
14-
* The name of the factory's corresponding model.
15-
*
16-
* @var string
17-
*/
18-
protected $model = User::class;
19-
2015
/**
2116
* Define the model's default state.
2217
*/

‎tests/Integration/Http/Controllers/Auth/LoginCheckpointControllerTest.php‎

Lines changed: 207 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,207 @@
1+
<?php
2+
3+
namespace Pterodactyl\Tests\Integration\Http\Controllers\Auth;
4+
5+
use Carbon\Carbon;
6+
use Pterodactyl\Models\User;
7+
use PragmaRX\Google2FA\Google2FA;
8+
use Illuminate\Auth\Events\Failed;
9+
use Illuminate\Support\Facades\Event;
10+
use Illuminate\Support\Facades\Session;
11+
use Pterodactyl\Events\Auth\DirectLogin;
12+
use PHPUnit\Framework\Attributes\TestWith;
13+
use Pterodactyl\Tests\Integration\Http\HttpTestCase;
14+
use Pterodactyl\Events\Auth\ProvidedAuthenticationToken;
15+
16+
class LoginCheckpointControllerTest extends HttpTestCase
17+
{
18+
public function setUp(): void
19+
{
20+
parent::setUp();
21+
22+
Event::fake([Failed::class, DirectLogin::class, ProvidedAuthenticationToken::class]);
23+
}
24+
25+
/**
26+
* Basic test that a user can be signed in using their TOTP token and that
27+
* the `totp_authenticated_at` field in the database is updated to the login
28+
* verification time.
29+
*/
30+
#[TestWith([null])]
31+
#[TestWith([-31])]
32+
#[TestWith([-60])]
33+
public function testUserCanSignInUsingTotpToken(?int $ts): void
34+
{
35+
$user = User::factory()->create([
36+
'use_totp' => true,
37+
'totp_secret' => encrypt(str_repeat('a', 16)),
38+
'totp_authenticated_at' => is_null($ts) ? null : Carbon::now()->addSeconds($ts),
39+
]);
40+
41+
Session::put('auth_confirmation_token', [
42+
'user_id' => $user->id,
43+
'token_value' => 'token',
44+
'expires_at' => now()->addMinutes(5),
45+
]);
46+
47+
$totp = $this->app->make(Google2FA::class)->getCurrentOtp(str_repeat('a', 16));
48+
49+
$this->withoutExceptionHandling()->postJson(route('auth.login-checkpoint', [
50+
'confirmation_token' => 'token',
51+
'authentication_code' => $totp,
52+
]))
53+
->assertOk()
54+
->assertSessionMissing('auth_confirmation_token')
55+
->assertJsonPath('data.complete', true)
56+
->assertJsonPath('data.intended', '/')
57+
->assertJsonPath('data.user.uuid', $user->uuid);
58+
59+
$this->assertEquals(now(), $user->refresh()->totp_authenticated_at);
60+
61+
$this->assertAuthenticatedAs($user);
62+
63+
Event::assertDispatched(fn (DirectLogin $event) => $event->user->is($user) && $event->remember);
64+
Event::assertDispatched(fn (ProvidedAuthenticationToken $event) => $event->user->is($user));
65+
}
66+
67+
/**
68+
* Test that a TOTP token cannot be reused by verifying that the OTP verification
69+
* logic fails if the token's timestamp is before the `totp_authenticated_at`
70+
* column value.
71+
*
72+
* @see https://github.com/pterodactyl/panel/security/advisories/GHSA-rgmp-4873-r683
73+
*/
74+
#[TestWith([1])]
75+
#[TestWith([30])]
76+
#[TestWith([80])]
77+
public function testTotpTokenCannotBeReused(int $seconds): void
78+
{
79+
$user = User::factory()->create([
80+
'use_totp' => true,
81+
'totp_secret' => encrypt(str_repeat('a', 16)),
82+
'totp_authenticated_at' => now()->addSeconds($seconds),
83+
]);
84+
85+
Session::put('auth_confirmation_token', [
86+
'user_id' => $user->id,
87+
'token_value' => 'token',
88+
'expires_at' => now()->addMinutes(5),
89+
]);
90+
91+
$totp = $this->app->make(Google2FA::class)->getCurrentOtp(str_repeat('a', 16));
92+
93+
$this->postJson(route('auth.login-checkpoint', [
94+
'confirmation_token' => 'token',
95+
'authentication_code' => $totp,
96+
]))
97+
->assertBadRequest()
98+
->assertJsonPath('errors.0.detail', 'The two-factor authentication token was invalid.');
99+
100+
$this->assertGuest();
101+
$this->assertEquals(now()->addSeconds($seconds), $user->refresh()->totp_authenticated_at);
102+
103+
Event::assertDispatched(fn (Failed $event) => $event->guard === 'auth' && $event->user->is($user));
104+
}
105+
106+
public function testEndpointReturnsErrorIfSessionMissing(): void
107+
{
108+
$this->postJson(route('auth.login-checkpoint'))
109+
->assertUnprocessable()
110+
->assertJsonPath('errors.0.meta.source_field', 'confirmation_token')
111+
->assertJsonPath('errors.1.meta.source_field', 'authentication_code')
112+
->assertJsonPath('errors.2.meta.source_field', 'recovery_token');
113+
114+
$this->postJson(route('auth.login-checkpoint', [
115+
'confirmation_token' => 'token',
116+
'authentication_code' => '123456',
117+
]))
118+
->assertBadRequest()
119+
->assertJsonPath('errors.0.detail', 'The authentication token provided has expired, please refresh the page and try again.');
120+
121+
$this->assertGuest();
122+
123+
Event::assertDispatched(fn (Failed $event) => $event->guard === 'auth');
124+
}
125+
126+
public function testEndpointAppliesThrottling(): void
127+
{
128+
for ($i = 0; $i < 5; ++$i) {
129+
$this->postJson(route('auth.login-checkpoint', ['confirmation_token' => 'token', 'authentication_code' => '123456']))
130+
->assertBadRequest();
131+
}
132+
133+
$this->postJson(route('auth.login-checkpoint', ['confirmation_token' => 'token', 'authentication_code' => '123456']))
134+
->assertTooManyRequests();
135+
}
136+
137+
public function testEndpointBlocksSessionDataMismatch(): void
138+
{
139+
$user = User::factory()->create([
140+
'use_totp' => true,
141+
'totp_secret' => str_repeat('a', 16),
142+
]);
143+
144+
Session::put('auth_confirmation_token', [
145+
'user_id' => $user->id,
146+
'token_value' => 'token',
147+
'expires_at' => now()->addMinutes(5),
148+
]);
149+
150+
$this->postJson(route('auth.login-checkpoint', [
151+
'confirmation_token' => 'wrong-token',
152+
'authentication_code' => $this->app->make(Google2FA::class)->getCurrentOtp(str_repeat('a', 16)),
153+
]))
154+
->assertBadRequest();
155+
156+
$this->assertGuest();
157+
158+
Event::assertDispatched(Failed::class);
159+
}
160+
161+
public function testEndpointReturnsErrorIfUserDoesNotExist(): void
162+
{
163+
Session::put('auth_confirmation_token', [
164+
'user_id' => 0,
165+
'token_value' => 'token',
166+
'expires_at' => now()->addMinutes(5),
167+
]);
168+
169+
$this->postJson(route('auth.login-checkpoint', [
170+
'confirmation_token' => 'token',
171+
'authentication_code' => '123456',
172+
]))
173+
->assertBadRequest()
174+
->assertJsonPath('errors.0.detail', 'The authentication token provided has expired, please refresh the page and try again.');
175+
}
176+
177+
public function testEndpointAllowsRecoveryToken(): void
178+
{
179+
$user = User::factory()->create();
180+
$token = $user->recoveryTokens()->forceCreate(['token' => password_hash('recovery', PASSWORD_DEFAULT)]);
181+
182+
Session::put('auth_confirmation_token', [
183+
'user_id' => $user->id,
184+
'token_value' => 'token',
185+
'expires_at' => now()->addMinutes(5),
186+
]);
187+
188+
$this->postJson(route('auth.login-checkpoint', [
189+
'confirmation_token' => 'token',
190+
'recovery_token' => 'invalid',
191+
]))
192+
->assertBadRequest()
193+
->assertJsonPath('errors.0.detail', 'The recovery token provided is not valid.');
194+
195+
$this->assertGuest();
196+
197+
$this->postJson(route('auth.login-checkpoint', [
198+
'confirmation_token' => 'token',
199+
'recovery_token' => 'recovery',
200+
]))
201+
->assertOk()
202+
->assertSessionMissing('auth_confirmation_token');
203+
204+
Event::assertDispatched(ProvidedAuthenticationToken::class);
205+
Event::assertDispatched(DirectLogin::class);
206+
}
207+
}

‎tests/TestCase.php‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,10 @@ public function setUp(): void
1515
{
1616
parent::setUp();
1717

18-
Carbon::setTestNow(Carbon::now());
19-
CarbonImmutable::setTestNow(Carbon::now());
18+
$now = Carbon::now()->startOfSecond();
19+
20+
Carbon::setTestNow($now);
21+
CarbonImmutable::setTestNow($now);
2022

2123
// Why, you ask? If we don't force this to false it is possible for certain exceptions
2224
// to show their error message properly in the integration test output, but not actually

0 commit comments

Comments
 (0)