Skip to content

fix: replace Imagick coder pinning with a magic-byte pre-check (OC10-164) - #41833

Closed
oc-tmueller wants to merge 3 commits into
fix/oc10-164-pin-imagick-coderfrom
fix/oc10-164-magic-byte-gate
Closed

oc-tmueller wants to merge 3 commits into
fix/oc10-164-pin-imagick-coderfrom
fix/oc10-164-magic-byte-gate

Conversation

@oc-tmueller

Copy link
Copy Markdown
Contributor

Summary

Follow-up to #41832 (this PR's base branch - stacked, since it replaces that PR's mechanism). Addresses a performance concern raised on #41832: writing every preview candidate's full content to a local temp file before decoding, on every preview generation, is unacceptable I/O overhead, and there's no guarantee the temp directory is tmpfs/RAM-backed in this codebase.

What changes

Replaces Imagick-level coder pinning with a fast, zero-I/O, pure-PHP magic-byte check per provider, run against the raw content before Imagick is ever invoked:

  • If the leading bytes don't match what a provider's format requires, throw before Imagick sees the content - same as the existing isDangerousToDecode() throw path, which is unchanged.
  • If they do match, decode with the original, simple, unpinned readImageBlob() - no temp file, no setFormat(), no coder pin of any kind.

This works because Imagick's own auto-sniffing keys off the exact same leading bytes the new check verifies: passing the check guarantees Imagick will independently reach the same conclusion, so there's no way to satisfy it with bytes that then decode via a different coder. It also reverts the decode call to code already proven correct pre-#41832, rather than working around the rasterization bug that mechanism had (setImageFormat('png') silently having no effect after a pinned blob read).

getImagickFormat(string $mimeType): string becomes hasExpectedMagicBytes(string $content): bool on every Bitmap subclass - no mime-type parameter needed anymore, since decoding is unpinned again and the check only needs "do these bytes plausibly belong to any format this provider covers", not "which single coder do I force". Font and Heic no longer branch on mime type at all.

SVG.php reverts fully to its pre-#41832 simple readImageBlob() call, no byte-check added: the content it decodes is DOMSanitizer's serialized DOM output, not raw uploaded bytes - a DOM serializer cannot emit PostScript/PDF/binary bytes as document-leading output, so the risk this whole change addresses cannot occur on that path. Office.php is untouched (its 'PDF:' prefix adds no new disk I/O).

Verification note

Ran the full suite in a fresh container this time (not a reused one) - an earlier container in this investigation had a manually-relaxed ImageMagick policy left over from prior testing, which silently masked an unrelated, pre-existing SVGTest/MVG-policy interaction. Confirmed that failure reproduces identically against the original pre-#41827 SVG.php with a bare Imagick object, so it predates all of OC10-164's work and isn't something this PR introduces or needs to fix.

Test plan

  • make test-php-style
  • make test-php-unit TEST_PHP_SUITE=tests/lib/Preview/ in a fresh owncloudci/php:8.3 container with a production-representative ImageMagick policy - 105 tests, 257 assertions, 0 failures, 9 skips (1 known CI-environment HEIF-coder gap from fix: pin the Imagick coder per provider instead of letting it sniff (OC10-164) #41832, rest unrelated missing Movie/Office providers)
  • Renamed CoderPinningTestMagicByteGateTest: same legitimate-content and foreign-content-rejection cases, plus new cross-format cases (a genuine fixture of one format fed to a provider for a different one - proves the gate itself stops decoding, not Imagick's leniency on garbage bytes) and direct ReflectionMethod boundary tests against hasExpectedMagicBytes() for every accepted/rejected signature, independent of any Imagick delegate being installed
  • grep -n "getTempManager\|getTemporaryFile" lib/private/Preview/Bitmap.php lib/private/Preview/SVG.php → no matches (confirms the I/O is actually gone)

Not yet verified (flagging, not blocking)

  • Magic-byte patterns are not cross-checked against ImageMagick's own magic.c table directly (no local ImageMagick source tree, no network access for this check). Any mismatch can only cause a false rejection (a legitimate variant loses its preview), never a false acceptance, since decoding stays unpinned and keys off the same bytes.
  • No .pfb fixture exists anywhere in this repo - Font's PFB check is implemented from the documented Adobe Type 1 Font Format spec only, never validated against a real binary sample.

…164)

#41832's path-based coder pin (write to a temp file, read via a "FORMAT:path"
prefix) writes every preview candidate's full content to local disk before
decoding, on every preview generation - unacceptable I/O overhead. There's
no guarantee the temp directory is tmpfs/RAM-backed (TempManager falls back
through system config, ini, env vars, sys_get_temp_dir(), none of which
forces tmpfs), and no in-memory-only Imagick pattern exists anywhere in this
codebase to avoid it another way.

