From e8dcd30e0cd3deae468d5351f256c6c3e44b4beb Mon Sep 17 00:00:00 2001 From: Dane Everitt Date: Tue, 19 Jan 2021 21:19:17 -0800 Subject: [PATCH] [security] fix resources not properly returning an error when they don't match the server in the URL Prior to this fix certain resources were accessible even when their assigned server was not the same as the server in the URL. This causes the resource server relationship to not match the server variable present on the request. Due to this failed logic it was possible for users to access resources they should not have been able to access otherwise for some areas of the panel. --- .../Server/AllocationBelongsToServer.php | 33 ------- .../Client/Server/ResourceBelongsToServer.php | 92 +++++++++++++++++++ .../Client/Server/SubuserBelongsToServer.php | 36 -------- routes/api-client.php | 7 +- 4 files changed, 96 insertions(+), 72 deletions(-) delete mode 100644 app/Http/Middleware/Api/Client/Server/AllocationBelongsToServer.php create mode 100644 app/Http/Middleware/Api/Client/Server/ResourceBelongsToServer.php delete mode 100644 app/Http/Middleware/Api/Client/Server/SubuserBelongsToServer.php diff --git a/app/Http/Middleware/Api/Client/Server/AllocationBelongsToServer.php b/app/Http/Middleware/Api/Client/Server/AllocationBelongsToServer.php deleted file mode 100644 index d027d563c..000000000 --- a/app/Http/Middleware/Api/Client/Server/AllocationBelongsToServer.php +++ /dev/null @@ -1,33 +0,0 @@ -route()->parameter('server'); - /** @var \Pterodactyl\Models\Allocation|null $allocation */ - $allocation = $request->route()->parameter('allocation'); - - if ($allocation && $allocation->server_id !== $server->id) { - throw new NotFoundHttpException; - } - - return $next($request); - } -} diff --git a/app/Http/Middleware/Api/Client/Server/ResourceBelongsToServer.php b/app/Http/Middleware/Api/Client/Server/ResourceBelongsToServer.php new file mode 100644 index 000000000..b57fee2e6 --- /dev/null +++ b/app/Http/Middleware/Api/Client/Server/ResourceBelongsToServer.php @@ -0,0 +1,92 @@ +route()->parameters(); + if (is_null($params) || ! $params['server'] instanceof Server) { + throw new InvalidArgumentException('This middleware cannot be used in a context that is missing a server in the parameters.'); + } + + /** @var \Pterodactyl\Models\Server $server */ + $server = $request->route()->parameter('server'); + $exception = new NotFoundHttpException('The requested resource was not found for this server.'); + foreach ($params as $key => $model) { + // Specifically skip the server, we're just trying to see if all of the + // other resources are assigned to this server. Also skip anything that + // is not currently a Model instance since those will just end up being + // a 404 down the road. + if ($key === 'server' || ! $model instanceof Model) { + continue; + } + + switch (get_class($model)) { + // All of these models use "server_id" as the field key for the server + // they are assigned to, so the logic is identical for them all. + case Allocation::class: + case Backup::class: + case Database::class: + case Schedule::class: + case Subuser::class: + if ($model->server_id !== $server->id) { + throw $exception; + } + break; + // Regular users are a special case here as we need to make sure they're + // currently assigned as a subuser on the server. + case User::class: + $subuser = $server->subusers()->where('user_id', $model->id)->first(); + if (is_null($subuser)) { + throw $exception; + } + // This is a special case to avoid an additional query being triggered + // in the underlying logic. + $request->attributes->set('subuser', $subuser); + break; + // Tasks are special since they're (currently) the only item in the API + // that requires something in addition to the server in order to be accessed. + case Task::class: + $schedule = $request->route()->parameter('schedule'); + if ($model->schedule_id !== $schedule->id || $schedule->server_id !== $server->id) { + throw $exception; + } + break; + default: + // Don't return a 404 here since we want to make sure no one relies + // on this middleware in a context in which it will not work. Fail safe. + throw new InvalidArgumentException('There is no handler configured for a resource of this type: ' . get_class($model)); + } + } + + return $next($request); + } +} diff --git a/app/Http/Middleware/Api/Client/Server/SubuserBelongsToServer.php b/app/Http/Middleware/Api/Client/Server/SubuserBelongsToServer.php deleted file mode 100644 index a80f6eefd..000000000 --- a/app/Http/Middleware/Api/Client/Server/SubuserBelongsToServer.php +++ /dev/null @@ -1,36 +0,0 @@ -route()->parameter('server'); - /** @var \Pterodactyl\Models\User $user */ - $user = $request->route()->parameter('user'); - - // Don't do anything if there isn't a user present in the request. - if (is_null($user)) { - return $next($request); - } - - $request->attributes->set('subuser', $server->subusers()->where('user_id', $user->id)->firstOrFail()); - - return $next($request); - } -} diff --git a/routes/api-client.php b/routes/api-client.php index a00938f55..a82be7151 100644 --- a/routes/api-client.php +++ b/routes/api-client.php @@ -3,6 +3,7 @@ use Illuminate\Support\Facades\Route; use Pterodactyl\Http\Middleware\RequireTwoFactorAuthentication; use Pterodactyl\Http\Middleware\Api\Client\Server\SubuserBelongsToServer; +use Pterodactyl\Http\Middleware\Api\Client\Server\ResourceBelongsToServer; use Pterodactyl\Http\Middleware\Api\Client\Server\AuthenticateServerAccess; use Pterodactyl\Http\Middleware\Api\Client\Server\AllocationBelongsToServer; @@ -39,7 +40,7 @@ Route::group(['prefix' => '/account'], function () { | Endpoint: /api/client/servers/{server} | */ -Route::group(['prefix' => '/servers/{server}', 'middleware' => [AuthenticateServerAccess::class]], function () { +Route::group(['prefix' => '/servers/{server}', 'middleware' => [AuthenticateServerAccess::class, ResourceBelongsToServer::class]], function () { Route::get('/', 'Servers\ServerController@index')->name('api:client:server.view'); Route::get('/websocket', 'Servers\WebsocketController')->name('api:client:server.ws'); Route::get('/resources', 'Servers\ResourceUtilizationController')->name('api:client:server.resources'); @@ -83,7 +84,7 @@ Route::group(['prefix' => '/servers/{server}', 'middleware' => [AuthenticateServ Route::delete('/{schedule}/tasks/{task}', 'Servers\ScheduleTaskController@delete'); }); - Route::group(['prefix' => '/network', 'middleware' => [AllocationBelongsToServer::class]], function () { + Route::group(['prefix' => '/network'], function () { Route::get('/allocations', 'Servers\NetworkAllocationController@index'); Route::post('/allocations', 'Servers\NetworkAllocationController@store'); Route::post('/allocations/{allocation}', 'Servers\NetworkAllocationController@update'); @@ -91,7 +92,7 @@ Route::group(['prefix' => '/servers/{server}', 'middleware' => [AuthenticateServ Route::delete('/allocations/{allocation}', 'Servers\NetworkAllocationController@delete'); }); - Route::group(['prefix' => '/users', 'middleware' => [SubuserBelongsToServer::class]], function () { + Route::group(['prefix' => '/users'], function () { Route::get('/', 'Servers\SubuserController@index'); Route::post('/', 'Servers\SubuserController@store'); Route::get('/{user}', 'Servers\SubuserController@view');