diff --git a/changelog/unreleased/41827 b/changelog/unreleased/41827 new file mode 100644 index 00000000000..ed593a0087d --- /dev/null +++ b/changelog/unreleased/41827 @@ -0,0 +1,15 @@ +Security: Reject SVG/script content before it reaches ImageMagick bitmap previews + +Bitmap previews (PDF, Font, ...) sanitized SVG content before decoding it, but +fell back to the original, unsanitized bytes whenever the sanitizer could not +parse the input - which happened for any malformed SVG or non-XML payload, +not only for genuinely broken SVG files. A crafted malformed SVG or a raw MVG +script could therefore reach ImageMagick unsanitized and trigger an MSL +script that reads or writes arbitrary files as the web server user. + +Bitmap previews no longer attempt to sanitize and fall back; they now reject +any content that is detected as text, XML, SVG, or MVG before ImageMagick +ever sees it, and decode through the same hardened Imagick options already +used by the dedicated SVG preview provider. + +https://github.com/owncloud/core/pull/41827 diff --git a/changelog/unreleased/41834 b/changelog/unreleased/41834 new file mode 100644 index 00000000000..a6d06ca3dc7 --- /dev/null +++ b/changelog/unreleased/41834 @@ -0,0 +1,38 @@ +Security: Pin the Imagick coder for each preview provider + +Bitmap and SVG previews decoded content with no format hint, so ImageMagick's +own content-sniffing - independent of the mime-type check that decides whether +a preview is attempted at all - could pick a different coder than the one a +provider actually serves. PostScript-looking content, which the mime check must +allow through for the PDF and Postscript providers, could therefore still reach +the Ghostscript delegate through any other bitmap provider (SGI, Font, +Illustrator, Photoshop, TIFF, Heic). + +Each provider now pins the exact Imagick coder it expects instead of letting +ImageMagick guess from the file's content. The pin is applied in memory and +introduces no temporary file of its own. + +Because media types are derived from the file name extension, a file whose +extension does not match its actual content no longer gets a preview: a JPEG +saved as photo.tif is routed to the TIFF provider, pinned to the TIFF coder, +and falls back to a media type icon where content sniffing previously rendered +it. This is the intended trade-off - content sniffing is what allowed a preview +provider to be steered to an unrelated coder in the first place. + +The affected extensions are ai, bw, eps, heic, heif, int, inta, pdf, ps, psd, +rgb, rgba, sgi, tif and tiff. Of those providers only SGI and Heic are registered +by default, so on a stock install this is visible for bw, int, inta, rgb, rgba, +sgi, heic and heif; the rest need their provider enabled in enabledPreviewProviders. + +The font extensions otf, pfb and ttf change differently: the font coder accepts +any bytes, so a mismatched file still produces a thumbnail, just one drawn by the +font coder rather than reflecting the file's real content. Real .otf files gain +previews they did not have before, because an unpinned read had no decode delegate +for them at all. + +Office documents and SVG are pinned too but are not affected. For Office the pin +covers the PDF LibreOffice has just produced rather than anything the user +uploaded, and for SVG content that is not parseable XML never reached a coder +before this change either. + +https://github.com/owncloud/core/pull/41834 diff --git a/lib/private/Preview/Bitmap.php b/lib/private/Preview/Bitmap.php index ac7b3ea08f1..e6652350a33 100644 --- a/lib/private/Preview/Bitmap.php +++ b/lib/private/Preview/Bitmap.php @@ -25,6 +25,7 @@ namespace OC\Preview; use Imagick; +use OC\Image\ImagickFactory; use OC\Preview; use OCP\Files\File; use OCP\Files\FileInfo; @@ -54,7 +55,13 @@ public function getThumbnail(File $file, $maxX, $maxY, $scalingUp) { // Creates \Imagick object from bitmap or vector file try { - $bp = $this->getResizedPreview($stream, $maxX, $maxY); + // cast on purpose: getMimeType() reaches a string-typed parameter, but it comes + // from FileInfo::getMimetype(), which hands back whatever Cache::get() stored - + // and that is MimeTypeLoader::getMimetypeById(), null for an id with no row in + // oc_mimetypes. No foreign key guards that column, so a dangling id is + // reachable, and an uncast null would raise a TypeError. Being an \Error that + // escapes the handler below, it would turn a missing preview into a 500. + $bp = $this->getResizedPreview($stream, $maxX, $maxY, (string)$file->getMimeType()); } catch (\Exception $e) { Util::writeLog('core', 'ImageMagick says: ' . $e->getmessage(), Util::ERROR); return false; @@ -89,31 +96,83 @@ public function isAvailable(FileInfo $file) { * @param resource $stream the handle of the file to convert * @param int $maxX * @param int $maxY + * @param string $mimeType the file's own detected mime type, used to pin the + * Imagick coder so it cannot be redirected by the file's actual content * * @return Imagick */ - private function getResizedPreview($stream, int $maxX, int $maxY): Imagick { - # file content can be SVG - we need to sanitize it first + private function getResizedPreview($stream, int $maxX, int $maxY, string $mimeType): Imagick { $content = \stream_get_contents($stream); - $output = SVG::sanitizeSVGContent($content); - # in case the content is not an SVG we use the original content - if ($output === '') { - $output = $content; + + if ($this->isDangerousToDecode($content)) { + throw new \RuntimeException('Refusing to decode text-based content for a bitmap preview'); } - $bp = new Imagick(); + $bp = ImagickFactory::create(); + + # Pin the coder instead of letting Imagick's own content-sniffing pick one: + # reading with no format set re-derives the format from a ~130-entry magic + # table independently of isDangerousToDecode()'s check above, so content that + # looks like PostScript/PDF (which that check must allow through for the + # Postscript/PDF providers) would otherwise reach the Ghostscript delegate via + # any Bitmap provider, not just those two. + # + # Deliberately not guarded by queryFormats(): if this build does not register + # the coder, throwing here is correct - the only alternative is falling back to + # the content-sniffing this pin exists to prevent. + $bp->setFormat($this->getImagickFormat($mimeType)); + $bp->readImageBlob($content); # setIteratorIndex(0) will make previews to be generated from the first page - $bp->readImageBlob($output); $bp->setIteratorIndex(0); $bp = $this->resize($bp, $maxX, $maxY); + # setFormat() above pins the wand's *output* format as well as the input coder, + # so both have to be set here. setImageFormat() alone would leave getThumbnail()'s + # (string) cast re-encoding back to the pinned input format instead of PNG. $bp->setImageFormat('png'); + $bp->setFormat('png'); return $bp; } + /** + * Maps this provider's own detected mime type(s) to the Imagick coder name that + * must decode them - the format pinned in getResizedPreview() above. + * + * $mimeType comes from $file->getMimeType(), deliberately not from the type that + * selected this provider (OC\Preview::$mimeType). Those two can differ, because + * callers may override the selection type via getThumbnail(['mimeType' => ...]) - + * apps/files_trashbin/ajax/preview.php does, and apps/dav passes the request's query + * parameters straight through. The file's own type cannot be steered by a request, + * which is the property the pin depends on. + * + * The consequence is that an implementation must cope with a mime type it does not + * serve: a trashed file reports application/octet-stream, because the .d + * suffix defeats extension-based detection. Returning a constant handles that + * correctly. Do NOT "fix" the divergence by rejecting a $mimeType that fails this + * provider's own getMimeType() regex - that rejects every trashbin preview. + */ + abstract protected function getImagickFormat(string $mimeType): string; + + /** + * Bitmap providers must never hand text-based content (SVG, XML, or any other + * text/* type, e.g. a raw MVG script) to Imagick::readImageBlob() - ImageMagick's + * text/vector coders can be abused to read and write arbitrary files. + */ + private function isDangerousToDecode(string $content): bool { + $mimeType = \OC::$server->getMimeTypeDetector()->detectString($content); + $mimeType = \strtolower(\trim(\explode(';', $mimeType, 2)[0])); + + // libmagic reports "image/svg" without the "+xml" suffix on some PHP/OS builds + if (\strpos($mimeType, 'text/') === 0 || \strpos($mimeType, 'image/svg') === 0) { + return true; + } + + return \in_array($mimeType, ['application/xml', 'image/x-mvg'], true); + } + /** * Returns a resized \Imagick object * diff --git a/lib/private/Preview/Font.php b/lib/private/Preview/Font.php index 775147d83ea..94c77880094 100644 --- a/lib/private/Preview/Font.php +++ b/lib/private/Preview/Font.php @@ -29,4 +29,21 @@ class Font extends Bitmap { public function getMimeType() { return '/application\/(?:font-sfnt|x-font$)/'; } + + protected function getImagickFormat(string $mimeType): string { + if ($mimeType === 'application/x-font') { + return 'PFB'; + } + # .otf and .ttf are indistinguishable by mime type alone (both are + # application/font-sfnt); TTF is what actually decodes real font files here, + # both tagged variants included. + # + # This is the only provider whose coder depends on $mimeType, so it is also the + # only one where the divergence documented on Bitmap::getImagickFormat() is + # observable: a .pfb whose stored mime type is not application/x-font - a trashed + # one reports application/octet-stream - lands here rather than in the branch + # above and gets no preview. Deciding from the content instead would mean + # re-deriving the format from magic bytes, which is what the pin exists to avoid. + return 'TTF'; + } } diff --git a/lib/private/Preview/Heic.php b/lib/private/Preview/Heic.php index 6e7bba4125f..040c7665ae6 100644 --- a/lib/private/Preview/Heic.php +++ b/lib/private/Preview/Heic.php @@ -29,4 +29,11 @@ class Heic extends Bitmap { public function getMimeType() { return '/image\/hei(f|c)/'; } + + protected function getImagickFormat(string $mimeType): string { + # image/heic and image/heif are the same container handled by the same coder + # module, and not every ImageMagick build registers a distinct HEIF coder - so + # both mime types pin HEIC rather than risk pinning a format that is absent. + return 'HEIC'; + } } diff --git a/lib/private/Preview/Illustrator.php b/lib/private/Preview/Illustrator.php index 06a98d9e4c5..0267b4712d1 100644 --- a/lib/private/Preview/Illustrator.php +++ b/lib/private/Preview/Illustrator.php @@ -30,4 +30,8 @@ class Illustrator extends Bitmap { public function getMimeType() { return '/application\/illustrator/'; } + + protected function getImagickFormat(string $mimeType): string { + return 'AI'; + } } diff --git a/lib/private/Preview/Office.php b/lib/private/Preview/Office.php index d8f038279b2..759b74fbf49 100644 --- a/lib/private/Preview/Office.php +++ b/lib/private/Preview/Office.php @@ -68,7 +68,12 @@ public function getThumbnail(File $file, $maxX, $maxY, $scalingUp) { $pdfPreview = $tmpDir . '/' . $pathInfo['filename'] . '.pdf'; # Note: no SVG sanitization of the file content required .... - $imagick = ImagickFactory::create($pdfPreview . '[0]'); + # Pin the coder: this is LibreOffice's own PDF output, but content-sniffing + # is avoided everywhere else Imagick decodes a file in this codebase, so pin + # it here too rather than rely on the ".pdf" path extension. Unlike + # setFormat(), a "FORMAT:path" constructor argument pins only the input + # coder, so setImageFormat('jpg') below is still all the output needs. + $imagick = ImagickFactory::create('PDF:' . $pdfPreview . '[0]'); $imagick->setImageFormat('jpg'); } catch (\Exception $e) { @\unlink($pdfPreview); diff --git a/lib/private/Preview/PDF.php b/lib/private/Preview/PDF.php index 0ab92bfb9cb..40ae6397091 100644 --- a/lib/private/Preview/PDF.php +++ b/lib/private/Preview/PDF.php @@ -30,4 +30,8 @@ class PDF extends Bitmap { public function getMimeType() { return '/application\/pdf/'; } + + protected function getImagickFormat(string $mimeType): string { + return 'PDF'; + } } diff --git a/lib/private/Preview/Photoshop.php b/lib/private/Preview/Photoshop.php index ca15dc1a12b..7f7add29fa9 100644 --- a/lib/private/Preview/Photoshop.php +++ b/lib/private/Preview/Photoshop.php @@ -30,4 +30,8 @@ class Photoshop extends Bitmap { public function getMimeType() { return '/application\/x-photoshop/'; } + + protected function getImagickFormat(string $mimeType): string { + return 'PSD'; + } } diff --git a/lib/private/Preview/Postscript.php b/lib/private/Preview/Postscript.php index bab73b80849..e852b66ba16 100644 --- a/lib/private/Preview/Postscript.php +++ b/lib/private/Preview/Postscript.php @@ -30,4 +30,10 @@ class Postscript extends Bitmap { public function getMimeType() { return '/application\/postscript/'; } + + protected function getImagickFormat(string $mimeType): string { + # EPS is the coder ImageMagick registers for application/postscript; it shares + # ReadPSImage() with the plain PS coder, so it covers .ps as well as .eps. + return 'EPS'; + } } diff --git a/lib/private/Preview/SGI.php b/lib/private/Preview/SGI.php index 81f43410fe3..c9c24d4ab52 100644 --- a/lib/private/Preview/SGI.php +++ b/lib/private/Preview/SGI.php @@ -27,4 +27,8 @@ class SGI extends Bitmap { public function getMimeType() { return '/image\/sgi/'; } + + protected function getImagickFormat(string $mimeType): string { + return 'SGI'; + } } diff --git a/lib/private/Preview/SVG.php b/lib/private/Preview/SVG.php index 00e474a4742..49e80f5de23 100644 --- a/lib/private/Preview/SVG.php +++ b/lib/private/Preview/SVG.php @@ -54,9 +54,25 @@ public function getThumbnail(File $file, $maxX, $maxY, $scalingUp) { # sanitize SVG content $output = self::sanitizeSVGContent($content); + if ($output === null) { + return false; + } + # Pin the coder so Imagick's own content-sniffing cannot pick a different one + # than the svg:sanitize/embed/decode options set by ImagickFactory assume. + # Guarded, unlike Bitmap.php: a build that registers no SVG coder cannot be + # pinned to it and cannot decode SVG at all either way, and $output here is + # already DOMSanitizer's serialized output rather than the raw file bytes. + if (\count(\Imagick::queryFormats('SVG')) > 0) { + $imagick->setFormat('SVG'); + } $imagick->readImageBlob($output); + + # setFormat() above pins the wand's *output* format as well as the input + # coder, so both have to be set - setImageFormat() alone would leave + # getImageBlob() below re-encoding back to SVG instead of PNG. $imagick->setImageFormat('png32'); + $imagick->setFormat('png32'); } catch (\Exception $e) { \OCP\Util::writeLog('core', $e->getmessage(), \OCP\Util::ERROR); return false; @@ -81,7 +97,7 @@ public function isAvailable(FileInfo $file) { return true; } - public static function sanitizeSVGContent(string $content): string { + public static function sanitizeSVGContent(string $content): ?string { $sanitizer = new DOMSanitizer(DOMSanitizer::SVG); $sanitizer->addDisallowedTags(['image']); $sanitizer->addDisallowedAttributes(['xlink:href']); @@ -90,6 +106,10 @@ public static function sanitizeSVGContent(string $content): string { // XML errors are expected here if the SVG is malformed \libxml_clear_errors(); + if (!\is_string($sanitized_content)) { + return null; + } + return $sanitized_content; } } diff --git a/lib/private/Preview/TIFF.php b/lib/private/Preview/TIFF.php index 25404d6e6a0..9f7ff4bfedc 100644 --- a/lib/private/Preview/TIFF.php +++ b/lib/private/Preview/TIFF.php @@ -30,4 +30,8 @@ class TIFF extends Bitmap { public function getMimeType() { return '/image\/tiff/'; } + + protected function getImagickFormat(string $mimeType): string { + return 'TIFF'; + } } diff --git a/tests/data/testimage.heic b/tests/data/testimage.heic new file mode 100644 index 00000000000..6b0bbd258cd Binary files /dev/null and b/tests/data/testimage.heic differ diff --git a/tests/data/testimage.psd b/tests/data/testimage.psd new file mode 100644 index 00000000000..16bee932e8d Binary files /dev/null and b/tests/data/testimage.psd differ diff --git a/tests/data/testimage.sgi b/tests/data/testimage.sgi new file mode 100644 index 00000000000..28f500ef08b Binary files /dev/null and b/tests/data/testimage.sgi differ diff --git a/tests/data/testimage.tiff b/tests/data/testimage.tiff new file mode 100644 index 00000000000..9db137a2f5a Binary files /dev/null and b/tests/data/testimage.tiff differ diff --git a/tests/lib/Preview/BitmapTest.php b/tests/lib/Preview/BitmapTest.php index 2c808cffb78..296c927dcb0 100644 --- a/tests/lib/Preview/BitmapTest.php +++ b/tests/lib/Preview/BitmapTest.php @@ -30,10 +30,17 @@ */ class BitmapTest extends Provider { public function setUp(): void { + # Postscript::getImagickFormat() pins EPS, so on a build that cannot decode through + # that coder this provider cannot produce a preview at all. Unguarded, that is a + # failure rather than a skip - previously ImageMagick's own sniffing hid the + # dependency. Registration alone does not answer it: coders/ps.c registers EPS + # whether or not Ghostscript is there, so the guard probes the fixture instead. + $fileName = 'testimage.eps'; + $fixture = \OC::$SERVERROOT . '/tests/data/' . $fileName; + $this->requireDecodableFixtureFile('EPS', $fixture); parent::setUp(); - $fileName = 'testimage.eps'; - $this->imgPath = $this->prepareTestFile($fileName, \OC::$SERVERROOT . '/tests/data/' . $fileName); + $this->imgPath = $this->prepareTestFile($fileName, $fixture); $this->width = 2400; $this->height = 1707; $this->provider = new \OC\Preview\Postscript; diff --git a/tests/lib/Preview/CoderPinningTest.php b/tests/lib/Preview/CoderPinningTest.php new file mode 100644 index 00000000000..4ae3c2673bc --- /dev/null +++ b/tests/lib/Preview/CoderPinningTest.php @@ -0,0 +1,374 @@ + + * + * @copyright Copyright (c) 2026, ownCloud GmbH + * @license AGPL-3.0 + * + * This code is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License, version 3, + * as published by the Free Software Foundation. + * + * 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, version 3, + * along with this program. If not, see + * + */ + +namespace Test\Preview; + +use Generator; +use OC\Image\ImagickFactory; +use OC\Preview\Bitmap; +use OC\Preview\Font; +use OC\Preview\Heic; +use OC\Preview\Illustrator; +use OC\Preview\PDF; +use OC\Preview\Photoshop; +use OC\Preview\Postscript; +use OC\Preview\SGI; +use OC\Preview\TIFF; +use OCP\Files\File; +use Test\TestCase; + +/** + * @requires extension imagick + */ +class CoderPinningTest extends TestCase { + /** + * The payload both negative tests feed to a provider it is foreign to. Deliberately + * minimal and harmless: what is under test is which coder ImageMagick hands it to, not + * what Ghostscript would draw from it. + * + * The bounding box is portrait so that it stays portrait however this build treats it. + * testFontNeverInvokesADangerousCoderForForeignContent() tells a Ghostscript render from + * FreeType's output by shape, and the two known behaviours both give a portrait raster: + * on owncloudci/php:8.3 an unpinned %!PS-Adobe read goes to the PS coder, which + * rasterizes a whole page at Ghostscript's default 612x792 and ignores %%BoundingBox, + * EPSF branding and setpagedevice alike (all three measured); where coders/ps.c instead + * derives Ghostscript's -g geometry from %%BoundingBox, this box yields 600x800. A square + * box would be portrait only under the first, so it would silently stop exercising the + * Font pin under the second. That test still asserts the shape at runtime rather than + * trusting either behaviour. + */ + private const FOREIGN_POSTSCRIPT = "%!PS-Adobe-3.0\n%%BoundingBox: 0 0 600 800\nshowpage\n"; + + private function makeFile(string $content, string $mimeType): File { + $stream = \fopen('php://memory', 'rb+'); + \fwrite($stream, $content); + \rewind($stream); + $file = $this->createMock(File::class); + $file->method('fopen')->willReturn($stream); + $file->method('getMimeType')->willReturn($mimeType); + return $file; + } + + /** + * Skip only when the one coder under test is absent from this ImageMagick build, + * rather than probing for some unrelated coder as a proxy for "extended build". + */ + private function requireCoder(string $coder): void { + if (\count(\Imagick::queryFormats($coder)) === 0) { + $this->markTestSkipped("This ImageMagick build registers no $coder coder"); + } + } + + /** + * A registered coder does not mean the delegate behind it can decode these particular + * bytes: coders/heic.c registers HEIC, HEIF and AVIF whenever libheif is present, but + * decoding the AVIF fixture additionally needs an AV1 decoder inside libheif. Read the + * fixture unpinned first - if this build cannot decode it at all, the pinned read + * failing below would say nothing about the pin, so skip rather than report a failure + * against the build. + * + * The unpinned read is content-sniffed, which is exactly what the pin exists to + * prevent. That is fine here: it is only ever a capability probe, never an assertion. + */ + private function requireDecodableFixture(string $coder, string $content): void { + $this->requireCoder($coder); + try { + $probe = ImagickFactory::create(); + $probe->readImageBlob($content); + $probe->clear(); + } catch (\Exception $e) { + # \Exception, not \ImagickException: imagick reports some delegate and policy + # conditions at warning severity, and PHPUnit 9 converts PHP warnings into + # PHPUnit\Framework\Error\Warning by default (convertWarningsToExceptions, + # unset in tests/phpunit-autotest.xml and defaulting to true - failOnWarning + # only decides whether an emitted warning fails the run). That class extends + # \Exception via PHPUnit\Framework\Exception, so one catch covers both, and + # an \Error still surfaces rather than being turned into a green skip. + $this->markTestSkipped("This ImageMagick build cannot decode the $coder fixture: " . $e->getMessage()); + } + } + + /** + * Skips unless this build can rasterize PostScript at all - the PS coder registered, + * permitted by policy.xml, with a working Ghostscript delegate behind it. Callers that + * also depend on the render's *shape* want requirePortraitPostScriptRender() instead. + * + * Returns the wand so a caller can measure it; clear() is the caller's to make. + * + * Reads unpinned, as every probe here does - a capability check, never an assertion. + */ + private function requireRenderablePostScript(string $content): \Imagick { + try { + $probe = ImagickFactory::create(); + $probe->readImageBlob($content); + return $probe; + } catch (\Exception $e) { + $this->markTestSkipped('This build cannot rasterize PostScript: ' . $e->getMessage()); + } + } + + /** + * As requireRenderablePostScript(), and additionally that the render comes out portrait. + * + * testFontNeverInvokesADangerousCoderForForeignContent() tells a Ghostscript render from + * FreeType's output by shape alone, and portrait-ness is not a given - it follows from + * Ghostscript's default page, which no part of the payload reliably pins (see + * FOREIGN_POSTSCRIPT). Asserting it means a build with a landscape default page skips + * instead of passing with the coder pin removed. + * + * Measured after reproducing what Bitmap::getResizedPreview() does before its assertion + * is observable - select the first frame, then bestfit to 32x32. A raster only a little + * taller than it is wide collapses to exactly 32x32 there, so checking the raw geometry + * would wave through a build on which the assertion cannot discriminate. + */ + private function requirePortraitPostScriptRender(string $content): void { + $probe = $this->requireRenderablePostScript($content); + + $probe->setIteratorIndex(0); + if ($probe->getImageWidth() > 32 || $probe->getImageHeight() > 32) { + $probe->resizeImage(32, 32, \Imagick::FILTER_LANCZOS, 1, true); + } + $isPortrait = $probe->getImageHeight() > $probe->getImageWidth(); + $geometry = $probe->getImageWidth() . 'x' . $probe->getImageHeight(); + $probe->clear(); + + if (!$isPortrait) { + $this->markTestSkipped( + "This build's PostScript render thumbnails to $geometry, so shape cannot tell " + . 'a Ghostscript render from the TTF specimen sheet' + ); + } + } + + /** + * isDangerousToDecode() is a deny-list over the *sniffed* type, and it denies text/*. + * On a build whose libmagic reported the payload as text/plain rather than + * application/postscript, the provider would reject it at that gate and the negative + * assertions below would hold without the coder pin ever running. Assert the detected + * type, so such a build fails loudly with an actionable message instead of passing + * vacuously. + */ + private function assertPayloadReachesTheCoderPin(string $content): void { + $detected = \OC::$server->getMimeTypeDetector()->detectString($content); + $this->assertStringStartsWith( + 'application/postscript', + $detected, + 'payload must survive isDangerousToDecode(), which denies text/* - libmagic here says: ' . $detected + ); + } + + /** + * @dataProvider providesLegitimateContent + */ + public function testDecodesItsOwnFormat(string $fixture, string $mimeType, Bitmap $provider, string $coder): void { + $content = \file_get_contents(\OC::$SERVERROOT . '/' . $fixture); + $this->requireDecodableFixture($coder, $content); + $file = $this->makeFile($content, $mimeType); + + $result = $provider->getThumbnail($file, 32, 32, false); + + $this->assertNotFalse($result, "$fixture via " . \get_class($provider) . ' should have decoded'); + } + + public function providesLegitimateContent(): Generator { + yield 'PDF' => ['tests/data/testimage.pdf', 'application/pdf', new PDF(), 'PDF']; + yield 'Postscript (EPS)' => ['tests/data/testimage.eps', 'application/postscript', new Postscript(), 'EPS']; + # Modern .ai files really are PDF containers, and ImageMagick's AI coder is a + # Ghostscript alias for the PDF one - so testimage.pdf is a faithful fixture for + # this case, and a separate .ai file would be a byte-identical copy of it. + yield 'Illustrator (AI)' => ['tests/data/testimage.pdf', 'application/illustrator', new Illustrator(), 'AI']; + yield 'Photoshop (PSD)' => ['tests/data/testimage.psd', 'application/x-photoshop', new Photoshop(), 'PSD']; + yield 'SGI' => ['tests/data/testimage.sgi', 'image/sgi', new SGI(), 'SGI']; + yield 'TIFF' => ['tests/data/testimage.tiff', 'image/tiff', new TIFF(), 'TIFF']; + # Reuses the in-tree OpenSans rather than adding a font fixture of its own. No + # genuine OTF ('OTTO'-tagged) case here: this environment's ImageMagick/FreeType + # delegate cannot decode CFF-outline OpenType fonts at all, pinned or not - + # confirmed against four real system .otf files. TTF-tagged content, which the TTF + # coder decodes fine, is what's actually exercised in practice for the font-sfnt + # mime type. + yield 'Font (font-sfnt, ttf bytes)' => ['core/fonts/OpenSans-Regular.ttf', 'application/font-sfnt', new Font(), 'TTF']; + # The HEIC fixture is AVIF-branded on purpose: coders/heic.c registers HEIC, HEIF + # and AVIF as three separate coders, so an AVIF-branded file served by the Heic + # provider is the case worth a real sample - and an HEVC-encoded one would need a + # libde265 delegate that is not present everywhere. + # + # Both mime types pin HEIC, so neither case needs a distinct HEIF coder to be + # registered - which is the point: pinning HEIF would break image/heif previews + # on every build that only registers HEIC. + yield 'Heic (image/heic)' => ['tests/data/testimage.heic', 'image/heic', new Heic(), 'HEIC']; + yield 'Heic (image/heif)' => ['tests/data/testimage.heic', 'image/heif', new Heic(), 'HEIC']; + } + + /** + * PostScript content is sniffed by libmagic as application/postscript, which + * isDangerousToDecode() must not reject since Postscript/PDF legitimately decode + * it - so the mime-type gate alone lets it through here too. Pinning the expected + * coder is what stops ImageMagick's own content-sniffing from handing it to the + * Ghostscript delegate through a provider that has nothing to do with PostScript. + * + * PDF/Postscript/Illustrator are deliberately not in this set: they are the + * Ghostscript-backed providers PostScript-ish content is NOT foreign to, so + * feeding it to them tests Ghostscript's own leniency, not cross-coder confusion. + * Font is also excluded: see testFontNeverInvokesADangerousCoderForForeignContent(). + * + * @dataProvider providesForeignProviders + */ + public function testRejectsPostScriptContentFromAForeignProvider(Bitmap $provider, string $mimeType, string $coder): void { + $this->requireCoder($coder); + # Without a usable PostScript path the mutant this guards against - the pin removed - + # cannot decode the payload either, so assertFalse() would hold with no pin in place. + $this->requireRenderablePostScript(self::FOREIGN_POSTSCRIPT)->clear(); + $this->assertPayloadReachesTheCoderPin(self::FOREIGN_POSTSCRIPT); + $file = $this->makeFile(self::FOREIGN_POSTSCRIPT, $mimeType); + + $result = $provider->getThumbnail($file, 32, 32, false); + + $this->assertFalse($result); + } + + public function providesForeignProviders(): Generator { + yield 'SGI' => [new SGI(), 'image/sgi', 'SGI']; + yield 'Photoshop' => [new Photoshop(), 'application/x-photoshop', 'PSD']; + yield 'TIFF' => [new TIFF(), 'image/tiff', 'TIFF']; + yield 'Heic' => [new Heic(), 'image/heic', 'HEIC']; + } + + public function testFontNeverInvokesADangerousCoderForForeignContent(): void { + $this->requireCoder('TTF'); + # Both preconditions the assertion below rests on, asserted rather than assumed: that + # the pin-removed mutant would rasterize this payload at all, and that it would come + # out portrait. Without Ghostscript, or under the stock Debian policy denying the PS + # coder, readImageBlob() throws, getThumbnail() returns false and the assertion would + # hold with no pin in place - green while protecting nothing. + $this->requirePortraitPostScriptRender(self::FOREIGN_POSTSCRIPT); + $this->assertPayloadReachesTheCoderPin(self::FOREIGN_POSTSCRIPT); + $file = $this->makeFile(self::FOREIGN_POSTSCRIPT, 'application/font-sfnt'); + + $result = (new Font())->getThumbnail($file, 32, 32, false); + + # FreeType fails on non-font bytes either by refusing them outright or by producing + # a placeholder, never by invoking Ghostscript or a script coder - both are safe + # outcomes. What must never happen is the PostScript page itself coming back + # rendered. Shape is what separates the two: ImageMagick's TTF coder draws a fixed + # 800x480 specimen sheet, landscape, while the PS coder rasterizes a full page at + # Ghostscript's default size, portrait. Measured on owncloudci/php:8.3, 32x19 through + # the TTF pin against 25x32 with the pin removed - 25x32 being 612x792 scaled, i.e. + # the default page, NOT anything this payload asked for. + # + # Size cannot be used for this. OC_Image::data() re-encodes through GD, and by then + # the image is already downscaled to fit 32x32, so both outcomes land within a few + # hundred bytes of each other - an earlier revision of this test asserted a 2048 + # byte ceiling and could not fail. Nor can assertFalse(): the placeholder is a + # valid image on this build, so that would fail where the pin is working. + # + # One branch-free expression on purpose - branching would leave the test + # assertion-less on a build that returns false, and failOnRisky in + # tests/phpunit-autotest.xml makes a zero-assertion test a hard failure. + $renderedThePortraitPage = $result !== false && $result->height() > $result->width(); + $this->assertFalse($renderedThePortraitPage, 'Font must not render PostScript content'); + } + + /** + * The pin is derived from the file's own mime type, not from the one that selected + * the provider - callers can override the latter via getThumbnail(['mimeType' => ...]), + * and apps/files_trashbin/ajax/preview.php does exactly that, because a trashed + * file's .d suffix defeats extension-based detection and leaves it + * reporting application/octet-stream. + * + * So a provider must still decode when handed a mime type it does not serve. Guard + * that: making the provider reject a mime type failing its own getMimeType() regex + * looks like a tightening, but it would silently kill every trashbin bitmap preview. + * + * @dataProvider providesForeignMimeTypeButOwnContent + */ + public function testDecodesWhenTheStoredMimeTypeIsNotTheProvidersOwn( + string $fixture, + Bitmap $provider, + string $coder + ): void { + $content = \file_get_contents(\OC::$SERVERROOT . '/' . $fixture); + $this->requireDecodableFixture($coder, $content); + # what a trashed "photo.tif.d1700000000" actually reports + $file = $this->makeFile($content, 'application/octet-stream'); + + $this->assertSame( + 0, + \preg_match($provider->getMimeType(), 'application/octet-stream'), + 'precondition: this mime type must NOT match the provider regex, or the case proves nothing' + ); + $this->assertNotFalse( + $provider->getThumbnail($file, 32, 32, false), + 'a provider pinning a constant coder must still decode its own content' + ); + } + + public function providesForeignMimeTypeButOwnContent(): Generator { + yield 'TIFF' => ['tests/data/testimage.tiff', new TIFF(), 'TIFF']; + yield 'Photoshop' => ['tests/data/testimage.psd', new Photoshop(), 'PSD']; + yield 'SGI' => ['tests/data/testimage.sgi', new SGI(), 'SGI']; + } + + /** + * The stored mime type can also be missing altogether, not just be the wrong one: + * FileInfo::getMimetype() returns whatever Cache::get() put in the row, which is + * MimeTypeLoader::getMimetypeById() - null for a mimetype id with no matching row in + * oc_mimetypes. Nothing constrains that column, so a dangling id survives there. + * + * Uncast, that null hits getResizedPreview()'s string parameter as a TypeError, and a + * TypeError is an \Error: it escapes getThumbnail()'s catch (\Exception) and reaches + * the caller as a 500 instead of degrading to a media-type icon. assertNotFalse() is + * what detects it - the \Error propagates out of this test as an error, not a failure. + */ + public function testDecodesWhenTheStoredMimeTypeIsMissingEntirely(): void { + $content = \file_get_contents(\OC::$SERVERROOT . '/tests/data/testimage.tiff'); + $this->requireDecodableFixture('TIFF', $content); + + $stream = \fopen('php://memory', 'rb+'); + \fwrite($stream, $content); + \rewind($stream); + $file = $this->createMock(File::class); + $file->method('fopen')->willReturn($stream); + $file->method('getMimeType')->willReturn(null); + + $this->assertNotFalse( + (new TIFF())->getThumbnail($file, 32, 32, false), + 'a null stored mime type must still decode, because TIFF pins a constant coder' + ); + } + + /** + * setFormat() pins the wand's *output* format as well as the input coder, so a + * provider that reset only the image format would hand back the input format + * re-encoded instead of a PNG. Guard that explicitly: for TIFF the re-encode is + * byte-identical to the input, which makes the mistake easy to reintroduce and + * hard to spot. + */ + public function testPinnedDecodeReturnsPngAndNotThePinnedInputFormat(): void { + $content = \file_get_contents(\OC::$SERVERROOT . '/tests/data/testimage.tiff'); + $this->requireDecodableFixture('TIFF', $content); + $file = $this->makeFile($content, 'image/tiff'); + + $result = (new TIFF())->getThumbnail($file, 32, 32, false); + + $this->assertNotFalse($result); + $this->assertSame('image/png', $result->mimeType()); + } +} diff --git a/tests/lib/Preview/PDFTest.php b/tests/lib/Preview/PDFTest.php index 4c7d700b7d8..30e1d9ca824 100644 --- a/tests/lib/Preview/PDFTest.php +++ b/tests/lib/Preview/PDFTest.php @@ -37,16 +37,19 @@ class PDFTest extends Provider { * @throws NotFoundException */ public function setUp(): void { - if (\count(\Imagick::queryFormats('SVG')) === 1) { - parent::setUp(); + # PDF is the coder PDF::getImagickFormat() pins. This used to gate on the SVG + # coder, which this provider never touches - so on any build registering no SVG + # coder (owncloudci/php:8.3 among them) every case here skipped, reporting "No + # PDF provider present" while the PDF coder was in fact present. A registration + # check is not the right replacement either: see requireDecodableFixtureFile(). + $fileName = 'testimage.pdf'; + $fixture = \OC::$SERVERROOT . '/tests/data/' . $fileName; + $this->requireDecodableFixtureFile('PDF', $fixture); + parent::setUp(); - $fileName = 'testimage.pdf'; - $this->imgPath = $this->prepareTestFile($fileName, \OC::$SERVERROOT . '/tests/data/' . $fileName); - $this->width = 595; - $this->height = 842; - $this->provider = new PDF(); - } else { - $this->markTestSkipped('No PDF provider present'); - } + $this->imgPath = $this->prepareTestFile($fileName, $fixture); + $this->width = 595; + $this->height = 842; + $this->provider = new PDF(); } } diff --git a/tests/lib/Preview/Provider.php b/tests/lib/Preview/Provider.php index 3dfeecf1eca..056fbf7f81e 100644 --- a/tests/lib/Preview/Provider.php +++ b/tests/lib/Preview/Provider.php @@ -21,6 +21,7 @@ namespace Test\Preview; +use OC\Image\ImagickFactory; use OC\Preview\TXT; use OCP\Files\File; use OCP\Files\Node; @@ -71,6 +72,52 @@ protected function tearDown(): void { parent::tearDown(); } + /** + * Skips unless this ImageMagick build can actually decode the file at $fixturePath + * through $coder - the coder the provider under test pins. + * + * Named for the *File* it takes, because Test\Preview\CoderPinningTest has a helper of + * the same purpose that takes the content blob instead. Passing a path where a blob is + * expected would make readImageBlob() throw, which a probe like this one converts into + * a skip - so the mistake would silently stop a test running rather than fail. + * + * Imagick::queryFormats() is not enough on its own: it reports only whether a coder is + * *registered*, and coders/pdf.c and coders/ps.c register PDF, AI and EPS + * unconditionally, wiring the Ghostscript delegate behind them separately. + * MagickQueryFormats() does not consult policy.xml either, and the stock Debian and + * Ubuntu policy denies the PDF/PS/EPS/XPS coders outright. So on a build with no + * Ghostscript, or under that policy, a registration check still answers "present" and + * the calling test fails where it should skip. Probing the fixture covers both. + * + * The probe reads unpinned, which is the very thing the pin exists to prevent. That is + * fine: it is only ever a capability probe, never an assertion. + */ + protected function requireDecodableFixtureFile(string $coder, string $fixturePath): void { + if (\count(\Imagick::queryFormats($coder)) === 0) { + $this->markTestSkipped("This ImageMagick build registers no $coder coder"); + } + + # read outside the try: an unreadable fixture is a broken test, not a build + # limitation, and must not be converted into a skip + $content = \file_get_contents($fixturePath); + $this->assertNotFalse($content, "fixture $fixturePath must be readable"); + + try { + $probe = ImagickFactory::create(); + $probe->readImageBlob($content); + $probe->clear(); + } catch (\Exception $e) { + # \Exception rather than \ImagickException: imagick reports some delegate and + # policy conditions at warning severity, and PHPUnit 9 converts PHP warnings + # into PHPUnit\Framework\Error\Warning by default (convertWarningsToExceptions, + # which phpunit-autotest.xml leaves unset; failOnWarning only decides whether an + # emitted warning fails the run). That class reaches \Exception via + # PHPUnit\Framework\Exception, so one catch covers both - and unlike \Throwable + # it still lets an \Error fail instead of becoming a green skip. + $this->markTestSkipped("This ImageMagick build cannot decode the $coder fixture: " . $e->getMessage()); + } + } + public static function dimensionsDataProvider() { return [ [-\random_int(5, 100), -\random_int(5, 100)], diff --git a/tests/lib/Preview/SVGTest.php b/tests/lib/Preview/SVGTest.php index dd4a2af53af..5926e0a6811 100644 --- a/tests/lib/Preview/SVGTest.php +++ b/tests/lib/Preview/SVGTest.php @@ -30,16 +30,17 @@ */ class SVGTest extends Provider { public function setUp(): void { - if (\count(\Imagick::queryFormats('SVG')) === 1) { - parent::setUp(); - - $fileName = 'testimagelarge.svg'; - $this->imgPath = $this->prepareTestFile($fileName, \OC::$SERVERROOT . '/tests/data/' . $fileName); - $this->width = 3000; - $this->height = 2000; - $this->provider = new \OC\Preview\SVG; - } else { - $this->markTestSkipped('No SVG provider present'); + # === 0 rather than === 1: a build may register SVG alongside SVGZ/MSVG, which + # would have skipped these cases while the SVG coder was present all along + if (\count(\Imagick::queryFormats('SVG')) === 0) { + $this->markTestSkipped('This ImageMagick build registers no SVG coder'); } + parent::setUp(); + + $fileName = 'testimagelarge.svg'; + $this->imgPath = $this->prepareTestFile($fileName, \OC::$SERVERROOT . '/tests/data/' . $fileName); + $this->width = 3000; + $this->height = 2000; + $this->provider = new \OC\Preview\SVG; } } diff --git a/tests/lib/Preview/SanitizeTest.php b/tests/lib/Preview/SanitizeTest.php index bcac9efb350..fb6065429b2 100644 --- a/tests/lib/Preview/SanitizeTest.php +++ b/tests/lib/Preview/SanitizeTest.php @@ -25,29 +25,61 @@ use OC\Preview\Bitmap; use OC\Preview\Font; use OC\Preview\PDF; +use OC\Preview\Postscript; use OCP\Files\File; use Test\TestCase; +/** + * @requires extension imagick + */ class SanitizeTest extends TestCase { /** * @dataProvider providesSVG */ - public function test(string $svgContent, Bitmap $provider): void { - if (\count(\Imagick::queryFormats('SVG')) === 0) { - $this->markTestSkipped('No SVG provider present'); - } + public function test(string $content, Bitmap $provider, string $mimeType): void { + # no coder guard on purpose: isDangerousToDecode() rejects this content before + # ImagickFactory::create() and before setFormat(), so these cases never reach a + # coder at all. Requiring one would only let a reduced build skip the OC10-164 + # regression assertions silently. + $this->assertPayloadIsDeniedByTheMimeGate($content); + # mock it all .... $stream = fopen('php://memory', 'rb+'); - fwrite($stream, $svgContent); + fwrite($stream, $content); rewind($stream); $file = $this->createMock(File::class); - $file->method('getContent')->willReturn($svgContent); + $file->method('getContent')->willReturn($content); $file->method('fopen')->willReturn($stream); + $file->method('getMimeType')->willReturn($mimeType); - # create the preview + # create the preview - SVG/text/script-shaped content must never reach Imagick via a Bitmap provider $return = $provider->getThumbnail($file, 32, 32, false); - $this->assertImage(__DIR__ . '/white-32x32.png', $return); + $this->assertFalse($return); + } + + /** + * Control assertion, mirroring Bitmap::isDangerousToDecode()'s deny-list. + * + * That gate keys on the *sniffed* type, and libmagic's answer varies by build - the + * same SVG is image/svg+xml on PHP 8.3 and image/svg on 7.4. A build that classified + * one of these payloads as something the deny-list misses would send it to the coder + * instead, where the pin would very likely reject it anyway and assertFalse() below + * would still pass: the case would go quiet rather than fail. Assert the precondition + * so such a build reports an actionable failure. + */ + private function assertPayloadIsDeniedByTheMimeGate(string $content): void { + $detected = \OC::$server->getMimeTypeDetector()->detectString($content); + $type = \strtolower(\trim(\explode(';', $detected, 2)[0])); + + $denied = \strpos($type, 'text/') === 0 + || \strpos($type, 'image/svg') === 0 + || \in_array($type, ['application/xml', 'image/x-mvg'], true); + + $this->assertTrue( + $denied, + 'payload must be one isDangerousToDecode() denies, or this case proves nothing - libmagic here says: ' . $detected + ); } public function providesSVG(): Generator { @@ -58,8 +90,49 @@ public function providesSVG(): Generator { SVG; + # malformed SVG (unclosed ) - the DOM sanitizer cannot parse this and + # used to fall back to the raw, unsanitized content + $malformedSvgWithMslHref = << + + +SVG; + + $rawMvg = << +SVG; + + # The payload below is the one the PDF and Postscript cases need, because those two + # providers pin a Ghostscript-backed coder and so are the only ones whose pin does + # NOT reject foreign content - for every other provider the pin is a second line of + # defence that makes the assertion hold with or without the mime gate. PostScript + # with its %!PS-Adobe header sniffs as application/postscript, which the gate must + # let through (Preview\Postscript and Preview\PDF have to decode real ones), so it + # cannot serve here. Drop the header and libmagic reports text/plain - denied by the + # gate - while an affirmed PDF:/EPS: pin still hands it straight to Ghostscript, + # which renders it at 595x842 / 612x792. That combination is what makes these cases + # fail if the gate is ever removed, instead of passing on the pin alone. + $headerlessPostScript = "newpath 10 10 moveto 50 50 lineto 4 setlinewidth stroke showpage\n"; + # all Bitmap based providers use the same thumbnailing logic - two is enough .... - yield 'PDF provider' => [$svgContent0, new PDF()]; - yield 'Font Provider' => [$svgContent0, new Font()]; + yield 'PDF provider - image tag' => [$svgContent0, new PDF(), 'application/pdf']; + yield 'Font Provider - image tag' => [$svgContent0, new Font(), 'application/font-sfnt']; + yield 'PDF provider - malformed SVG with MSL href' => [$malformedSvgWithMslHref, new PDF(), 'application/pdf']; + yield 'Font Provider - malformed SVG with MSL href' => [$malformedSvgWithMslHref, new Font(), 'application/font-sfnt']; + yield 'PDF provider - raw MVG' => [$rawMvg, new PDF(), 'application/pdf']; + yield 'Font Provider - raw MVG' => [$rawMvg, new Font(), 'application/font-sfnt']; + yield 'PDF provider - well-formed SVG' => [$wellFormedSvg, new PDF(), 'application/pdf']; + yield 'Font Provider - well-formed SVG' => [$wellFormedSvg, new Font(), 'application/font-sfnt']; + yield 'PDF provider - headerless PostScript' => [$headerlessPostScript, new PDF(), 'application/pdf']; + yield 'Postscript provider - headerless PostScript' => [ + $headerlessPostScript, new Postscript(), 'application/postscript' + ]; } }