diff --git a/CHANGELOG.md b/CHANGELOG.md index 7f04a6d8..e09ae67f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,9 @@ This file is a running track of new features and fixes to each version of the pa This project follows [Semantic Versioning](http://semver.org) guidelines. +## v1.2.2 +* **[security]** Fixes authentication bypass allowing a user to take control of specific server actions such as executing schedules, rotating database passwords, and viewing or deleting a backup. + ## v1.2.1 ### Fixed * Fixes URL-encoding of filenames when working in the filemanager to fix issues when moving, renaming, or deleting files. 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 d027d563..00000000 --- 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 00000000..b57fee2e --- /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 a80f6eef..00000000 --- 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/database/factories/BackupFactory.php b/database/factories/BackupFactory.php new file mode 100644 index 00000000..4bbd5ec1 --- /dev/null +++ b/database/factories/BackupFactory.php @@ -0,0 +1,32 @@ + Uuid::uuid4()->toString(), + 'is_successful' => true, + 'name' => $this->faker->sentence, + 'disk' => Backup::ADAPTER_WINGS, + ]; + } +} diff --git a/database/factories/DatabaseFactory.php b/database/factories/DatabaseFactory.php index 01933a92..f3931069 100644 --- a/database/factories/DatabaseFactory.php +++ b/database/factories/DatabaseFactory.php @@ -29,7 +29,7 @@ class DatabaseFactory extends Factory 'database' => Str::random(10), 'username' => Str::random(10), 'remote' => '%', - 'password' => $password ?: bcrypt('test123'), + 'password' => $password ?: encrypt('test123'), 'created_at' => Carbon::now(), 'updated_at' => Carbon::now(), ]; diff --git a/database/factories/SubuserFactory.php b/database/factories/SubuserFactory.php index 64f366f7..d570e733 100644 --- a/database/factories/SubuserFactory.php +++ b/database/factories/SubuserFactory.php @@ -3,6 +3,7 @@ namespace Database\Factories; use Pterodactyl\Models\Subuser; +use Pterodactyl\Models\Permission; use Illuminate\Database\Eloquent\Factories\Factory; class SubuserFactory extends Factory @@ -21,6 +22,10 @@ class SubuserFactory extends Factory */ public function definition(): array { - return []; + return [ + 'permissions' => [ + Permission::ACTION_WEBSOCKET_CONNECT + ], + ]; } } diff --git a/routes/api-client.php b/routes/api-client.php index a00938f5..a82be715 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'); diff --git a/tests/Integration/Api/Client/ClientApiIntegrationTestCase.php b/tests/Integration/Api/Client/ClientApiIntegrationTestCase.php index 56e777f6..9479780c 100644 --- a/tests/Integration/Api/Client/ClientApiIntegrationTestCase.php +++ b/tests/Integration/Api/Client/ClientApiIntegrationTestCase.php @@ -10,11 +10,15 @@ use Pterodactyl\Models\Task; use Pterodactyl\Models\User; use Webmozart\Assert\Assert; use Pterodactyl\Models\Server; +use Pterodactyl\Models\Backup; use Pterodactyl\Models\Subuser; use Pterodactyl\Models\Location; use Pterodactyl\Models\Schedule; +use Pterodactyl\Models\Database; use Illuminate\Support\Collection; use Pterodactyl\Models\Allocation; +use Pterodactyl\Models\DatabaseHost; +use Pterodactyl\Tests\Integration\TestResponse; use Pterodactyl\Tests\Integration\IntegrationTestCase; use Pterodactyl\Transformers\Api\Client\BaseClientTransformer; @@ -25,6 +29,9 @@ abstract class ClientApiIntegrationTestCase extends IntegrationTestCase */ protected function tearDown(): void { + Database::query()->forceDelete(); + DatabaseHost::query()->forceDelete(); + Backup::query()->forceDelete(); Server::query()->forceDelete(); Node::query()->forceDelete(); Location::query()->forceDelete(); @@ -44,6 +51,19 @@ abstract class ClientApiIntegrationTestCase extends IntegrationTestCase CarbonImmutable::setTestNow(Carbon::now()); } + /** + * Override the default createTestResponse from Illuminate so that we can + * just dump 500-level errors to the screen in the tests without having + * to keep re-assigning variables. + * + * @param \Illuminate\Http\Response $response + * @return \Illuminate\Testing\TestResponse + */ + protected function createTestResponse($response) + { + return TestResponse::fromBaseResponse($response); + } + /** * Returns a link to the specific resource using the client API. * diff --git a/tests/Integration/Api/Client/Server/Allocation/AllocationAuthorizationTest.php b/tests/Integration/Api/Client/Server/Allocation/AllocationAuthorizationTest.php new file mode 100644 index 00000000..e46c4620 --- /dev/null +++ b/tests/Integration/Api/Client/Server/Allocation/AllocationAuthorizationTest.php @@ -0,0 +1,63 @@ +generateTestAccount(); + // Will be a subuser of $server2. + $server2 = $this->createServerModel(); + // And as no access to $server3. + $server3 = $this->createServerModel(); + + // Set the API $user as a subuser of server 2, but with no permissions + // to do anything with the allocations for that server. + factory(Subuser::class)->create(['server_id' => $server2->id, 'user_id' => $user->id]); + + $allocation1 = factory(Allocation::class)->create(['server_id' => $server1->id, 'node_id' => $server1->node_id]); + $allocation2 = factory(Allocation::class)->create(['server_id' => $server2->id, 'node_id' => $server2->node_id]); + $allocation3 = factory(Allocation::class)->create(['server_id' => $server3->id, 'node_id' => $server3->node_id]); + + // This is the only valid call for this test, accessing the allocation for the same + // server that the API user is the owner of. + $response = $this->actingAs($user)->json($method, $this->link($server1, "/network/allocations/" . $allocation1->id . $endpoint)); + $this->assertTrue($response->status() <= 204 || $response->status() === 400 || $response->status() === 422); + + // This request fails because the allocation is valid for that server but the user + // making the request is not authorized to perform that action. + $this->actingAs($user)->json($method, $this->link($server2, "/network/allocations/" . $allocation2->id . $endpoint))->assertForbidden(); + + // Both of these should report a 404 error due to the allocations being linked to + // servers that are not the same as the server in the request, or are assigned + // to a server for which the user making the request has no access to. + $this->actingAs($user)->json($method, $this->link($server1, "/network/allocations/" . $allocation2->id . $endpoint))->assertNotFound(); + $this->actingAs($user)->json($method, $this->link($server1, "/network/allocations/" . $allocation3->id . $endpoint))->assertNotFound(); + $this->actingAs($user)->json($method, $this->link($server2, "/network/allocations/" . $allocation3->id . $endpoint))->assertNotFound(); + $this->actingAs($user)->json($method, $this->link($server3, "/network/allocations/" . $allocation3->id . $endpoint))->assertNotFound(); + } + + /** + * @return \string[][] + */ + public function methodDataProvider(): array + { + return [ + ["POST", ""], + ["DELETE", ""], + ["POST", "/primary"], + ]; + } +} diff --git a/tests/Integration/Api/Client/Server/Backup/BackupAuthorizationTest.php b/tests/Integration/Api/Client/Server/Backup/BackupAuthorizationTest.php new file mode 100644 index 00000000..b680b534 --- /dev/null +++ b/tests/Integration/Api/Client/Server/Backup/BackupAuthorizationTest.php @@ -0,0 +1,71 @@ +generateTestAccount(); + // Will be a subuser of $server2. + $server2 = $this->createServerModel(); + // And as no access to $server3. + $server3 = $this->createServerModel(); + + // Set the API $user as a subuser of server 2, but with no permissions + // to do anything with the backups for that server. + factory(Subuser::class)->create(['server_id' => $server2->id, 'user_id' => $user->id]); + + $backup1 = factory(Backup::class)->create(['server_id' => $server1->id, 'completed_at' => CarbonImmutable::now()]); + $backup2 = factory(Backup::class)->create(['server_id' => $server2->id, 'completed_at' => CarbonImmutable::now()]); + $backup3 = factory(Backup::class)->create(['server_id' => $server3->id, 'completed_at' => CarbonImmutable::now()]); + + $this->instance(DeleteBackupService::class, $mock = Mockery::mock(DeleteBackupService::class)); + + if ($method === 'DELETE') { + $mock->expects('handle')->andReturnUndefined(); + } + + // This is the only valid call for this test, accessing the backup for the same + // server that the API user is the owner of. + $this->actingAs($user)->json($method, $this->link($server1, "/backups/" . $backup1->uuid . $endpoint)) + ->assertStatus($method === 'DELETE' ? 204 : 200); + + // This request fails because the backup is valid for that server but the user + // making the request is not authorized to perform that action. + $this->actingAs($user)->json($method, $this->link($server2, "/backups/" . $backup2->uuid . $endpoint))->assertForbidden(); + + // Both of these should report a 404 error due to the backup being linked to + // servers that are not the same as the server in the request, or are assigned + // to a server for which the user making the request has no access to. + $this->actingAs($user)->json($method, $this->link($server1, "/backups/" . $backup2->uuid . $endpoint))->assertNotFound(); + $this->actingAs($user)->json($method, $this->link($server1, "/backups/" . $backup3->uuid . $endpoint))->assertNotFound(); + $this->actingAs($user)->json($method, $this->link($server2, "/backups/" . $backup3->uuid . $endpoint))->assertNotFound(); + $this->actingAs($user)->json($method, $this->link($server3, "/backups/" . $backup3->uuid . $endpoint))->assertNotFound(); + } + + /** + * @return \string[][] + */ + public function methodDataProvider(): array + { + return [ + ["GET", ""], + ["GET", "/download"], + ["DELETE", ""], + ]; + } +} diff --git a/tests/Integration/Api/Client/Server/Database/DatabaseAuthorizationTest.php b/tests/Integration/Api/Client/Server/Database/DatabaseAuthorizationTest.php new file mode 100644 index 00000000..aecd71db --- /dev/null +++ b/tests/Integration/Api/Client/Server/Database/DatabaseAuthorizationTest.php @@ -0,0 +1,78 @@ +generateTestAccount(); + // Will be a subuser of $server2. + $server2 = $this->createServerModel(); + // And as no access to $server3. + $server3 = $this->createServerModel(); + + $host = factory(DatabaseHost::class)->create([]); + + // Set the API $user as a subuser of server 2, but with no permissions + // to do anything with the databases for that server. + factory(Subuser::class)->create(['server_id' => $server2->id, 'user_id' => $user->id]); + + $database1 = factory(Database::class)->create(['server_id' => $server1->id, 'database_host_id' => $host->id]); + $database2 = factory(Database::class)->create(['server_id' => $server2->id, 'database_host_id' => $host->id]); + $database3 = factory(Database::class)->create(['server_id' => $server3->id, 'database_host_id' => $host->id]); + + $this->instance(DatabasePasswordService::class, $mock = Mockery::mock(DatabasePasswordService::class)); + $this->instance(DatabaseManagementService::class, $mock2 = Mockery::mock(DatabaseManagementService::class)); + + if ($method === 'POST') { + $mock->expects('handle')->andReturnUndefined(); + } else { + $mock2->expects('delete')->andReturnUndefined(); + } + + $hashids = $this->app->make(HashidsInterface::class); + // This is the only valid call for this test, accessing the database for the same + // server that the API user is the owner of. + $this->actingAs($user)->json($method, $this->link($server1, "/databases/" . $hashids->encode($database1->id) . $endpoint)) + ->assertStatus($method === 'DELETE' ? 204 : 200); + + // This request fails because the database is valid for that server but the user + // making the request is not authorized to perform that action. + $this->actingAs($user)->json($method, $this->link($server2, "/databases/" . $hashids->encode($database2->id) . $endpoint))->assertForbidden(); + + // Both of these should report a 404 error due to the database being linked to + // servers that are not the same as the server in the request, or are assigned + // to a server for which the user making the request has no access to. + $this->actingAs($user)->json($method, $this->link($server1, "/databases/" . $hashids->encode($database2->id) . $endpoint))->assertNotFound(); + $this->actingAs($user)->json($method, $this->link($server1, "/databases/" . $hashids->encode($database3->id) . $endpoint))->assertNotFound(); + $this->actingAs($user)->json($method, $this->link($server2, "/databases/" . $hashids->encode($database3->id) . $endpoint))->assertNotFound(); + $this->actingAs($user)->json($method, $this->link($server3, "/databases/" . $hashids->encode($database3->id) . $endpoint))->assertNotFound(); + } + + /** + * @return \string[][] + */ + public function methodDataProvider(): array + { + return [ + ["POST", "/rotate-password"], + ["DELETE", ""], + ]; + } +} diff --git a/tests/Integration/Api/Client/Server/Schedule/ScheduleAuthorizationTest.php b/tests/Integration/Api/Client/Server/Schedule/ScheduleAuthorizationTest.php new file mode 100644 index 00000000..583fb4ae --- /dev/null +++ b/tests/Integration/Api/Client/Server/Schedule/ScheduleAuthorizationTest.php @@ -0,0 +1,72 @@ +generateTestAccount(); + // Will be a subuser of $server2. + $server2 = $this->createServerModel(); + // And as no access to $server3. + $server3 = $this->createServerModel(); + + // Set the API $user as a subuser of server 2, but with no permissions + // to do anything with the schedules for that server. + factory(Subuser::class)->create(['server_id' => $server2->id, 'user_id' => $user->id]); + + $schedule1 = factory(Schedule::class)->create(['server_id' => $server1->id]); + $schedule2 = factory(Schedule::class)->create(['server_id' => $server2->id]); + $schedule3 = factory(Schedule::class)->create(['server_id' => $server3->id]); + + // This is the only valid call for this test, accessing the schedule for the same + // server that the API user is the owner of. + $response = $this->actingAs($user)->json($method, $this->link($server1, "/schedules/" . $schedule1->id . $endpoint)); + $this->assertTrue($response->status() <= 204 || $response->status() === 400 || $response->status() === 422); + + // This request fails because the schedule is valid for that server but the user + // making the request is not authorized to perform that action. + $this->actingAs($user)->json($method, $this->link($server2, "/schedules/" . $schedule2->id . $endpoint))->assertForbidden(); + + // Both of these should report a 404 error due to the schedules being linked to + // servers that are not the same as the server in the request, or are assigned + // to a server for which the user making the request has no access to. + $this->actingAs($user)->json($method, $this->link($server1, "/schedules/" . $schedule2->id . $endpoint))->assertNotFound(); + $this->actingAs($user)->json($method, $this->link($server1, "/schedules/" . $schedule3->id . $endpoint))->assertNotFound(); + $this->actingAs($user)->json($method, $this->link($server2, "/schedules/" . $schedule3->id . $endpoint))->assertNotFound(); + $this->actingAs($user)->json($method, $this->link($server3, "/schedules/" . $schedule3->id . $endpoint))->assertNotFound(); + } + + /** + * @return \string[][] + */ + public function methodDataProvider(): array + { + return [ + ["GET", ""], + ["POST", ""], + ["DELETE", ""], + ["POST", "/execute"], + ["POST", "/tasks"], + ]; + } +} diff --git a/tests/Integration/Api/Client/Server/Subuser/SubuserAuthorizationTest.php b/tests/Integration/Api/Client/Server/Subuser/SubuserAuthorizationTest.php new file mode 100644 index 00000000..f852ed06 --- /dev/null +++ b/tests/Integration/Api/Client/Server/Subuser/SubuserAuthorizationTest.php @@ -0,0 +1,61 @@ +create(); + + // The API $user is the owner of $server1. + [$user, $server1] = $this->generateTestAccount(); + // Will be a subuser of $server2. + $server2 = $this->createServerModel(); + // And as no access to $server3. + $server3 = $this->createServerModel(); + + // Set the API $user as a subuser of server 2, but with no permissions + // to do anything with the subusers for that server. + factory(Subuser::class)->create(['server_id' => $server2->id, 'user_id' => $user->id]); + + factory(Subuser::class)->create(['server_id' => $server1->id, 'user_id' => $internal->id]); + factory(Subuser::class)->create(['server_id' => $server2->id, 'user_id' => $internal->id]); + factory(Subuser::class)->create(['server_id' => $server3->id, 'user_id' => $internal->id]); + + $this->instance(DaemonServerRepository::class, $mock = Mockery::mock(DaemonServerRepository::class)); + if ($method === 'DELETE') { + $mock->expects('setServer->revokeUserJTI')->with($internal->id)->andReturnUndefined(); + } + + // This route is acceptable since they're accessing a subuser on their own server. + $this->actingAs($user)->json($method, $this->link($server1, "/users/" . $internal->uuid))->assertStatus($method === 'POST' ? 422 : ($method === 'DELETE' ? 204 : 200)); + + // This route can be revealed since the subuser belongs to the correct server, but + // errors out with a 403 since $user does not have the right permissions for this. + $this->actingAs($user)->json($method, $this->link($server2, "/users/" . $internal->uuid))->assertForbidden(); + $this->actingAs($user)->json($method, $this->link($server3, "/users/" . $internal->uuid))->assertNotFound(); + } + + /** + * @return \string[][] + */ + public function methodDataProvider(): array + { + return [["GET"], ["POST"], ["DELETE"]]; + } +} diff --git a/tests/Integration/TestResponse.php b/tests/Integration/TestResponse.php new file mode 100644 index 00000000..a812f37b --- /dev/null +++ b/tests/Integration/TestResponse.php @@ -0,0 +1,37 @@ +getStatusCode(); + + // Dump the response to the screen before making the assertion which is going + // to fail so that debugging isn't such a nightmare. + if ($actual !== $status && $status !== 500) { + $this->dump(); + if (! is_null($this->exception) && ! $this->exception instanceof DisplayException && ! $this->exception instanceof ValidationException) { + dump($this->exception); + } + } + + PHPUnit::assertSame($actual, $status, "Expected status code {$status} but received {$actual}."); + + return $this; + } +}