mbk-lab
/
nextcloud
2
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Merge pull request #20138 from nextcloud/bugfix/noid/make-remote-checking-more-generic 

Make remote checking more generic
This commit is contained in:
Joas Schilling 2020-04-15 12:48:49 +02:00 committed by GitHub
parent 95ad9ab4ac 4e846dda4d
commit 5c0637bc90
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
14 changed files with 300 additions and 65 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

53
apps/dav/lib/CalDAV/WebcalCaching/RefreshWebcalService.php
View File

@ -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;
}

55
apps/dav/tests/unit/CalDAV/WebcalCaching/RefreshWebcalServiceTest.php
View File

@ -31,6 +31,7 @@ use OCA\DAV\CalDAV\WebcalCaching\RefreshWebcalService;
use OCP\Http\Client\IClient;
use OCP\Http\Client\IClientService;
use OCP\Http\Client\IResponse;
use OCP\Http\Client\LocalServerException;
use OCP\IConfig;
use OCP\ILogger;
use PHPUnit\Framework\MockObject\MockObject;
@ -170,8 +171,12 @@ class RefreshWebcalServiceTest extends TestCase {
* @param string $source
*/
public function testRunLocalURL($source) {
$refreshWebcalService = new RefreshWebcalService($this->caldavBackend,
$this->clientService, $this->config, $this->logger);
$refreshWebcalService = new RefreshWebcalService(
$this->caldavBackend,
$this->clientService,
$this->config,
$this->logger
);
$this->caldavBackend->expects($this->once())
->method('getSubscriptionsForUser')
@ -199,8 +204,13 @@ class RefreshWebcalServiceTest extends TestCase {
->with('dav', 'webcalAllowLocalAccess', 'no')
->willReturn('no');
$client->expects($this->never())
->method('get');
$client->expects($this->once())
->method('get')
->willThrowException(new LocalServerException());
$this->logger->expects($this->once())
->method('logException')
->with($this->isInstanceOf(LocalServerException::class), $this->anything());
$refreshWebcalService->refreshSubscription('principals/users/testuser', 'sub123');
}
@ -221,7 +231,42 @@ class RefreshWebcalServiceTest extends TestCase {
['10.0.0.1'],
['another-host.local'],
['service.localhost'],
['!@#$'], // test invalid url
];
}
public function testInvalidUrl() {
$refreshWebcalService = new RefreshWebcalService($this->caldavBackend,
$this->clientService, $this->config, $this->logger);
$this->caldavBackend->expects($this->once())
->method('getSubscriptionsForUser')
->with('principals/users/testuser')
->willReturn([
[
'id' => 42,
'uri' => 'sub123',
'refreshreate' => 'P1H',
'striptodos' => 1,
'stripalarms' => 1,
'stripattachments' => 1,
'source' => '!@#$'
],
]);
$client = $this->createMock(IClient::class);
$this->clientService->expects($this->once())
->method('newClient')
->with()
->willReturn($client);
$this->config->expects($this->once())
->method('getAppValue')
->with('dav', 'webcalAllowLocalAccess', 'no')
->willReturn('no');
$client->expects($this->never())
->method('get');
$refreshWebcalService->refreshSubscription('principals/users/testuser', 'sub123');
}
}

2
apps/files_sharing/lib/Controller/ExternalSharesController.php
View File

@ -131,7 +131,7 @@ class ExternalSharesController extends Controller {
* @return DataResponse
*/
public function testRemote($remote) {
if (strpos($remote, '#') !== false || strpos($remote, '?') !== false) {
if (strpos($remote, '#') !== false || strpos($remote, '?') !== false || strpos($remote, ';') !== false) {
return new DataResponse(false);
}

1
apps/files_sharing/tests/Controller/ExternalShareControllerTest.php
View File

@ -162,6 +162,7 @@ class ExternalShareControllerTest extends \Test\TestCase {
return [
['nextcloud.com?query'],
['nextcloud.com/#anchor'],
['nextcloud.com/;tomcat'],
];
}

2
build/integration/run.sh
View File

@ -16,6 +16,8 @@ INSTALLED=$($OCC status | grep installed: | cut -d " " -f 5)
if [ "$INSTALLED" == "true" ]; then
# Disable bruteforce protection because the integration tests do trigger them
$OCC config:system:set auth.bruteforce.protection.enabled --value false --type bool
# Allow local remote urls otherwise we can not share
$OCC config:system:set allow_local_remote_servers --value true --type bool
else
if [ "$SCENARIO_TO_RUN" != "setup_features/setup.feature" ]; then
echo "Nextcloud instance needs to be installed" >&2

7
config/config.sample.php
View File

@ -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)
*

1
lib/composer/composer/autoload_classmap.php
View File

@ -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',

1
lib/composer/composer/autoload_static.php
View File

@ -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',

65
lib/private/Http/Client/Client.php
View File

@ -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);
}

11
lib/private/Http/Client/ClientService.php
View File

@ -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());
}
}

1
lib/private/Server.php
View File

@ -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(),

29
lib/public/Http/Client/LocalServerException.php Normal file
View File

@ -0,0 +1,29 @@
<?php
declare(strict_types=1);
/**
* @copyright Copyright (c) 2020 Joas Schilling <coding@schilljs.com>
*
* @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 OCP\Http\Client;
/**
* @since 19.0.0
*/
class LocalServerException extends \RuntimeException {
}

6
tests/lib/Http/Client/ClientServiceTest.php
View File

@ -13,6 +13,7 @@ use OC\Http\Client\Client;
use OC\Http\Client\ClientService;
use OCP\ICertificateManager;
use OCP\IConfig;
use OCP\ILogger;
/**
* Class ClientServiceTest
@ -23,10 +24,11 @@ class ClientServiceTest extends \Test\TestCase {
$config = $this->createMock(IConfig::class);
/** @var ICertificateManager $certificateManager */
$certificateManager = $this->createMock(ICertificateManager::class);
$logger = $this->createMock(ILogger::class);
$clientService = new ClientService($config, $certificateManager);
$clientService = new ClientService($config, $logger, $certificateManager);
$this->assertEquals(
new Client($config, $certificateManager, new GuzzleClient()),
new Client($config, $logger, $certificateManager, new GuzzleClient()),
$clientService->newClient()
);
}

131
tests/lib/Http/Client/ClientTest.php
View File

@ -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([]);