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

[oc-tmueller]

Summary

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. SVG::sanitizeSVGContent() returns '' for any content libxml cannot parse, not just genuinely malformed SVG - so a malformed SVG (or any non-XML payload such as a raw MVG script) reached ImageMagick unsanitized. There, an <image xlink:href="MSL:..."> 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::getResizedPreview() no longer sanitizes-then-falls-back. It 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 (isDangerousToDecode()), and goes through ImagickFactory::create() so the svg:sanitize/svg:embed/svg:decode hardening options apply here too.
• SVG::sanitizeSVGContent()'s return type changes from string to ?string, returning null when the underlying sanitizer does not return a string, so callers can distinguish "could not sanitize" from "sanitized to an empty document". The SVG provider's own call site now bails out on null instead of silently passing empty content to Imagick.
• SanitizeTest is updated: SVG content fed to a Bitmap provider (PDF, Font) now must return false instead of a rendered PNG, since Bitmap providers no longer attempt to handle SVG-shaped content at all - that's the dedicated SVG provider's job. 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.

Note: image/x-mvg is included in the deny-list because libmagic (file-5.41, standard Ubuntu magic database) classifies a raw MVG script as image/x-mvg, not as any text/*/xml type - without it, the MVG regression case would not be blocked.

Not in scope for this PR (flagging separately): Bitmap::getThumbnail() leaks $stream when getResizedPreview() throws - it returns at line 54 before the fclose() at line 57. That's a resource leak, not a security defect, and deserves its own focused PR.

Test plan

• make test-php-style
• make test-php-unit TEST_PHP_SUITE=tests/lib/Preview/ - 58 tests, 8 skipped (unrelated missing Movie/Office providers), 0 failures
• Confirmed RED before the fix: 6/8 SanitizeTest cases failed against unfixed code (the well-formed/sanitizable/malformed SVG cases all reached Imagick and rendered); confirmed GREEN after
• tests/lib/Preview/PDFTest.php (testimage.pdf) and tests/lib/Preview/BitmapTest.php (testimage.eps) still produce previews

[update-docs]

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

[kw-fscheuer]

Follow-up suggestion rather than a change request on this PR — the fail-open fix here looks right to
me, and this is about removing the bug class rather than the instance.

What this approach leaves open by design. isDangerousToDecode() is a deny-list over the
sniffed type, and the decode that follows re-derives the format independently. Two consequences:

1. The provider is selected by extension; the coder is selected by content. Detection.php maps
   an extension to a MIME type, which picks the provider — but readImageBlob() passes no format, so
   ImageMagick consults its own magic table (magick/magic.c, ~130 entries) and selects whatever the
   bytes look like. The set of coders reachable from any Bitmap provider is therefore the whole
   sniffable set, not the single MIME type the provider was registered for. Some of those coders
   shell out to external binaries through delegates.xml.
2. Two different sniffing engines make the decision and take the action. The security check uses
   libmagic (getMimeTypeDetector()->detectString()); the decode uses ImageMagick's own table. Where
   they disagree, the check passes on libmagic's answer and the decode happens on ImageMagick's —
   the same shape as the libxml-vs-ImageMagick differential that made the original fail-open
   reachable in the first place.

There is a concrete residual path in this class. I've put the specifics on OC10-164 rather than here.

Suggested follow-up: pin the coder instead of letting ImageMagick guess. Each provider already
knows its format — SGI is registered for image/sgi and nothing else — so state it:

$bp->readImageBlob($content, 'SGI:blob');

That is a hard pin, not a hint. BlobToImage() sniffs only when the format is unset
(magick/blob.c:359-360), and an explicit FORMAT: prefix sets affirm in SetImageInfo()
(magick/image.c:2945), which returns before the "determine the image format from the first few
bytes" block at :2976. Verified against tag 6.9.12-98. No new Imagick API and no version floor.

Scope is small. There are three Imagick read paths in the whole repo —
Bitmap::getResizedPreview(), SVG::getThumbnail() and Office::getThumbnail() (which already
reads a file it produced itself) — plus one format declaration per Bitmap subclass. Roughly 15-20
lines across 11 files.

Two things worth watching:

• Map MIME type → coder, not provider class → coder. Heic serves both image/heic and
  image/heif, and Font serves application/font-sfnt and application/x-font, so a single
  constant per class forces a wrong guess for one of them. Note that coders/heic.c registers
  HEIC, HEIF and AVIF as three separate coders, so an AVIF-branded file served by the Heic
  provider is the one case I'd want a real test sample for.
• Subimage selection. Office::getThumbnail() passes [0], so a pinned filename becomes
  PDF:/tmp/x.pdf[0]. Frame parsing happens in the same SetImageInfo path, but it deserves a test.

Suggested tests in tests/lib/Preview/: a "pinned format still decodes" case per provider, plus a
negative case asserting that content of one format, under an extension mapping to a different
provider, no longer decodes.

The same change would need to follow on the 10.x line afterwards — no need to touch #41828 until the
v11 approach is agreed here.