fix: prevent arbitrary file write via unsanitized SVG/MVG bitmap previews [10.16] (OC10-164) by kw-tmueller · Pull Request #41828 · owncloud/core

kw-tmueller · 2026-09-11T10:02:08Z

Backport of #41827 to 10.16.

#41834 (the per-provider Imagick coder pin) was merged into #41827's branch, so master's squash commit carries both and so does this backport. Two master-only preview commits come with it:

master PR	why it is here
#41827 + #41834	the backport itself: media-type gate, `Detection::detectString()` contract fix, `ImagickFactory` hardening, `getImagickFormat()` on all 8 `Bitmap` subclasses, `CoderPinningTest` and its fixtures
#41838	required — without it the suite is red, because `BitmapStreamTest` fed a PNG through the `Photoshop` provider and the `PSD` pin correctly refuses that mismatch
#41855	folded in on request: `!is_resource($stream)` in both providers, plus `fclose()` in a `finally` in the SVG one, and `SVGStreamTest`

This PR previously held the pre-#41834 shape (mime gate only) and was deliberately held until the pinning approach was settled. It has been rebuilt from master's merged commits now that #41827 has landed.

What PHP 7.4 required

10.16 builds on PHP 7.4 only, and master justifies several guards by PHP 8 raising an \Error that escapes catch (\Exception). On 7.4 the same calls only warn, so those claims were re-derived on the target runtime instead of carried across. Measured, not inferred:

call	PHP 7.4	PHP 8
`finfo_buffer(false, …)`	warns, returns `false`	`TypeError`
`stream_get_contents(false)` / `(null)`	warns, returns `false`	`TypeError`
`fclose(null)`	warns, returns `false`	`TypeError`
`fopen(false, 'wb')`	warns, returns `false`	`ValueError`
`null` into a userland `string` parameter	`TypeError`	`TypeError`

Two consequences:

The detectString() fix matters more here, not less. On 7.4 the unguarded finfo_buffer() returned false to Bitmap::isDangerousToDecode(), where it collapses to '' and matches no deny-list entry — so the missing guard admitted the content the list exists to reject. On PHP 8 it is a 500 instead. The comments and changelog say that.
SVGStreamTest's unopenable-file case asserts on warnings, not on the return value. Master's assertFalse() passes with the guard reverted on this runtime, so it could never go red. BitmapStreamTest's equivalent already had the 7.4 form and gains fix: return no preview when a preview file cannot be opened or read #41855's false/null data provider.

Also corrected: master's note that an unstubbed getMimeType() mock yields a TypeError. getThumbnail() casts the value, so it yields '' — worse for a test, since seven of the eight providers answer '' with the same constant they answer anything with.

Verification

All in owncloudci/php:7.4, over tests/lib/Preview/ plus tests/lib/Files/Type/DetectionTest.php:

tree	result
pristine `10.16`	41 tests / 123 assertions / 18 skipped / 0 failures
this branch	72 tests / 219 assertions / 12 skipped / 0 failures

Skips are environment only — Movie (4), Office (4), SVG (4). The four PDF cases run here for the first time: their guard used to probe the SVG coder, which this image does not register, so they had been skipping under a misleading "No PDF provider present".

Red before green, with production code reverted and the tests kept — 9 failures, each for the right reason:

CoderPinningTest::testRejectsPostScriptContentFromAForeignProvider ×4 — PostScript rendered to a PNG through the SGI, Photoshop, TIFF and Heic providers. This is the residual path the pin closes.
CoderPinningTest::testFontNeverInvokesADangerousCoderForForeignContent — the Font provider rendered the PostScript page.
SVGStreamTest ×3 — two unopenable-handle rows warning from stream_get_contents() and fclose(), and the handle still open after a read that throws.
BitmapStreamTest — the null handle row, warning twice.

SanitizeTest cannot discriminate on the CI image, which registers no SVG or MVG coder, so the pre-fix sanitize-and-fallback path also ends in "no preview" there. Proved separately on a build that has them (libmagickcore-6.q16-6-extra installed, policy.xml opened): 6 of 10 cases fail pre-fix — every SVG-shaped payload, through both the PDF and Font providers. On that same build this branch is green at 72 tests / 245 assertions / 8 skipped, with the SVG cases running too.

