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>

4 files changed, 321 insertions, 42 deletions

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