diff options
author | Lukas Reschke <lukas@statuscode.ch> | 2021-03-23 16:41:31 +0000 |
---|---|---|
committer | GitHub <noreply@github.com> | 2021-04-06 11:37:47 +0000 |
commit | 5f3abffe6f37b4f8639fde8bcaf35d873a17636c (patch) | |
tree | 3498450ac8351f5a292dacc7cb17de9b27e4535b | |
parent | 2056b76c5fb29fa9273c50e17e54c5cf43f8a5fc (diff) | |
download | nextcloud-server-5f3abffe6f37b4f8639fde8bcaf35d873a17636c.tar.gz nextcloud-server-5f3abffe6f37b4f8639fde8bcaf35d873a17636c.zip |
Improve networking checks
Whilst we currently state that SSRF is generally outside of our threat model, this is something where we should invest to improve this.
Signed-off-by: Lukas Reschke <lukas@statuscode.ch>
-rw-r--r-- | lib/private/Http/Client/Client.php | 68 | ||||
-rw-r--r-- | lib/private/Http/Client/ClientService.php | 26 | ||||
-rw-r--r-- | lib/private/Http/Client/DnsPinMiddleware.php | 128 | ||||
-rw-r--r-- | lib/private/Http/Client/LocalAddressChecker.php | 85 | ||||
-rw-r--r-- | lib/private/Http/Client/NegativeDnsCache.php | 51 | ||||
-rw-r--r-- | lib/private/Server.php | 19 | ||||
-rw-r--r-- | tests/lib/Http/Client/ClientServiceTest.php | 33 | ||||
-rw-r--r-- | tests/lib/Http/Client/ClientTest.php | 131 | ||||
-rw-r--r-- | tests/lib/Http/Client/LocalAddressCheckerTest.php | 134 | ||||
-rw-r--r-- | tests/lib/Http/Client/NegativeDnsCacheTest.php | 65 |
10 files changed, 656 insertions, 84 deletions
diff --git a/lib/private/Http/Client/Client.php b/lib/private/Http/Client/Client.php index 12d1efb45a2..c1426d141d1 100644 --- a/lib/private/Http/Client/Client.php +++ b/lib/private/Http/Client/Client.php @@ -38,7 +38,6 @@ 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; @@ -57,17 +56,21 @@ class Client implements IClient { private $logger; /** @var ICertificateManager */ private $certificateManager; + /** @var LocalAddressChecker */ + private $localAddressChecker; public function __construct( IConfig $config, ILogger $logger, ICertificateManager $certificateManager, - GuzzleClient $client + GuzzleClient $client, + LocalAddressChecker $localAddressChecker ) { $this->config = $config; $this->logger = $logger; $this->client = $client; $this->certificateManager = $certificateManager; + $this->localAddressChecker = $localAddressChecker; } private function buildRequestOptions(array $options): array { @@ -78,6 +81,21 @@ class Client implements IClient { RequestOptions::TIMEOUT => 30, ]; + $options['nextcloud']['allow_local_address'] = $this->isLocalAddressAllowed($options); + if ($options['nextcloud']['allow_local_address'] === false) { + $onRedirectFunction = function ( + \Psr\Http\Message\RequestInterface $request, + \Psr\Http\Message\ResponseInterface $response, + \Psr\Http\Message\UriInterface $uri + ) use ($options) { + $this->preventLocalAddress($uri->__toString(), $options); + }; + + $defaults[RequestOptions::ALLOW_REDIRECTS] = [ + 'on_redirect' => $onRedirectFunction + ]; + } + // Only add RequestOptions::PROXY if Nextcloud is explicitly // configured to use a proxy. This is needed in order not to override // Guzzle default values. @@ -155,51 +173,21 @@ class Client implements IClient { return $proxy; } - protected function preventLocalAddress(string $uri, array $options): void { + private function isLocalAddressAllowed(array $options) : bool { 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 || $host === null) { - $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); + return true; } - // 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'); - } + return false; + } - 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'); + protected function preventLocalAddress(string $uri, array $options): void { + if ($this->isLocalAddressAllowed($options)) { + return; } - // 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'); - } - } + $this->localAddressChecker->ThrowIfLocalAddress($uri); } /** diff --git a/lib/private/Http/Client/ClientService.php b/lib/private/Http/Client/ClientService.php index 3858032308a..231436004ba 100644 --- a/lib/private/Http/Client/ClientService.php +++ b/lib/private/Http/Client/ClientService.php @@ -28,6 +28,8 @@ declare(strict_types=1); namespace OC\Http\Client; use GuzzleHttp\Client as GuzzleClient; +use GuzzleHttp\HandlerStack; +use GuzzleHttp\Handler\CurlHandler; use OCP\Http\Client\IClient; use OCP\Http\Client\IClientService; use OCP\ICertificateManager; @@ -46,19 +48,39 @@ class ClientService implements IClientService { private $logger; /** @var ICertificateManager */ private $certificateManager; + /** @var DnsPinMiddleware */ + private $dnsPinMiddleware; + /** @var LocalAddressChecker */ + private $localAddressChecker; public function __construct(IConfig $config, ILogger $logger, - ICertificateManager $certificateManager) { + ICertificateManager $certificateManager, + DnsPinMiddleware $dnsPinMiddleware, + LocalAddressChecker $localAddressChecker) { $this->config = $config; $this->logger = $logger; $this->certificateManager = $certificateManager; + $this->dnsPinMiddleware = $dnsPinMiddleware; + $this->localAddressChecker = $localAddressChecker; } /** * @return Client */ public function newClient(): IClient { - return new Client($this->config, $this->logger, $this->certificateManager, new GuzzleClient()); + $handler = new CurlHandler(); + $stack = HandlerStack::create($handler); + $stack->push($this->dnsPinMiddleware->addDnsPinning()); + + $client = new GuzzleClient(['handler' => $stack]); + + return new Client( + $this->config, + $this->logger, + $this->certificateManager, + $client, + $this->localAddressChecker + ); } } diff --git a/lib/private/Http/Client/DnsPinMiddleware.php b/lib/private/Http/Client/DnsPinMiddleware.php new file mode 100644 index 00000000000..5a87ff705a6 --- /dev/null +++ b/lib/private/Http/Client/DnsPinMiddleware.php @@ -0,0 +1,128 @@ +<?php + +declare(strict_types=1); + +/** + * @copyright Copyright (c) 2021, Lukas Reschke <lukas@statuscode.ch> + * + * @author Lukas Reschke <lukas@statuscode.ch> + * + * @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 <http://www.gnu.org/licenses/>. + * + */ + +namespace OC\Http\Client; + +use Psr\Http\Message\RequestInterface; + +class DnsPinMiddleware { + /** @var NegativeDnsCache */ + private $negativeDnsCache; + /** @var LocalAddressChecker */ + private $localAddressChecker; + + public function __construct( + NegativeDnsCache $negativeDnsCache, + LocalAddressChecker $localAddressChecker) { + $this->negativeDnsCache = $negativeDnsCache; + $this->localAddressChecker = $localAddressChecker; + } + + private function dnsResolve(string $target, int $recursionCount) : array { + if ($recursionCount >= 10) { + return []; + } + + $recursionCount = $recursionCount++; + $targetIps = []; + + $soaDnsEntry = dns_get_record($target, DNS_SOA); + if (isset($soaDnsEntry[0]) && isset($soaDnsEntry[0]['minimum-ttl'])) { + $dnsNegativeTtl = $soaDnsEntry[0]['minimum-ttl']; + } else { + $dnsNegativeTtl = null; + } + + $dnsTypes = [DNS_A, DNS_AAAA, DNS_CNAME]; + foreach ($dnsTypes as $key => $dnsType) { + if ($this->negativeDnsCache->isNegativeCached($target, $dnsType)) { + unset($dnsTypes[$key]); + continue; + } + + $dnsResponses = dns_get_record($target, $dnsType); + $canHaveCnameRecord = true; + if (count($dnsResponses) > 0) { + foreach ($dnsResponses as $key => $dnsResponse) { + if (isset($dnsResponse['ip'])) { + $targetIps[] = $dnsResponse['ip']; + $canHaveCnameRecord = false; + } elseif (isset($dnsResponse['ipv6'])) { + $targetIps[] = $dnsResponse['ipv6']; + $canHaveCnameRecord = false; + } elseif (isset($dnsResponse['target']) && $canHaveCnameRecord) { + $targetIps = array_merge($targetIps, $this->dnsResolve($dnsResponse['target'], $recursionCount)); + $canHaveCnameRecord = true; + } + } + } else { + if ($dnsNegativeTtl !== null) { + $this->negativeDnsCache->setNegativeCacheForDnsType($target, $dnsType, $dnsNegativeTtl); + } + } + } + + return $targetIps; + } + + public function addDnsPinning() { + return function (callable $handler) { + return function ( + RequestInterface $request, + array $options + ) use ($handler) { + if ($options['nextcloud']['allow_local_address'] === true) { + return $handler($request, $options); + } + + $hostName = (string)$request->getUri()->getHost(); + $port = $request->getUri()->getPort(); + + $ports = [ + '80', + '443', + ]; + + if ($port != null) { + $ports[] = (string)$port; + } + + $targetIps = $this->dnsResolve($hostName, 0); + + foreach ($targetIps as $ip) { + $this->localAddressChecker->ThrowIfLocalIp($ip); + + foreach ($ports as $port) { + $curlEntry = $hostName . ':' . $port . ':' . $ip; + $options['curl'][CURLOPT_RESOLVE][] = $curlEntry; + } + } + + return $handler($request, $options); + }; + }; + } +} diff --git a/lib/private/Http/Client/LocalAddressChecker.php b/lib/private/Http/Client/LocalAddressChecker.php new file mode 100644 index 00000000000..e9c92da4a5c --- /dev/null +++ b/lib/private/Http/Client/LocalAddressChecker.php @@ -0,0 +1,85 @@ +<?php + +declare(strict_types=1); + +/** + * @copyright Copyright (c) 2021, Lukas Reschke <lukas@statuscode.ch> + * + * @author Lukas Reschke <lukas@statuscode.ch> + * + * @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 <http://www.gnu.org/licenses/>. + * + */ + +namespace OC\Http\Client; + +use OCP\ILogger; +use OCP\Http\Client\LocalServerException; + +class LocalAddressChecker { + /** @var ILogger */ + private $logger; + + public function __construct(ILogger $logger) { + $this->logger = $logger; + } + + public function ThrowIfLocalIp(string $ip) : void { + if ((bool)filter_var($ip, FILTER_VALIDATE_IP) && !filter_var($ip, FILTER_VALIDATE_IP, FILTER_FLAG_NO_PRIV_RANGE | FILTER_FLAG_NO_RES_RANGE)) { + $this->logger->warning("Host $ip 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($ip, FILTER_VALIDATE_IP, FILTER_FLAG_IPV6) && substr_count($ip, '.') > 0) { + $delimiter = strrpos($ip, ':'); // Get last colon + $ipv4Address = substr($ip, $delimiter + 1); + + if (!filter_var($ipv4Address, FILTER_VALIDATE_IP, FILTER_FLAG_NO_PRIV_RANGE | FILTER_FLAG_NO_RES_RANGE)) { + $this->logger->warning("Host $ip was not connected to because it violates local access rules"); + throw new LocalServerException('Host violates local access rules'); + } + } + } + + public function ThrowIfLocalAddress(string $uri) : void { + $host = parse_url($uri, PHP_URL_HOST); + if ($host === false || $host === null) { + $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 && !(bool)filter_var($host, FILTER_VALIDATE_IP, FILTER_FLAG_IPV6)) { + $this->logger->warning("Host $host was not connected to because it violates local access rules"); + throw new LocalServerException('Host violates local access rules'); + } + + $this->ThrowIfLocalIp($host); + } +} diff --git a/lib/private/Http/Client/NegativeDnsCache.php b/lib/private/Http/Client/NegativeDnsCache.php new file mode 100644 index 00000000000..550d75a9c08 --- /dev/null +++ b/lib/private/Http/Client/NegativeDnsCache.php @@ -0,0 +1,51 @@ +<?php + +declare(strict_types=1); + +/** + * @copyright Copyright (c) 2021, Lukas Reschke <lukas@statuscode.ch> + * + * @author Lukas Reschke <lukas@statuscode.ch> + * + * @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 <http://www.gnu.org/licenses/>. + * + */ + +namespace OC\Http\Client; + +use OCP\ICache; +use OCP\ICacheFactory; + +class NegativeDnsCache { + /** @var ICache */ + private $cache; + + public function __construct(ICacheFactory $memcache) { + $this->cache = $memcache->createLocal('NegativeDnsCache'); + } + + private function createCacheKey(string $domain, int $type) : string { + return $domain . "-" . (string)$type; + } + + public function setNegativeCacheForDnsType(string $domain, int $type, int $ttl) : void { + $this->cache->set($this->createCacheKey($domain, $type), "true", $ttl); + } + + public function isNegativeCached(string $domain, int $type) : bool { + return $this->cache->hasKey($this->createCacheKey($domain, $type)); + } +} diff --git a/lib/private/Server.php b/lib/private/Server.php index 6a1550f83e0..c09ec0a8e18 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -100,6 +100,9 @@ use OC\Files\Type\Loader; use OC\Files\View; use OC\FullTextSearch\FullTextSearchManager; use OC\Http\Client\ClientService; +use OC\Http\Client\DnsPinMiddleware; +use OC\Http\Client\LocalAddressChecker; +use OC\Http\Client\NegativeDnsCache; use OC\IntegrityCheck\Checker; use OC\IntegrityCheck\Helpers\AppLocator; use OC\IntegrityCheck\Helpers\EnvironmentHelper; @@ -822,6 +825,22 @@ class Server extends ServerContainer implements IServerContainer { $this->registerAlias(ICertificateManager::class, CertificateManager::class); $this->registerAlias(IClientService::class, ClientService::class); + $this->registerService(LocalAddressChecker::class, function (ContainerInterface $c) { + return new LocalAddressChecker( + $c->get(ILogger::class), + ); + }); + $this->registerService(NegativeDnsCache::class, function (ContainerInterface $c) { + return new NegativeDnsCache( + $c->get(ICacheFactory::class), + ); + }); + $this->registerService(DnsPinMiddleware::class, function (ContainerInterface $c) { + return new DnsPinMiddleware( + $c->get(NegativeDnsCache::class), + $c->get(LocalAddressChecker::class) + ); + }); $this->registerDeprecatedAlias('HttpClientService', IClientService::class); $this->registerService(IEventLogger::class, function (ContainerInterface $c) { $eventLogger = new EventLogger(); diff --git a/tests/lib/Http/Client/ClientServiceTest.php b/tests/lib/Http/Client/ClientServiceTest.php index b1bc5a188ce..f529e25966e 100644 --- a/tests/lib/Http/Client/ClientServiceTest.php +++ b/tests/lib/Http/Client/ClientServiceTest.php @@ -9,8 +9,12 @@ namespace Test\Http\Client; use GuzzleHttp\Client as GuzzleClient; +use GuzzleHttp\HandlerStack; +use GuzzleHttp\Handler\CurlHandler; use OC\Http\Client\Client; use OC\Http\Client\ClientService; +use OC\Http\Client\DnsPinMiddleware; +use OC\Http\Client\LocalAddressChecker; use OCP\ICertificateManager; use OCP\IConfig; use OCP\ILogger; @@ -25,10 +29,35 @@ class ClientServiceTest extends \Test\TestCase { /** @var ICertificateManager $certificateManager */ $certificateManager = $this->createMock(ICertificateManager::class); $logger = $this->createMock(ILogger::class); + $dnsPinMiddleware = $this->createMock(DnsPinMiddleware::class); + $dnsPinMiddleware + ->expects($this->atLeastOnce()) + ->method('addDnsPinning') + ->willReturn(function () { + }); + $localAddressChecker = $this->createMock(LocalAddressChecker::class); + + $clientService = new ClientService( + $config, + $logger, + $certificateManager, + $dnsPinMiddleware, + $localAddressChecker + ); + + $handler = new CurlHandler(); + $stack = HandlerStack::create($handler); + $stack->push($dnsPinMiddleware->addDnsPinning()); + $guzzleClient = new GuzzleClient(['handler' => $stack]); - $clientService = new ClientService($config, $logger, $certificateManager); $this->assertEquals( - new Client($config, $logger, $certificateManager, new GuzzleClient()), + new Client( + $config, + $logger, + $certificateManager, + $guzzleClient, + $localAddressChecker + ), $clientService->newClient() ); } diff --git a/tests/lib/Http/Client/ClientTest.php b/tests/lib/Http/Client/ClientTest.php index 78054725e8c..395e82b8a4c 100644 --- a/tests/lib/Http/Client/ClientTest.php +++ b/tests/lib/Http/Client/ClientTest.php @@ -10,6 +10,7 @@ namespace Test\Http\Client; use GuzzleHttp\Psr7\Response; use OC\Http\Client\Client; +use OC\Http\Client\LocalAddressChecker; use OC\Security\CertificateManager; use OCP\Http\Client\LocalServerException; use OCP\ICertificateManager; @@ -31,6 +32,8 @@ class ClientTest extends \Test\TestCase { private $config; /** @var ILogger|MockObject */ private $logger; + /** @var LocalAddressChecker|MockObject */ + private $localAddressChecker; /** @var array */ private $defaultRequestOptions; @@ -40,17 +43,18 @@ class ClientTest extends \Test\TestCase { $this->logger = $this->createMock(ILogger::class); $this->guzzleClient = $this->createMock(\GuzzleHttp\Client::class); $this->certificateManager = $this->createMock(ICertificateManager::class); + $this->localAddressChecker = $this->createMock(LocalAddressChecker::class); $this->client = new Client( $this->config, $this->logger, $this->certificateManager, - $this->guzzleClient + $this->guzzleClient, + $this->localAddressChecker ); } public function testGetProxyUri(): void { $this->config - ->expects($this->at(0)) ->method('getSystemValue') ->with('proxy', null) ->willReturn(null); @@ -58,21 +62,16 @@ class ClientTest extends \Test\TestCase { } public function testGetProxyUriProxyHostEmptyPassword(): void { + $map = [ + ['proxy', '', 'foo'], + ['proxyuserpwd', '', null], + ['proxyexclude', [], []], + ]; + $this->config - ->expects($this->at(0)) - ->method('getSystemValue') - ->with('proxy', null) - ->willReturn('foo'); - $this->config - ->expects($this->at(1)) - ->method('getSystemValue') - ->with('proxyuserpwd', null) - ->willReturn(null); - $this->config - ->expects($this->at(2)) ->method('getSystemValue') - ->with('proxyexclude', []) - ->willReturn([]); + ->will($this->returnValueMap($map)); + $this->assertEquals([ 'http' => 'foo', 'https' => 'foo' @@ -178,15 +177,6 @@ class ClientTest extends \Test\TestCase { * @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') @@ -219,6 +209,12 @@ class ClientTest extends \Test\TestCase { */ public function testPreventLocalAddressOnGet(string $uri): void { $this->expectException(LocalServerException::class); + $this->localAddressChecker + ->expects($this->once()) + ->method('ThrowIfLocalAddress') + ->with('http://' . $uri) + ->will($this->throwException(new LocalServerException())); + $this->client->get('http://' . $uri); } @@ -228,6 +224,12 @@ class ClientTest extends \Test\TestCase { */ public function testPreventLocalAddressOnHead(string $uri): void { $this->expectException(LocalServerException::class); + $this->localAddressChecker + ->expects($this->once()) + ->method('ThrowIfLocalAddress') + ->with('http://' . $uri) + ->will($this->throwException(new LocalServerException())); + $this->client->head('http://' . $uri); } @@ -237,6 +239,12 @@ class ClientTest extends \Test\TestCase { */ public function testPreventLocalAddressOnPost(string $uri): void { $this->expectException(LocalServerException::class); + $this->localAddressChecker + ->expects($this->once()) + ->method('ThrowIfLocalAddress') + ->with('http://' . $uri) + ->will($this->throwException(new LocalServerException())); + $this->client->post('http://' . $uri); } @@ -246,6 +254,12 @@ class ClientTest extends \Test\TestCase { */ public function testPreventLocalAddressOnPut(string $uri): void { $this->expectException(LocalServerException::class); + $this->localAddressChecker + ->expects($this->once()) + ->method('ThrowIfLocalAddress') + ->with('http://' . $uri) + ->will($this->throwException(new LocalServerException())); + $this->client->put('http://' . $uri); } @@ -255,29 +269,30 @@ class ClientTest extends \Test\TestCase { */ public function testPreventLocalAddressOnDelete(string $uri): void { $this->expectException(LocalServerException::class); + $this->localAddressChecker + ->expects($this->once()) + ->method('ThrowIfLocalAddress') + ->with('http://' . $uri) + ->will($this->throwException(new LocalServerException())); + $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(1)) - ->method('getSystemValue') - ->with('proxy', null) - ->willReturn('foo'); + $map = [ + ['proxy', '', 'foo'], + ['proxyuserpwd', '', null], + ['proxyexclude', [], []], + ]; + $this->config - ->expects($this->at(2)) ->method('getSystemValue') - ->with('proxyuserpwd', null) - ->willReturn(null); + ->will($this->returnValueMap($map)); $this->config - ->expects($this->at(3)) - ->method('getSystemValue') - ->with('proxyexclude', []) - ->willReturn([]); + ->method('getSystemValueBool') + ->with('allow_local_remote_servers', false) + ->willReturn(true); + $this->certificateManager ->expects($this->once()) ->method('getAbsoluteBundlePath') @@ -295,6 +310,9 @@ class ClientTest extends \Test\TestCase { 'Accept-Encoding' => 'gzip', ], 'timeout' => 30, + 'nextcloud' => [ + 'allow_local_address' => true, + ], ]; } @@ -471,6 +489,17 @@ class ClientTest extends \Test\TestCase { 'Accept-Encoding' => 'gzip', ], 'timeout' => 30, + 'nextcloud' => [ + 'allow_local_address' => false, + ], + 'allow_redirects' => [ + 'on_redirect' => function ( + \Psr\Http\Message\RequestInterface $request, + \Psr\Http\Message\ResponseInterface $response, + \Psr\Http\Message\UriInterface $uri + ) { + }, + ], ], self::invokePrivate($this->client, 'buildRequestOptions', [[]])); } @@ -507,6 +536,17 @@ class ClientTest extends \Test\TestCase { 'Accept-Encoding' => 'gzip', ], 'timeout' => 30, + 'nextcloud' => [ + 'allow_local_address' => false, + ], + 'allow_redirects' => [ + 'on_redirect' => function ( + \Psr\Http\Message\RequestInterface $request, + \Psr\Http\Message\ResponseInterface $response, + \Psr\Http\Message\UriInterface $uri + ) { + }, + ], ], self::invokePrivate($this->client, 'buildRequestOptions', [[]])); } @@ -544,6 +584,17 @@ class ClientTest extends \Test\TestCase { 'Accept-Encoding' => 'gzip', ], 'timeout' => 30, + 'nextcloud' => [ + 'allow_local_address' => false, + ], + 'allow_redirects' => [ + 'on_redirect' => function ( + \Psr\Http\Message\RequestInterface $request, + \Psr\Http\Message\ResponseInterface $response, + \Psr\Http\Message\UriInterface $uri + ) { + }, + ], ], self::invokePrivate($this->client, 'buildRequestOptions', [[]])); } } diff --git a/tests/lib/Http/Client/LocalAddressCheckerTest.php b/tests/lib/Http/Client/LocalAddressCheckerTest.php new file mode 100644 index 00000000000..b2e09c0700b --- /dev/null +++ b/tests/lib/Http/Client/LocalAddressCheckerTest.php @@ -0,0 +1,134 @@ +<?php + +declare(strict_types=1); + +/** + * @copyright Copyright (c) 2021, Lukas Reschke <lukas@statuscode.ch> + * + * @author Lukas Reschke <lukas@statuscode.ch> + * + * @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 <http://www.gnu.org/licenses/>. + * + */ + +namespace Test\Http\Client; + +use OCP\ILogger; +use OCP\Http\Client\LocalServerException; +use OC\Http\Client\LocalAddressChecker; + +class LocalAddressCheckerTest extends \Test\TestCase { + /** @var LocalAddressChecker */ + private $localAddressChecker; + + protected function setUp(): void { + parent::setUp(); + + $logger = $this->createMock(ILogger::class); + $this->localAddressChecker = new LocalAddressChecker($logger); + } + + /** + * @dataProvider dataPreventLocalAddress + * @param string $uri + */ + public function testThrowIfLocalAddress($uri) : void { + $this->expectException(LocalServerException::class); + $this->localAddressChecker->ThrowIfLocalAddress('http://' . $uri); + } + + /** + * @dataProvider dataAllowLocalAddress + * @param string $uri + */ + public function testThrowIfLocalAddressGood($uri) : void { + $this->localAddressChecker->ThrowIfLocalAddress('http://' . $uri); + $this->assertTrue(true); + } + + + /** + * @dataProvider dataInternalIPs + * @param string $ip + */ + public function testThrowIfLocalIpBad($ip) : void { + $this->expectException(LocalServerException::class); + $this->localAddressChecker->ThrowIfLocalIp($ip); + } + + /** + * @dataProvider dataPublicIPs + * @param string $ip + */ + public function testThrowIfLocalIpGood($ip) : void { + $this->localAddressChecker->ThrowIfLocalIp($ip); + $this->assertTrue(true); + } + + public function dataPublicIPs() : array { + return [ + ['8.8.8.8'], + ['8.8.4.4'], + ['2001:4860:4860::8888'], + ['2001:4860:4860::8844'], + ]; + } + + public function dataInternalIPs() : array { + return [ + ['192.168.0.1'], + ['fe80::200:5aee:feaa:20a2'], + ['0:0:0:0:0:0:10.0.0.1'], + ['0:0:0:0:0:ffff:127.0.0.0'], + ['10.0.0.1'], + ['::'], + ['::1'], + ]; + } + + 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 + ]; + } + + public function dataAllowLocalAddress():array { + return [ + ['example.com/foo.bar'], + ['example.net/foo.bar'], + ['example.org/foo.bar'], + ['8.8.8.8/bla.blub'], + ['8.8.4.4/bla.blub'], + ['8.8.8.8'], + ['8.8.4.4'], + ['[2001:4860:4860::8888]/secret.ics'], + ]; + } +} diff --git a/tests/lib/Http/Client/NegativeDnsCacheTest.php b/tests/lib/Http/Client/NegativeDnsCacheTest.php new file mode 100644 index 00000000000..237e30865d0 --- /dev/null +++ b/tests/lib/Http/Client/NegativeDnsCacheTest.php @@ -0,0 +1,65 @@ +<?php + +declare(strict_types=1); + +/** + * @copyright Copyright (c) 2021, Lukas Reschke <lukas@statuscode.ch> + * + * @author Lukas Reschke <lukas@statuscode.ch> + * + * @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 <http://www.gnu.org/licenses/>. + * + */ + +namespace Test\Http\Client; + +use OCP\ICache; +use OCP\ICacheFactory; + +class NegativeDnsCacheTest extends \Test\TestCase { + /** @var ICache */ + private $cache; + /** @var ICacheFactory */ + private $cacheFactory; + /** @var NegativeDnsCache */ + private $negativeDnsCache; + + protected function setUp(): void { + parent::setUp(); + + $this->cache = $this->createMock(ICache::class); + $this->cacheFactory = $this->createMock(ICacheFactory::class); + $this->cacheFactory->expects($this->at(0)) + ->method('createLocal') + ->with('NegativeDnsCache') + ->willReturn($this->cache); + + $this->negativeDnsCache = new NegativeDnsCache($this->cacheFactory); + } + + public function testSetNegativeCacheForDnsType() : void { + $this->cache->set($this->createCacheKey($domain, $type), "true", $ttl); + } + + public function testIsNegativeCached() { + $this->cache->expects($this->at(0)) + ->method('createDistributed') + ->with('hasKey', 'www.example.com-0') + ->willReturn($imagePathCache); + + $this->negativeDnsCache->hasKey('www.example.com', DNS_A); + } +} |