New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validations are run after versions created #1320

Closed
arcwhite opened this Issue Feb 14, 2014 · 11 comments

Comments

Projects
None yet
7 participants
@arcwhite

arcwhite commented Feb 14, 2014

I'm currently attempting to mitigate a DoS attack against sites with CarrierWave enabled. An attacker can upload an image with an arbitrarily large width and height in the image header (and no correspondingly large image data), which causes tools like imageMagick to consume significant resources on the server.

This is pretty not great.

I've added a validator to my User model, which checks the dimensions of the image and adds errors to it if dimensions are greater than some arbitrary threshold (e.g. 1000x1000).

The validations are not called before image manipulation happens (server still chokes).

I can use a callback on the uploader (e.g. before :cache) to raise an exception, but this is an inelegant solution (I'd much prefer to run validations before mogrifications, saving the creation of the versions, and showing the user a nice clean error message)

Is there some other solution?

@arcwhite

This comment has been minimized.

Show comment
Hide comment
@arcwhite

arcwhite Feb 27, 2014

Bump, because unless I'm missing something, this is a DoS on every single Rails site that uses CarrierWave "out of the box".

arcwhite commented Feb 27, 2014

Bump, because unless I'm missing something, this is a DoS on every single Rails site that uses CarrierWave "out of the box".

@krainboltgreene

This comment has been minimized.

Show comment
Hide comment
@krainboltgreene

krainboltgreene May 2, 2014

I just encountered a similar problem with same root:

version's :if option gets called where the @model is something that is completely empty, AKA before any data is set from things like *_attributes.

krainboltgreene commented May 2, 2014

I just encountered a similar problem with same root:

version's :if option gets called where the @model is something that is completely empty, AKA before any data is set from things like *_attributes.

@teoulas

This comment has been minimized.

Show comment
Hide comment
@teoulas

teoulas May 7, 2014

👍
This is a serious problem. There must be a way to return an error early, before any processing.

teoulas commented May 7, 2014

👍
This is a serious problem. There must be a way to return an error early, before any processing.

@mirzali

This comment has been minimized.

Show comment
Hide comment
@mirzali

mirzali Sep 5, 2014

Anything happened with this? I notice that the process! is called with before :cache, so maybe it should be moved to before :store ?

mirzali commented Sep 5, 2014

Anything happened with this? I notice that the process! is called with before :cache, so maybe it should be moved to before :store ?

@thomasfedb

This comment has been minimized.

Show comment
Hide comment
@thomasfedb

thomasfedb Nov 26, 2015

Contributor

@bensie Your thoughts on this? Appears that it may be a fairly serious issue. Resolving it would be a breaking change, however.

Contributor

thomasfedb commented Nov 26, 2015

@bensie Your thoughts on this? Appears that it may be a fairly serious issue. Resolving it would be a breaking change, however.

@thomasfedb thomasfedb added the bug label Nov 26, 2015

@arcwhite

This comment has been minimized.

Show comment
Hide comment
@arcwhite

arcwhite Nov 26, 2015

I can give you an example payload, if that will help

arcwhite commented Nov 26, 2015

I can give you an example payload, if that will help

@arcwhite

This comment has been minimized.

Show comment
Hide comment
@arcwhite

arcwhite Nov 27, 2015

As an addendum: https://github.com/DarthSim/carrierwave-bombshelter fixes the underlying issue (e.g. memory exhaustion if the image file header claims it's, say, 64250x64250 pixels -- looks like we're not using ImageMagick's --limit option anywhere?)

arcwhite commented Nov 27, 2015

As an addendum: https://github.com/DarthSim/carrierwave-bombshelter fixes the underlying issue (e.g. memory exhaustion if the image file header claims it's, say, 64250x64250 pixels -- looks like we're not using ImageMagick's --limit option anywhere?)

@bensie

This comment has been minimized.

Show comment
Hide comment
@bensie

bensie Nov 27, 2015

Member

carrierwave-bombshell looks reasonable if you're concerned about this issue. Not all Rails apps are inherently vulnerable as stated above (not all process images, not all use ImageMagick).

This is a system/ImageMagick issue, not a CarrierWave issue. CarrierWave itself has no code that interacts with ImageMagick. The bombshelter gem chooses to address the problem using CarrierWave, but that doesn't make it CarrierWave's problem.

Addressing it with environment variables (MAGICK_MEMORY_LIMIT) or the -limit option are another approach (link), but CarrierWave cannot be responsible for handling limits on processes that your web app happens to shell out to.

Member

bensie commented Nov 27, 2015

carrierwave-bombshell looks reasonable if you're concerned about this issue. Not all Rails apps are inherently vulnerable as stated above (not all process images, not all use ImageMagick).

This is a system/ImageMagick issue, not a CarrierWave issue. CarrierWave itself has no code that interacts with ImageMagick. The bombshelter gem chooses to address the problem using CarrierWave, but that doesn't make it CarrierWave's problem.

Addressing it with environment variables (MAGICK_MEMORY_LIMIT) or the -limit option are another approach (link), but CarrierWave cannot be responsible for handling limits on processes that your web app happens to shell out to.

@bensie bensie closed this Nov 27, 2015

@DarthSim

This comment has been minimized.

Show comment
Hide comment
@DarthSim

DarthSim Jan 28, 2016

Contributor

@bensie Actually, this is not about bombshelter or DoS attacks at all. When you process files before validations, you can process something that shouldn't ever be processed. This could be a serious vulnerability or just a waste of time.

Contributor

DarthSim commented Jan 28, 2016

@bensie Actually, this is not about bombshelter or DoS attacks at all. When you process files before validations, you can process something that shouldn't ever be processed. This could be a serious vulnerability or just a waste of time.

@thomasfedb

This comment has been minimized.

Show comment
Hide comment
@thomasfedb

thomasfedb Jan 28, 2016

Contributor

@DarthSim are you able to produce a proof-of-concept?

Contributor

thomasfedb commented Jan 28, 2016

@DarthSim are you able to produce a proof-of-concept?

@DarthSim

This comment has been minimized.

Show comment
Hide comment
@DarthSim

DarthSim Jan 28, 2016

Contributor

@thomasfedb

  1. First case is protection from image bomb, of course. Yeah, it's not CarrierWave problem, but the current version makes it hard to solve the problem easily.
  2. Video processing. Suppose we want to check some info about a video before processing (length, codec, etc). With the current version of CarrierWave we can spend a lot of time processing video with invalid length or codec.
  3. Tons of other cases where processing isn't safe or cheap.

As a solution, I can propose a special callback for validations, so we'll be able to do the following:

after :validate, :check_video_length

def check_video_length(new_file)
  return if videl_length_is_ok?
  fail CarrierWave::IntegrityError, "Video is too long"
end

Standard validations could be wrapped into this callback.

Contributor

DarthSim commented Jan 28, 2016

@thomasfedb

  1. First case is protection from image bomb, of course. Yeah, it's not CarrierWave problem, but the current version makes it hard to solve the problem easily.
  2. Video processing. Suppose we want to check some info about a video before processing (length, codec, etc). With the current version of CarrierWave we can spend a lot of time processing video with invalid length or codec.
  3. Tons of other cases where processing isn't safe or cheap.

As a solution, I can propose a special callback for validations, so we'll be able to do the following:

after :validate, :check_video_length

def check_video_length(new_file)
  return if videl_length_is_ok?
  fail CarrierWave::IntegrityError, "Video is too long"
end

Standard validations could be wrapped into this callback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment