From 40c74ae1e7d67fd2ed319191bec10ce38df321fd Mon Sep 17 00:00:00 2001 From: Dane Everitt Date: Sat, 10 Mar 2018 13:10:40 -0600 Subject: [PATCH] Add validation to prevent invalid ports, closes #1034 --- CHANGELOG.md | 1 + .../Allocation/CidrOutOfRangeException.php | 16 ++ .../InvalidPortMappingException.php | 18 ++ .../Allocation/PortOutOfRangeException.php | 16 ++ .../TooManyPortsInRangeException.php | 16 ++ .../Controllers/Admin/NodesController.php | 5 +- .../Nodes/AllocationController.php | 5 +- .../Allocations/AssignmentService.php | 55 +++-- resources/lang/en/exceptions.php | 3 +- .../Allocations/AssignmentServiceTest.php | 222 ++++++++---------- 10 files changed, 205 insertions(+), 152 deletions(-) create mode 100644 app/Exceptions/Service/Allocation/CidrOutOfRangeException.php create mode 100644 app/Exceptions/Service/Allocation/InvalidPortMappingException.php create mode 100644 app/Exceptions/Service/Allocation/PortOutOfRangeException.php create mode 100644 app/Exceptions/Service/Allocation/TooManyPortsInRangeException.php diff --git a/CHANGELOG.md b/CHANGELOG.md index 17ab8f05c..1c7c91194 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ This project follows [Semantic Versioning](http://semver.org) guidelines. * Correct permissions check in UI to allow subusers with permission to `view-allocations` the ability to actually see the sidebar link. * Fixes improper behavior when marking an egg as copying the configuration from another. * Debug bar is only checked when the app is set to debug mode in the API session handler, rather than when it is in local mode to match the plugin settings. +* Added validation to port allocations to prevent allocation of restricted or invalid ports. ### Changed * Panel now throws proper 504: Gateway Timeout errors on server listing when daemon is offline. diff --git a/app/Exceptions/Service/Allocation/CidrOutOfRangeException.php b/app/Exceptions/Service/Allocation/CidrOutOfRangeException.php new file mode 100644 index 000000000..4852dc106 --- /dev/null +++ b/app/Exceptions/Service/Allocation/CidrOutOfRangeException.php @@ -0,0 +1,16 @@ + $port])); + } +} diff --git a/app/Exceptions/Service/Allocation/PortOutOfRangeException.php b/app/Exceptions/Service/Allocation/PortOutOfRangeException.php new file mode 100644 index 000000000..695925f89 --- /dev/null +++ b/app/Exceptions/Service/Allocation/PortOutOfRangeException.php @@ -0,0 +1,16 @@ +. - * - * This software is licensed under the terms of the MIT license. - * https://opensource.org/licenses/MIT - */ namespace Pterodactyl\Services\Allocations; use IPTools\Network; use Pterodactyl\Models\Node; use Illuminate\Database\ConnectionInterface; -use Pterodactyl\Exceptions\DisplayException; use Pterodactyl\Contracts\Repository\AllocationRepositoryInterface; +use Pterodactyl\Exceptions\Service\Allocation\CidrOutOfRangeException; +use Pterodactyl\Exceptions\Service\Allocation\PortOutOfRangeException; +use Pterodactyl\Exceptions\Service\Allocation\InvalidPortMappingException; +use Pterodactyl\Exceptions\Service\Allocation\TooManyPortsInRangeException; class AssignmentService { const CIDR_MAX_BITS = 27; const CIDR_MIN_BITS = 32; + const PORT_FLOOR = 1024; + const PORT_CEIL = 65535; const PORT_RANGE_LIMIT = 1000; - const PORT_RANGE_REGEX = '/^(\d{1,5})-(\d{1,5})$/'; + const PORT_RANGE_REGEX = '/^(\d{4,5})-(\d{4,5})$/'; /** * @var \Illuminate\Database\ConnectionInterface @@ -38,10 +36,8 @@ class AssignmentService * @param \Pterodactyl\Contracts\Repository\AllocationRepositoryInterface $repository * @param \Illuminate\Database\ConnectionInterface $connection */ - public function __construct( - AllocationRepositoryInterface $repository, - ConnectionInterface $connection - ) { + public function __construct(AllocationRepositoryInterface $repository, ConnectionInterface $connection) + { $this->connection = $connection; $this->repository = $repository; } @@ -49,21 +45,20 @@ class AssignmentService /** * Insert allocations into the database and link them to a specific node. * - * @param int|\Pterodactyl\Models\Node $node - * @param array $data + * @param \Pterodactyl\Models\Node $node + * @param array $data * - * @throws \Pterodactyl\Exceptions\DisplayException + * @throws \Pterodactyl\Exceptions\Service\Allocation\CidrOutOfRangeException + * @throws \Pterodactyl\Exceptions\Service\Allocation\PortOutOfRangeException + * @throws \Pterodactyl\Exceptions\Service\Allocation\InvalidPortMappingException + * @throws \Pterodactyl\Exceptions\Service\Allocation\TooManyPortsInRangeException */ - public function handle($node, array $data) + public function handle(Node $node, array $data) { - if ($node instanceof Node) { - $node = $node->id; - } - $explode = explode('/', $data['allocation_ip']); if (count($explode) !== 1) { if (! ctype_digit($explode[1]) || ($explode[1] > self::CIDR_MIN_BITS || $explode[1] < self::CIDR_MAX_BITS)) { - throw new DisplayException(trans('exceptions.allocations.cidr_out_of_range')); + throw new CidrOutOfRangeException; } } @@ -71,7 +66,7 @@ class AssignmentService foreach (Network::parse(gethostbyname($data['allocation_ip'])) as $ip) { foreach ($data['allocation_ports'] as $port) { if (! is_digit($port) && ! preg_match(self::PORT_RANGE_REGEX, $port)) { - throw new DisplayException(trans('exceptions.allocations.invalid_mapping', ['port' => $port])); + throw new InvalidPortMappingException($port); } $insertData = []; @@ -79,12 +74,16 @@ class AssignmentService $block = range($matches[1], $matches[2]); if (count($block) > self::PORT_RANGE_LIMIT) { - throw new DisplayException(trans('exceptions.allocations.too_many_ports')); + throw new TooManyPortsInRangeException; + } + + if ((int) $matches[1] <= self::PORT_FLOOR || (int) $matches[2] > self::PORT_CEIL) { + throw new PortOutOfRangeException; } foreach ($block as $unit) { $insertData[] = [ - 'node_id' => $node, + 'node_id' => $node->id, 'ip' => $ip->__toString(), 'port' => (int) $unit, 'ip_alias' => array_get($data, 'allocation_alias'), @@ -92,8 +91,12 @@ class AssignmentService ]; } } else { + if ((int) $port <= self::PORT_FLOOR || (int) $port > self::PORT_CEIL) { + throw new PortOutOfRangeException; + } + $insertData[] = [ - 'node_id' => $node, + 'node_id' => $node->id, 'ip' => $ip->__toString(), 'port' => (int) $port, 'ip_alias' => array_get($data, 'allocation_alias'), diff --git a/resources/lang/en/exceptions.php b/resources/lang/en/exceptions.php index 73b910d0d..96395baed 100644 --- a/resources/lang/en/exceptions.php +++ b/resources/lang/en/exceptions.php @@ -8,9 +8,10 @@ return [ ], 'allocations' => [ 'server_using' => 'A server is currently assigned to this allocation. An allocation can only be deleted if no server is currently assigned.', - 'too_many_ports' => 'Adding more than 1000 ports at a single time is not supported. Please use a smaller range.', + 'too_many_ports' => 'Adding more than 1000 ports in a single range at once is not supported.', 'invalid_mapping' => 'The mapping provided for :port was invalid and could not be processed.', 'cidr_out_of_range' => 'CIDR notation only allows masks between /25 and /32.', + 'port_out_of_range' => 'Ports in an allocation must be greater than 1024 and less than or equal to 65535.', ], 'nest' => [ 'delete_has_servers' => 'A Nest with active servers attached to it cannot be deleted from the Panel.', diff --git a/tests/Unit/Services/Allocations/AssignmentServiceTest.php b/tests/Unit/Services/Allocations/AssignmentServiceTest.php index 805b789a3..7a4eb067c 100644 --- a/tests/Unit/Services/Allocations/AssignmentServiceTest.php +++ b/tests/Unit/Services/Allocations/AssignmentServiceTest.php @@ -1,30 +1,19 @@ . - * - * This software is licensed under the terms of the MIT license. - * https://opensource.org/licenses/MIT - */ namespace Tests\Unit\Services\Allocations; use Exception; use Mockery as m; use Tests\TestCase; -use phpmock\phpunit\PHPMock; use Pterodactyl\Models\Node; use Illuminate\Database\ConnectionInterface; -use Pterodactyl\Exceptions\DisplayException; use Pterodactyl\Services\Allocations\AssignmentService; use Pterodactyl\Contracts\Repository\AllocationRepositoryInterface; class AssignmentServiceTest extends TestCase { - use PHPMock; - /** - * @var \Illuminate\Database\ConnectionInterface + * @var \Illuminate\Database\ConnectionInterface|\Mockery\Mock */ protected $connection; @@ -34,15 +23,10 @@ class AssignmentServiceTest extends TestCase protected $node; /** - * @var \Pterodactyl\Contracts\Repository\AllocationRepositoryInterface + * @var \Pterodactyl\Contracts\Repository\AllocationRepositoryInterface|\Mockery\Mock */ protected $repository; - /** - * @var \Pterodactyl\Services\Allocations\AssignmentService - */ - protected $service; - /** * Setup tests. */ @@ -50,19 +34,9 @@ class AssignmentServiceTest extends TestCase { parent::setUp(); - // Due to a bug in PHP, this is necessary since we only have a single test - // that relies on this mock. If this does not exist the test will fail to register - // correctly. - // - // This can also be avoided if tests were run in isolated processes, or if that test - // came first, but neither of those are good solutions, so this is the next best option. - PHPMock::defineFunctionMock('\\Pterodactyl\\Services\\Allocations', 'gethostbyname'); - $this->node = factory(Node::class)->make(); $this->connection = m::mock(ConnectionInterface::class); $this->repository = m::mock(AllocationRepositoryInterface::class); - - $this->service = new AssignmentService($this->repository, $this->connection); } /** @@ -72,22 +46,22 @@ class AssignmentServiceTest extends TestCase { $data = [ 'allocation_ip' => '192.168.1.1', - 'allocation_ports' => ['1024'], + 'allocation_ports' => ['2222'], ]; - $this->connection->shouldReceive('beginTransaction')->withNoArgs()->once()->andReturnNull(); + $this->connection->shouldReceive('beginTransaction')->once()->withNoArgs()->andReturnNull(); $this->repository->shouldReceive('insertIgnore')->with([ [ 'node_id' => $this->node->id, 'ip' => '192.168.1.1', - 'port' => 1024, + 'port' => 2222, 'ip_alias' => null, 'server_id' => null, ], ])->once()->andReturn(true); - $this->connection->shouldReceive('commit')->withNoArgs()->once()->andReturnNull(); + $this->connection->shouldReceive('commit')->once()->withNoArgs()->andReturnNull(); - $this->service->handle($this->node->id, $data); + $this->getService()->handle($this->node, $data); } /** @@ -97,18 +71,11 @@ class AssignmentServiceTest extends TestCase { $data = [ 'allocation_ip' => '192.168.1.1', - 'allocation_ports' => ['1024-1026'], + 'allocation_ports' => ['1025-1027'], ]; - $this->connection->shouldReceive('beginTransaction')->withNoArgs()->once()->andReturnNull(); - $this->repository->shouldReceive('insertIgnore')->with([ - [ - 'node_id' => $this->node->id, - 'ip' => '192.168.1.1', - 'port' => 1024, - 'ip_alias' => null, - 'server_id' => null, - ], + $this->connection->shouldReceive('beginTransaction')->once()->withNoArgs()->andReturnNull(); + $this->repository->shouldReceive('insertIgnore')->once()->with([ [ 'node_id' => $this->node->id, 'ip' => '192.168.1.1', @@ -123,10 +90,17 @@ class AssignmentServiceTest extends TestCase 'ip_alias' => null, 'server_id' => null, ], - ])->once()->andReturn(true); - $this->connection->shouldReceive('commit')->withNoArgs()->once()->andReturnNull(); + [ + 'node_id' => $this->node->id, + 'ip' => '192.168.1.1', + 'port' => 1027, + 'ip_alias' => null, + 'server_id' => null, + ], + ])->andReturn(true); + $this->connection->shouldReceive('commit')->once()->withNoArgs()->andReturnNull(); - $this->service->handle($this->node->id, $data); + $this->getService()->handle($this->node, $data); } /** @@ -136,23 +110,23 @@ class AssignmentServiceTest extends TestCase { $data = [ 'allocation_ip' => '192.168.1.1', - 'allocation_ports' => ['1024'], + 'allocation_ports' => ['2222'], 'allocation_alias' => 'my.alias.net', ]; - $this->connection->shouldReceive('beginTransaction')->withNoArgs()->once()->andReturnNull(); - $this->repository->shouldReceive('insertIgnore')->with([ + $this->connection->shouldReceive('beginTransaction')->once()->withNoArgs()->andReturnNull(); + $this->repository->shouldReceive('insertIgnore')->once()->with([ [ 'node_id' => $this->node->id, 'ip' => '192.168.1.1', - 'port' => 1024, + 'port' => 2222, 'ip_alias' => 'my.alias.net', 'server_id' => null, ], - ])->once()->andReturn(true); - $this->connection->shouldReceive('commit')->withNoArgs()->once()->andReturnNull(); + ])->andReturn(true); + $this->connection->shouldReceive('commit')->once()->withNoArgs()->andReturnNull(); - $this->service->handle($this->node->id, $data); + $this->getService()->handle($this->node, $data); } /** @@ -161,26 +135,23 @@ class AssignmentServiceTest extends TestCase public function testDomainNamePassedInPlaceOfIPAddress() { $data = [ - 'allocation_ip' => 'test-domain.com', - 'allocation_ports' => ['1024'], + 'allocation_ip' => 'unit-test-static.pterodactyl.io', + 'allocation_ports' => ['2222'], ]; - $this->getFunctionMock('\\Pterodactyl\\Services\\Allocations', 'gethostbyname') - ->expects($this->once())->willReturn('192.168.1.1'); - - $this->connection->shouldReceive('beginTransaction')->withNoArgs()->once()->andReturnNull(); - $this->repository->shouldReceive('insertIgnore')->with([ + $this->connection->shouldReceive('beginTransaction')->once()->withNoArgs()->andReturnNull(); + $this->repository->shouldReceive('insertIgnore')->once()->with([ [ 'node_id' => $this->node->id, - 'ip' => '192.168.1.1', - 'port' => 1024, + 'ip' => '127.0.0.1', + 'port' => 2222, 'ip_alias' => null, 'server_id' => null, ], - ])->once()->andReturn(true); - $this->connection->shouldReceive('commit')->withNoArgs()->once()->andReturnNull(); + ])->andReturn(true); + $this->connection->shouldReceive('commit')->once()->withNoArgs()->andReturnNull(); - $this->service->handle($this->node->id, $data); + $this->getService()->handle($this->node, $data); } /** @@ -190,54 +161,55 @@ class AssignmentServiceTest extends TestCase { $data = [ 'allocation_ip' => '192.168.1.100/31', - 'allocation_ports' => ['1024'], + 'allocation_ports' => ['2222'], ]; - $this->connection->shouldReceive('beginTransaction')->withNoArgs()->once()->andReturnNull(); - $this->repository->shouldReceive('insertIgnore')->with([ + $this->connection->shouldReceive('beginTransaction')->once()->withNoArgs()->andReturnNull(); + $this->repository->shouldReceive('insertIgnore')->once()->with([ [ 'node_id' => $this->node->id, 'ip' => '192.168.1.100', - 'port' => 1024, + 'port' => 2222, 'ip_alias' => null, 'server_id' => null, ], - ])->once()->andReturn(true); + ])->andReturn(true); - $this->repository->shouldReceive('insertIgnore')->with([ + $this->repository->shouldReceive('insertIgnore')->once()->with([ [ 'node_id' => $this->node->id, 'ip' => '192.168.1.101', - 'port' => 1024, + 'port' => 2222, 'ip_alias' => null, 'server_id' => null, ], - ])->once()->andReturn(true); - $this->connection->shouldReceive('commit')->withNoArgs()->once()->andReturnNull(); + ])->andReturn(true); + $this->connection->shouldReceive('commit')->once()->withNoArgs()->andReturnNull(); - $this->service->handle($this->node->id, $data); + $this->getService()->handle($this->node, $data); } /** * Test that a CIDR IP address with a range works properly. + * + * @expectedException \Pterodactyl\Exceptions\Service\Allocation\CidrOutOfRangeException + * @expectedExceptionMessage CIDR notation only allows masks between /25 and /32. */ public function testCIDRNotatedIPAddressOutsideRangeLimit() { $data = [ 'allocation_ip' => '192.168.1.100/20', - 'allocation_ports' => ['1024'], + 'allocation_ports' => ['2222'], ]; - try { - $this->service->handle($this->node->id, $data); - } catch (Exception $exception) { - $this->assertInstanceOf(DisplayException::class, $exception); - $this->assertEquals(trans('exceptions.allocations.cidr_out_of_range'), $exception->getMessage()); - } + $this->getService()->handle($this->node, $data); } /** * Test that an exception is thrown if there are too many ports. + * + * @expectedException \Pterodactyl\Exceptions\Service\Allocation\TooManyPortsInRangeException + * @expectedExceptionMessage Adding more than 1000 ports in a single range at once is not supported. */ public function testAllocationWithPortsExceedingLimit() { @@ -246,22 +218,16 @@ class AssignmentServiceTest extends TestCase 'allocation_ports' => ['5000-7000'], ]; - $this->connection->shouldReceive('beginTransaction')->withNoArgs()->once()->andReturnNull(); + $this->connection->shouldReceive('beginTransaction')->once()->withNoArgs()->andReturnNull(); - try { - $this->service->handle($this->node->id, $data); - } catch (Exception $exception) { - if (! $exception instanceof DisplayException) { - throw $exception; - } - - $this->assertInstanceOf(DisplayException::class, $exception); - $this->assertEquals(trans('exceptions.allocations.too_many_ports'), $exception->getMessage()); - } + $this->getService()->handle($this->node, $data); } /** * Test that an exception is thrown if an invalid port is provided. + * + * @expectedException \Pterodactyl\Exceptions\Service\Allocation\InvalidPortMappingException + * @expectedExceptionMessage The mapping provided for test123 was invalid and could not be processed. */ public function testInvalidPortProvided() { @@ -270,42 +236,52 @@ class AssignmentServiceTest extends TestCase 'allocation_ports' => ['test123'], ]; - $this->connection->shouldReceive('beginTransaction')->withNoArgs()->once()->andReturnNull(); - - try { - $this->service->handle($this->node->id, $data); - } catch (Exception $exception) { - if (! $exception instanceof DisplayException) { - throw $exception; - } - - $this->assertInstanceOf(DisplayException::class, $exception); - $this->assertEquals(trans('exceptions.allocations.invalid_mapping', ['port' => 'test123']), $exception->getMessage()); - } + $this->connection->shouldReceive('beginTransaction')->once()->withNoArgs()->andReturnNull(); + $this->getService()->handle($this->node, $data); } /** - * Test that a model can be passed in place of an ID. + * Test that ports outside of defined limits throw an error. + * + * @param array $ports + * + * @dataProvider invalidPortsDataProvider + * @expectedException \Pterodactyl\Exceptions\Service\Allocation\PortOutOfRangeException + * @expectedExceptionMessage Ports in an allocation must be greater than 1024 and less than or equal to 65535. */ - public function testModelCanBePassedInPlaceOfNodeModel() + public function testPortRangeOutsideOfRangeLimits(array $ports) { - $data = [ - 'allocation_ip' => '192.168.1.1', - 'allocation_ports' => ['1024'], + $data = ['allocation_ip' => '192.168.1.1', 'allocation_ports' => $ports]; + + $this->connection->shouldReceive('beginTransaction')->once()->withNoArgs()->andReturnNull(); + $this->getService()->handle($this->node, $data); + } + + /** + * Provide ports and ranges of ports that exceed the viable port limits for the software. + * + * @return array + */ + public function invalidPortsDataProvider(): array + { + return [ + [['65536']], + [['1024']], + [['1000']], + [['0']], + [['65530-65540']], + [['65540-65560']], + [[PHP_INT_MAX]], ]; + } - $this->connection->shouldReceive('beginTransaction')->withNoArgs()->once()->andReturnNull(); - $this->repository->shouldReceive('insertIgnore')->with([ - [ - 'node_id' => $this->node->id, - 'ip' => '192.168.1.1', - 'port' => 1024, - 'ip_alias' => null, - 'server_id' => null, - ], - ])->once()->andReturn(true); - $this->connection->shouldReceive('commit')->withNoArgs()->once()->andReturnNull(); - - $this->service->handle($this->node, $data); + /** + * Returns an instance of the service with mocked dependencies for testing. + * + * @return \Pterodactyl\Services\Allocations\AssignmentService + */ + private function getService(): AssignmentService + { + return new AssignmentService($this->repository, $this->connection); } }