diff --git a/changelog/unreleased/41828 b/changelog/unreleased/41828 new file mode 100644 index 000000000000..b6aedcdf6596 --- /dev/null +++ b/changelog/unreleased/41828 @@ -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/41828 diff --git a/lib/private/Preview/Bitmap.php b/lib/private/Preview/Bitmap.php index e004f93c2cd1..7da2584fa59d 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,23 @@ 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])); + + // 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/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()]; } }