The coder-pin measurements were re-taken on owncloudci/php:7.4, which ships the same ImageMagick 6.9.11-60 and Ghostscript 9.55.0 as the 8.3 image; every figure reproduced.

Also clean: php -l under 7.4 on all changed files; php-cs-fixer 0 of 2412; phpcs over the CI scope; OCPSinceChecker. phpstan reports 311 pre-existing errors in this local environment where CI reports none — a known artefact of a locally installed lib/composer — and none of them names a file this branch touches.

Known and deliberate

SVG::sanitizeSVGContent()'s ?string/null sentinel is dead here as on master: the pinned rhukster/dom-sanitizer 1.0.17 declares sanitize(): string, and the real sentinel for malformed input is ''. Kept unchanged so the two branches stay identical.
Pinning costs previews for extension-mislabelled files — a JPEG saved as photo.tif now gets a media type icon. The changelog records this; it is the trade-off the pin buys.
Font's PFB branch has no coverage, because no .pfb/.pfa fixture exists in the repository.

🤖 Generated with Claude Code

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

…iews [10.16] Backport of #41827. That PR carries the per-provider Imagick coder pin from #41834 as well, because #41834 was merged into its branch, so the squash commit on master contains both changes and so does this backport. 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 only 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. getResizedPreview() now rejects content whose libmagic-detected media type is text/*, image/svg*, application/xml or image/x-mvg before calling into Imagick at all, and each provider pins the exact coder it serves instead of letting ImageMagick re-derive the format from the content. Adapted for PHP 7.4, the only version 10.16 supports. Master justifies several of these guards by PHP 8 raising an \Error that escapes catch (\Exception); on 7.4 the same calls only warn, so every such claim was re-derived on the target runtime rather than carried over: - finfo_buffer(false, ...) warns and returns false on 7.4, and detectString() returned that false to the new deny-list, where it collapses to '' and matches no entry. Here the missing guard admitted the content the list exists to reject; it is on PHP 8 that it turns a missing preview into a 500. - the same holds for popen()/fgets()/pclose() in detect() and for fopen(false, ...) in the branch taken without ext-fileinfo. - the (string) cast on $file->getMimeType() is required on 7.4 too: passing null to a userland string-typed parameter is a TypeError on 7.4 as well. Measured, because the surrounding guards are not. 10.16 keeps its own "$stream === false" check and $image->loadFromData($bp); the is_resource() form and the (string) cast on that call are master-only, from #41855 and #41449, and the three-way merge preserved both correctly. The measurements the coder-pin comments rest on were re-taken on owncloudci/php:7.4, this branch's own CI image. It ships the same ImageMagick 6.9.11-60 and Ghostscript 9.55.0 as the 8.3 image and every figure reproduced: plain PostScript and EPSF-branded content both render 612x792 unpinned, pinned EPS and pinned PS alike; a %!PS-Adobe payload read unpinned reaches the PS coder at 612x792 while the TTF pin gives 800x480. That image also registers no SVG coder and no HEIF coder distinct from HEIC - which is precisely what PDFTest's old SVG-based guard got wrong and what Heic's single HEIC pin is there for. Verified in owncloudci/php:7.4: tests/lib/Preview/ plus tests/lib/Files/Type/DetectionTest.php at 68 tests / 207 assertions, against 41 tests / 123 assertions before, with 12 environment skips (Movie, Office, SVG). The PDF cases run here for the first time. php -l clean under 7.4 on all 21 changed files. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com>