Replace coder pinning with a fast, zero-I/O, pure-PHP magic-byte check per
provider, run against the raw content before Imagick is ever invoked. If the
leading bytes don't match what this provider's format requires, throw
before Imagick sees the content - same as the existing isDangerousToDecode()
throw path, which is unchanged. If they do match, decode with the original,
simple, unpinned readImageBlob() - no temp file, no setFormat(), no coder
pin of any kind. This works because Imagick's own auto-sniffing keys off the
exact same leading bytes the new check verifies: passing the check
guarantees Imagick will independently reach the same conclusion, so there is
no way to satisfy it with bytes that then decode via a different coder.
This also reverts the decode call to code already proven correct
pre-#41832, rather than working around the rasterization bug that mechanism
had.

getImagickFormat(string $mimeType): string becomes
hasExpectedMagicBytes(string $content): bool on every Bitmap subclass - no
mime-type parameter, since decoding is unpinned again and the check only
needs to answer "do these bytes plausibly belong to any format this
provider covers", not pick one specific coder to force. Font and Heic no
longer need to branch on mime type at all (that branching existed only to
choose which single coder to pin for PFB-vs-TTF or HEIC-vs-HEIF).

SVG.php reverts fully to its pre-#41832 simple readImageBlob() call, with no
byte-check added: the content it decodes is DOMSanitizer's serialized DOM
output, not raw uploaded bytes - sanitizeSVGContent() already returns null
(→ false, Imagick untouched) for anything that didn't parse as well-formed
XML/SVG. A DOM serializer cannot emit PostScript/PDF/binary bytes as
document-leading output, so the cross-coder-confusion risk this whole
change addresses cannot occur on that path. Office.php is untouched - its
'PDF:' prefix adds no new disk I/O, it only prefixes a path LibreOffice
itself already wrote, and is exactly the "FORMAT:path" pattern already
confirmed to decode correctly.

Rename CoderPinningTest -> MagicByteGateTest (the mechanism under test is
no longer coder pinning). Folds the separate Font "false-or-safe-blank-image"
leniency test into the main foreign-content rejection set, since Font now
also rejects deterministically before Imagick/FreeType ever runs. Adds
stronger cross-format negative cases (a genuine fixture of one format fed to
a provider for a different one - proves the gate itself, not Imagick's own
leniency on garbage bytes, is what stops decoding) and direct
ReflectionMethod boundary tests against hasExpectedMagicBytes() covering
every accepted/rejected signature per provider, independent of any Imagick
delegate being installed.

Verified in a fresh owncloudci/php:8.3 container (not a reused one - a
stale relaxed ImageMagick policy in an earlier container silently masked an
unrelated, pre-existing SVGTest/MVG-policy interaction during earlier
testing in this investigation): full tests/lib/Preview/ suite green (105
tests, 257 assertions, 9 skips - one CI-environment HEIF-coder gap already
known from #41832, the rest unrelated missing Movie/Office providers).
Confirmed the one SVGTest failure that does appear under this container's
stricter-than-production default ImageMagick policy (denies MVG, which some
SVG rendering paths need internally) is unrelated to this change - it
reproduces identically against the pre-#41827 original SVG.php code with a
bare, unhardened Imagick object, so it predates all of OC10-164's work.

Not yet verified: the magic-byte patterns against ImageMagick's own
magic.c table directly (no local ImageMagick source tree, no network
access) - any mismatch there can only cause a false rejection (a legitimate
variant Imagick would have decoded fine loses its preview), never a false
acceptance, since decoding stays unpinned and keys off the same bytes. No
.pfb fixture exists anywhere in this repo; Font's PFB check is implemented
from the documented Adobe Type 1 Font Format spec only.

Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com>
@oc-tmueller
oc-tmueller requested a review from a team as a code owner September 14, 2026 10:40
@update-docs

update-docs Bot commented Sep 14, 2026

Copy link
Copy Markdown

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.

Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com>
…e (OC10-164)

#41833's patterns were derived from documented format specs and my own
fixture testing, disclosed there as "not yet cross-checked against
ImageMagick's own magic.c table". Fetched the exact upstream source (Ubuntu
22.04's imagemagick source package, 6.9.11.60+dfsg, via apt-get source) and
diffed every pattern against magick/magic.c's compiled-in MagicMap[] - the
actual table SetImageInfo() consults for unpinned content-sniffing. Six
real discrepancies found, all fixed:

- Postscript/Illustrator only accepted plain "%!". ImageMagick's "PS" entry
  also matches "\004%!" (DOS EPS ASCII) and "\305\320\323\306" (DOS EPS
  binary preamble) - real, still-encountered Windows-generated EPS
  variants. Added both via a new shared hasPostScriptSignature() helper.

- Font's TTF check used the 4-byte conceptual sfnt version tag; the real
  entry is 5 bytes (the high byte of numTables, which is 0 for any font
  under 256 tables - true of every real font, but the previous 4-byte
  check was one byte short of what ImageMagick itself requires).

- Font accepted "OTTO"/"true"/"ttcf" - none of the three have an entry in
  ImageMagick's magic table at all, matching what this investigation
  already found empirically (no decode delegate for genuine OTF content,
  pinned or not). Removed as dead weight that matched nothing real.

- Font's PFB check was wrong: it validated the generic 0x80+segment-byte
  framing every PFB file has, not what ImageMagick's own "PFB" entry
  actually requires - "%!PS-AdobeFont-1.0" at offset 6, i.e. that a valid
  Adobe Type 1 font program specifically follows the framing. Fixed to the
  real pattern.

- Heic's brand allow-list had 9 entries; only 4 ("heic", "heix", "avif",
  "mif1") are registered in ImageMagick's table under the "HEIC" coder
  name. Notably "avif" was missing - previously excluded on the assumption
  AVIF is a distinct format ImageMagick sniffs separately, which is wrong:
  ImageMagick's own table classifies avif-branded content as "HEIC".
  Fixed the list to match exactly; dropped the five entries that don't
  correspond to any real ImageMagick pattern.

