Block unfuzzed loaders with libvips 8.13+

renchap commented 2 years ago

There is a new libvips 8.13+ to block unfuzzed loaders using vips-block-untrusted-set.

I think this should be enabled by default to improve security, as most of the time the images processed by image_processing come from an user-controlled source.

janko commented 2 years ago

Thanks for the heads up. Any idea how to call it via ruby-vips? I'm guessing that's something that should be called globally at require time, rather than per image.

renchap commented 2 years ago

Yes, this should be a global call.

As far as I can see, there is no support for this in ruby-vips yet. We could set the VIPS_BLOCK_UNTRUSTED env variable, but this does not require a change in image_processing and users can do it themselves.

I am not sure of the proper way to handle this in image_processing and if this gem needs to use more granular blocking

jcupitt commented 1 year ago

Heroku have libvips in their stack images!

https://devcenter.heroku.com/changelog-items/2549

But they include a lot of insecure load libraries :( Anyone using image_processing on heroku will (probably) be trivially vulnerable to remote code execution attacks, or at the very least denial of service attacks.

How about setting the VIPS_BLOCK_UNTRUSTED env var on by default? It would fix this potential issue, for recent libvips at least.

edit sigh, the libvips that supports blocking is in ubuntu from 22.10, so won't be in heroku for ages. Blocking would still help people who have built their own libvips.

jonian commented 1 year ago

The libvips versions in heroku stack images are old. It is better to continue using custom buildpacks.

@jcupitt I maintain a custom buildpack here that has the latest libvips version (currently 8.14.1). I have read your suggestions on other buildpacks and used your docker files as reference. Any suggestions that can increase security and make the buildpack better are highly appreciated.

jcupitt commented 1 year ago

Oh, nice @jonian! I'll open an issue with some comments.

renchap commented 6 months ago

https://github.com/libvips/ruby-vips/pull/382 has been merged a few months ago, and it should allow to block untrusted loaders by default.

janko commented 6 months ago

Since this is a global setting, I'm not sure it's a good idea for ImageProcessing gem to be overriding it, because it would change behavior of code that calls vips directly.

If this could be set at the individual processing pipeline level, I would consider it.

midnight-wonderer commented 3 months ago

Should we mention

::Vips.block_untrusted(true)

in the README?
You may delegate this to me if you don't have much spare time.

jcupitt commented 3 months ago

Maybe another option would be for image_processing to block untrusted loaders during require? People could always call ::Vips.block_untrusted(false) at some later point if they wished.

I think "secure by default" is likely to be the best choice when dealing with untrusted data (ie. everyone who uses image_processing).

janko commented 3 months ago

I'm planning to turn this on by default in the upcoming major version of ImageProcessing.

midnight-wonderer commented 3 months ago

May I object to the decision? I totally agree with your previous statement.
If it is global, there should be one and only one entity that controls the flag.

midnight-wonderer commented 3 months ago

The default could change, but not at a 3rd-party-gem level.
If it has to be changed, it should be done at ruby-vips.

jcupitt commented 3 months ago

ruby-vips is used quite widely outside Rails, so changing the default there would force a source code change on a lot of other projects. ImageProcessing is more specialised and really only deals with untrusted data so, to my mind, it's the least-worst place to do it.

Perhaps the upcoming libvips 8.16 should simply block all unfuzzed loaders for everyone, with an env var allow list for people that are dealing with trusted data. Though it will cause a lot of people a lot of annoyance.

midnight-wonderer commented 3 months ago

I happen to be working on a Rails project that interacts directly with ruby-vips. I have a potential solution and a request from the top of my head.

Potential solution: Making it obvious

In the README, we could add something to direct users to this flag they should be aware of. For example, add this to the example.

require "image_processing/vips"
::Vips.block_untrusted(true) # add this

pipeline = ImageProcessing::Vips
  .source(file)
  .convert("png")

large  = pipeline.resize_to_limit!(800, 800)
medium = pipeline.resize_to_limit!(500, 500)
small  = pipeline.resize_to_limit!(300, 300)

Libvips could support local options in the future. Once released, we can't retract the setting. By keeping it the way it is, we are open to that possibility.

Request: In case I cannot prevent `image_processing` gem from setting the flag.

This flag has side effects to the whole process; please consider:

Bumping the major version
Provide a method to prevent image_processing from setting the flag without setting it back and forth; maybe with ENV? Imagine multiple libraries wanting to set the flag, and they all can be dynamically require.
Making the method notable in the README

I hope the request is reasonable enough. And thank you for the gem. 🙏

janko commented 3 months ago

I think Ruby bindings for libvips isn't the appropriate abstraction level for turning this by default. ImageProcessing, on the other hand, is primarily intended for handling file attachments on the web (it's used by Active Storage, Shrine, CarrierWave and Refile), where files come from users.

