From 5e402f8aaeacf05f956c6a73d7300e7849bc4bae Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 24 Mar 2020 14:19:57 +0100 Subject: [PATCH] Check all remotes for local access Signed-off-by: Joas Schilling --- .../WebcalCaching/RefreshWebcalService.php | 53 ++----- config/config.sample.php | 7 + lib/composer/composer/autoload_classmap.php | 1 + lib/composer/composer/autoload_static.php | 1 + lib/private/Http/Client/Client.php | 65 ++++++++- lib/private/Http/Client/ClientService.php | 11 +- lib/private/Server.php | 1 + .../Http/Client/LocalServerException.php | 29 ++++ tests/lib/Http/Client/ClientTest.php | 131 ++++++++++++++++-- 9 files changed, 242 insertions(+), 57 deletions(-) create mode 100644 lib/public/Http/Client/LocalServerException.php diff --git a/apps/dav/lib/CalDAV/WebcalCaching/RefreshWebcalService.php b/apps/dav/lib/CalDAV/WebcalCaching/RefreshWebcalService.php index fadf61fd7d..8883a5d353 100644 --- a/apps/dav/lib/CalDAV/WebcalCaching/RefreshWebcalService.php +++ b/apps/dav/lib/CalDAV/WebcalCaching/RefreshWebcalService.php @@ -32,6 +32,7 @@ use GuzzleHttp\HandlerStack; use GuzzleHttp\Middleware; use OCA\DAV\CalDAV\CalDavBackend; use OCP\Http\Client\IClientService; +use OCP\Http\Client\LocalServerException; use OCP\IConfig; use OCP\ILogger; use Psr\Http\Message\RequestInterface; @@ -215,48 +216,15 @@ class RefreshWebcalService { return null; } - if ($allowLocalAccess !== 'yes') { - $host = strtolower(parse_url($url, PHP_URL_HOST)); - // remove brackets from IPv6 addresses - if (strpos($host, '[') === 0 && substr($host, -1) === ']') { - $host = substr($host, 1, -1); - } - - // Disallow localhost and local network - if ($host === 'localhost' || substr($host, -6) === '.local' || substr($host, -10) === '.localhost') { - $this->logger->warning("Subscription $subscriptionId was not refreshed because it violates local access rules"); - return null; - } - - // Disallow hostname only - if (substr_count($host, '.') === 0) { - $this->logger->warning("Subscription $subscriptionId was not refreshed because it violates local access rules"); - return null; - } - - if ((bool)filter_var($host, FILTER_VALIDATE_IP) && !filter_var($host, FILTER_VALIDATE_IP, FILTER_FLAG_NO_PRIV_RANGE | FILTER_FLAG_NO_RES_RANGE)) { - $this->logger->warning("Subscription $subscriptionId was not refreshed because it violates local access rules"); - return null; - } - - // Also check for IPv6 IPv4 nesting, because that's not covered by filter_var - if ((bool)filter_var($host, FILTER_VALIDATE_IP, FILTER_FLAG_IPV6) && substr_count($host, '.') > 0) { - $delimiter = strrpos($host, ':'); // Get last colon - $ipv4Address = substr($host, $delimiter + 1); - - if (!filter_var($ipv4Address, FILTER_VALIDATE_IP, FILTER_FLAG_NO_PRIV_RANGE | FILTER_FLAG_NO_RES_RANGE)) { - $this->logger->warning("Subscription $subscriptionId was not refreshed because it violates local access rules"); - return null; - } - } - } - try { $params = [ 'allow_redirects' => [ 'redirects' => 10 ], 'handler' => $handlerStack, + 'nextcloud' => [ + 'allow_local_address' => $allowLocalAccess === 'yes', + ] ]; $user = parse_url($subscription['source'], PHP_URL_USER); @@ -306,9 +274,18 @@ class RefreshWebcalService { } return $vCalendar->serialize(); } + } catch (LocalServerException $ex) { + $this->logger->logException($ex, [ + 'message' => "Subscription $subscriptionId was not refreshed because it violates local access rules", + 'level' => ILogger::WARN, + ]); + + return null; } catch (Exception $ex) { - $this->logger->logException($ex); - $this->logger->warning("Subscription $subscriptionId could not be refreshed due to a network error"); + $this->logger->logException($ex, [ + 'message' => "Subscription $subscriptionId could not be refreshed due to a network error", + 'level' => ILogger::WARN, + ]); return null; } diff --git a/config/config.sample.php b/config/config.sample.php index 9e5f7b6baf..00e3a6779f 100644 --- a/config/config.sample.php +++ b/config/config.sample.php @@ -558,6 +558,13 @@ $CONFIG = [ */ 'proxyexclude' => [], +/** + * Allow remote servers with local addresses e.g. in federated shares, webcal services and more + * + * Defaults to false + */ +'allow_local_remote_servers' => true, + /** * Deleted Items (trash bin) * diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index c600c15068..f20b42f79e 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -323,6 +323,7 @@ return array( 'OCP\\Http\\Client\\IClient' => $baseDir . '/lib/public/Http/Client/IClient.php', 'OCP\\Http\\Client\\IClientService' => $baseDir . '/lib/public/Http/Client/IClientService.php', 'OCP\\Http\\Client\\IResponse' => $baseDir . '/lib/public/Http/Client/IResponse.php', + 'OCP\\Http\\Client\\LocalServerException' => $baseDir . '/lib/public/Http/Client/LocalServerException.php', 'OCP\\IAddressBook' => $baseDir . '/lib/public/IAddressBook.php', 'OCP\\IAppConfig' => $baseDir . '/lib/public/IAppConfig.php', 'OCP\\IAvatar' => $baseDir . '/lib/public/IAvatar.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 87a9460f77..07f3e6a969 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -352,6 +352,7 @@ class ComposerStaticInit53792487c5a8370acc0b06b1a864ff4c 'OCP\\Http\\Client\\IClient' => __DIR__ . '/../../..' . '/lib/public/Http/Client/IClient.php', 'OCP\\Http\\Client\\IClientService' => __DIR__ . '/../../..' . '/lib/public/Http/Client/IClientService.php', 'OCP\\Http\\Client\\IResponse' => __DIR__ . '/../../..' . '/lib/public/Http/Client/IResponse.php', + 'OCP\\Http\\Client\\LocalServerException' => __DIR__ . '/../../..' . '/lib/public/Http/Client/LocalServerException.php', 'OCP\\IAddressBook' => __DIR__ . '/../../..' . '/lib/public/IAddressBook.php', 'OCP\\IAppConfig' => __DIR__ . '/../../..' . '/lib/public/IAppConfig.php', 'OCP\\IAvatar' => __DIR__ . '/../../..' . '/lib/public/IAvatar.php', diff --git a/lib/private/Http/Client/Client.php b/lib/private/Http/Client/Client.php index 19d5877f9f..d21ce413f1 100644 --- a/lib/private/Http/Client/Client.php +++ b/lib/private/Http/Client/Client.php @@ -34,8 +34,10 @@ use GuzzleHttp\Client as GuzzleClient; use GuzzleHttp\RequestOptions; use OCP\Http\Client\IClient; use OCP\Http\Client\IResponse; +use OCP\Http\Client\LocalServerException; use OCP\ICertificateManager; use OCP\IConfig; +use OCP\ILogger; /** * Class Client @@ -47,20 +49,19 @@ class Client implements IClient { private $client; /** @var IConfig */ private $config; + /** @var ILogger */ + private $logger; /** @var ICertificateManager */ private $certificateManager; - /** - * @param IConfig $config - * @param ICertificateManager $certificateManager - * @param GuzzleClient $client - */ public function __construct( IConfig $config, + ILogger $logger, ICertificateManager $certificateManager, GuzzleClient $client ) { $this->config = $config; + $this->logger = $logger; $this->client = $client; $this->certificateManager = $certificateManager; } @@ -144,6 +145,53 @@ class Client implements IClient { return $proxy; } + protected function preventLocalAddress(string $uri, array $options): void { + if (($options['nextcloud']['allow_local_address'] ?? false) || + $this->config->getSystemValueBool('allow_local_remote_servers', false)) { + return; + } + + $host = parse_url($uri, PHP_URL_HOST); + if ($host === false) { + $this->logger->warning("Could not detect any host in $uri"); + throw new LocalServerException('Could not detect any host'); + } + + $host = strtolower($host); + // remove brackets from IPv6 addresses + if (strpos($host, '[') === 0 && substr($host, -1) === ']') { + $host = substr($host, 1, -1); + } + + // Disallow localhost and local network + if ($host === 'localhost' || substr($host, -6) === '.local' || substr($host, -10) === '.localhost') { + $this->logger->warning("Host $host was not connected to because it violates local access rules"); + throw new LocalServerException('Host violates local access rules'); + } + + // Disallow hostname only + if (substr_count($host, '.') === 0) { + $this->logger->warning("Host $host was not connected to because it violates local access rules"); + throw new LocalServerException('Host violates local access rules'); + } + + if ((bool)filter_var($host, FILTER_VALIDATE_IP) && !filter_var($host, FILTER_VALIDATE_IP, FILTER_FLAG_NO_PRIV_RANGE | FILTER_FLAG_NO_RES_RANGE)) { + $this->logger->warning("Host $host was not connected to because it violates local access rules"); + throw new LocalServerException('Host violates local access rules'); + } + + // Also check for IPv6 IPv4 nesting, because that's not covered by filter_var + if ((bool)filter_var($host, FILTER_VALIDATE_IP, FILTER_FLAG_IPV6) && substr_count($host, '.') > 0) { + $delimiter = strrpos($host, ':'); // Get last colon + $ipv4Address = substr($host, $delimiter + 1); + + if (!filter_var($ipv4Address, FILTER_VALIDATE_IP, FILTER_FLAG_NO_PRIV_RANGE | FILTER_FLAG_NO_RES_RANGE)) { + $this->logger->warning("Host $host was not connected to because it violates local access rules"); + throw new LocalServerException('Host violates local access rules'); + } + } + } + /** * Sends a GET request * @@ -174,6 +222,7 @@ class Client implements IClient { * @throws \Exception If the request could not get completed */ public function get(string $uri, array $options = []): IResponse { + $this->preventLocalAddress($uri, $options); $response = $this->client->request('get', $uri, $this->buildRequestOptions($options)); $isStream = isset($options['stream']) && $options['stream']; return new Response($response, $isStream); @@ -204,6 +253,7 @@ class Client implements IClient { * @throws \Exception If the request could not get completed */ public function head(string $uri, array $options = []): IResponse { + $this->preventLocalAddress($uri, $options); $response = $this->client->request('head', $uri, $this->buildRequestOptions($options)); return new Response($response); } @@ -238,6 +288,8 @@ class Client implements IClient { * @throws \Exception If the request could not get completed */ public function post(string $uri, array $options = []): IResponse { + $this->preventLocalAddress($uri, $options); + if (isset($options['body']) && is_array($options['body'])) { $options['form_params'] = $options['body']; unset($options['body']); @@ -276,6 +328,7 @@ class Client implements IClient { * @throws \Exception If the request could not get completed */ public function put(string $uri, array $options = []): IResponse { + $this->preventLocalAddress($uri, $options); $response = $this->client->request('put', $uri, $this->buildRequestOptions($options)); return new Response($response); } @@ -310,6 +363,7 @@ class Client implements IClient { * @throws \Exception If the request could not get completed */ public function delete(string $uri, array $options = []): IResponse { + $this->preventLocalAddress($uri, $options); $response = $this->client->request('delete', $uri, $this->buildRequestOptions($options)); return new Response($response); } @@ -344,6 +398,7 @@ class Client implements IClient { * @throws \Exception If the request could not get completed */ public function options(string $uri, array $options = []): IResponse { + $this->preventLocalAddress($uri, $options); $response = $this->client->request('options', $uri, $this->buildRequestOptions($options)); return new Response($response); } diff --git a/lib/private/Http/Client/ClientService.php b/lib/private/Http/Client/ClientService.php index 2b18daaf73..55f03f3039 100644 --- a/lib/private/Http/Client/ClientService.php +++ b/lib/private/Http/Client/ClientService.php @@ -32,6 +32,7 @@ use OCP\Http\Client\IClient; use OCP\Http\Client\IClientService; use OCP\ICertificateManager; use OCP\IConfig; +use OCP\ILogger; /** * Class ClientService @@ -41,16 +42,16 @@ use OCP\IConfig; class ClientService implements IClientService { /** @var IConfig */ private $config; + /** @var ILogger */ + private $logger; /** @var ICertificateManager */ private $certificateManager; - /** - * @param IConfig $config - * @param ICertificateManager $certificateManager - */ public function __construct(IConfig $config, + ILogger $logger, ICertificateManager $certificateManager) { $this->config = $config; + $this->logger = $logger; $this->certificateManager = $certificateManager; } @@ -58,6 +59,6 @@ class ClientService implements IClientService { * @return Client */ public function newClient(): IClient { - return new Client($this->config, $this->certificateManager, new GuzzleClient()); + return new Client($this->config, $this->logger, $this->certificateManager, new GuzzleClient()); } } diff --git a/lib/private/Server.php b/lib/private/Server.php index 1a3eabc852..a7432342a2 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -804,6 +804,7 @@ class Server extends ServerContainer implements IServerContainer { $uid = $user ? $user : null; return new ClientService( $c->getConfig(), + $c->getLogger(), new \OC\Security\CertificateManager( $uid, new View(), diff --git a/lib/public/Http/Client/LocalServerException.php b/lib/public/Http/Client/LocalServerException.php new file mode 100644 index 0000000000..22e533bf19 --- /dev/null +++ b/lib/public/Http/Client/LocalServerException.php @@ -0,0 +1,29 @@ + + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ + +namespace OCP\Http\Client; + +/** + * @since 19.0.0 + */ +class LocalServerException extends \RuntimeException { +} diff --git a/tests/lib/Http/Client/ClientTest.php b/tests/lib/Http/Client/ClientTest.php index 2f9e70a8bb..b136a0ca30 100644 --- a/tests/lib/Http/Client/ClientTest.php +++ b/tests/lib/Http/Client/ClientTest.php @@ -11,33 +11,38 @@ namespace Test\Http\Client; use GuzzleHttp\Psr7\Response; use OC\Http\Client\Client; use OC\Security\CertificateManager; +use OCP\Http\Client\LocalServerException; use OCP\ICertificateManager; use OCP\IConfig; +use OCP\ILogger; +use PHPUnit\Framework\MockObject\MockObject; /** * Class ClientTest */ class ClientTest extends \Test\TestCase { - /** @var \GuzzleHttp\Client|\PHPUnit_Framework_MockObject_MockObject */ + /** @var \GuzzleHttp\Client|MockObject */ private $guzzleClient; - /** @var CertificateManager|\PHPUnit_Framework_MockObject_MockObject */ + /** @var CertificateManager|MockObject */ private $certificateManager; /** @var Client */ private $client; - /** @var IConfig|\PHPUnit_Framework_MockObject_MockObject */ + /** @var IConfig|MockObject */ private $config; + /** @var ILogger|MockObject */ + private $logger; /** @var array */ private $defaultRequestOptions; protected function setUp(): void { parent::setUp(); $this->config = $this->createMock(IConfig::class); - $this->guzzleClient = $this->getMockBuilder(\GuzzleHttp\Client::class) - ->disableOriginalConstructor() - ->getMock(); + $this->logger = $this->createMock(ILogger::class); + $this->guzzleClient = $this->createMock(\GuzzleHttp\Client::class); $this->certificateManager = $this->createMock(ICertificateManager::class); $this->client = new Client( $this->config, + $this->logger, $this->certificateManager, $this->guzzleClient ); @@ -149,19 +154,127 @@ class ClientTest extends \Test\TestCase { ], self::invokePrivate($this->client, 'getProxyUri')); } + public function dataPreventLocalAddress():array { + return [ + ['localhost/foo.bar'], + ['localHost/foo.bar'], + ['random-host/foo.bar'], + ['[::1]/bla.blub'], + ['[::]/bla.blub'], + ['192.168.0.1'], + ['172.16.42.1'], + ['[fdf8:f53b:82e4::53]/secret.ics'], + ['[fe80::200:5aee:feaa:20a2]/secret.ics'], + ['[0:0:0:0:0:0:10.0.0.1]/secret.ics'], + ['[0:0:0:0:0:ffff:127.0.0.0]/secret.ics'], + ['10.0.0.1'], + ['another-host.local'], + ['service.localhost'], + ['!@#$'], // test invalid url + ]; + } + + /** + * @dataProvider dataPreventLocalAddress + * @param string $uri + */ + public function testPreventLocalAddress(string $uri): void { + $this->expectException(LocalServerException::class); + self::invokePrivate($this->client, 'preventLocalAddress', ['http://' . $uri, []]); + } + + /** + * @dataProvider dataPreventLocalAddress + * @param string $uri + */ + public function testPreventLocalAddressDisabledByGlobalConfig(string $uri): void { + $this->config->expects($this->once()) + ->method('getSystemValueBool') + ->with('allow_local_remote_servers', false) + ->willReturn(true); + +// $this->expectException(LocalServerException::class); + + self::invokePrivate($this->client, 'preventLocalAddress', ['http://' . $uri, []]); + } + + /** + * @dataProvider dataPreventLocalAddress + * @param string $uri + */ + public function testPreventLocalAddressDisabledByOption(string $uri): void { + $this->config->expects($this->never()) + ->method('getSystemValueBool'); + +// $this->expectException(LocalServerException::class); + + self::invokePrivate($this->client, 'preventLocalAddress', ['http://' . $uri, [ + 'nextcloud' => ['allow_local_address' => true], + ]]); + } + + /** + * @dataProvider dataPreventLocalAddress + * @param string $uri + */ + public function testPreventLocalAddressOnGet(string $uri): void { + $this->expectException(LocalServerException::class); + $this->client->get('http://' . $uri); + } + + /** + * @dataProvider dataPreventLocalAddress + * @param string $uri + */ + public function testPreventLocalAddressOnHead(string $uri): void { + $this->expectException(LocalServerException::class); + $this->client->head('http://' . $uri); + } + + /** + * @dataProvider dataPreventLocalAddress + * @param string $uri + */ + public function testPreventLocalAddressOnPost(string $uri): void { + $this->expectException(LocalServerException::class); + $this->client->post('http://' . $uri); + } + + /** + * @dataProvider dataPreventLocalAddress + * @param string $uri + */ + public function testPreventLocalAddressOnPut(string $uri): void { + $this->expectException(LocalServerException::class); + $this->client->put('http://' . $uri); + } + + /** + * @dataProvider dataPreventLocalAddress + * @param string $uri + */ + public function testPreventLocalAddressOnDelete(string $uri): void { + $this->expectException(LocalServerException::class); + $this->client->delete('http://' . $uri); + } + private function setUpDefaultRequestOptions(): void { + $this->config->expects($this->once()) + ->method('getSystemValueBool') + ->with('allow_local_remote_servers', false) + ->willReturn(true); $this->config - ->expects($this->at(0)) + ->expects($this->at(1)) ->method('getSystemValue') ->with('proxy', null) ->willReturn('foo'); $this->config - ->expects($this->at(1)) + ->expects($this->at(2)) ->method('getSystemValue') ->with('proxyuserpwd', null) ->willReturn(null); $this->config - ->expects($this->at(2)) + ->expects($this->at(3)) ->method('getSystemValue') ->with('proxyexclude', []) ->willReturn([]);