Skip to content

Commit ef82119

Browse files
ssddanbrownssddanbrown
committed
MFA: Added verify attempt rate limiting
1 parent 1b9ec75 commit ef82119

7 files changed

Lines changed: 135 additions & 2 deletions

File tree

‎app/Access/Controllers/HandlesPartialLogins.php‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,4 +22,10 @@ protected function currentOrLastAttemptedUser(): User
2222

2323
return $user;
2424
}
25+
26+
protected function clearLastAttemptedUser(): void
27+
{
28+
$loginService = app()->make(LoginService::class);
29+
$loginService->clearLastLoginAttempted();
30+
}
2531
}

‎app/Access/Controllers/MfaBackupCodesController.php‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
use BookStack\Access\Mfa\BackupCodeService;
77
use BookStack\Access\Mfa\MfaSession;
88
use BookStack\Access\Mfa\MfaValue;
9+
use BookStack\Access\Mfa\MfaVerificationLimiter;
910
use BookStack\Activity\ActivityType;
1011
use BookStack\Exceptions\NotFoundException;
1112
use BookStack\Http\Controller;
@@ -19,6 +20,11 @@ class MfaBackupCodesController extends Controller
1920

2021
protected const SETUP_SECRET_SESSION_KEY = 'mfa-setup-backup-codes';
2122

