Skip to content

Commit 4a84c36

Browse files
DaneEverittDaneEveritt
committed
Fix security vulnerability when authenticating a two-factor authentication token for a user
See associated security advisory for technical details on the content of this security fix. GHSA ID: GHSA-5vfx-8w6m-h3v4
1 parent 5fdb0a5 commit 4a84c36

File tree

3 files changed

+85
-109
lines changed

3 files changed

+85
-109
lines changed

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

Lines changed: 12 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
use Illuminate\Auth\AuthManager;
88
use Illuminate\Http\JsonResponse;
99
use Illuminate\Auth\Events\Failed;
10-
use Illuminate\Contracts\Config\Repository;
10+
use Illuminate\Container\Container;
1111
use Pterodactyl\Exceptions\DisplayException;
1212
use Pterodactyl\Http\Controllers\Controller;
1313
use Illuminate\Contracts\Auth\Authenticatable;
@@ -17,6 +17,8 @@ abstract class AbstractLoginController extends Controller
1717
{
1818
use AuthenticatesUsers;
1919

20+
protected AuthManager $auth;
21+
2022
/**
2123
* Lockout time for failed login requests.
2224
*
@@ -38,26 +40,14 @@ abstract class AbstractLoginController extends Controller
3840
*/
3941
protected $redirectTo = '/';
4042

41-
/**
42-
* @var \Illuminate\Auth\AuthManager
43-
*/
44-
protected $auth;
45-
46-
/**
47-
* @var \Illuminate\Contracts\Config\Repository
48-
*/
49-
protected $config;
50-
5143
/**
5244
* LoginController constructor.
5345
*/
54-
public function __construct(AuthManager $auth, Repository $config)
46+
public function __construct()
5547
{
56-
$this->lockoutTime = $config->get('auth.lockout.time');
57-
$this->maxLoginAttempts = $config->get('auth.lockout.attempts');
58-
59-
$this->auth = $auth;
60-
$this->config = $config;
48+
$this->lockoutTime = config('auth.lockout.time');
49+
$this->maxLoginAttempts = config('auth.lockout.attempts');
50+
$this->auth = Container::getInstance()->make(AuthManager::class);
6151
}
6252

6353
/**
@@ -84,12 +74,14 @@ protected function sendFailedLoginResponse(Request $request, Authenticatable $us
8474
*/
8575
protected function sendLoginResponse(User $user, Request $request): JsonResponse
8676
{
77+
$request->session()->remove('auth_confirmation_token');
8778
$request->session()->regenerate();
79+
8880
$this->clearLoginAttempts($request);
8981

9082
$this->auth->guard()->login($user, true);
9183

92-
return JsonResponse::create([
84+
return new JsonResponse([
9385
'data' => [
9486
'complete' => true,
9587
'intended' => $this->redirectPath(),
@@ -101,7 +93,8 @@ protected function sendLoginResponse(User $user, Request $request): JsonResponse
10193
/**
10294
* Determine if the user is logging in using an email or username,.
10395
*
104-
* @param string $input
96+
* @param string|null $input
97+
* @return string
10598
*/
10699
protected function getField(string $input = null): string
107100
{

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

Lines changed: 54 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -2,64 +2,36 @@
22

33
namespace Pterodactyl\Http\Controllers\Auth;
44

5+
use Carbon\CarbonInterface;
6+
use Carbon\CarbonImmutable;
57
use Pterodactyl\Models\User;
6-
use Illuminate\Auth\AuthManager;
78
use Illuminate\Http\JsonResponse;
89
use PragmaRX\Google2FA\Google2FA;
9-
use Illuminate\Contracts\Config\Repository;
1010
use Illuminate\Contracts\Encryption\Encrypter;
1111
use Illuminate\Database\Eloquent\ModelNotFoundException;
1212
use Pterodactyl\Http\Requests\Auth\LoginCheckpointRequest;
13-
use Illuminate\Contracts\Cache\Repository as CacheRepository;
14-
use Pterodactyl\Contracts\Repository\UserRepositoryInterface;
15-
use Pterodactyl\Repositories\Eloquent\RecoveryTokenRepository;
13+
use Illuminate\Contracts\Validation\Factory as ValidationFactory;
1614

1715
class LoginCheckpointController extends AbstractLoginController
1816
{
19-
/**
20-
* @var \Illuminate\Contracts\Cache\Repository
21-
*/
22-
private $cache;
23-
24-
/**
25-
* @var \Pterodactyl\Contracts\Repository\UserRepositoryInterface
26-
*/
27-
private $repository;
17+
private const TOKEN_EXPIRED_MESSAGE = 'The authentication token provided has expired, please refresh the page and try again.';
2818

29-
/**
30-
* @var \PragmaRX\Google2FA\Google2FA
31-
*/
32-
private $google2FA;
19+
private ValidationFactory $validation;
3320

34-
/**
35-
* @var \Illuminate\Contracts\Encryption\Encrypter
36-
*/
37-
private $encrypter;
21+
private Google2FA $google2FA;
3822

39-
/**
40-
* @var \Pterodactyl\Repositories\Eloquent\RecoveryTokenRepository
41-
*/
42-
private $recoveryTokenRepository;
23+
private Encrypter $encrypter;
4324

4425
/**
4526
* LoginCheckpointController constructor.
4627
*/
47-
public function __construct(
48-
AuthManager $auth,
49-
Encrypter $encrypter,
50-
Google2FA $google2FA,
51-
Repository $config,
52-
CacheRepository $cache,
53-
RecoveryTokenRepository $recoveryTokenRepository,
54-
UserRepositoryInterface $repository
55-
) {
56-
parent::__construct($auth, $config);
28+
public function __construct(Encrypter $encrypter, Google2FA $google2FA, ValidationFactory $validation)
29+
{
30+
parent::__construct();
5731

5832
$this->google2FA = $google2FA;
59-
$this->cache = $cache;
60-
$this->repository = $repository;
6133
$this->encrypter = $encrypter;
62-
$this->recoveryTokenRepository = $recoveryTokenRepository;
34+
$this->validation = $validation;
6335
}
6436

6537
/**
@@ -81,18 +53,20 @@ public function __invoke(LoginCheckpointRequest $request): JsonResponse
8153
$this->sendLockoutResponse($request);
8254
}
8355

84-
$token = $request->input('confirmation_token');
56+
$details = $request->session()->get('auth_confirmation_token');
57+
if (!$this->hasValidSessionData($details)) {
58+
$this->sendFailedLoginResponse($request, null, self::TOKEN_EXPIRED_MESSAGE);
59+
}
60+
61+
if (!hash_equals($request->input('confirmation_token') ?? '', $details['token_value'])) {
62+
$this->sendFailedLoginResponse($request);
63+
}
64+
8565
try {
8666
/** @var \Pterodactyl\Models\User $user */
87-
$user = User::query()->findOrFail($this->cache->get($token, 0));
67+
$user = User::query()->findOrFail($details['user_id']);
8868
} catch (ModelNotFoundException $exception) {
89-
$this->incrementLoginAttempts($request);
90-
91-
return $this->sendFailedLoginResponse(
92-
$request,
93-
null,
94-
'The authentication token provided has expired, please refresh the page and try again.'
95-
);
69+
$this->sendFailedLoginResponse($request, null, self::TOKEN_EXPIRED_MESSAGE);
9670
}
9771

9872
// Recovery tokens go through a slightly different pathway for usage.
@@ -104,15 +78,11 @@ public function __invoke(LoginCheckpointRequest $request): JsonResponse
10478
$decrypted = $this->encrypter->decrypt($user->totp_secret);
10579

10680
if ($this->google2FA->verifyKey($decrypted, (string) $request->input('authentication_code') ?? '', config('pterodactyl.auth.2fa.window'))) {
107-
$this->cache->delete($token);
108-
10981
return $this->sendLoginResponse($user, $request);
11082
}
11183
}
11284

113-
$this->incrementLoginAttempts($request);
114-
115-
return $this->sendFailedLoginResponse($request, $user, !empty($recoveryToken) ? 'The recovery token provided is not valid.' : null);
85+
$this->sendFailedLoginResponse($request, $user, !empty($recoveryToken) ? 'The recovery token provided is not valid.' : null);
11686
}
11787

11888
/**
@@ -135,4 +105,35 @@ protected function isValidRecoveryToken(User $user, string $value)
135105

136106
return false;
137107
}
108+
109+
/**
110+
* Determines if the data provided from the session is valid or not. This
111+
* will return false if the data is invalid, or if more time has passed than
112+
* was configured when the session was written.
113+
*
114+
* @param array $data
115+
* @return bool
116+
*/
117+
protected function hasValidSessionData(array $data): bool
118+
{
119+
$validator = $this->validation->make($data, [
120+
'user_id' => 'required|integer|min:1',
121+
'token_value' => 'required|string',
122+
'expires_at' => 'required',
123+
]);
124+
125+
if ($validator->fails()) {
126+
return false;
127+
}
128+
129+
if (!$data['expires_at'] instanceof CarbonInterface) {
130+
return false;
131+
}
132+
133+
if ($data['expires_at']->isBefore(CarbonImmutable::now())) {
134+
return false;
135+
}
136+
137+
return true;
138+
}
138139
}

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

Lines changed: 19 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -5,47 +5,24 @@
55
use Carbon\CarbonImmutable;
66
use Illuminate\Support\Str;
77
use Illuminate\Http\Request;
8-
use Illuminate\Auth\AuthManager;
8+
use Pterodactyl\Models\User;
99
use Illuminate\Http\JsonResponse;
1010
use Illuminate\Contracts\View\View;
11-
use Illuminate\Contracts\Config\Repository;
1211
use Illuminate\Contracts\View\Factory as ViewFactory;
13-
use Illuminate\Contracts\Cache\Repository as CacheRepository;
14-
use Pterodactyl\Contracts\Repository\UserRepositoryInterface;
15-
use Pterodactyl\Exceptions\Repository\RecordNotFoundException;
12+
use Illuminate\Database\Eloquent\ModelNotFoundException;
1613

1714
class LoginController extends AbstractLoginController
1815
{
19-
/**
20-
* @var \Illuminate\Contracts\View\Factory
21-
*/
22-
private $view;
23-
24-
/**
25-
* @var \Illuminate\Contracts\Cache\Repository
26-
*/
27-
private $cache;
28-
29-
/**
30-
* @var \Pterodactyl\Contracts\Repository\UserRepositoryInterface
31-
*/
32-
private $repository;
16+
private ViewFactory $view;
3317

3418
/**
3519
* LoginController constructor.
3620
*/
37-
public function __construct(
38-
AuthManager $auth,
39-
Repository $config,
40-
CacheRepository $cache,
41-
UserRepositoryInterface $repository,
42-
ViewFactory $view
43-
) {
44-
parent::__construct($auth, $config);
21+
public function __construct(ViewFactory $view)
22+
{
23+
parent::__construct();
4524

4625
$this->view = $view;
47-
$this->cache = $cache;
48-
$this->repository = $repository;
4926
}
5027

5128
/**
@@ -68,31 +45,36 @@ public function index(): View
6845
*/
6946
public function login(Request $request): JsonResponse
7047
{
71-
$username = $request->input('user');
72-
$useColumn = $this->getField($username);
73-
7448
if ($this->hasTooManyLoginAttempts($request)) {
7549
$this->fireLockoutEvent($request);
7650
$this->sendLockoutResponse($request);
7751
}
7852

7953
try {
80-
$user = $this->repository->findFirstWhere([[$useColumn, '=', $username]]);
81-
} catch (RecordNotFoundException $exception) {
82-
return $this->sendFailedLoginResponse($request);
54+
$username = $request->input('user');
55+
56+
/** @var \Pterodactyl\Models\User $user */
57+
$user = User::query()->where($this->getField($username), $username)->firstOrFail();
58+
} catch (ModelNotFoundException $exception) {
59+
$this->sendFailedLoginResponse($request);
8360
}
8461

8562
// Ensure that the account is using a valid username and password before trying to
8663
// continue. Previously this was handled in the 2FA checkpoint, however that has
8764
// a flaw in which you can discover if an account exists simply by seeing if you
8865
// can proceede to the next step in the login process.
8966
if (!password_verify($request->input('password'), $user->password)) {
90-
return $this->sendFailedLoginResponse($request, $user);
67+
$this->sendFailedLoginResponse($request, $user);
9168
}
9269

9370
if ($user->use_totp) {
9471
$token = Str::random(64);
95-
$this->cache->put($token, $user->id, CarbonImmutable::now()->addMinutes(5));
72+
73+
$request->session()->put('auth_confirmation_token', [
74+
'user_id' => $user->id,
75+
'token_value' => $token,
76+
'expires_at' => CarbonImmutable::now()->addMinutes(5),
77+
]);
9678

9779
return new JsonResponse([
9880
'data' => [

0 commit comments

Comments
 (0)