fix(settings): verify source of app-discover mediabackport/54019/stable29

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

1 files changed, 50 insertions, 3 deletions

diff --git a/apps/settings/lib/Controller/AppSettingsController.php b/apps/settings/lib/Controller/AppSettingsController.php
index 35751522c4c..627aa7b6f68 100644
--- a/apps/settings/lib/Controller/AppSettingsController.php
+++ b/apps/settings/lib/Controller/AppSettingsController.php
@@ -66,7 +66,9 @@ use OCP\IL10N;
 use OCP\INavigationManager;
 use OCP\IRequest;
 use OCP\IURLGenerator;
+use OCP\IUserSession;
 use OCP\L10N\IFactory;
+use OCP\Security\RateLimiting\ILimiter;
 use Psr\Log\LoggerInterface;
 
 #[OpenAPI(scope: OpenAPI::SCOPE_IGNORE)]
@@ -136,7 +138,6 @@ class AppSettingsController extends Controller {
 	}
 
 	/**
-	 * @PublicPage
 	 * @NoCSRFRequired
 	 *
 	 * Get a image for the app discover section - this is proxied for privacy and CSP reasons
@@ -144,8 +145,9 @@ class AppSettingsController extends Controller {
 	 * @param string $image
 	 * @throws \Exception
 	 */
-	public function getAppDiscoverMedia(string $fileName): Response {
-		$etag = $this->discoverFetcher->getETag() ?? date('Y-m');
+	public function getAppDiscoverMedia(string $fileName, ILimiter $limiter, IUserSession $session): Response {
+		$getEtag = $this->discoverFetcher->getETag() ?? date('Y-m');
+		$etag = trim($getEtag, '"');
 		$folder = null;
 		try {
 			$folder = $this->appData->getFolder('app-discover-cache');
@@ -172,6 +174,26 @@ class AppSettingsController extends Controller {
 		$file = reset($file);
 		// If not found request from Web
 		if ($file === false) {
+			$user = $session->getUser();
+			// this route is not public thus we can assume a user is logged-in
+			assert($user !== null);
+			// Register a user request to throttle fetching external data
+			// this will prevent using the server for DoS of other systems.
+			$limiter->registerUserRequest(
+				'settings-discover-media',
+				// allow up to 24 media requests per hour
+				// this should be a sane default when a completely new section is loaded
+				// keep in mind browsers request all files from a source-set
+				24,
+				60 * 60,
+				$user,
+			);
+
+			if (!$this->checkCanDownloadMedia($fileName)) {
+				$this->logger->warning('Tried to load media files for app discover section from untrusted source');
+				return new NotFoundResponse(Http::STATUS_BAD_REQUEST);
+			}
+
 			try {
 				$client = $this->clientService->newClient();
 				$fileResponse = $client->get($fileName);
@@ -193,6 +215,31 @@ class AppSettingsController extends Controller {
 		return $response;
 	}
 
+	private function checkCanDownloadMedia(string $filename): bool {
+		$urlInfo = parse_url($filename);
+		if (!isset($urlInfo['host']) || !isset($urlInfo['path'])) {
+			return false;
+		}
+
+		// Always allowed hosts
+		if ($urlInfo['host'] === 'nextcloud.com') {
+			return true;
+		}
+
+		// Hosts that need further verification
+		// Github is only allowed if from our organization
+		$ALLOWED_HOSTS = ['github.com', 'raw.githubusercontent.com'];
+		if (!in_array($urlInfo['host'], $ALLOWED_HOSTS)) {
+			return false;
+		}
+
+		if (str_starts_with($urlInfo['path'], '/nextcloud/') || str_starts_with($urlInfo['path'], '/nextcloud-gmbh/')) {
+			return true;
+		}
+
+		return false;
+	}
+
 	/**
 	 * Remove orphaned folders from the image cache that do not match the current etag
 	 * @param ISimpleFolder $folder The folder to clear