From 8bb03b3c94c4f96c5dc884f13e53ebf9080f1ea9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <323649642+oc-tmueller@users.noreply.github.com> Date: Fri, 11 Sep 2026 11:40:07 +0200 Subject: [PATCH 1/4] fix: prevent arbitrary file write via unsanitized SVG/MVG bitmap previews (OC10-164) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bitmap::getResizedPreview() sanitized SVG content before handing it to Imagick::readImageBlob(), but fell back to the ORIGINAL, unsanitized bytes whenever the sanitizer returned an empty string - which it does for any content libxml cannot parse, not just for genuinely malformed SVG. A malformed SVG (or any non-XML payload such as a raw MVG script) therefore reached ImageMagick unsanitized, where an or an MVG "fill 'url(...)'" primitive can execute an MSL script that reads and writes arbitrary files as the web user (CVSS 8.8). Bitmap providers (PDF, Font, Postscript, ...) only ever need to decode real bitmap/vector image formats, never SVG or script-shaped text content - that belongs exclusively to the dedicated SVG provider. getResizedPreview() now rejects any content whose libmagic-detected media type is text/*, image/svg+xml, application/xml, or image/x-mvg before ever calling into Imagick, instead of trying to sanitize and falling back on failure. It also now goes through ImagickFactory::create() so the svg:sanitize/svg:embed/ svg:decode hardening options apply here as they already did in the SVG provider. SVG::sanitizeSVGContent() return type changes from string to ?string so it can report "could not sanitize" (null) separately from "sanitized to an empty document" (''); its own provider now bails out on null instead of silently passing empty content to Imagick. The removal of the sanitize-with-fallback path in Bitmap changes the behaviour asserted by SanitizeTest: SVG content fed to a Bitmap provider (PDF, Font) now yields false instead of a rendered PNG, since Bitmap providers no longer attempt to handle SVG-shaped content at all. Added regression cases for a malformed SVG with an MSL xlink:href, a raw MVG script, and a well-formed SVG - all must return false from a Bitmap provider. Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> --- lib/private/Preview/Bitmap.php | 29 +++++++++++++++++++------- lib/private/Preview/SVG.php | 9 +++++++- tests/lib/Preview/SanitizeTest.php | 33 ++++++++++++++++++++++++++---- 3 files changed, 59 insertions(+), 12 deletions(-) diff --git a/lib/private/Preview/Bitmap.php b/lib/private/Preview/Bitmap.php index e004f93c2cd1..c62392796fa5 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; @@ -85,18 +86,16 @@ public function isAvailable(FileInfo $file) { * @return Imagick */ private function getResizedPreview($stream, int $maxX, int $maxY): Imagick { - # file content can be SVG - we need to sanitize it first $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(); # setIteratorIndex(0) will make previews to be generated from the first page - $bp->readImageBlob($output); + $bp->readImageBlob($content); $bp->setIteratorIndex(0); $bp = $this->resize($bp, $maxX, $maxY); @@ -106,6 +105,22 @@ private function getResizedPreview($stream, int $maxX, int $maxY): Imagick { return $bp; } + /** + * 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])); + + if (\strpos($mimeType, 'text/') === 0) { + return true; + } + + return \in_array($mimeType, ['image/svg+xml', 'application/xml', 'image/x-mvg'], true); + } + /** * Returns a resized \Imagick object * diff --git a/lib/private/Preview/SVG.php b/lib/private/Preview/SVG.php index d7d385809366..1863d2bf1c46 100644 --- a/lib/private/Preview/SVG.php +++ b/lib/private/Preview/SVG.php @@ -54,6 +54,9 @@ public function getThumbnail(File $file, $maxX, $maxY, $scalingUp) { # sanitize SVG content $output = self::sanitizeSVGContent($content); + if ($output === null) { + return false; + } $imagick->readImageBlob($output); $imagick->setImageFormat('png32'); @@ -81,7 +84,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 +93,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/tests/lib/Preview/SanitizeTest.php b/tests/lib/Preview/SanitizeTest.php index bcac9efb3500..882a1230866c 100644 --- a/tests/lib/Preview/SanitizeTest.php +++ b/tests/lib/Preview/SanitizeTest.php @@ -44,10 +44,10 @@ public function test(string $svgContent, Bitmap $provider): void { $file->method('getContent')->willReturn($svgContent); $file->method('fopen')->willReturn($stream); - # 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); } public function providesSVG(): Generator { @@ -56,10 +56,35 @@ 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; # 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()]; + yield 'Font Provider - image tag' => [$svgContent0, new Font()]; + yield 'PDF provider - malformed SVG with MSL href' => [$malformedSvgWithMslHref, new PDF()]; + yield 'Font Provider - malformed SVG with MSL href' => [$malformedSvgWithMslHref, new Font()]; + yield 'PDF provider - raw MVG' => [$rawMvg, new PDF()]; + yield 'Font Provider - raw MVG' => [$rawMvg, new Font()]; + yield 'PDF provider - well-formed SVG' => [$wellFormedSvg, new PDF()]; + yield 'Font Provider - well-formed SVG' => [$wellFormedSvg, new Font()]; } } From 28e597d51a707d155f93e519ff49b3fe90e66a24 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <323649642+oc-tmueller@users.noreply.github.com> Date: Fri, 11 Sep 2026 11:41:56 +0200 Subject: [PATCH 2/4] docs: add changelog entry for OC10-164 bitmap preview fix (#41827) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> --- changelog/unreleased/41827 | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 changelog/unreleased/41827 diff --git a/changelog/unreleased/41827 b/changelog/unreleased/41827 new file mode 100644 index 000000000000..ed593a0087d3 --- /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 From 7ea13d89134eed6e78edd86dbc6da596c0aeb9f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <323649642+oc-tmueller@users.noreply.github.com> Date: Fri, 11 Sep 2026 12:01:31 +0200 Subject: [PATCH 3/4] fix: match "image/svg" without the +xml suffix in the OC10-164 mime gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Confirmed while testing this backport: PHP 7.4's bundled fileinfo extension reports the same SVG content as "image/svg", not "image/svg+xml" - the exact-match check silently let it through on this runtime while still catching it on PHP 8.3. Match by prefix instead so the gate is not dependent on which libmagic build a given PHP runtime happens to link. Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> --- lib/private/Preview/Bitmap.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/private/Preview/Bitmap.php b/lib/private/Preview/Bitmap.php index c62392796fa5..7da2584fa59d 100644 --- a/lib/private/Preview/Bitmap.php +++ b/lib/private/Preview/Bitmap.php @@ -114,11 +114,12 @@ private function isDangerousToDecode(string $content): bool { $mimeType = \OC::$server->getMimeTypeDetector()->detectString($content); $mimeType = \strtolower(\trim(\explode(';', $mimeType, 2)[0])); - if (\strpos($mimeType, 'text/') === 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, ['image/svg+xml', 'application/xml', 'image/x-mvg'], true); + return \in_array($mimeType, ['application/xml', 'image/x-mvg'], true); } /** From 05e94c3d80f589b509e3835fc11267c735e5afaa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <323649642+oc-tmueller@users.noreply.github.com> Date: Fri, 11 Sep 2026 12:02:32 +0200 Subject: [PATCH 4/4] docs: point the changelog entry at the 10.16 PR (#41828) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> --- changelog/unreleased/{41827 => 41828} | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename changelog/unreleased/{41827 => 41828} (94%) diff --git a/changelog/unreleased/41827 b/changelog/unreleased/41828 similarity index 94% rename from changelog/unreleased/41827 rename to changelog/unreleased/41828 index ed593a0087d3..b6aedcdf6596 100644 --- a/changelog/unreleased/41827 +++ b/changelog/unreleased/41828 @@ -12,4 +12,4 @@ 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 +https://github.com/owncloud/core/pull/41828