From 24352d984e284491013d1f2d392d2225070b51af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <1005065+DeepDiver1975@users.noreply.github.com> Date: Fri, 5 Jun 2026 14:26:10 +0200 Subject: [PATCH 1/2] fix(files_external): add SSRF host validation to external storage backend MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit UserStoragesController::create()/update() (@NoAdminRequired) accepted arbitrary host values for DAV, SMB, and other network backends without validating against private IP ranges, loopback, or link-local addresses. With user mounting enabled, authenticated users could force the server to make HTTP requests to cloud metadata endpoints (169.254.169.254), localhost services, or internal network hosts. Add validateHostOption() to StoragesController::validate(), which blocks RFC-1918 private ranges, loopback (127.x.x.x / ::1), IPv4 link-local (169.254.x.x), IPv6 link-local (fe80::/10), and ULA (fc00::/7). Admin escape-hatch: files_external_allow_private_address=true in config. Signed-off-by: Thomas Müller Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com> --- .../Controller/GlobalStoragesController.php | 8 +- .../lib/Controller/StoragesController.php | 138 ++++++++++- .../lib/Controller/UserStoragesController.php | 7 +- .../GlobalStoragesControllerTest.php | 14 +- .../Controller/StoragesControllerTest.php | 222 ++++++++++++++++++ .../Controller/UserStoragesControllerTest.php | 16 +- 6 files changed, 398 insertions(+), 7 deletions(-) diff --git a/apps/files_external/lib/Controller/GlobalStoragesController.php b/apps/files_external/lib/Controller/GlobalStoragesController.php index 6692bacc54d8..6183a3a9e694 100644 --- a/apps/files_external/lib/Controller/GlobalStoragesController.php +++ b/apps/files_external/lib/Controller/GlobalStoragesController.php @@ -29,6 +29,7 @@ use OCP\AppFramework\Http\DataResponse; use OCP\Files\External\NotFoundException; use OCP\Files\External\Service\IGlobalStoragesService; +use OCP\IConfig; use OCP\IL10N; use OCP\ILogger; use OCP\IRequest; @@ -45,20 +46,23 @@ class GlobalStoragesController extends StoragesController { * @param IL10N $l10n l10n service * @param IGlobalStoragesService $globalStoragesService storage service * @param ILogger $logger + * @param IConfig $config */ public function __construct( $AppName, IRequest $request, IL10N $l10n, IGlobalStoragesService $globalStoragesService, - ILogger $logger + ILogger $logger, + IConfig $config = null ) { parent::__construct( $AppName, $request, $l10n, $globalStoragesService, - $logger + $logger, + $config ); } diff --git a/apps/files_external/lib/Controller/StoragesController.php b/apps/files_external/lib/Controller/StoragesController.php index 21de089ab642..f717d5a19bc0 100644 --- a/apps/files_external/lib/Controller/StoragesController.php +++ b/apps/files_external/lib/Controller/StoragesController.php @@ -36,6 +36,7 @@ use OCP\Files\External\NotFoundException; use OCP\Files\External\Service\IStoragesService; use OCP\Files\StorageNotAvailableException; +use OCP\IConfig; use OCP\IL10N; use OCP\ILogger; use OCP\IRequest; @@ -63,6 +64,11 @@ abstract class StoragesController extends Controller { */ protected $logger; + /** + * @var IConfig + */ + protected $config; + /** * Creates a new storages controller. * @@ -71,18 +77,21 @@ abstract class StoragesController extends Controller { * @param IL10N $l10n l10n service * @param IStoragesService $storagesService storage service * @param ILogger $logger + * @param IConfig $config */ public function __construct( $AppName, IRequest $request, IL10N $l10n, IStoragesService $storagesService, - ILogger $logger + ILogger $logger, + IConfig $config = null ) { parent::__construct($AppName, $request); $this->l10n = $l10n; $this->service = $storagesService; $this->logger = $logger; + $this->config = $config ?? \OC::$server->getConfig(); } /** @@ -217,9 +226,136 @@ protected function validate(IStorageConfig $storage) { ); } + $hostValidation = $this->validateHostOption($storage); + if ($hostValidation !== null) { + return $hostValidation; + } + return null; } + /** + * Validate the host backend option against SSRF (Server-Side Request Forgery). + * + * Rejects private/reserved/loopback/link-local addresses unless the admin + * has explicitly allowed them via the 'files_external_allow_private_address' + * system config key. + * + * @param IStorageConfig $storage storage config + * @return DataResponse|null returns a 403 response when the host is blocked, + * null when the host is acceptable + */ + protected function validateHostOption(IStorageConfig $storage) { + // Admins may explicitly allow private-network targets (e.g. internal NAS) + if ($this->config->getSystemValue('files_external_allow_private_address', false)) { + return null; + } + + $host = $storage->getBackendOption('host'); + if ($host === null || $host === '') { + return null; + } + + // Strip a trailing path component and an explicit scheme so that + // parse_url() can reliably extract just the hostname / IP. + // Handles all of these forms: + // 192.168.1.1 + // 192.168.1.1:445 + // https://192.168.1.1/path + // 169.254.169.254/latest/meta-data/ + $normalised = (string)$host; + if (!\preg_match('#^[a-zA-Z][a-zA-Z0-9+\-.]*://#', $normalised)) { + // Prepend a dummy scheme so parse_url works on bare host[:port][/path] + $normalised = 'dummy://' . $normalised; + } + $parsed = \parse_url($normalised); + $hostname = isset($parsed['host']) ? \trim($parsed['host'], '[]') : ''; + + if ($hostname === '') { + // Cannot parse host — reject to be safe + return new DataResponse( + [ + 'message' => (string)$this->l10n->t('Invalid host') + ], + Http::STATUS_UNPROCESSABLE_ENTITY + ); + } + + // Resolve hostname to an IP address for range checks. + // gethostbyname() returns the original string on failure, which is fine + // because filter_var() will then just validate it as-is. + $ip = \filter_var($hostname, FILTER_VALIDATE_IP) + ? $hostname + : \gethostbyname($hostname); + + if ($this->isBlockedAddress($ip, $hostname)) { + return new DataResponse( + [ + 'message' => (string)$this->l10n->t( + 'Host "%s" is not allowed (private/reserved address)', + [$host] + ) + ], + Http::STATUS_FORBIDDEN + ); + } + + return null; + } + + /** + * Returns true when the given IP (or unresolvable hostname) targets a + * private, loopback, link-local, or otherwise reserved address. + * + * @param string $ip The resolved IPv4 / IPv6 address (or original hostname + * when DNS resolution failed). + * @param string $hostname The original hostname before resolution. + * @return bool + */ + private function isBlockedAddress($ip, $hostname) { + // Explicit loopback hostnames + if (\in_array(\strtolower($hostname), ['localhost', 'localhost.localdomain'], true)) { + return true; + } + + // If the value is not a valid IP at all we cannot perform range checks — + // allow it (the backend will fail with a connection error anyway). + if (\filter_var($ip, FILTER_VALIDATE_IP) === false) { + return false; + } + + // Block loopback (127.0.0.0/8, ::1) + if (\filter_var($ip, FILTER_VALIDATE_IP, FILTER_FLAG_NO_RES_RANGE) === false) { + return true; + } + + // Block RFC-1918 / ULA private ranges + if (\filter_var($ip, FILTER_VALIDATE_IP, FILTER_FLAG_NO_PRIV_RANGE) === false) { + return true; + } + + // Block IPv4 link-local (169.254.0.0/16) — not covered by FILTER_FLAG_NO_PRIV_RANGE + if (\filter_var($ip, FILTER_VALIDATE_IP, FILTER_FLAG_IPV4) !== false) { + $long = \ip2long($ip); + // 169.254.0.0 = 0xA9FE0000, mask /16 = 0xFFFF0000 + if (($long & 0xFFFF0000) === 0xA9FE0000) { + return true; + } + } + + // Block IPv6 link-local (fe80::/10) + if (\filter_var($ip, FILTER_VALIDATE_IP, FILTER_FLAG_IPV6) !== false) { + // fe80:: through febf:: + if (\stripos($ip, 'fe80:') === 0 || + \stripos($ip, 'fe81:') === 0 || + \preg_match('/^fe[89ab][0-9a-f]:/i', $ip) === 1) { + return true; + } + } + + return false; + } + protected function manipulateStorageConfig(IStorageConfig $storage) { /** @var AuthMechanism */ $authMechanism = $storage->getAuthMechanism(); diff --git a/apps/files_external/lib/Controller/UserStoragesController.php b/apps/files_external/lib/Controller/UserStoragesController.php index ef727275e65d..91617f2efd63 100644 --- a/apps/files_external/lib/Controller/UserStoragesController.php +++ b/apps/files_external/lib/Controller/UserStoragesController.php @@ -31,6 +31,7 @@ use OCP\Files\External\IStorageConfig; use OCP\Files\External\NotFoundException; use OCP\Files\External\Service\IUserStoragesService; +use OCP\IConfig; use OCP\IL10N; use OCP\ILogger; use OCP\IRequest; @@ -51,14 +52,16 @@ public function __construct( IL10N $l10n, IUserStoragesService $userStoragesService, IUserSession $userSession, - ILogger $logger + ILogger $logger, + IConfig $config = null ) { parent::__construct( $AppName, $request, $l10n, $userStoragesService, - $logger + $logger, + $config ); $this->userSession = $userSession; } diff --git a/apps/files_external/tests/Controller/GlobalStoragesControllerTest.php b/apps/files_external/tests/Controller/GlobalStoragesControllerTest.php index dab05e43b6f0..f4b1a9f839cb 100644 --- a/apps/files_external/tests/Controller/GlobalStoragesControllerTest.php +++ b/apps/files_external/tests/Controller/GlobalStoragesControllerTest.php @@ -44,7 +44,19 @@ public function setUp(): void { $this->createMock('\OCP\IRequest'), $this->createMock('\OCP\IL10N'), $this->service, - $this->createMock('\OCP\ILogger') + $this->createMock('\OCP\ILogger'), + $this->config + ); + } + + protected function rebuildControllerWithConfig(\OCP\IConfig $config) { + $this->controller = new GlobalStoragesController( + 'files_external', + $this->createMock('\OCP\IRequest'), + $this->createMock('\OCP\IL10N'), + $this->service, + $this->createMock('\OCP\ILogger'), + $config ); } diff --git a/apps/files_external/tests/Controller/StoragesControllerTest.php b/apps/files_external/tests/Controller/StoragesControllerTest.php index 3df57c9d04df..fc744d813c15 100644 --- a/apps/files_external/tests/Controller/StoragesControllerTest.php +++ b/apps/files_external/tests/Controller/StoragesControllerTest.php @@ -31,6 +31,7 @@ use OCP\Files\External\Service\IGlobalStoragesService; use OCP\Files\External\Service\IStoragesService; use OCP\Files\StorageNotAvailableException; +use OCP\IConfig; use PHPUnit\Framework\MockObject\MockObject; abstract class StoragesControllerTest extends \Test\TestCase { @@ -44,9 +45,24 @@ abstract class StoragesControllerTest extends \Test\TestCase { */ protected $service; + /** + * @var IConfig | MockObject + */ + protected $config; + public function setUp(): void { \OC_Mount_Config::$skipTest = true; \OC::$server->getSystemConfig()->setValue('files_external_allow_create_new_local', true); + + $this->config = $this->createMock(IConfig::class); + // Default: private addresses are NOT allowed (SSRF protection active) + $this->config->method('getSystemValue') + ->willReturnCallback(function ($key, $default = null) { + if ($key === 'files_external_allow_private_address') { + return false; + } + return $default; + }); } public function tearDown(): void { @@ -596,4 +612,210 @@ public function testShow() { $this->assertEquals(Http::STATUS_OK, $result->getStatus()); $this->assertEquals($expectedStorage, $actual); } + + // ------------------------------------------------------------------------- + // SSRF host validation tests + // ------------------------------------------------------------------------- + + /** + * Data provider with private/reserved host values that must be rejected. + */ + public function blockedHostProvider() { + return [ + // IPv4 loopback + ['127.0.0.1'], + ['127.255.255.255'], + // localhost hostname + ['localhost'], + // RFC-1918 private ranges + ['10.0.0.1'], + ['10.255.255.255'], + ['172.16.0.1'], + ['172.31.255.255'], + ['192.168.0.1'], + ['192.168.255.255'], + // IPv4 link-local (APIPA / AWS metadata) + ['169.254.0.1'], + ['169.254.169.254'], + ['169.254.169.254/latest/meta-data/iam/security-credentials/'], + // with explicit port + ['192.168.1.100:445'], + // with scheme (as used by DAV/ownCloud backends) + ['http://192.168.1.1/dav'], + ['https://10.0.0.1:8080/path'], + ]; + } + + /** + * @dataProvider blockedHostProvider + */ + public function testCreateBlockedSsrfHost($blockedHost) { + $backend = $this->getBackendMock(); + $backend->method('validateStorage')->willReturn(true); + $backend->method('isVisibleFor')->willReturn(true); + + $authMech = $this->getAuthMechMock(); + $authMech->method('validateStorage')->willReturn(true); + $authMech->method('isVisibleFor')->willReturn(true); + + $storageConfig = new StorageConfig(1); + $storageConfig->setMountPoint('mount'); + $storageConfig->setBackend($backend); + $storageConfig->setAuthMechanism($authMech); + $storageConfig->setBackendOptions(['host' => $blockedHost]); + + $this->service->expects($this->once()) + ->method('createStorage') + ->willReturn($storageConfig); + $this->service->expects($this->never()) + ->method('addStorage'); + + $response = $this->controller->create( + 'mount', + '\OCA\Files_External\Lib\Storage\DAV', + '\OCA\Files_External\Lib\Auth\Password\Password', + ['host' => $blockedHost], + [], + [], + [], + null + ); + + $this->assertEquals(Http::STATUS_FORBIDDEN, $response->getStatus()); + } + + /** + * @dataProvider blockedHostProvider + */ + public function testUpdateBlockedSsrfHost($blockedHost) { + $backend = $this->getBackendMock(); + $backend->method('validateStorage')->willReturn(true); + $backend->method('isVisibleFor')->willReturn(true); + + $authMech = $this->getAuthMechMock(); + $authMech->method('validateStorage')->willReturn(true); + $authMech->method('isVisibleFor')->willReturn(true); + + $storageConfig = new StorageConfig(1); + $storageConfig->setMountPoint('mount'); + $storageConfig->setBackend($backend); + $storageConfig->setAuthMechanism($authMech); + $storageConfig->setBackendOptions(['host' => $blockedHost]); + + $this->service->expects($this->once()) + ->method('createStorage') + ->willReturn($storageConfig); + $this->service->expects($this->never()) + ->method('updateStorage'); + + $response = $this->controller->update( + 1, + 'mount', + '\OCA\Files_External\Lib\Storage\DAV', + '\OCA\Files_External\Lib\Auth\Password\Password', + ['host' => $blockedHost], + [], + [], + [], + null + ); + + $this->assertEquals(Http::STATUS_FORBIDDEN, $response->getStatus()); + } + + public function testCreateAllowedPublicHost() { + $backend = $this->getBackendMock(); + $backend->method('validateStorage')->willReturn(true); + $backend->method('isVisibleFor')->willReturn(true); + + $authMech = $this->getAuthMechMock(); + $authMech->method('validateStorage')->willReturn(true); + $authMech->method('isVisibleFor')->willReturn(true); + + $storageConfig = new StorageConfig(1); + $storageConfig->setMountPoint('mount'); + $storageConfig->setBackend($backend); + $storageConfig->setAuthMechanism($authMech); + $storageConfig->setBackendOptions(['host' => 'example.com']); + + $this->service->expects($this->once()) + ->method('createStorage') + ->willReturn($storageConfig); + $this->service->expects($this->once()) + ->method('addStorage') + ->willReturn($storageConfig); + + $response = $this->controller->create( + 'mount', + '\OCA\Files_External\Lib\Storage\DAV', + '\OCA\Files_External\Lib\Auth\Password\Password', + ['host' => 'example.com'], + [], + [], + [], + null + ); + + // Must succeed (not blocked) + $this->assertEquals(Http::STATUS_CREATED, $response->getStatus()); + } + + public function testCreateBlockedSsrfHostAllowedByAdmin() { + // When the admin sets files_external_allow_private_address = true, + // private addresses must be accepted. + $this->config = $this->createMock(IConfig::class); + $this->config->method('getSystemValue') + ->willReturnCallback(function ($key, $default = null) { + if ($key === 'files_external_allow_private_address') { + return true; + } + return $default; + }); + + // Rebuild the controller with the permissive config + $this->rebuildControllerWithConfig($this->config); + + $backend = $this->getBackendMock(); + $backend->method('validateStorage')->willReturn(true); + $backend->method('isVisibleFor')->willReturn(true); + + $authMech = $this->getAuthMechMock(); + $authMech->method('validateStorage')->willReturn(true); + $authMech->method('isVisibleFor')->willReturn(true); + + $storageConfig = new StorageConfig(1); + $storageConfig->setMountPoint('mount'); + $storageConfig->setBackend($backend); + $storageConfig->setAuthMechanism($authMech); + $storageConfig->setBackendOptions(['host' => '192.168.1.1']); + + $this->service->expects($this->once()) + ->method('createStorage') + ->willReturn($storageConfig); + $this->service->expects($this->once()) + ->method('addStorage') + ->willReturn($storageConfig); + + $response = $this->controller->create( + 'mount', + '\OCA\Files_External\Lib\Storage\DAV', + '\OCA\Files_External\Lib\Auth\Password\Password', + ['host' => '192.168.1.1'], + [], + [], + [], + null + ); + + // Admin override must allow it through + $this->assertEquals(Http::STATUS_CREATED, $response->getStatus()); + } + + /** + * Rebuild the concrete controller implementation with a given IConfig. + * Subclasses must implement this to recreate their specific controller. + * + * @param \OCP\IConfig $config + */ + abstract protected function rebuildControllerWithConfig(\OCP\IConfig $config); } diff --git a/apps/files_external/tests/Controller/UserStoragesControllerTest.php b/apps/files_external/tests/Controller/UserStoragesControllerTest.php index fe6a14b8e35e..02d70699fea0 100644 --- a/apps/files_external/tests/Controller/UserStoragesControllerTest.php +++ b/apps/files_external/tests/Controller/UserStoragesControllerTest.php @@ -31,6 +31,7 @@ use OCP\Files\External\IStorageConfig; use OCP\Files\External\Service\IStoragesService; use OCP\Files\StorageNotAvailableException; +use OCP\IConfig; use OCP\ILogger; use OCP\IUserSession; use OCP\IL10N; @@ -50,7 +51,20 @@ public function setUp(): void { $this->createMock(IL10N::class), $this->service, $this->createMock(IUserSession::class), - $this->createMock(ILogger::class) + $this->createMock(ILogger::class), + $this->config + ); + } + + protected function rebuildControllerWithConfig(\OCP\IConfig $config) { + $this->controller = new UserStoragesController( + 'files_external', + $this->createMock(IRequest::class), + $this->createMock(IL10N::class), + $this->service, + $this->createMock(IUserSession::class), + $this->createMock(ILogger::class), + $config ); } From 070c076c460babd3cf3067a1dcc07907beced21e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <1005065+DeepDiver1975@users.noreply.github.com> Date: Fri, 5 Jun 2026 15:58:57 +0200 Subject: [PATCH 2/2] chore: add changelog entry for #41581 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com> --- changelog/unreleased/41581 | 12 ++++++++++++ 1 file changed, 12 insertions(+) create mode 100644 changelog/unreleased/41581 diff --git a/changelog/unreleased/41581 b/changelog/unreleased/41581 new file mode 100644 index 000000000000..9e85d222f973 --- /dev/null +++ b/changelog/unreleased/41581 @@ -0,0 +1,12 @@ +Security: Add SSRF host validation to external storage backend configuration + +The external storage controller accepted arbitrary host values for +network-capable backends such as WebDAV without validating against +private IP ranges, loopback, or link-local addresses. When user +storage mounting was enabled, any authenticated user could force the +server to make HTTP requests to cloud metadata endpoints or internal +services. Validation now blocks RFC-1918, loopback, link-local, and +equivalent IPv6 ranges, with an admin opt-out for legitimate internal +storage. + +https://github.com/owncloud/core/pull/41581