Backport of #41838. Without it the previous commit leaves this file red: testClosesTheStreamOnSuccess fed a PNG through the Photoshop provider, and the PSD pin refuses exactly that mismatch. The fixture is replaced by a PSD Imagick writes itself, so the case needs no fixture and cannot skip - it must not, since the success-path fclose() assertion is the whole subject of the file. The undecodable payload becomes bytes no coder claims, with a control assertion mirroring isDangerousToDecode(): both existing assertions are satisfied by any early return, so a build whose libmagic read the old payload as text/* would have had it refused at the new mime gate, stayed green, and silently stopped covering the decode. Measured on owncloudci/php:7.4 as application/octet-stream, which does reach the decode. Diverges from master in three places, all because this branch is PHP 7.4: - the unopenable-file case keeps its 10.16 shape, asserting that no warning is emitted. Master asserts the return value, which cannot fail here: stream_get_contents(false) only warns on 7.4 and the result is false either way, so the returned value cannot tell an unopenable file from an undecodable one. Only the warning can. - master's note that an unstubbed getMimeType() mock yields a TypeError does not hold on this branch: getThumbnail() casts the value, so an unstubbed mock gives ''. That is worse rather than better for a test - seven of the eight providers answer '' with the same constant they answer anything with, so the case would pass while proving nothing about which coder ran. Font is the one provider that branches on the mime type. The comment says that instead. - owncloudci/php:7.4 rather than :8.3 named as the build with no SVG renderer, confirmed on it. Verified in owncloudci/php:7.4: 3 tests / 7 assertions / 0 failures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com>

…e [10.16] Backport of #41855, folded into this backport at the maintainer's request rather than opened as its own 10.16 PR. SVG::getThumbnail() read the handle from $file->fopen('r') without checking it, and released it only on the success path. Bitmap::getThumbnail() checked for false but not for null. Both now use !is_resource(), and the SVG provider closes the handle in a finally so a read that throws from the wrapper stack - the encryption module does, on a missing or damaged key - cannot hold the descriptor and the view's shared lock for the rest of the request. Reworded and re-tested for PHP 7.4, where the consequence differs from master's: - master's changelog and comments say an unopened file made the request fail with a server error. On 7.4 it does not. stream_get_contents() warns and hands on false, the prefix check turns that into a bare XML declaration, and Imagick rejects it - so the preview already degraded to a media type icon, after two misleading log lines. It is on PHP 8 that those calls raise a TypeError, an \Error that escapes the catch (\Exception) as a 500. The changelog now describes the logging noise, which is what this fix removes here. - SVGStreamTest's unopenable case therefore asserts that no warning is emitted, not that the return value is false: master's assertFalse() passes with the guard reverted on this runtime, so it could never go red. Renamed accordingly, and the handler honours error_reporting() so that diagnostics the code under test silences with @ cannot fail it, the same line OC\Log\ErrorHandler::onError() draws. Verified red before green - see the PR description for the counts. - BitmapStreamTest's equivalent case keeps its existing 7.4 assertion and gains #41855's data provider, so the null handle is covered here too. Verified in owncloudci/php:7.4: tests/lib/Preview/ plus tests/lib/Files/Type/DetectionTest.php at 72 tests / 219 assertions / 0 failures, 12 environment skips (Movie, Office, SVG - this image registers no SVG coder). php -l clean under 7.4. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com>

oc-tmueller · 2026-09-24T13:39:18Z