- TIFF didn't accept BigTIFF ("TIFF64" in ImageMagick's table, distinct
  4/8-byte patterns from classic TIFF). BigTIFF decoded fine through the
  original, unpinned readImageBlob() this whole change is built on top of,
  so omitting it would have been a real functional regression for that
  format, not just an unverified edge case. Added.

None of these were security gaps - every one was either a false rejection
(a real format's own provider would have rejected valid content) or dead,
unreachable code (accepting a signature ImageMagick itself never sniffs,
which just fails safely downstream with "no decode delegate"). Confirmed
directly: MVG/MSL/MSVG have no entry in ImageMagick's magic table at all,
so unpinned content-sniffing can never route into them regardless of what
any provider's check accepts - the cross-coder-confusion class this whole
line of work closes was never at risk from any of these six findings.

Verified in a fresh owncloudci/php:8.3 container: full tests/lib/Preview/
suite green (109 tests, 263 assertions, 0 failures, 9 unrelated skips).

Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com>
@oc-tmueller

Copy link
Copy Markdown
Contributor Author

Verified the magic-byte patterns against ImageMagick's real magic table

Follow-up to my own "not yet verified" disclosure above. Fetched the exact upstream source Ubuntu ships (apt-get source imagemagick-6.q16, 6.9.11.60+dfsg) and diffed every pattern in this PR against magick/magic.c's compiled-in MagicMap[] - the actual table SetImageInfo() consults for unpinned content-sniffing (confirmed via magic.c, not the documentation-stub config/magic.xml, which is all commented-out examples).

Six real discrepancies found and fixed in the latest commit, all false-rejection/completeness gaps, none security-relevant - confirmed MVG/MSL/MSVG have no entry in this table at all, so unpinned sniffing can never route into them regardless of what any provider's check accepts:

  • Postscript/Illustrator only accepted plain "%!". ImageMagick's real PS entry also matches "\004%!" (DOS EPS ASCII) and "\305\320\323\306" (DOS EPS binary preamble) - real Windows-generated EPS variants. Added both.
  • Font's TTF check was 4 bytes; the real entry is 5 (\x00\x01\x00\x00\x00).
  • Font accepted OTTO/true/ttcf - none have an entry in ImageMagick's table at all, matching what I'd already found empirically (no decode delegate for genuine OTF content). Removed as dead weight.
  • Font's PFB check was outright wrong: I'd validated the generic PFB segment framing every .pfb file has, not what ImageMagick's real PFB entry requires - "%!PS-AdobeFont-1.0" at offset 6. Fixed to the real pattern.
  • Heic's brand list had 9 entries, only 4 (heic/heix/avif/mif1) are real. I'd explicitly excluded avif on the assumption it's sniffed separately - wrong, ImageMagick classifies it under HEIC.
  • TIFF didn't accept BigTIFF (TIFF64 in ImageMagick's table) - a real functional regression versus the original unpinned code, now fixed.

Re-verified in a fresh container: full tests/lib/Preview/ suite green (109 tests, 263 assertions, 0 failures).

@oc-tmueller

Copy link
Copy Markdown
Contributor Author

Superseding this with an updated #41832: rather than replacing coder-pinning with the magic-byte pre-check here, #41832 now sources its pinning temp file from a new RAM-backed (tmpfs) TempManager method instead of disk, which resolves the performance objection that motivated this PR without giving up the exact-format pinning Florian's original spec called for.

Closing in favor of #41832.

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.

1 participant