Skip to content

Commit e8dcd30

Browse files
committed
[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.
1 parent eecd550 commit e8dcd30

File tree

4 files changed

+96
-72
lines changed

4 files changed

+96
-72
lines changed

app/Http/Middleware/Api/Client/Server/AllocationBelongsToServer.php

Lines changed: 0 additions & 33 deletions
This file was deleted.
Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,92 @@
1+
<?php
2+
3+
namespace Pterodactyl\Http\Middleware\Api\Client\Server;
4+
5+
use Closure;
6+
use Illuminate\Http\Request;
7+
use Pterodactyl\Models\Task;
8+
use Pterodactyl\Models\User;
9+
use InvalidArgumentException;
10+
use Pterodactyl\Models\Server;
11+
use Pterodactyl\Models\Backup;
12+
use Pterodactyl\Models\Subuser;
13+
use Pterodactyl\Models\Schedule;
14+
use Pterodactyl\Models\Database;
15+
use Pterodactyl\Models\Allocation;
16+
use Illuminate\Database\Eloquent\Model;
17+
use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
18+
19+
class ResourceBelongsToServer
20+
{
21+
/**
22+
* Looks at the request parameters to determine if the given resource belongs
23+
* to the requested server. If not, a 404 error will be returned to the caller.
24+
*
25+
* This is critical to ensuring that all subsequent logic is using exactly the
26+
* server that is expected, and that we're not accessing a resource completely
27+
* unrelated to the server provided in the request.
28+
*
29+
* @param \Illuminate\Http\Request $request
30+
* @param \Closure $next
31+
* @return mixed
32+
*/
33+
public function handle(Request $request, Closure $next)
34+
{
35+
$params = $request->route()->parameters();
36+
if (is_null($params) || ! $params['server'] instanceof Server) {
37+
throw new InvalidArgumentException('This middleware cannot be used in a context that is missing a server in the parameters.');
38+
}
39+
40+
/** @var \Pterodactyl\Models\Server $server */
41+
$server = $request->route()->parameter('server');
42+
$exception = new NotFoundHttpException('The requested resource was not found for this server.');
43+
foreach ($params as $key => $model) {
44+
// Specifically skip the server, we're just trying to see if all of the
45+
// other resources are assigned to this server. Also skip anything that
46+
// is not currently a Model instance since those will just end up being
47+
// a 404 down the road.
48+
if ($key === 'server' || ! $model instanceof Model) {
49+
continue;
50+
}
51+
52+
switch (get_class($model)) {
53+
// All of these models use "server_id" as the field key for the server
54+
// they are assigned to, so the logic is identical for them all.
55+
case Allocation::class:
56+
case Backup::class:
57+
case Database::class:
58+
case Schedule::class:
59+
case Subuser::class:
60+
if ($model->server_id !== $server->id) {
61+
throw $exception;
62+
}
63+
break;
64+
// Regular users are a special case here as we need to make sure they're
65+
// currently assigned as a subuser on the server.
66+
case User::class:
67+
$subuser = $server->subusers()->where('user_id', $model->id)->first();
68+
if (is_null($subuser)) {
69+
throw $exception;
70+
}
71+
// This is a special case to avoid an additional query being triggered
72+
// in the underlying logic.
73+
$request->attributes->set('subuser', $subuser);
74+
break;
75+
// Tasks are special since they're (currently) the only item in the API
76+
// that requires something in addition to the server in order to be accessed.
77+
case Task::class:
78+
$schedule = $request->route()->parameter('schedule');
79+
if ($model->schedule_id !== $schedule->id || $schedule->server_id !== $server->id) {
80+
throw $exception;
81+
}
82+
break;
83+
default:
84+
// Don't return a 404 here since we want to make sure no one relies
85+
// on this middleware in a context in which it will not work. Fail safe.
86+
throw new InvalidArgumentException('There is no handler configured for a resource of this type: ' . get_class($model));
87+
}
88+
}
89+
90+
return $next($request);
91+
}
92+
}

app/Http/Middleware/Api/Client/Server/SubuserBelongsToServer.php

Lines changed: 0 additions & 36 deletions
This file was deleted.

routes/api-client.php

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
use Illuminate\Support\Facades\Route;
44
use Pterodactyl\Http\Middleware\RequireTwoFactorAuthentication;
55
use Pterodactyl\Http\Middleware\Api\Client\Server\SubuserBelongsToServer;
6+
use Pterodactyl\Http\Middleware\Api\Client\Server\ResourceBelongsToServer;
67
use Pterodactyl\Http\Middleware\Api\Client\Server\AuthenticateServerAccess;
78
use Pterodactyl\Http\Middleware\Api\Client\Server\AllocationBelongsToServer;
89

@@ -39,7 +40,7 @@
3940
| Endpoint: /api/client/servers/{server}
4041
|
4142
*/
42-
Route::group(['prefix' => '/servers/{server}', 'middleware' => [AuthenticateServerAccess::class]], function () {
43+
Route::group(['prefix' => '/servers/{server}', 'middleware' => [AuthenticateServerAccess::class, ResourceBelongsToServer::class]], function () {
4344
Route::get('/', 'Servers\ServerController@index')->name('api:client:server.view');
4445
Route::get('/websocket', 'Servers\WebsocketController')->name('api:client:server.ws');
4546
Route::get('/resources', 'Servers\ResourceUtilizationController')->name('api:client:server.resources');
@@ -83,15 +84,15 @@
8384
Route::delete('/{schedule}/tasks/{task}', 'Servers\ScheduleTaskController@delete');
8485
});
8586

86-
Route::group(['prefix' => '/network', 'middleware' => [AllocationBelongsToServer::class]], function () {
87+
Route::group(['prefix' => '/network'], function () {
8788
Route::get('/allocations', 'Servers\NetworkAllocationController@index');
8889
Route::post('/allocations', 'Servers\NetworkAllocationController@store');
8990
Route::post('/allocations/{allocation}', 'Servers\NetworkAllocationController@update');
9091
Route::post('/allocations/{allocation}/primary', 'Servers\NetworkAllocationController@setPrimary');
9192
Route::delete('/allocations/{allocation}', 'Servers\NetworkAllocationController@delete');
9293
});
9394

94-
Route::group(['prefix' => '/users', 'middleware' => [SubuserBelongsToServer::class]], function () {
95+
Route::group(['prefix' => '/users'], function () {
9596
Route::get('/', 'Servers\SubuserController@index');
9697
Route::post('/', 'Servers\SubuserController@store');
9798
Route::get('/{user}', 'Servers\SubuserController@view');

0 commit comments

Comments
 (0)