Reject vips transformations that dispatch arbitrary libvips operations - #3025
Reject vips transformations that dispatch arbitrary libvips operations#3025jeremy wants to merge 3 commits into
Conversation
Active Storage's ImageMagick transformer enforces ActiveStorage.supported_image_processing_methods, but the vips transformer we run never reads that allowlist. So CVE-2025-24293's removal of apply, loader, and saver is inert on our path, and a signed variation key can still carry them. On vips those methods are not inert. ImageProcessing dispatches the loader and saver by name -- Vips::Image.public_send(:"#{loader}load", ...) and image.public_send(:"#{saver}save", ...) -- whenever the options carry a nested loader:/saver: selector, and apply re-enters the builder to reach the same selectors. config/initializers/vips.rb already calls Vips.block_untrusted(true), which gates untrusted loaders, so loader: { loader: "openslide" } is blocked -- but savers are not flagged untrusted, so saver: { saver: "dz" } still runs dzsave (an unbounded on-disk tile pyramid) and csv/matrix emit text served back as an image. Prepend a guard onto ActiveStorage::Transformers::Vips that rejects apply and a nested loader:/saver: selector, while still accepting ordinary option hashes -- notably loader: { n: -1 }, which Attachments::VARIANTS relies on to keep animated GIF frames and which is signed into immortal variant URLs. The mechanism holds at the locked image_processing 1.14.0 and survives the pending 2.0 bump (PR #2922), which hardens apply against Kernel methods but not against loader/saver dispatch.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f5638d55e2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…/saver
Rejecting apply and the nested loader:/saver: selectors closed the three
builder entry points but left the wider primitive open: ImageProcessing's
Chainable#method_missing turns any *other* transformation name into an
operation, and Processor#apply_operation hands it to Vips::Image#public_send.
No allowlist is enforced anywhere on the vips path, so the name was free.
That is reachable with Vips.block_untrusted(true) on, and the argument is the
destination path rather than Active Storage's tempfile:
dzsave: "<path>" wrote a 36-entry tile pyramid where the key said
csvsave/matrixsave wrote image bytes out as text
write_to_file: "<path>" wrote an arbitrary file
instance_eval: "<ruby>" executed arbitrary Ruby on image_processing 1.14.0
Each raises afterwards, on the operation's nil return, so the side effect has
already landed by the time the pipeline fails. The exception is not a defense.
Enforce ActiveStorage.supported_image_processing_methods here the way the
ImageMagick transformer does. The list admits nothing dangerous on this path:
of its 284 names, 6 reach an ImageProcessing macro, 6 reach a pure Vips::Image
accessor, and 272 resolve to nothing. loader and saver stay permitted as names
because Attachments::VARIANTS signs loader: { n: -1 } into variant URLs that
never expire; only their nested same-name selector is rejected. apply needs no
special case now, being absent from the list.
Tests assert the absence of the filesystem side effect, not just the exception
class -- these operations raised before the guard too, after writing.
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Vips::Image#method_missing resolves an unknown name against libvips operation nicknames, so the surface an allowlisted name can reach is wider than Vips::Image's own methods -- a fact the previous commit's comment missed. It claimed 12 of the 284 allowlisted names resolve to anything; counting operation dispatch too, it is 30: 6 ImageProcessing macros, 21 libvips operations, and a few pure accessors. The conclusion is unchanged, and now measured rather than assumed: none of the 30 writes to disk or takes a filename, and every *save/*load nickname is absent from the list. Corrected the comment and added a test that a real operation nickname (gaussblur, invert) is rejected despite libvips knowing it. Also pin the super chain. The guard raises for its own cases but must keep delegating, or the base transformer's combine_options check silently disappears; nothing covered that.
|
🤖 @codex review Pushed f3f6dc4 on top of the commit you cleared. It corrects a measurement I got wrong (libvips operation nicknames are reachable via |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
The defect
CVE-2025-24293 (rails/rails
1ca278a6a7, "Active Storage: Remove dangerous transformations") removedapply,loader, andsaverfromActiveStorage.supported_image_processing_methods. But that allowlist is only enforced by the ImageMagick transformer —ImageMagick#validate_transformationis the sole reader ofsupported_image_processing_methodsin the whole tree. The vips transformer (transformers/vips.rb, 15 lines) only definesprocessor; it never overridesvalidate_transformation, and the baseImageProcessingTransformer#validate_transformationrejects onlycombine_options.Fizzy renders variants with vips (Rails default under
load_defaults 8.1). So on our path there is no method-name enforcement at all — not merely a three-method gap. A signed variation key picks the method.Worth naming because it is why this was easy to miss: the CVE commit names neither ImageMagick nor mini_magick.
git show 1ca278a6a7 --stattouches exactly two files (lib/active_storage.rb,test/models/variant_test.rb) and no CHANGELOG —activestorage/CHANGELOG.mdat our pinned Rails revision contains zero mention of the CVE. The only in-repo signal that the fix is ImageMagick-only is that both changed tests sit insideprocess_variants_with :mini_magickblocks; the:vipsblocks in the same file were never extended.Every transformation name reaches libvips by dispatch
ImageProcessing's builder handlesapply,loader, andsaveritself, and sends every other name throughChainable#method_missinginto the operations list, whereProcessor#apply_operationhands it toVips::Image#public_send:That leaves three distinct primitives:
and
applylaunders all three: it re-enters the builder for each of its entries, and those entries are never revalidated, because only top-level transformations reachvalidate_transformation.config/initializers/vips.rbalready callsVips.block_untrusted(true), which gates untrusted loaders — soloader: { loader: "openslide" }is blocked. Savers are not flagged untrusted, and direct dispatch is not gated at all.Threat model — defense-in-depth, not standalone RCE
A variation key is a signed serialization of the transformations hash (signed with
secret_key_base); an attacker cannot forge one without the key. This is defense-in-depth on the liveRAILS_MASTER_KEY-exfiltration incident (HEY #3916984): if the key leaks, arbitraryVips::Imagemethod dispatch is the attacker's next hop.The impact of that next hop is worse than this PR first claimed. For the direct-dispatch case the argument is the destination path, so it is not confined to Active Storage's tempfile — it is arbitrary-path write, and on our locked
image_processing 1.14.0the accumulator's inheritedObjectmethods are reachable too.Reproduction
Replicating
ImageProcessingTransformer#processon the vips path at Fizzy's lockedimage_processing 1.14.0/ruby-vips 2.2.5/ libvips 8.18.4, withVips.block_untrusted(true)on, and with the first version of this guard (rejecting onlyapply+ nestedloader:/saver:) in place:Each dangerous case does raise — on the operation's
nilreturn, when the pipeline tries to save it. The side effect has already landed by then. The exception is not a defense, which is why the tests assert the absence of the file, not just the exception class.The fix
Prepend a guard onto
ActiveStorage::Transformers::Vips(via the app'slib/rails_ext+to_prepareidiom) that enforcesActiveStorage.supported_image_processing_methods, the way the ImageMagick transformer does.The allowlist admits nothing dangerous on this path. Measuring that needs care, because the reachable surface is wider than
Vips::Image's own methods:Vips::Image#method_missingresolves an unknown name against libvips operation nicknames, so an allowlisted name reaches those too. Counting all three routes, 30 of its 284 names resolve to anything at all:ImageProcessingmacro (resize_to_limit,resize_to_fit,resize_to_fill,resize_and_pad,rotate,composite)affine,canny,clamp,colourspace,complex,copy,crop,flatten,flip,gamma,gravity,insert,morph,mosaic,resize,scale,sharpen,thumbnail,cache, plus the two macro-shadowed ones)clone,format,log,median,size)None of the 30 writes to disk or takes a filename, and every
*save/*loadnickname (dzsave,csvsave,matrixsave,rawsave,magickload,openslideload,pdfload,svgload, …) is absent from the list. The onlyObjectmethod admitted isclone.loaderandsaverstay permitted as names — CVE-2025-24293 removed them, butAttachments::VARIANTSsignsloader: { n: -1 }into variant URLs that never expire, so dropping the names would break every existing animated-GIF URL. Only their nested same-name selector is rejected; ordinary option hashes (loader: { n: -1 },saver: { quality: 80 }) keep working.applyneeds no special case, being absent from the list.Residual, out of scope for this PR
compositeis on the upstream allowlist, and itsImageProcessingmacro accepts a String path, which it opens withVips::Image.new_from_file. So with a compromised key it still reads an arbitrary server-side file:Two distinguishable errors make that a file-existence oracle, and a readable image gets composited into the returned variant.
This is not a regression here and not something this guard introduces:
compositeis allowlisted upstream and behaves the same on the ImageMagick path, wherevalidate_arg_stringonly screens for ImageMagick CLI flags and lets a plain path through. Closing it means departing from the upstream list, which is a separate decision from closing the dispatch primitive. Flagging it rather than folding it in.Options considered
loaderin the allowlist (what HEY does on its mini_magick path) — rejected: it re-opens the nested-selector dispatch it is meant to close.apply+ nestedloader:/saver:(this PR's first commit) — rejected: closes the three builder entry points but leaves direct dispatch to anyVips::Image(orObject) method wide open. Caught in review by Codex.loader/saveras names (this PR) — closes the whole primitive, preservesloader: { n: -1 }and every immortal URL.loaderout ofVARIANTSand internalize it (what bc3 did — "moved out of URLs into internal boilerplate") — cleaner long-term (stops putting loader options in immortal URLs) but materially more invasive, and orthogonal. Worth doing later; not required to fix this.Holds across the pending
image_processing2.0 upgrade (#2922): 2.0 hardensapplyagainstKernel/Objectmethods but not againstloader/saverdispatch or direct operation names, so this guard stays necessary.Mutation tests
Each mutation was verified to have actually applied (marker grepped in the mutated file) before its run was trusted.
validate_transformation→ justsuper)loader:{n:-1},saver:{quality:80}, resize_to_fill, resize)apply+ unknown-name fail; nested passloader/saverfail; direct dispatch still caught(Counts are for the final 14-test suite: neuter → 8, reject-all → 1 failure + 5 errors, no-allowlist → 6, no-nested-selector → 2.)
The last two are the load-bearing pair: they show neither half is redundant. The allowlist alone does not cover nested selectors (because
loader/saverare permitted as names), and the nested-selector check alone does not cover direct dispatch.With the fix in place: 14 tests, 23 assertions, 0 failures. Variant-adjacent suites (
test/lib/rails_ext, avatar, comment, storage, exportable): 127 tests, 307 assertions, 0 failures. Full suite atf3f6dc4fe: 1556 tests, 5850 assertions, 0 failures, 0 errors, 3 skips.Do not merge yet — coordinated with the bc3 and upstream Rails companions.