Skip to content

fix: prevent arbitrary file write via unsanitized SVG/MVG bitmap previews [10.16] (OC10-164) - #41828

Draft
kw-tmueller wants to merge 4 commits into
10.16from
fix/oc10-164-bitmap-preview-arbitrary-file-write-10.16
Draft

kw-tmueller wants to merge 4 commits into
10.16from
fix/oc10-164-bitmap-preview-arbitrary-file-write-10.16

Conversation

@kw-tmueller

Copy link
Copy Markdown
Contributor

Summary

Backport of #41827 to the 10.16 maintenance line.

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 (with or without the +xml suffix - see note below), 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. 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.

Two notes specific to what surfaced while verifying this backport on PHP 7.4:

  • image/x-mvg is in the deny-list because libmagic classifies a raw MVG script as image/x-mvg, not as any text/*/xml type.
  • The SVG check matches by prefix (image/svg), not exact equality against image/svg+xml: PHP 7.4's bundled fileinfo build reports the identical SVG content as image/svg (no +xml), while PHP 8.3 reports image/svg+xml. An exact match would have silently passed SVG content through unblocked on this branch's PHP 7.4 target while still blocking it on master's PHP 8.3 - confirmed by running the regression suite in both a owncloudci/php:7.4 and owncloudci/php:8.3 container before settling on the prefix check.

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/ on owncloudci/php:7.4 - 44 tests, 8 skipped (unrelated missing Movie/Office providers), 0 failures
  • Confirmed RED before the fix, GREEN after, on this branch's own PHP 7.4 target (not just carried over from the master PR)
  • tests/lib/Preview/PDFTest.php (testimage.pdf) and tests/lib/Preview/BitmapTest.php (testimage.eps) still produce previews

…iews (OC10-164)

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 <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 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>
Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com>
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>
Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants