From b04449b0cf91c2193ee5e3b23faa367840bad202 Mon Sep 17 00:00:00 2001 From: erseco Date: Sat, 26 Sep 2026 07:00:28 +0100 Subject: [PATCH] fix(security): refuse top-level asset loads and cap screenshot reads The server asset fallback served package HTML under the Nextcloud origin with no sandbox, so a crafted shared .elpx opened directly at /apps/exelearning/asset//index.html ran its scripts as the victim. Refuse requests whose Sec-Fetch-Dest is 'document'; the viewer iframe sends 'iframe' and keeps working. screenshot.png reads were only bounded by the 500 MB package limit, and getFromName() reserves the declared size up front, so one package could fatal every thumbnail run. Cap that read at 10 MB and map an oversized screenshot to 404 in ThumbnailController. --- lib/Controller/AssetController.php | 7 ++++++ lib/Controller/ThumbnailController.php | 7 +++++- lib/Preview/ElpxPreviewProvider.php | 2 +- lib/Service/ZipEntryService.php | 24 ++++++++++++------- tests/Unit/Controller/AssetControllerTest.php | 14 ++++++++++- .../Controller/ThumbnailControllerTest.php | 12 +++++++++- tests/Unit/Service/ZipEntryServiceTest.php | 9 +++++++ 7 files changed, 63 insertions(+), 12 deletions(-) diff --git a/lib/Controller/AssetController.php b/lib/Controller/AssetController.php index 2d6e22a..9a34362 100644 --- a/lib/Controller/AssetController.php +++ b/lib/Controller/AssetController.php @@ -69,6 +69,13 @@ public function fetch(string $sessionId, string $path): DataDisplayResponse|Data if ($user === null) { return new DataResponse(['error' => 'Not authenticated'], Http::STATUS_UNAUTHORIZED); } + // Package HTML may only render inside the viewer iframe. Opened as a + // top-level page it would run author scripts on the Nextcloud origin. + // ponytail: browsers without Fetch Metadata pass; the opaque-origin + // sandbox is the real fix. + if ($this->request->getHeader('Sec-Fetch-Dest') === 'document') { + return new DataResponse(['error' => 'Not embeddable here'], Http::STATUS_FORBIDDEN); + } $fileId = (int)$sessionId; if ($fileId <= 0) { return new DataResponse(['error' => 'Invalid session'], Http::STATUS_BAD_REQUEST); diff --git a/lib/Controller/ThumbnailController.php b/lib/Controller/ThumbnailController.php index d680062..9a43aa1 100644 --- a/lib/Controller/ThumbnailController.php +++ b/lib/Controller/ThumbnailController.php @@ -16,6 +16,7 @@ use OCP\Files\NotPermittedException; use OCP\IRequest; use OCP\IUserSession; +use RuntimeException; /** * Returns the `screenshot.png` from inside an `.elpx` package, suitable for @@ -49,7 +50,11 @@ public function byFileId(int $fileId): DataDisplayResponse|DataResponse { return new DataResponse(['error' => $e->getMessage()], Http::STATUS_FORBIDDEN); } - $bytes = $this->zipEntries->readEntry($file, 'screenshot.png'); + try { + $bytes = $this->zipEntries->readEntry($file, 'screenshot.png', ZipEntryService::MAX_SCREENSHOT_BYTES); + } catch (RuntimeException) { + $bytes = null; + } if ($bytes === null) { return new DataResponse(['error' => 'No screenshot'], Http::STATUS_NOT_FOUND); } diff --git a/lib/Preview/ElpxPreviewProvider.php b/lib/Preview/ElpxPreviewProvider.php index 5868eda..593363e 100644 --- a/lib/Preview/ElpxPreviewProvider.php +++ b/lib/Preview/ElpxPreviewProvider.php @@ -59,7 +59,7 @@ public function isAvailable(\OCP\Files\FileInfo $file): bool { public function getThumbnail(File $file, int $maxX, int $maxY): ?IImage { try { - $bytes = $this->zipEntries->readEntry($file, 'screenshot.png'); + $bytes = $this->zipEntries->readEntry($file, 'screenshot.png', ZipEntryService::MAX_SCREENSHOT_BYTES); if ($bytes !== null) { $image = new Image(); $image->loadFromData($bytes); diff --git a/lib/Service/ZipEntryService.php b/lib/Service/ZipEntryService.php index 8a42797..f13bb58 100644 --- a/lib/Service/ZipEntryService.php +++ b/lib/Service/ZipEntryService.php @@ -18,6 +18,12 @@ class ZipEntryService { public const MAX_ENTRIES = 5000; public const MAX_UNCOMPRESSED_SIZE_BYTES = 500 * 1024 * 1024; + /** + * Cap for `screenshot.png`. Thumbnails run in cron and the Files grid, so + * a package declaring a huge screenshot must not reserve a buffer anywhere + * near `memory_limit`. + */ + public const MAX_SCREENSHOT_BYTES = 10 * 1024 * 1024; /** * The limits are injectable so tests can exercise them with small @@ -36,8 +42,10 @@ public function __construct( * Entry names that are not already canonical — traversal, absolute paths, * dot segments, doubled slashes, backslashes — are rejected outright by * {@see self::normalizeEntry()} rather than repaired. + * + * `$maxBytes` tightens the uncompressed-size limit for this one read. */ - public function readEntry(File $file, string $entry): ?string { + public function readEntry(File $file, string $entry, ?int $maxBytes = null): ?string { $normalized = $this->normalizeEntry($entry); if ($normalized === null) { return null; @@ -45,7 +53,7 @@ public function readEntry(File $file, string $entry): ?string { $localPath = $file->getStorage()->getLocalFile($file->getInternalPath()); if (!is_string($localPath) || $localPath === '') { - return $this->readEntryFromStream($file, $normalized); + return $this->readEntryFromStream($file, $normalized, $maxBytes); } $zip = new ZipArchive(); @@ -54,10 +62,10 @@ public function readEntry(File $file, string $entry): ?string { // Playground filesystem) expose a nominal local path that native // ZipArchive still cannot open. Treat it like any other non-local // storage and retry through the portable File::fopen() path. - return $this->readEntryFromStream($file, $normalized); + return $this->readEntryFromStream($file, $normalized, $maxBytes); } try { - return $this->extractEntry($zip, $normalized); + return $this->extractEntry($zip, $normalized, $maxBytes); } finally { $zip->close(); } @@ -68,7 +76,7 @@ public function readEntry(File $file, string $entry): ?string { * the stream fallback go through here so they enforce identical limits * and agree on returning null for missing entries. */ - private function extractEntry(ZipArchive $zip, string $entry): ?string { + private function extractEntry(ZipArchive $zip, string $entry, ?int $maxBytes): ?string { if ($zip->numFiles > $this->maxEntries) { return null; } @@ -77,7 +85,7 @@ private function extractEntry(ZipArchive $zip, string $entry): ?string { return null; } $declaredSize = $stat['size']; - if ($declaredSize > $this->maxUncompressedSizeBytes) { + if ($declaredSize > min($this->maxUncompressedSizeBytes, $maxBytes ?? PHP_INT_MAX)) { throw new RuntimeException('Uncompressed entry exceeds limit'); } // getFromName() allocates its whole $len buffer up front, so the read @@ -161,7 +169,7 @@ public function normalizeEntry(string $entry): ?string { * Fallback for storages that cannot expose a local file (object storage, * external mounts). Copies the package to a temp file first. */ - private function readEntryFromStream(File $file, string $entry): ?string { + private function readEntryFromStream(File $file, string $entry, ?int $maxBytes): ?string { $tmp = tempnam(sys_get_temp_dir(), 'elpx_'); if ($tmp === false) { return null; @@ -183,7 +191,7 @@ private function readEntryFromStream(File $file, string $entry): ?string { return null; } try { - return $this->extractEntry($zip, $entry); + return $this->extractEntry($zip, $entry, $maxBytes); } finally { $zip->close(); } diff --git a/tests/Unit/Controller/AssetControllerTest.php b/tests/Unit/Controller/AssetControllerTest.php index 4fed135..3c86195 100644 --- a/tests/Unit/Controller/AssetControllerTest.php +++ b/tests/Unit/Controller/AssetControllerTest.php @@ -17,18 +17,20 @@ use PHPUnit\Framework\TestCase; final class AssetControllerTest extends TestCase { + private IRequest $request; private IUserSession $userSession; private ElpxPackageService $packages; private ZipEntryService $zipEntries; private AssetController $controller; protected function setUp(): void { + $this->request = $this->createMock(IRequest::class); $this->userSession = $this->createMock(IUserSession::class); $this->packages = $this->createMock(ElpxPackageService::class); $this->zipEntries = $this->createMock(ZipEntryService::class); $this->controller = new AssetController( 'exelearning', - $this->createMock(IRequest::class), + $this->request, $this->userSession, $this->packages, $this->zipEntries, @@ -44,6 +46,16 @@ public function testRejectsAnonymousRequests(): void { self::assertSame(['error' => 'Not authenticated'], $response->getData()); } + public function testRefusesTopLevelNavigation(): void { + $this->authenticate(); + $this->request->method('getHeader')->with('Sec-Fetch-Dest')->willReturn('document'); + $this->packages->expects(self::never())->method('getForUserById'); + + $response = $this->controller->fetch('42', 'index.html'); + + self::assertSame(Http::STATUS_FORBIDDEN, $response->getStatus()); + } + public function testRejectsInvalidSessionId(): void { $this->authenticate(); diff --git a/tests/Unit/Controller/ThumbnailControllerTest.php b/tests/Unit/Controller/ThumbnailControllerTest.php index 207009f..ac2c610 100644 --- a/tests/Unit/Controller/ThumbnailControllerTest.php +++ b/tests/Unit/Controller/ThumbnailControllerTest.php @@ -60,7 +60,7 @@ public function testReturnsNotFoundWithoutScreenshot(): void { $this->authenticate(); $file = $this->createMock(File::class); $this->packages->method('getForUserById')->willReturn($file); - $this->zipEntries->method('readEntry')->with($file, 'screenshot.png')->willReturn(null); + $this->zipEntries->method('readEntry')->with($file, 'screenshot.png', ZipEntryService::MAX_SCREENSHOT_BYTES)->willReturn(null); $response = $this->controller->byFileId(42); @@ -68,6 +68,16 @@ public function testReturnsNotFoundWithoutScreenshot(): void { self::assertSame(['error' => 'No screenshot'], $response->getData()); } + public function testTreatsOversizedScreenshotAsMissing(): void { + $this->authenticate(); + $this->packages->method('getForUserById')->willReturn($this->createMock(File::class)); + $this->zipEntries->method('readEntry')->willThrowException(new \RuntimeException('too big')); + + $response = $this->controller->byFileId(42); + + self::assertSame(Http::STATUS_NOT_FOUND, $response->getStatus()); + } + public function testReturnsPngWithPrivateCacheHeaders(): void { $this->authenticate(); $file = $this->createMock(File::class); diff --git a/tests/Unit/Service/ZipEntryServiceTest.php b/tests/Unit/Service/ZipEntryServiceTest.php index 9ecf6c0..c6e90a4 100644 --- a/tests/Unit/Service/ZipEntryServiceTest.php +++ b/tests/Unit/Service/ZipEntryServiceTest.php @@ -157,6 +157,15 @@ public function testLocalPathRejectsEntriesOverTheUncompressedSizeLimit(): void } } + public function testPerReadCapTightensTheUncompressedSizeLimit(): void { + $archive = $this->createTestArchive('screenshot.png', '0123456789'); + $file = $this->createFakeFile($archive, ''); + + self::assertSame('0123456789', $this->service->readEntry($file, 'screenshot.png', 10)); + $this->expectException(RuntimeException::class); + $this->service->readEntry($file, 'screenshot.png', 9); + } + public function testStreamFallbackReturnsNullWhenArchiveHasTooManyEntries(): void { $service = new ZipEntryService(maxEntries: 1); $archivePath = tempnam(sys_get_temp_dir(), 'elpx_test_');