I agree with @jcupitt that it would make sense for ImageProcessing to provide secure defaults for the majority of web apps, and I already mentioned it would come with a major version bump and of course be documented. I don't really understand the need for an environment variable when Vips.block_untrusted(false) can be called. It's unlikely other gems will set this flag, and if they do, then the developer should be aware of it; I don't see how an ENV variable would make this less problematic.

midnight-wonderer commented 3 months ago

I don't really understand the need for an environment variable when Vips.block_untrusted(false) can be called. It's unlikely other gems will set this flag, and if they do, then the developer should be aware of it; I don't see how an ENV variable would make this less problematic.

One can set

::Vips.block_untrusted(false)

but timing matters.

People who want to, say, process SVG have to do that:

before use
but after every other place set it to true

The problem is most of the time, we don't know when other places have done setting the flag. (The timing is unknown.)

Interestingly, you, @janko, are probably the one who made me aware of the issue. I can't remember where I read this from, but you are in the argument about where to place require. You are in favor of doing it inside the method instead of at the top of the file. The argument is sound: By doing it in the method, the script is only loaded when needed, and the performance impact is minimal. (Backed by code benchmark.)
If my memory served me correctly, that is.

If you plan to do it at some require, the problem is people don't know when exactly Vips.block_untrusted(true) gets called. Making them unable to Vips.block_untrusted(false) afterward. Providing a way to disable the side effects in the first place would prevent the timing issue.

I might be wrong; if there is a better way to view the issue, please let me know.

janko commented 3 months ago

In this case, the fact that ImageProcessing autoloads the MiniMagick and Vips backend code is wrong, and I plan to require explicit requires in the upcoming major version. Library code should never be loaded at runtime after the application boots, especially not one that have bindings to C libraries. This will make it obvious where the default is set, which will be on require "image_processing/vips".

midnight-wonderer commented 3 months ago

When to set the flag will never be obvious when there are multiple places setting the flag.

The end consumers are probably not the ones who requires image_processing but Active Storage, Shrine, CarrierWave, and Refile. The only surefire way to set the flag is to require the code where the side effects reside and set the flag before any of those libraries get lazy(auto)-loaded (if applicable).

I can't even guess how people find out they have to look at this (the middle) gem's readme to be aware of the side effects. On the flipside, that applies to my proposal, too.

This is one of those situations when there is no right answer.

For me, having everyone mention the issue about libvips is better than everyone mentioning the side effects of middle gems. (prioritize details awareness)
For some, hiding side effects in this gem is the least-worst choice for security. (prioritize footgun control)

I don't know.
Wherever this gem goes, it won't affect me because I am in this convo.
Functional programming fans will probably prefer the least-side-effects one, though. I just think about a discussion like this one that I am not participating in that will affect me in the future.

janko commented 3 months ago

When to set the flag will never be obvious when there are multiple places setting the flag.

You can always force gems to be required on Bundler.require in Rails applications, and then change the libvips flag below the Bundler.require call. In the next major version, I plan to change Rails generators to add the following in the Gemfile:

gem "image_processing", "~> 2.0", require: "image_processing/vips"

This is one of those situations when there is no right answer.

ImageProcessing gem is intended to be used with file attachment libraries that accept images from the web, so this would be a secure default that some will want to opt out of, which is better than an insecure default that most will want to opt into (but won't, because it's not the default and they won't read the README). Rails also prioritizes security by default, and it's the biggest dependent of ImageProcessing.

midnight-wonderer commented 3 months ago

@janko Hey, thanks for the reply. I thought I got ghosted. This might be okay with you.

References

Please refer to libvips' documentation specifically here. They already have an environment variable for the task.

Set the environment variable VIPS_BLOCK_UNTRUSTED to block all untrusted operations on vips_init().

How about when the variable is already set, image_processing refrains from calling block_untrusted?

The logic is that if the variable is set, but the library is operating in untrusted mode, it indicates that some other libraries are already called ::Vips.block_untrusted(false). This should not affect what you try to accomplish and is probably an agreeable approach in the community if other gems also want to follow suit.

I haven't tried it personally, so I'm not quite sure if one can unblock after the environment variable is set. However, according to the document, it should work.

In conclusion (TL;DR)

The proposal is if VIPS_BLOCK_UNTRUSTED is already set, image_processing does nothing. What do you say?

janko commented 3 months ago

Yes, that's completely reasonable, thanks for bringing it to my attention 👍

janko / image_processing