fix: replace Imagick coder pinning with a magic-byte pre-check (OC10-164)

changelog/unreleased/41833

@@ -0,0 +1,10 @@
+Security: Replace bitmap preview Imagick coder pinning with a magic-byte check
+
+The previous hardening for bitmap previews pinned the ImageMagick coder for
+each provider by writing the file's content to a temporary file before
+decoding, which added a disk write to every preview generation. Each
+provider now instead verifies its own expected file signature against the
+raw content in memory before ImageMagick is invoked at all, closing the
+same class of cross-provider decoding confusion without the extra I/O.
+
+https://github.com/owncloud/core/pull/41833

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, $file->getMimeType());
+			$bp = $this->getResizedPreview($stream, $maxX, $maxY);
 		} catch (\Exception $e) {
 			Util::writeLog('core', 'ImageMagick says: ' . $e->getmessage(), Util::ERROR);
 			return false;
@@ -82,40 +82,34 @@ 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 can't be redirected by the file's actual content
 	 *
 	 * @return Imagick
 	 */
-	private function getResizedPreview($stream, int $maxX, int $maxY, string $mimeType): Imagick {
+	private function getResizedPreview($stream, int $maxX, int $maxY): Imagick {
 		$content = \stream_get_contents($stream);
 
 		if ($this->isDangerousToDecode($content)) {
 			throw new \RuntimeException('Refusing to decode text-based content for a bitmap preview');
 		}
 
-		$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.
-		#
-		# The pin has to be a "FORMAT:path" read, not setFormat()+readImageBlob(): the
-		# latter does reliably reject a mismatched format, but for several coders here
-		# (PDF, EPS, AI, PSD, SGI, TIFF, HEIC, HEIF - i.e. all of them) it also silently
-		# skips the actual rasterization step, so setImageFormat('png') below ends up
-		# with no effect and getImageBlob() returns the original, undecoded bytes.
-		$tmpPath = \OC::$server->getTempManager()->getTemporaryFile();
-		\file_put_contents($tmpPath, $content);
-		try {
-			$bp->readImage($this->getImagickFormat($mimeType) . ':' . $tmpPath);
-		} finally {
-			\unlink($tmpPath);
+		# Reject content whose own leading bytes don't match this provider's expected
+		# format before Imagick ever sees it, instead of pinning the coder Imagick
+		# itself uses to decode: readImageBlob() with no format set picks its coder
+		# from the same leading bytes via its own ~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. Since this check and Imagick's own sniffing key
+		# off the same bytes, passing it guarantees Imagick will independently reach the
+		# same, safe conclusion - there's no way to satisfy this check with bytes that
+		# then decode via a different coder.
+		if (!$this->hasExpectedMagicBytes($content)) {
+			throw new \RuntimeException("Refusing to decode content whose signature does not match this provider's expected format");
 		}
 
+		$bp = ImagickFactory::create();
+		$bp->readImageBlob($content);
+
 		# setIteratorIndex(0) will make previews to be generated from the first page
 		$bp->setIteratorIndex(0);
 
@@ -144,10 +138,34 @@ private function isDangerousToDecode(string $content): bool {
 	}
 
 	/**
-	 * Maps this provider's own detected mime type(s) to the Imagick coder name that
-	 * must decode them - the format pinned in getResizedPreview() above.
+	 * True if $content's own leading bytes plausibly belong to a format this
+	 * provider is registered to decode - checked before Imagick ever sees the
+	 * content, per getResizedPreview() above.
+	 */
+	abstract protected function hasExpectedMagicBytes(string $content): bool;
+
+	/**
+	 * True if $content has $signature's exact bytes starting at $offset.
+	 */
+	protected function hasSignatureAt(string $content, string $signature, int $offset = 0): bool {
+		return \substr($content, $offset, \strlen($signature)) === $signature;
+	}
+
+	/**
+	 * True if $content is one of the three PostScript variants ImageMagick's own
+	 * magic table (magick/magic.c, MagicMap[], coder name "PS") recognizes: plain
+	 * ASCII PostScript, DOS EPS ASCII (a leading Ctrl-D byte), or DOS EPS binary
+	 * (a 4-byte binary preamble). Shared by Postscript and Illustrator, since
+	 * Illustrator's pre-9, non-PDF-based files use a plain PostScript header too.
 	 */
-	abstract protected function getImagickFormat(string $mimeType): string;
+	protected function hasPostScriptSignature(string $content): bool {
+		foreach (['%!', "\x04%!", "\xC5\xD0\xD3\xC6"] as $signature) {
+			if ($this->hasSignatureAt($content, $signature)) {
+				return true;
+			}
+		}
+		return false;
+	}
 
 	/**
 	 * Returns a resized \Imagick object

lib/private/Preview/Font.php

@@ -30,12 +30,22 @@ public function getMimeType() {
 		return '/application\/(?:font-sfnt|x-font$)/';
 	}
 
-	protected function getImagickFormat(string $mimeType): string {
-		if ($mimeType === 'application/x-font') {
-			return 'PFB';
+	protected function hasExpectedMagicBytes(string $content): bool {
+		# Verified against ImageMagick's own compiled-in magic table
+		# (magick/magic.c, MagicMap[]): the sfnt version-1.0 tag is the ONLY font
+		# signature it recognizes ("TTF", 5 bytes including the high byte of
+		# numTables, which is 0 for any font with fewer than 256 tables - true in
+		# practice for every real font). It has no entry at all for "OTTO"/"true"/
+		# "ttcf" - confirmed empirically too: this environment's Imagick has no
+		# decode delegate for genuine OTF ('OTTO'-tagged) content regardless of
+		# how it's read, pinned or not.
+		if ($this->hasSignatureAt($content, "\x00\x01\x00\x00\x00")) {
+			return true;
 		}
-		# .otf and .ttf are indistinguishable by mime type alone (both application/font-sfnt);
-		# TTF is what actually decodes real font files here, both tagged variants included.
-		return 'TTF';
+		# PFB ("Printer Font Binary"): the entry is "PFB", offset 6,
+		# "%!PS-AdobeFont-1.0" - the first 6 bytes are the PFB binary segment
+		# header (0x80, segment type, 4-byte little-endian length), followed by
+		# the standard Adobe Type 1 font program identification string.
+		return $this->hasSignatureAt($content, '%!PS-AdobeFont-1.0', 6);
 	}
 }

lib/private/Preview/Heic.php

@@ -30,10 +30,16 @@ public function getMimeType() {
 		return '/image\/hei(f|c)/';
 	}
 
-	protected function getImagickFormat(string $mimeType): string {
-		if ($mimeType === 'image/heif') {
-			return 'HEIF';
+	protected function hasExpectedMagicBytes(string $content): bool {
+		// ISO-BMFF: bytes[0:4] are a variable box size (not checked - any value is
+		// structurally valid), bytes[4:8] must be "ftyp", bytes[8:12] are the brand.
+		// Brand list verified against ImageMagick's own compiled-in magic table
+		// (magick/magic.c, MagicMap[]): "avif"/"heic"/"heix"/"mif1" are registered
+		// there under the "HEIC" coder name - no others are.
+		if (!$this->hasSignatureAt($content, 'ftyp', 4)) {
+			return false;
 		}
-		return 'HEIC';
+		$brand = \substr($content, 8, 4);
+		return \in_array($brand, ['avif', 'heic', 'heix', 'mif1'], true);
 	}
 }

lib/private/Preview/Illustrator.php

@@ -31,7 +31,10 @@ public function getMimeType() {
 		return '/application\/illustrator/';
 	}
 
-	protected function getImagickFormat(string $mimeType): string {
-		return 'AI';
+	protected function hasExpectedMagicBytes(string $content): bool {
+		// Modern (9+) Illustrator files are PDF-compatible ("%PDF-"); legacy pre-9
+		// files use a plain PostScript header. Both already reach the same
+		// Ghostscript/PDF delegate as the Postscript and PDF providers.
+		return $this->hasSignatureAt($content, '%PDF-') || $this->hasPostScriptSignature($content);
 	}
 }

lib/private/Preview/PDF.php

@@ -31,7 +31,7 @@ public function getMimeType() {
 		return '/application\/pdf/';
 	}
 
-	protected function getImagickFormat(string $mimeType): string {
-		return 'PDF';
+	protected function hasExpectedMagicBytes(string $content): bool {
+		return $this->hasSignatureAt($content, '%PDF-');
 	}
 }

lib/private/Preview/Photoshop.php

@@ -31,7 +31,7 @@ public function getMimeType() {
 		return '/application\/x-photoshop/';
 	}
 
-	protected function getImagickFormat(string $mimeType): string {
-		return 'PSD';
+	protected function hasExpectedMagicBytes(string $content): bool {
+		return $this->hasSignatureAt($content, '8BPS');
 	}
 }

lib/private/Preview/Postscript.php

@@ -31,7 +31,7 @@ public function getMimeType() {
 		return '/application\/postscript/';
 	}
 
-	protected function getImagickFormat(string $mimeType): string {
-		return 'EPS';
+	protected function hasExpectedMagicBytes(string $content): bool {
+		return $this->hasPostScriptSignature($content);
 	}
 }

lib/private/Preview/SGI.php

@@ -28,7 +28,7 @@ public function getMimeType() {
 		return '/image\/sgi/';
 	}
 
-	protected function getImagickFormat(string $mimeType): string {
-		return 'SGI';
+	protected function hasExpectedMagicBytes(string $content): bool {
+		return $this->hasSignatureAt($content, "\x01\xDA");
 	}
 }

lib/private/Preview/SVG.php

@@ -58,18 +58,13 @@ public function getThumbnail(File $file, $maxX, $maxY, $scalingUp) {
 				return false;
 			}
 
-			# Pin the coder: reading with no format set would let Imagick's own content-
-			# sniffing pick the coder independently of the svg:sanitize/embed/decode
-			# options above and of the isDangerousToDecode()-style reasoning in Bitmap.php.
-			# This has to be a "SVG:path" read, not setFormat()+readImageBlob(): the latter
-			# silently skips the actual rasterization step, same as in Bitmap.php.
-			$tmpPath = \OC::$server->getTempManager()->getTemporaryFile();
-			\file_put_contents($tmpPath, $output);
-			try {
-				$imagick->readImage('SVG:' . $tmpPath);
-			} finally {
-				\unlink($tmpPath);
-			}
+			# $output is DOMSanitizer's serialized DOM output, not raw uploaded bytes -
+			# sanitizeSVGContent() already returned null (handled above) for anything
+			# that didn't parse as well-formed XML/SVG. A DOM serializer cannot emit
+			# PostScript/PDF/binary bytes as document-leading output, so there's no
+			# cross-coder-confusion risk here the way there is for raw file content in
+			# Bitmap.php - no need to pin the coder Imagick decodes this with.
+			$imagick->readImageBlob($output);
 			$imagick->setImageFormat('png32');
 		} catch (\Exception $e) {
 			\OCP\Util::writeLog('core', $e->getmessage(), \OCP\Util::ERROR);

lib/private/Preview/TIFF.php

@@ -31,7 +31,17 @@ public function getMimeType() {
 		return '/image\/tiff/';
 	}
 
-	protected function getImagickFormat(string $mimeType): string {
-		return 'TIFF';
+	protected function hasExpectedMagicBytes(string $content): bool {
+		// Classic TIFF, both byte orders, plus BigTIFF ("TIFF64" in ImageMagick's own
+		// magic table) - verified against magick/magic.c's MagicMap[]. BigTIFF was
+		// reachable via the original, unpinned readImageBlob() call this replaces, so
+		// leaving it out here would be a real functional regression, not just an
+		// unverified edge case.
+		foreach (["II*\0", "MM\0*", "II+\0\x08\x00\x00\x00", "MM\0+\x00\x08\x00\x00"] as $signature) {
+			if ($this->hasSignatureAt($content, $signature)) {
+				return true;
+			}
+		}
+		return false;
 	}
 }