From 4a84c36009be10dbd83051ac1771662c056e4977 Mon Sep 17 00:00:00 2001 From: Dane Everitt Date: Tue, 21 Sep 2021 21:30:08 -0700 Subject: [PATCH] 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 --- .../Auth/AbstractLoginController.php | 31 ++--- .../Auth/LoginCheckpointController.php | 107 +++++++++--------- app/Http/Controllers/Auth/LoginController.php | 56 ++++----- 3 files changed, 85 insertions(+), 109 deletions(-) diff --git a/app/Http/Controllers/Auth/AbstractLoginController.php b/app/Http/Controllers/Auth/AbstractLoginController.php index 601cdc142..e50a8050f 100644 --- a/app/Http/Controllers/Auth/AbstractLoginController.php +++ b/app/Http/Controllers/Auth/AbstractLoginController.php @@ -7,7 +7,7 @@ use Pterodactyl\Models\User; use Illuminate\Auth\AuthManager; use Illuminate\Http\JsonResponse; use Illuminate\Auth\Events\Failed; -use Illuminate\Contracts\Config\Repository; +use Illuminate\Container\Container; use Pterodactyl\Exceptions\DisplayException; use Pterodactyl\Http\Controllers\Controller; use Illuminate\Contracts\Auth\Authenticatable; @@ -17,6 +17,8 @@ abstract class AbstractLoginController extends Controller { use AuthenticatesUsers; + protected AuthManager $auth; + /** * Lockout time for failed login requests. * @@ -38,26 +40,14 @@ abstract class AbstractLoginController extends Controller */ protected $redirectTo = '/'; - /** - * @var \Illuminate\Auth\AuthManager - */ - protected $auth; - - /** - * @var \Illuminate\Contracts\Config\Repository - */ - protected $config; - /** * LoginController constructor. */ - public function __construct(AuthManager $auth, Repository $config) + public function __construct() { - $this->lockoutTime = $config->get('auth.lockout.time'); - $this->maxLoginAttempts = $config->get('auth.lockout.attempts'); - - $this->auth = $auth; - $this->config = $config; + $this->lockoutTime = config('auth.lockout.time'); + $this->maxLoginAttempts = config('auth.lockout.attempts'); + $this->auth = Container::getInstance()->make(AuthManager::class); } /** @@ -84,12 +74,14 @@ abstract class AbstractLoginController extends Controller */ protected function sendLoginResponse(User $user, Request $request): JsonResponse { + $request->session()->remove('auth_confirmation_token'); $request->session()->regenerate(); + $this->clearLoginAttempts($request); $this->auth->guard()->login($user, true); - return JsonResponse::create([ + return new JsonResponse([ 'data' => [ 'complete' => true, 'intended' => $this->redirectPath(), @@ -101,7 +93,8 @@ abstract class AbstractLoginController extends Controller /** * Determine if the user is logging in using an email or username,. * - * @param string $input + * @param string|null $input + * @return string */ protected function getField(string $input = null): string { diff --git a/app/Http/Controllers/Auth/LoginCheckpointController.php b/app/Http/Controllers/Auth/LoginCheckpointController.php index 3710da3a2..fdb5b7e13 100644 --- a/app/Http/Controllers/Auth/LoginCheckpointController.php +++ b/app/Http/Controllers/Auth/LoginCheckpointController.php @@ -2,64 +2,36 @@ namespace Pterodactyl\Http\Controllers\Auth; +use Carbon\CarbonInterface; +use Carbon\CarbonImmutable; use Pterodactyl\Models\User; -use Illuminate\Auth\AuthManager; use Illuminate\Http\JsonResponse; use PragmaRX\Google2FA\Google2FA; -use Illuminate\Contracts\Config\Repository; use Illuminate\Contracts\Encryption\Encrypter; use Illuminate\Database\Eloquent\ModelNotFoundException; use Pterodactyl\Http\Requests\Auth\LoginCheckpointRequest; -use Illuminate\Contracts\Cache\Repository as CacheRepository; -use Pterodactyl\Contracts\Repository\UserRepositoryInterface; -use Pterodactyl\Repositories\Eloquent\RecoveryTokenRepository; +use Illuminate\Contracts\Validation\Factory as ValidationFactory; class LoginCheckpointController extends AbstractLoginController { - /** - * @var \Illuminate\Contracts\Cache\Repository - */ - private $cache; + private const TOKEN_EXPIRED_MESSAGE = 'The authentication token provided has expired, please refresh the page and try again.'; - /** - * @var \Pterodactyl\Contracts\Repository\UserRepositoryInterface - */ - private $repository; + private ValidationFactory $validation; - /** - * @var \PragmaRX\Google2FA\Google2FA - */ - private $google2FA; + private Google2FA $google2FA; - /** - * @var \Illuminate\Contracts\Encryption\Encrypter - */ - private $encrypter; - - /** - * @var \Pterodactyl\Repositories\Eloquent\RecoveryTokenRepository - */ - private $recoveryTokenRepository; + private Encrypter $encrypter; /** * LoginCheckpointController constructor. */ - public function __construct( - AuthManager $auth, - Encrypter $encrypter, - Google2FA $google2FA, - Repository $config, - CacheRepository $cache, - RecoveryTokenRepository $recoveryTokenRepository, - UserRepositoryInterface $repository - ) { - parent::__construct($auth, $config); + public function __construct(Encrypter $encrypter, Google2FA $google2FA, ValidationFactory $validation) + { + parent::__construct(); $this->google2FA = $google2FA; - $this->cache = $cache; - $this->repository = $repository; $this->encrypter = $encrypter; - $this->recoveryTokenRepository = $recoveryTokenRepository; + $this->validation = $validation; } /** @@ -81,18 +53,20 @@ class LoginCheckpointController extends AbstractLoginController $this->sendLockoutResponse($request); } - $token = $request->input('confirmation_token'); + $details = $request->session()->get('auth_confirmation_token'); + if (!$this->hasValidSessionData($details)) { + $this->sendFailedLoginResponse($request, null, self::TOKEN_EXPIRED_MESSAGE); + } + + if (!hash_equals($request->input('confirmation_token') ?? '', $details['token_value'])) { + $this->sendFailedLoginResponse($request); + } + try { /** @var \Pterodactyl\Models\User $user */ - $user = User::query()->findOrFail($this->cache->get($token, 0)); + $user = User::query()->findOrFail($details['user_id']); } catch (ModelNotFoundException $exception) { - $this->incrementLoginAttempts($request); - - return $this->sendFailedLoginResponse( - $request, - null, - 'The authentication token provided has expired, please refresh the page and try again.' - ); + $this->sendFailedLoginResponse($request, null, self::TOKEN_EXPIRED_MESSAGE); } // Recovery tokens go through a slightly different pathway for usage. @@ -104,15 +78,11 @@ class LoginCheckpointController extends AbstractLoginController $decrypted = $this->encrypter->decrypt($user->totp_secret); if ($this->google2FA->verifyKey($decrypted, (string) $request->input('authentication_code') ?? '', config('pterodactyl.auth.2fa.window'))) { - $this->cache->delete($token); - return $this->sendLoginResponse($user, $request); } } - $this->incrementLoginAttempts($request); - - return $this->sendFailedLoginResponse($request, $user, !empty($recoveryToken) ? 'The recovery token provided is not valid.' : null); + $this->sendFailedLoginResponse($request, $user, !empty($recoveryToken) ? 'The recovery token provided is not valid.' : null); } /** @@ -135,4 +105,35 @@ class LoginCheckpointController extends AbstractLoginController return false; } + + /** + * Determines if the data provided from the session is valid or not. This + * will return false if the data is invalid, or if more time has passed than + * was configured when the session was written. + * + * @param array $data + * @return bool + */ + protected function hasValidSessionData(array $data): bool + { + $validator = $this->validation->make($data, [ + 'user_id' => 'required|integer|min:1', + 'token_value' => 'required|string', + 'expires_at' => 'required', + ]); + + if ($validator->fails()) { + return false; + } + + if (!$data['expires_at'] instanceof CarbonInterface) { + return false; + } + + if ($data['expires_at']->isBefore(CarbonImmutable::now())) { + return false; + } + + return true; + } } diff --git a/app/Http/Controllers/Auth/LoginController.php b/app/Http/Controllers/Auth/LoginController.php index db74ab125..3c477c556 100644 --- a/app/Http/Controllers/Auth/LoginController.php +++ b/app/Http/Controllers/Auth/LoginController.php @@ -5,47 +5,24 @@ namespace Pterodactyl\Http\Controllers\Auth; use Carbon\CarbonImmutable; use Illuminate\Support\Str; use Illuminate\Http\Request; -use Illuminate\Auth\AuthManager; +use Pterodactyl\Models\User; use Illuminate\Http\JsonResponse; use Illuminate\Contracts\View\View; -use Illuminate\Contracts\Config\Repository; use Illuminate\Contracts\View\Factory as ViewFactory; -use Illuminate\Contracts\Cache\Repository as CacheRepository; -use Pterodactyl\Contracts\Repository\UserRepositoryInterface; -use Pterodactyl\Exceptions\Repository\RecordNotFoundException; +use Illuminate\Database\Eloquent\ModelNotFoundException; class LoginController extends AbstractLoginController { - /** - * @var \Illuminate\Contracts\View\Factory - */ - private $view; - - /** - * @var \Illuminate\Contracts\Cache\Repository - */ - private $cache; - - /** - * @var \Pterodactyl\Contracts\Repository\UserRepositoryInterface - */ - private $repository; + private ViewFactory $view; /** * LoginController constructor. */ - public function __construct( - AuthManager $auth, - Repository $config, - CacheRepository $cache, - UserRepositoryInterface $repository, - ViewFactory $view - ) { - parent::__construct($auth, $config); + public function __construct(ViewFactory $view) + { + parent::__construct(); $this->view = $view; - $this->cache = $cache; - $this->repository = $repository; } /** @@ -68,18 +45,18 @@ class LoginController extends AbstractLoginController */ public function login(Request $request): JsonResponse { - $username = $request->input('user'); - $useColumn = $this->getField($username); - if ($this->hasTooManyLoginAttempts($request)) { $this->fireLockoutEvent($request); $this->sendLockoutResponse($request); } try { - $user = $this->repository->findFirstWhere([[$useColumn, '=', $username]]); - } catch (RecordNotFoundException $exception) { - return $this->sendFailedLoginResponse($request); + $username = $request->input('user'); + + /** @var \Pterodactyl\Models\User $user */ + $user = User::query()->where($this->getField($username), $username)->firstOrFail(); + } catch (ModelNotFoundException $exception) { + $this->sendFailedLoginResponse($request); } // Ensure that the account is using a valid username and password before trying to @@ -87,12 +64,17 @@ class LoginController extends AbstractLoginController // a flaw in which you can discover if an account exists simply by seeing if you // can proceede to the next step in the login process. if (!password_verify($request->input('password'), $user->password)) { - return $this->sendFailedLoginResponse($request, $user); + $this->sendFailedLoginResponse($request, $user); } if ($user->use_totp) { $token = Str::random(64); - $this->cache->put($token, $user->id, CarbonImmutable::now()->addMinutes(5)); + + $request->session()->put('auth_confirmation_token', [ + 'user_id' => $user->id, + 'token_value' => $token, + 'expires_at' => CarbonImmutable::now()->addMinutes(5), + ]); return new JsonResponse([ 'data' => [