fix: pin the Imagick coder per provider without a temporary file (OC10-164)

changelog/unreleased/41834

@@ -0,0 +1,23 @@
+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, so
+preview generation adds no filesystem access and no temporary file.
+
+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 affects the tif, psd, sgi, heic and ai extensions and is the intended
+trade-off - content sniffing is what allowed a preview provider to be steered
+to an unrelated coder in the first place.
+
+https://github.com/owncloud/core/pull/41834

lib/private/Preview/Bitmap.php

@@ -49,7 +49,7 @@ public function getThumbnail(File $file, $maxX, $maxY, $scalingUp) {
 
 		// Creates \Imagick object from bitmap or vector file
 		try {
-			$bp = $this->getResizedPreview($stream, $maxX, $maxY);
+			$bp = $this->getResizedPreview($stream, $maxX, $maxY, $file->getMimeType());
 		} catch (\Exception $e) {
 			Util::writeLog('core', 'ImageMagick says: ' . $e->getmessage(), Util::ERROR);
 			return false;
@@ -82,10 +82,12 @@ 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 {
+	private function getResizedPreview($stream, int $maxX, int $maxY, string $mimeType): Imagick {
 		$content = \stream_get_contents($stream);
 
 		if ($this->isDangerousToDecode($content)) {
@@ -94,17 +96,52 @@ private function getResizedPreview($stream, int $maxX, int $maxY): Imagick {
 
 		$bp = ImagickFactory::create();
 
-		# setIteratorIndex(0) will make previews to be generated from the first page
+		# 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->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<timestamp>
+	 * 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

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

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

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

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

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

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

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

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

lib/private/Preview/SVG.php

@@ -58,8 +58,21 @@ public function getThumbnail(File $file, $maxX, $maxY, $scalingUp) {
 				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;

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

tests/lib/Preview/BitmapTest.php

@@ -30,6 +30,12 @@
  */
 class BitmapTest extends Provider {
 	public function setUp(): void {
+		# Postscript::getImagickFormat() pins EPS, so on a build without that coder this
+		# provider cannot decode the fixture at all. Unguarded, that is a failure rather
+		# than a skip - previously ImageMagick's own sniffing hid the dependency.
+		if (\count(\Imagick::queryFormats('EPS')) === 0) {
+			$this->markTestSkipped('This ImageMagick build registers no EPS coder');
+		}
 		parent::setUp();
 
 		$fileName = 'testimage.eps';