23+
public function __construct(
24+
protected MfaVerificationLimiter $limiter,
25+
) {
26+
}
27+
2228
/**
2329
* Show a view that generates and displays backup codes.
2430
*/
@@ -71,6 +77,12 @@ public function confirm()
7177
public function verify(Request $request, BackupCodeService $codeService, MfaSession $mfaSession, LoginService $loginService)
7278
{
7379
$user = $this->currentOrLastAttemptedUser();
80+
$this->limiter->incrementAttempts($user, $request);
81+
if ($this->limiter->hasHitLimit($user, $request)) {
82+
$this->clearLastAttemptedUser();
83+
$this->limiter->throwException();
84+
}
85+
7486
$codes = MfaValue::getValueForUser($user, MfaValue::METHOD_BACKUP_CODES) ?? '[]';
7587

7688
$this->validate($request, [
@@ -89,6 +101,7 @@ function ($attribute, $value, $fail) use ($codeService, $codes) {
89101

90102
$mfaSession->markVerifiedForUser($user);
91103
$loginService->reattemptLoginFor($user);
104+
$this->limiter->decrementAttempts($user, $request);
92105

93106
if ($codeService->countCodesInSet($updatedCodes) < 5) {
94107
$this->showWarningNotification(trans('auth.mfa_backup_codes_usage_limit_warning'));

‎app/Access/Controllers/MfaTotpController.php‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
use BookStack\Access\LoginService;
66
use BookStack\Access\Mfa\MfaSession;
77
use BookStack\Access\Mfa\MfaValue;
8+
use BookStack\Access\Mfa\MfaVerificationLimiter;
89
use BookStack\Access\Mfa\TotpService;
910
use BookStack\Access\Mfa\TotpValidationRule;
1011
use BookStack\Activity\ActivityType;
@@ -20,7 +21,8 @@ class MfaTotpController extends Controller
2021
protected const SETUP_SECRET_SESSION_KEY = 'mfa-setup-totp-secret';
2122

2223
public function __construct(
23-
protected TotpService $totp
24+
protected TotpService $totp,
25+
protected MfaVerificationLimiter $limiter,
2426
) {
2527
}
2628

@@ -86,6 +88,12 @@ public function confirm(Request $request)
8688
public function verify(Request $request, LoginService $loginService, MfaSession $mfaSession)
8789
{
8890
$user = $this->currentOrLastAttemptedUser();
91+
$this->limiter->incrementAttempts($user, $request);
92+
if ($this->limiter->hasHitLimit($user, $request)) {
93+
$this->clearLastAttemptedUser();
94+
$this->limiter->throwException();
95+
}
96+
8997
$totpSecret = MfaValue::getValueForUser($user, MfaValue::METHOD_TOTP);
9098

9199
$this->validate($request, [
@@ -98,6 +106,7 @@ public function verify(Request $request, LoginService $loginService, MfaSession
98106

99107
$mfaSession->markVerifiedForUser($user);
100108
$loginService->reattemptLoginFor($user);
109+
$this->limiter->decrementAttempts($user, $request);
101110

102111
return redirect()->intended();
103112
}

‎app/Access/LoginService.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,7 @@ protected function setLastLoginAttemptedForUser(User $user, string $method, bool
126126
/**
127127
* Clear the last login attempted session value.
128128
*/
129-
protected function clearLastLoginAttempted(): void
129+
public function clearLastLoginAttempted(): void
130130
{
131131
session()->remove(self::LAST_LOGIN_ATTEMPTED_SESSION_KEY);
132132
}

‎app/Access/Mfa/MfaVerificationLimiter.php‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
<?php
2+
3+
namespace BookStack\Access\Mfa;
4+
5+
use BookStack\Exceptions\NotifyException;
6+
use BookStack\Users\Models\User;
7+
use Illuminate\Cache\RateLimiter;
8+
use Illuminate\Http\Request;
9+
use Illuminate\Http\Response;
10+
11+
/**
12+
* A rate limit specifically for MFA verification.
13+
* Limits across both the attempted user (on a tight limit) and the
14+
* request IP (on a less strict limit).
15+
*/
16+
class MfaVerificationLimiter
17+
{
18+
protected int $maxUserAttemptsPerMinute = 5;
19+
protected int $maxIpAttemptsPerMinute = 60;
20+
21+
public function __construct(
22+
protected RateLimiter $rateLimiter
23+
) {
24+
}
25+
26+
public function throwException(): never
27+
{
28+
throw new NotifyException(
29+
trans('auth.mfa_throttle', ['seconds' => 60]),
30+
'/login',
31+
Response::HTTP_TOO_MANY_REQUESTS
32+
);
33+
}
34+
35+
public function incrementAttempts(User $user, Request $request): void
36+
{
37+
$this->rateLimiter->hit($this->getUserKey($user));
38+
$this->rateLimiter->hit($this->getRequestKey($request));
39+
}
40+
41+
public function decrementAttempts(User $user, Request $request): void
42+
{
43+
$this->rateLimiter->decrement($this->getUserKey($user));
44+
$this->rateLimiter->decrement($this->getRequestKey($request));
45+
}
46+
47+
public function hasHitLimit(User $user, Request $request): bool
48+
{
49+
return $this->rateLimiter->tooManyAttempts($this->getUserKey($user), $this->maxUserAttemptsPerMinute + 1)
50+
|| $this->rateLimiter->tooManyAttempts($this->getRequestKey($request), $this->maxIpAttemptsPerMinute + 1);
51+
}
52+
53+
protected function getUserKey(User $user): string
54+
{
55+
return "mfa-attempt::user::{$user->id}";
56+
}
57+
58+
protected function getRequestKey(Request $request): string
59+
{
60+
return "mfa-attempt::request::{$request->ip()}";
61+
}
62+
}

‎lang/en/auth.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88

99
'failed' => 'These credentials do not match our records.',
1010
'throttle' => 'Too many login attempts. Please try again in :seconds seconds.',
11+
'mfa_throttle' => 'Too many multi-factor verification attempts. Please try again in :seconds seconds.',
1112

1213
// Login & Register
1314
'sign_up' => 'Sign up',

‎tests/Auth/MfaVerificationTest.php‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,27 @@ public function test_totp_form_has_autofill_configured()
6666
$html->assertElementExists('input[autocomplete="one-time-code"][name="code"]');
6767
}
6868

69+
public function test_totp_verification_is_rate_limited()
70+
{
71+
[$user, $secret, $loginResp] = $this->startTotpLogin();
72+
$loginService = $this->app->make(LoginService::class);
73+
74+
$resp = $this->get('/mfa/verify');
75+
for ($i = 0; $i < 5; $i++) {
76+
$this->post('/mfa/totp/verify', [
77+
'code' => '123456',
78+
])->assertRedirect('/mfa/verify');
79+
$this->assertNotNull($loginService->getLastLoginAttemptUser());
80+
}
81+
82+
$resp = $this->post('/mfa/totp/verify', [
83+
'code' => '123456',
84+
]);
85+
$resp->assertRedirect('/login');
86+
$this->assertSessionError('Too many multi-factor verification attempts. Please try again in 60 seconds.');
87+
$this->assertNull($loginService->getLastLoginAttemptUser());
88+
}
89+
6990
public function test_backup_code_verification()
7091
{
7192
[$user, $codes, $loginResp] = $this->startBackupCodeLogin();
@@ -147,6 +168,27 @@ public function test_backup_code_verification_shows_warning_when_limited_codes_r
147168
$resp->assertSeeText('You have less than 5 backup codes remaining, Please generate and store a new set before you run out of codes to prevent being locked out of your account.');
148169
}
149170

171+
public function test_backup_code_verification_is_rate_limited()
172+
{
173+
[$user, $codes, $loginResp] = $this->startBackupCodeLogin(['abc12-def45', 'abc12-def46']);
174+
$loginService = $this->app->make(LoginService::class);
175+
176+
$resp = $this->get('/mfa/verify');
177+
for ($i = 0; $i < 5; $i++) {
178+
$this->post('/mfa/backup_codes/verify', [
179+
'code' => '123456abcd',
180+
])->assertRedirect('/mfa/verify');
181+
$this->assertNotNull($loginService->getLastLoginAttemptUser());
182+
}
183+
184+
$resp = $this->post('/mfa/backup_codes/verify', [
185+
'code' => '123456abcd',
186+
]);
187+
$resp->assertRedirect('/login');
188+
$this->assertSessionError('Too many multi-factor verification attempts. Please try again in 60 seconds.');
189+
$this->assertNull($loginService->getLastLoginAttemptUser());
190+
}
191+
150192
public function test_backup_code_form_has_autofill_configured()
151193
{
152194
[$user, $codes, $loginResp] = $this->startBackupCodeLogin();

0 commit comments

Comments
 (0)