fix: prevent arbitrary file write via unsanitized SVG/MVG bitmap previews (OC10-164)

changelog/unreleased/41827

@@ -0,0 +1,24 @@
+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.
+
+Media type detection from file content now always reports a media type. It
+previously passed an unusable value on to its caller when the magic database
+behind it could not be loaded, which made a bitmap preview fail the request
+with a server error rather than fall back to a media type icon, and left the
+new check above with nothing to test the content against.
+
+Previews that read a file also no longer pass it to ImageMagick before the
+hardened Imagick options are applied.
+
+https://github.com/owncloud/core/pull/41827

changelog/unreleased/41834

@@ -0,0 +1,46 @@
+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.
+
+One route is deliberately left open, and is worth stating so the expectation is
+set: which provider handles a preview can be steered by the request, so asking for
+a file to be previewed as a PDF hands that file's bytes to the PDF coder whatever
+they are. This is not a change - content sniffing reached the same coder before -
+and the PDF, PostScript and EPS coders are the ones a distribution's ImageMagick
+policy denies by default. Deployments that enable those coders should keep that
+policy as the control, because it applies process-wide rather than per provider.
+
+https://github.com/owncloud/core/pull/41834

lib/private/Files/Type/Detection.php

@@ -237,19 +237,30 @@ public function detect($path) {
 			// use mime magic extension if available
 			$mimeType = \mime_content_type($path);
 		}