Closing to reopen this backport under the correct account: this PR was opened as kw-tmueller, while the rest of the OC10-164 family (#41827, #41834, #41835, #41837) is oc-tmueller. A PR's author cannot be changed, and GitHub's squash-merge takes the commit author from the PR author rather than from the branch commits, so merging this one would put the wrong identity on the commit in 10.16.

Nothing about the change itself is affected: the branch is untouched, and all three commits on it are already authored, committed and signed as oc-tmueller. Replacement PR link to follow in a moment.

oc-tmueller · 2026-09-24T13:39:38Z

Superseded by #41863 — same branch, same commits (faa8db09cb), opened as oc-tmueller.

The backport PR was reopened as #41863, because #41828 had been opened under the wrong GitHub account and a PR's author cannot be changed. The three changelog entries linked #41828, which is now closed, so they move to #41863. The master PR links are unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com>

…iews [10.16] (OC10-164) (#41863) * fix: prevent arbitrary file write via unsanitized SVG/MVG bitmap previews [10.16] Backport of #41827. That PR carries the per-provider Imagick coder pin from #41834 as well, because #41834 was merged into its branch, so the squash commit on master contains both changes and so does this backport. 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 only 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. getResizedPreview() now rejects content whose libmagic-detected media type is text/*, image/svg*, application/xml or image/x-mvg before calling into Imagick at all, and each provider pins the exact coder it serves instead of letting ImageMagick re-derive the format from the content. Adapted for PHP 7.4, the only version 10.16 supports. Master justifies several of these guards by PHP 8 raising an \Error that escapes catch (\Exception); on 7.4 the same calls only warn, so every such claim was re-derived on the target runtime rather than carried over: - finfo_buffer(false, ...) warns and returns false on 7.4, and detectString() returned that false to the new deny-list, where it collapses to '' and matches no entry. Here the missing guard admitted the content the list exists to reject; it is on PHP 8 that it turns a missing preview into a 500. - the same holds for popen()/fgets()/pclose() in detect() and for fopen(false, ...) in the branch taken without ext-fileinfo. - the (string) cast on $file->getMimeType() is required on 7.4 too: passing null to a userland string-typed parameter is a TypeError on 7.4 as well. Measured, because the surrounding guards are not. 10.16 keeps its own "$stream === false" check and $image->loadFromData($bp); the is_resource() form and the (string) cast on that call are master-only, from #41855 and #41449, and the three-way merge preserved both correctly. The measurements the coder-pin comments rest on were re-taken on owncloudci/php:7.4, this branch's own CI image. It ships the same ImageMagick 6.9.11-60 and Ghostscript 9.55.0 as the 8.3 image and every figure reproduced: plain PostScript and EPSF-branded content both render 612x792 unpinned, pinned EPS and pinned PS alike; a %!PS-Adobe payload read unpinned reaches the PS coder at 612x792 while the TTF pin gives 800x480. That image also registers no SVG coder and no HEIF coder distinct from HEIC - which is precisely what PDFTest's old SVG-based guard got wrong and what Heic's single HEIC pin is there for. Verified in owncloudci/php:7.4: tests/lib/Preview/ plus tests/lib/Files/Type/DetectionTest.php at 68 tests / 207 assertions, against 41 tests / 123 assertions before, with 12 environment skips (Movie, Office, SVG). The PDF cases run here for the first time. php -l clean under 7.4 on all 21 changed files. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * test: make BitmapStreamTest survive the OC10-164 coder pin [10.16] Backport of #41838. Without it the previous commit leaves this file red: testClosesTheStreamOnSuccess fed a PNG through the Photoshop provider, and the PSD pin refuses exactly that mismatch. The fixture is replaced by a PSD Imagick writes itself, so the case needs no fixture and cannot skip - it must not, since the success-path fclose() assertion is the whole subject of the file. The undecodable payload becomes bytes no coder claims, with a control assertion mirroring isDangerousToDecode(): both existing assertions are satisfied by any early return, so a build whose libmagic read the old payload as text/* would have had it refused at the new mime gate, stayed green, and silently stopped covering the decode. Measured on owncloudci/php:7.4 as application/octet-stream, which does reach the decode. Diverges from master in three places, all because this branch is PHP 7.4: - the unopenable-file case keeps its 10.16 shape, asserting that no warning is emitted. Master asserts the return value, which cannot fail here: stream_get_contents(false) only warns on 7.4 and the result is false either way, so the returned value cannot tell an unopenable file from an undecodable one. Only the warning can. - master's note that an unstubbed getMimeType() mock yields a TypeError does not hold on this branch: getThumbnail() casts the value, so an unstubbed mock gives ''. That is worse rather than better for a test - seven of the eight providers answer '' with the same constant they answer anything with, so the case would pass while proving nothing about which coder ran. Font is the one provider that branches on the mime type. The comment says that instead. - owncloudci/php:7.4 rather than :8.3 named as the build with no SVG renderer, confirmed on it. Verified in owncloudci/php:7.4: 3 tests / 7 assertions / 0 failures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * fix: report a preview file that cannot be opened without logging noise [10.16] Backport of #41855, folded into this backport at the maintainer's request rather than opened as its own 10.16 PR. SVG::getThumbnail() read the handle from $file->fopen('r') without checking it, and released it only on the success path. Bitmap::getThumbnail() checked for false but not for null. Both now use !is_resource(), and the SVG provider closes the handle in a finally so a read that throws from the wrapper stack - the encryption module does, on a missing or damaged key - cannot hold the descriptor and the view's shared lock for the rest of the request. Reworded and re-tested for PHP 7.4, where the consequence differs from master's: - master's changelog and comments say an unopened file made the request fail with a server error. On 7.4 it does not. stream_get_contents() warns and hands on false, the prefix check turns that into a bare XML declaration, and Imagick rejects it - so the preview already degraded to a media type icon, after two misleading log lines. It is on PHP 8 that those calls raise a TypeError, an \Error that escapes the catch (\Exception) as a 500. The changelog now describes the logging noise, which is what this fix removes here. - SVGStreamTest's unopenable case therefore asserts that no warning is emitted, not that the return value is false: master's assertFalse() passes with the guard reverted on this runtime, so it could never go red. Renamed accordingly, and the handler honours error_reporting() so that diagnostics the code under test silences with @ cannot fail it, the same line OC\Log\ErrorHandler::onError() draws. Verified red before green - see the PR description for the counts. - BitmapStreamTest's equivalent case keeps its existing 7.4 assertion and gains #41855's data provider, so the null handle is covered here too. Verified in owncloudci/php:7.4: tests/lib/Preview/ plus tests/lib/Files/Type/DetectionTest.php at 72 tests / 219 assertions / 0 failures, 12 environment skips (Movie, Office, SVG - this image registers no SVG coder). php -l clean under 7.4. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * docs: point the changelog entries at the reopened backport PR The backport PR was reopened as #41863, because #41828 had been opened under the wrong GitHub account and a PR's author cannot be changed. The three changelog entries linked #41828, which is now closed, so they move to #41863. The master PR links are unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> 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> Co-authored-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

oc-tmueller added a commit that referenced this pull request Sep 11, 2026

docs: point the changelog entry at the 10.16 PR (#41828)

05e94c3

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

kw-fscheuer mentioned this pull request Sep 11, 2026

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

Merged

4 tasks

This was referenced Sep 14, 2026

fix: pin the Imagick coder per provider instead of letting it sniff (OC10-164) #41832

Closed

fix: pin the Imagick coder per provider without a temporary file (OC10-164) #41834

Merged

oc-tmueller marked this pull request as draft September 16, 2026 11:48

This was referenced Sep 16, 2026

fix: release the stream when a bitmap preview cannot be decoded [10.16] #41837

Merged

test: make BitmapStreamTest survive the OC10-164 coder pin #41838

Merged

oc-tmueller and others added 3 commits September 24, 2026 13:56

oc-tmueller force-pushed the fix/oc10-164-bitmap-preview-arbitrary-file-write-10.16 branch from 05e94c3 to faa8db0 Compare September 24, 2026 12:58

oc-tmueller marked this pull request as ready for review September 24, 2026 13:13

oc-tmueller closed this Sep 24, 2026

Navigation Menu

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

fix: prevent arbitrary file write via unsanitized SVG/MVG bitmap previews [10.16] (OC10-164)#41828
kw-tmueller wants to merge 3 commits into
10.16from
fix/oc10-164-bitmap-preview-arbitrary-file-write-10.16

kw-tmueller commented Sep 11, 2026 •

edited by oc-tmueller

Loading

Uh oh!

oc-tmueller commented Sep 24, 2026

Uh oh!

oc-tmueller commented Sep 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

kw-tmueller commented Sep 11, 2026 • edited by oc-tmueller Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What PHP 7.4 required

Verification

Known and deliberate

Uh oh!

oc-tmueller commented Sep 24, 2026

Uh oh!

oc-tmueller commented Sep 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kw-tmueller commented Sep 11, 2026 •

edited by oc-tmueller

Loading