diff options
author | Lukas Reschke <lukas@statuscode.ch> | 2021-03-23 19:41:31 +0300 |
---|---|---|
committer | GitHub <noreply@github.com> | 2021-04-06 14:37:47 +0300 |
commit | 5f3abffe6f37b4f8639fde8bcaf35d873a17636c (patch) | |
tree | 3498450ac8351f5a292dacc7cb17de9b27e4535b /lib | |
parent | 2056b76c5fb29fa9273c50e17e54c5cf43f8a5fc (diff) |
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>
Diffstat (limited to 'lib')
-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 |
6 files changed, 335 insertions, 42 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(); |