-		if (!$isWrapped and $mimeType === 'application/octet-stream' && \OC_Helper::canExecute("file")) {
+		// canExecute() only tells us the binary is there, not that we may start it:
+		// popen() is a common entry in disable_functions, and on PHP 8 a disabled function
+		// is undefined, so calling it raises an \Error. popen() is also documented to
+		// return false if the process cannot be forked, which makes fgets() and pclose()
+		// raise a TypeError. All three are \Error rather than \Exception, so they escape
+		// callers that guard a failed detection - OC\Preview\Bitmap::getThumbnail() among
+		// them, where the result is a failed request instead of a media type icon.
+		if (!$isWrapped and $mimeType === 'application/octet-stream' && \OC_Helper::canExecute("file")
+			&& \function_exists('popen')
+		) {
 			// it looks like we have a 'file' command,
 			// lets see if it does have mime support
 			$path = \escapeshellarg($path);
 			$fp = \popen("file -b --mime-type $path 2>/dev/null", "r");
-			$reply = \fgets($fp);
-			\pclose($fp);
+			if (\is_resource($fp)) {
+				$reply = \fgets($fp);
+				\pclose($fp);
 
-			//trim the newline
-			$mimeType = \trim($reply);
+				//trim the newline
+				$mimeType = \trim((string)$reply);
 
-			if (empty($mimeType)) {
-				$mimeType = 'application/octet-stream';
+				if (empty($mimeType)) {
+					$mimeType = 'application/octet-stream';
+				}
 			}
 		}
 		return $mimeType;
@@ -262,12 +273,42 @@ public function detect($path) {
 	 * @return string
 	 */
 	public function detectString($data) {
-		if (\function_exists('finfo_open') and \function_exists('finfo_file')) {
-			$finfo = \finfo_open(FILEINFO_MIME);
-			return \finfo_buffer($finfo, $data);
+		if (\function_exists('finfo_open') and \function_exists('finfo_buffer')) {
+			// suppressed like finfo_file() below: libmagic warns when it cannot load its
+			// magic database - reachable through the MAGIC environment variable, or a
+			// broken install - and then returns false. Handing that false to
+			// finfo_buffer() is a TypeError, an \Error rather than an \Exception, so it
+			// escapes callers that guard against a failed detection: it would turn a
+			// missing bitmap preview into a 500 in OC\Preview\Bitmap::getThumbnail().
+			$finfo = @\finfo_open(FILEINFO_MIME);
+			if ($finfo === false) {
+				return 'application/octet-stream';
+			}
+
+			// finfo_buffer() is typed string|false. An unusable return has to become the
+			// fallback rather than reach a caller, because this method is documented as
+			// returning a string and OC\Preview\Bitmap compares the result against a
+			// deny-list of media types it refuses to decode - '' matches no entry there
+			// and would admit the very content the list exists to reject.
+			$mimeType = \finfo_buffer($finfo, $data);
+			return \is_string($mimeType) && $mimeType !== '' ? $mimeType : 'application/octet-stream';
 		} else {
+			// This branch is only reached without ext-fileinfo, but it is now reached from
+			// a preview request, so the same reasoning as above applies: getTemporaryFile()
+			// returns false when its directory is not writable, and fopen(false, ...) is a
+			// ValueError on PHP 8 - an \Error, so it would escape the catch (\Exception) in
+			// OC\Preview\Bitmap::getThumbnail() and fail the request instead of falling
+			// back to a media type icon.
 			$tmpFile = \OC::$server->getTempManager()->getTemporaryFile();
+			if ($tmpFile === false) {
+				return 'application/octet-stream';
+			}
+
 			$fh = \fopen($tmpFile, 'wb');
+			if ($fh === false) {
+				return 'application/octet-stream';
+			}
+
 			\fwrite($fh, $data, 8024);
 			\fclose($fh);
 			$mime = $this->detect($tmpFile);

lib/private/Image/ImagickFactory.php

@@ -7,18 +7,32 @@
 
 class ImagickFactory {
 	/**
-	 * @param mixed $files <p>
-	 * The path to an image to load or an array of paths. Paths can include
-	 * wildcards for file names, or can be URLs.
-	 * </p>
+	 * Returns an Imagick instance with the hardened SVG options already applied.
+	 *
+	 * @param string|null $file path of an image to read, optionally prefixed with an
+	 *   explicit "FORMAT:" to pin the input coder. Left null, nothing is read and the
+	 *   caller decodes itself.
 	 * @return Imagick
 	 * @throws ImagickException
 	 */
-	public static function create($files = null): Imagick {
-		$imagick = new Imagick($files);
+	public static function create(?string $file = null): Imagick {
+		// Deliberately not "new Imagick($file)": the constructor reads immediately, so the
+		// options below would only be applied to an already decoded image. That is exactly
+		// what the path-taking form used to do, leaving it the one unhardened way in.
+		$imagick = new Imagick();
 		$imagick->setOption('svg:sanitize', 'true');
 		$imagick->setOption('svg:embed', 'false');
 		$imagick->setOption('svg:decode', 'true');
+
+		if ($file !== null) {
+			// A "FORMAT:path" argument pins the input coder here just as it does in the
+			// constructor, and - unlike setFormat() - it does not pin the wand's output
+			// format as well, so callers still only need setImageFormat(). Verified
+			// against Office's "PDF:<path>[0]": identical geometry and bytes to the
+			// constructor form, and a foreign image pinned to PDF is still rejected.
+			$imagick->readImage($file);
+		}
+
 		return $imagick;
 	}
 

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;
@@ -57,7 +58,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;
@@ -92,31 +99,94 @@ 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<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
+	 * text/vector coders can be abused to read and write arbitrary files.
+	 *
+	 * Known limitation, pre-dating this check and deliberately not closed here: the
+	 * deny-list can only be as good as the detection behind it. Detection::detectString()
+	 * needs either ext-fileinfo or the "file" binary, and ext-fileinfo is not actually
+	 * required to run ownCloud - OC_Util::checkServer() does not list it, and
+	 * OC_Util::fileInfoLoaded() only raises an admin-panel recommendation. On an install
+	 * with neither, every payload is reported as application/octet-stream and this method
+	 * returns false for all of them, leaving the per-provider coder pin in
+	 * getResizedPreview() as the only remaining layer. Failing closed instead would cost
+	 * every bitmap preview on a configuration ownCloud supports, which is why it is
+	 * recorded here rather than rejected.
+	 */
+	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
 	 *

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