From bf9cbe2c6d5266c6914223e067c56175de7fc3a5 Mon Sep 17 00:00:00 2001 From: Dane Everitt Date: Tue, 16 Nov 2021 20:02:18 -0800 Subject: [PATCH] Add consistent CSRF token verification to API endpoints; address security concern with non-CSRF protected endpoints --- app/Http/Kernel.php | 2 + app/Http/Middleware/Api/AuthenticateKey.php | 3 +- app/Http/Middleware/VerifyCsrfToken.php | 41 +++++++++++++++---- resources/scripts/api/http.ts | 13 +++++- .../admin/nodes/view/configuration.blade.php | 6 ++- resources/views/admin/settings/mail.blade.php | 4 +- routes/admin.php | 4 +- 7 files changed, 59 insertions(+), 14 deletions(-) diff --git a/app/Http/Kernel.php b/app/Http/Kernel.php index 04a663f33..3fac902c1 100644 --- a/app/Http/Kernel.php +++ b/app/Http/Kernel.php @@ -75,6 +75,7 @@ class Kernel extends HttpKernel ApiSubstituteBindings::class, 'api..key:' . ApiKey::TYPE_APPLICATION, AuthenticateApplicationUser::class, + VerifyCsrfToken::class, AuthenticateIPAccess::class, ], 'client-api' => [ @@ -85,6 +86,7 @@ class Kernel extends HttpKernel SubstituteClientApiBindings::class, 'api..key:' . ApiKey::TYPE_ACCOUNT, AuthenticateIPAccess::class, + VerifyCsrfToken::class, // This is perhaps a little backwards with the Client API, but logically you'd be unable // to create/get an API key without first enabling 2FA on the account, so I suppose in the // end it makes sense. diff --git a/app/Http/Middleware/Api/AuthenticateKey.php b/app/Http/Middleware/Api/AuthenticateKey.php index 397ba8c23..eb25dac6f 100644 --- a/app/Http/Middleware/Api/AuthenticateKey.php +++ b/app/Http/Middleware/Api/AuthenticateKey.php @@ -8,6 +8,7 @@ use Illuminate\Http\Request; use Pterodactyl\Models\User; use Pterodactyl\Models\ApiKey; use Illuminate\Auth\AuthManager; +use Illuminate\Support\Facades\Session; use Illuminate\Contracts\Encryption\Encrypter; use Symfony\Component\HttpKernel\Exception\HttpException; use Pterodactyl\Exceptions\Repository\RecordNotFoundException; @@ -55,7 +56,7 @@ class AuthenticateKey public function handle(Request $request, Closure $next, int $keyType) { if (is_null($request->bearerToken()) && is_null($request->user())) { - throw new HttpException(401, null, null, ['WWW-Authenticate' => 'Bearer']); + throw new HttpException(401, 'A bearer token or valid user session cookie must be provided to access this endpoint.', null, ['WWW-Authenticate' => 'Bearer']); } // This is a request coming through using cookies, we have an authenticated user diff --git a/app/Http/Middleware/VerifyCsrfToken.php b/app/Http/Middleware/VerifyCsrfToken.php index 08d960353..de10dc1a8 100644 --- a/app/Http/Middleware/VerifyCsrfToken.php +++ b/app/Http/Middleware/VerifyCsrfToken.php @@ -2,18 +2,45 @@ namespace Pterodactyl\Http\Middleware; +use Closure; +use Pterodactyl\Models\ApiKey; use Illuminate\Foundation\Http\Middleware\VerifyCsrfToken as BaseVerifier; class VerifyCsrfToken extends BaseVerifier { /** - * The URIs that should be excluded from CSRF verification. + * The URIs that should be excluded from CSRF verification. These are + * never hit by the front-end, and require specific token validation + * to work. * - * @var array + * @var string[] */ - protected $except = [ - 'remote/*', - 'daemon/*', - 'api/*', - ]; + protected $except = ['remote/*', 'daemon/*']; + + /** + * Manually apply CSRF protection to routes depending on the authentication + * mechanism being used. If the API request is using an API key that exists + * in the database we can safely ignore CSRF protections, since that would be + * a manually initiated request by a user or server. + * + * All other requests should go through the standard CSRF protections that + * Laravel affords us. This code will be removed in v2 since we have switched + * to using Sanctum for the API endpoints, which handles that for us automatically. + * + * @param \Illuminate\Http\Request $request + * @param \Closure $next + * @return mixed + * + * @throws \Illuminate\Session\TokenMismatchException + */ + public function handle($request, Closure $next) + { + $key = $request->attributes->get('api_key'); + + if ($key instanceof ApiKey && $key->exists) { + return $next($request); + } + + return parent::handle($request, $next); + } } diff --git a/resources/scripts/api/http.ts b/resources/scripts/api/http.ts index a642bb16e..d9f64ede2 100644 --- a/resources/scripts/api/http.ts +++ b/resources/scripts/api/http.ts @@ -7,10 +7,21 @@ const http: AxiosInstance = axios.create({ 'X-Requested-With': 'XMLHttpRequest', Accept: 'application/json', 'Content-Type': 'application/json', - 'X-CSRF-Token': (window as any).X_CSRF_TOKEN as string || '', }, }); +http.interceptors.request.use(req => { + const cookies = document.cookie.split(';').reduce((obj, val) => { + const [ key, value ] = val.trim().split('=').map(decodeURIComponent); + + return { ...obj, [key]: value }; + }, {} as Record); + + req.headers['X-XSRF-TOKEN'] = cookies['XSRF-TOKEN'] || 'nil'; + + return req; +}); + http.interceptors.request.use(req => { if (!req.url?.endsWith('/resources') && (req.url?.indexOf('_debugbar') || -1) < 0) { store.getActions().progress.startContinuous(); diff --git a/resources/views/admin/nodes/view/configuration.blade.php b/resources/views/admin/nodes/view/configuration.blade.php index 9c7e987b5..35e3f5f0a 100644 --- a/resources/views/admin/nodes/view/configuration.blade.php +++ b/resources/views/admin/nodes/view/configuration.blade.php @@ -70,7 +70,11 @@ @parent