ImageProcessor.Web and DoS

Created on 23 May 2014 · 55Comments · Source: JimBobSquarePants/ImageProcessor

I see it a bit problematic, that ImageProcessor.Web accepts any combination of querystrings.
Nothing would prevent an attacker to request thousands of different crops/sizes.

Maby encrypting the querystring when serverside url generation happens would prevent this?

discussion framework

Source

meixger

Most helpful comment

Rather than taking a RestrictTo approach, does it not make sense to instead define "Templates" which can be called, example;

https://mysite.com/image-api/myimage.jpg?template=large-thumbnail

Where large-thumbnail has a configuration which maps to something like
width=256&height=256&quality=80&format=png

A similar approach to this is used in another PHP library, intervention
http://image.intervention.io/use/url

Update:

Sorry I have just seen that this library allows presets

 <presets>
    <preset name="demo" value="width=300&#038;height=150&#038;bgcolor=transparent"/>
  </presets>

How can these presets be used and can we restrict the processor to use presets only?

blowsie on 17 Jan 2018

👍2

All 55 comments

it isn't ImageProcessor's responsibility to fend off DoS attacks?

gfyans on 23 May 2014

Greg is correct. Imageprocessor would just be a single point, of many on a site to attack. If your site is under a DDOS attack then encrypted querystrings are not going to be much help. You can't decrypt a hash anyway, you can only compare so I would have to have a table of every possible combination stored somewhere to test against which is simply infeasible.

That said, I do actually have a few things built into the web component designed to minimise the attack surface.

If you look at the sample configuration here - scroll down a bit to processing.config you will see that there are settings designed to restrict the range of resizing dimensions. I also have made it possible to turn off all but the plugins you need to restrict usage also.

ImageProcessor scales well. It can handle a heavy user load and will use the native static file handler to serve Images on subsequent requests. It shouldn't be the weak point in your website.

JimBobSquarePants on 23 May 2014

@JimBobSquarePants

I have looked at the documentation regarding limiting resizing configs (i.e. limit to 5000x5000) but I'm still concerned. If someone wanted to simply write a scraper, get all of a sites images and then per image and request height x width for all of a sites images, the site will be taxed (during crop creation) and the disk could easily be filled up causing a server crash. Also it could create financial liability for users who have auto-scaled resources (disk and CPU).

Is there any way to restrict crops to exact dimension combinations? i.e. 100x200 and 400x600 only?

If a setup is restricted to even 100x100, a single image could generate 10,000 images in the disk cache. i.e. 1x100, 2x100... 100x100.

Concerned, but otherwise very happy with ImageProcessor.

kgiszewski on 10 Aug 2015

It looks like maybe resizing config doesn't apply to crops :) So I guess same question still though.

kgiszewski on 10 Aug 2015

Hi Kevin,

Good question. I dunno how this disappeared over time but there is actually a setting that can be applied to ImageProcessor.Web to restrict the system to a particular set of dimensions when resizing.

If you add a setting with the key RestrictTo to the resize configuration you can do just this.

  <setting key="RestrictTo" value="width=300&#038;height=150,width=500&#038;height=250"/>

Would restrict you to two separate sets of dimensions.

The code that parses this is here.

https://github.com/JimBobSquarePants/ImageProcessor/blob/4240a638695673faa62e825b14cc83b64fd2d6ea/src/ImageProcessor.Web/Processors/Resize.cs#L102

JimBobSquarePants on 11 Aug 2015

Wonderful, I'll give it a try today thank you.

kgiszewski on 11 Aug 2015

Confirmed it works. Thank you.

kgiszewski on 11 Aug 2015

@JimBobSquarePants

I've done more testing and I'm afraid I might have bad news.

Give this scenario:

Allow crops to be 100x100 in the config.

When requesting any image at 100x100, a crop is made and returned.

When the same image is requested but at say 100x101, the native resolution is returned, but a crop is still generated (at native resolution) thus the root issue is still present. Requesting images of any size will still fill up a hard drive eventually if an attacker desired to.

I'm guessing this is true because the url is hashed (I haven't look thru code yet) and still creates the crop despite not actually serving it.

What is desired is the crop just simply doesn't get created when it is not explicitly allowed.

Any chance we can re-open this issue?

kgiszewski on 11 Aug 2015

I'm gonna try to send you a PR if you don't mind.

kgiszewski on 11 Aug 2015

@JimBobSquarePants, I was hoping to send you a PR but it the solution seems more difficult that I had hoped.

What needs to happen (I think) is that if one of the modules in the processing pipeline detects an issue, it should be able set a flag to avoid the eventual caching process.

What appears to be happening is the resizer is detecting an invalid crop but still allowing the cache to occur.

Thoughts?

kgiszewski on 11 Aug 2015

I really don't think that it is the responsibility of ImageProcessor to protect you from DoS attacks. There's so many attack vectors on any webpage that the only real way to protect yourself would be through firewalling.

That said we should prevent the system from caching if we can. Maybe passing an out parameter on AutoProcess with the count of matching processors and altering the Resize method to check against restrictions sooner before returning its sort order would do the trick?

If the processor match count is zero, we don't cache.

https://github.com/JimBobSquarePants/ImageProcessor/blob/c957771d2076a988b3334aafce152ab86e6dafac/src/ImageProcessor.Web/ImageFactoryExtensions.cs#L41

https://github.com/JimBobSquarePants/ImageProcessor/blob/c957771d2076a988b3334aafce152ab86e6dafac/src/ImageProcessor.Web/Processors/Resize.cs#L101

JimBobSquarePants on 12 Aug 2015

Will throw 403 in next release.

JimBobSquarePants on 21 Aug 2015

Thanks James for looking at this again.

I've been thinking about how to overcome this a bit. Throwing an exception is probably a good idea.

Rather than return it to the browser, can ImageProcessor catch the exception during pipeline processing and just return the native image (thus preventing the cache and further processing)?

Reason for this is due to editors used by Umbraco and the like.

In an RTE images can be resized to just about anything.

So given a website that is restricted to only crop 300x500, 200x200; the RTE might ask for an image cropped to 250x200 (and some with weird rounded numbers). Rather than fail (by letting the 403 bubble up to the browser), I propose that the image is simply returned as the native size so that it fails gracefully.

Pseudo code:

//catch any module errors thrown
try
{
//resizer could throw 403
 var image = ProcessImagePipeline()

//we made it here without any exceptions
image.Cache()
)
catch (Exception ex)
{
  return native image
}

Have a good day :)

kgiszewski on 21 Aug 2015

Do you fancy giving something like that a go? I'm a bit wary myself but if you can get a working version though that'd be great. It'd have to be very soon, I've got a release pending. :)

JimBobSquarePants on 21 Aug 2015

Yea no worries I'll see what I can hack up.

kgiszewski on 21 Aug 2015

That'd be ace. :+1: Just make sure you're working against the v2 branch so you have the latest code.

JimBobSquarePants on 21 Aug 2015

@JimBobSquarePants Well I think I can properly indicate to the various structures that a cache should not occur, but I'm reading through the HttpModule and it (appears) that all images must be served from the cache. If that's the case then I'm going to have to de-mystify :) the cache a bit when I have more time myself.

Ideally when an illegal operation is detected, the image should just be served without further processing without any caching involved.

Long story short, don't wait the release for me :)

kgiszewski on 21 Aug 2015

Well... We have a stream so we could serve that.

https://github.com/JimBobSquarePants/ImageProcessor/blob/600169d5e1fa10f71273a1cfe7e84fe88b01626b/src/ImageProcessor.Web/HttpModules/ImageProcessingModule.cs#L540

I'll put some thought to it anyway but yeah, I'll probably ship in the next couple of days.

JimBobSquarePants on 22 Aug 2015

I'll recharge my brain this weekend and take another look next week.

kgiszewski on 22 Aug 2015

Ace thanks!

JimBobSquarePants on 22 Aug 2015

Hi guys, reviving this conversation a little as I just need to get (and provide) some feedback here.

I've been aware of the issue and have had some discussion with regards to ways to mitigate the issue but there is unfortunately not perfect way especially since people use ImageProcessor in many different ways and in ways we don't know.

@JimBobSquarePants you mentioned that you can restrict the plugins and the image dimensions above.

Can I just confirm that this is possible today with the latest ImageProcessor version?
I'm assuming that based on those recent commits that with the ImageProcessor options available that it is possible to mitigate any attacks in which hackers may attempt to just fill up the hard disk with randomly sized images with a various assortment of filters/plugins applied?

I'd love your feedback on this idea though since as far as I can see this is really the only way to mitigate this issue entirely:

We would require appending an anti-forgery token as part of the image request query string
This anti-forgery token is created on the server side when creating the image URL - this must be done with a UrlHelper instance (or whatever, but it's based on server side logic)
The anti-forgery token is then verified on the server side before allowing the image to be viewed.

Here's the problems with this:

LOTS of developers just create an image processor image URL using strings and not any server side helper method
LOTS of packages do the same thing
Therefore enforcing this token will break tons of sites

One other alternative that folks could do right now AFAIK is to attach an event handler to ImageProcessingModule.OnProcessQuerystring and if the query string parameters contain any transformations not explicitly allowed (based on any custom logic that a developer decides), they could just throw an exception? Or is there another way that a developer could write custom logic for the request to deny a response using the current codebase?

Moving forward though what do you guys think of this? I think it would be possible to build this mechanism into ImageProcessor in a flexible way so that it can be 'pluggable'. What I'm unsure about though is what other issues this might cause?

Cheers!

Shazwazza on 21 Mar 2016

👍1

Hey @Shazwazza

Yeah you can still turn off individual processors. To do so it is a case of installing the configuration package and removing individual plugins from processing.config

For example this config restricts processing to what I would call the essential collection. It also limits the maximum size to 1920px in either dimension

<processing preserveExifMetaData="false" interceptAllRequests="false">
  <presets>
  </presets>
  <!-- List of plugins. -->
  <plugins>
    <plugin name="AutoRotate" type="ImageProcessor.Web.Processors.AutoRotate, ImageProcessor.Web"/>
    <plugin name="Quality" type="ImageProcessor.Web.Processors.Quality, ImageProcessor.Web"/>
    <plugin name="Resize" type="ImageProcessor.Web.Processors.Resize, ImageProcessor.Web">
      <settings>
        <setting key="MaxWidth" value="1920"/>
        <setting key="MaxHeight" value="1920"/>
      </settings>
    </plugin>
  </plugins>
</processing>

There's also an additional setting you can use for the Resize plugin to restrict dimension to specific dimensions.

<setting key="RestrictTo" value="width=300&height=500,width=150&height=250"/>

I quite like the idea of the addition of an anti-forgery token. It's is something that we could optionally enable with a method helper for generating the tokens. I'd probably add that to the main product though rather than a plugin as an increasing plugin count makes it harder for me to maintain. Wil need community involvement to get it added though, my plate is quite full.

I've never thought myself about using ImageProcessingModule.OnProcessQuerystring. That's a neat idea!

Cheers

James

JimBobSquarePants on 22 Mar 2016

@JimBobSquarePants I'm absolutely happy to assist with this one, i just wanted to get an idea of how much we can limit functionality OOTB right now. I'll play around with this a little, my main concern (along with various other community members) is the ability to continually fill up the disk as part of an 'attack'. I'll get you a PR soon and can discuss there. Which branch would I target for this ?

Shazwazza on 22 Mar 2016

@Shazwazza Ace! Target the Framework branch please, that's the default branch for the current (hopefully soonish legacy version). Feel free to have a good dig around in there. If you spot anything silly give me a shout. I've been hearing some worrying reports regarding performance recently.

JimBobSquarePants on 22 Mar 2016

@Shazwazza @JimBobSquarePants FYI, I once started working on a package for Umbraco which 100% locked it down unfortunately I haven't had the time to finish it. Anyhow it went like this:
Added a parameter to GetCropUrl (preset:true) which generated the IP.Web url and a guid, then it created a preset element in processing.config (if it didn't exist already) using the guid as the name and the encoded querystring as the value, returned /media/1000/thing.jpg?preset=generatedguid
(I also was caching the collection to memory for performance.)
Set ImageProcessor.Web to only accept presets (I can't remember how you do this, but I'm 90% sure you can).

Obviously this is a Umbraco only solution and would require that all ImageProcessor url requests come through the GetCropUrl method (which they can). But I would imagine a similar helper method could potentially be part of ImageProcessor.Web if this approach has legs.

This is all possible with ImageProcessor v2 right now.

It was actually a really good package idea, if only there were more hours in the day!

Jeavon on 22 Mar 2016

👍1

@Jeavon Hi Jeavon,

it would really help if we could switch on the setting to only accept presets but I have no clue where I can find this. Any idea since your last post?

Thanks, Jeffrey

JeffreyPerplex on 25 Apr 2016

@JeffreyPerplex Presets are set in the config, there's a demo one specified in there.

http://imageprocessor.org/imageprocessor-web/configuration/#processingconfig

That doesn't restrict you to particular sizes only though. You need to additionally restrict sizes as described above.

<setting key="RestrictTo" value="width=300&height=500,width=150&height=250"/>

JimBobSquarePants on 26 Apr 2016

@JimBobSquarePants Thanks for the reply! Is is possible to use the RestrictTo on all plugins, such as Quality and Alpha? Or is only possible for the resize plugin?

JeffreyPerplex on 29 Apr 2016

@JeffreyPerplex Happy to help :smile:

No, I'm afraid not. It's a pretty good idea though!

JimBobSquarePants on 30 Apr 2016

@JimBobSquarePants I guess a setting for ImageProcessor.Web to only accept presets was a figment of my imagination. Is it feasible to add?

Jeavon on 30 Apr 2016

@Jeavon Aye probably. I'll add it to my TODO list.

JimBobSquarePants on 30 Apr 2016

Reopening this issue. Following a conversation with @JeffreyPerplex we're going to have a look at adding the ability to restrict all plugins to particular values.

JimBobSquarePants on 5 May 2016

HI all, I've created a PR here which will give you max flexibility for securing these requests based on an event:

https://github.com/JimBobSquarePants/ImageProcessor/pull/381

Using this you could just cancel the request if the parameters don't match what you want. An example (NOTE: I put no thought into this example!) :

ImageProcessingModule.ValidatingRequest += (sender, args) =>
{
    if (!string.IsNullOrWhiteSpace(args.QueryString))
    {
        //Don't support alpha whatsoever
        var queryCollection = HttpUtility.ParseQueryString(args.QueryString);
        if (queryCollection.AllKeys.Contains("alpha"))
        {
            args.Cancel = true;
            return;
        }

        //If there's a crop parameter, force it to always just be a specific value
        if (queryCollection.AllKeys.Contains("crop"))
        {
            queryCollection["crop"] = "10,10,10,10";
            //this performs the reverse of ParseQueryString since the result of ParseQueryString
            // is actually an instance of System.Web.HttpValueCollection
            args.QueryString = queryCollection.ToString();
        }
    }
};

Shazwazza on 6 May 2016

Thanks @Shazwazza for this I think it's a great enhancement.

JimBobSquarePants on 10 May 2016

XMLProposal.zip
XMLProposal.txt

Hi guys,

I was thinking to add settings to all plugins in the processing.config. Especially adding minvalue, maxvalue, default and/or allowedvalues.

Minvalue: Restrict the minimumvalue for this plugin or setting in a plugin. Applicable if the plugin expects a integer or decimal.
Maxvalue: Restrict the maximumvalue for this plugin or setting in a plugin. Applicable if the plugin expects a integer or decimal.
AllowedValues: A comma seperated string of values that are allowed. If empty or not specified it allows everything.
Default: If a default value is available it is specified here. For example in GaussianBlur the Sigma-querystring is not mandatory. In the config file you can specify the default setting that is used.

Furthermore I've added the option "AllowPresetsOny" in the preset setting. If implemented I would recommend every new site to use this option. Furthermore I would recommend turning each plugin off by default and let a developer who's using it turn individual plugins on. This is a defensive approach to the DOS-problem and is probably the best option.

The whole processing.config can look something like the attachment (It became bit messy due to my comments)

With this processing.config you can lock down ImageProcessor as far as you want.

Looking forward to your reaction,
Jeffrey

JeffreyPerplex on 10 May 2016

Sorry @JeffreyPerplex just got onto this. From a first glance I think it' going to add a lot of complexity parsing each set of values for each plugin. It'll also mean I have to alter the IWebGraphicProcesor to add the enabled property. I'll have a good think about it.

JimBobSquarePants on 19 May 2016

Hi @JimBobSquarePants, I know it will have impact, but I think the benefits of security definitely outweigh the cons. As said, the possibility of bringing down a site is way too big right now. But I definitely agree that his a some impact on your code. Give a good think, and I'll hear from you :)!

JeffreyPerplex on 20 May 2016

I've been reflecting further on this and I cannot make too may changes without a major version number (Which I cannot do as it breaks the upgrade path for many libraries).

Now that we have the ability to intercept and block calls, as a compromise I think for this version I shall enable only the following processors as default:

AutoRotate
BackgroundColor
Crop (I might be able to ditch this @Shazwazza does Umbraco use it or just ResizeMode.Crop?)
Quality
Resize

That dramatically limits the scope of attack, any additional processors can only be enabled by installing the config and specifically turning them on.

Thoughts?

JimBobSquarePants on 24 May 2016

Hrm, to be honest I'm not sure ;) I'll have to ask jeavon I think, will ping him on twitter

Shazwazza on 24 May 2016

Yes we do use the crop processor in Umbraco

Jeavon on 24 May 2016

Cheers. That stays then...

JimBobSquarePants on 24 May 2016

Ok... So this is the new pipeline.

Processors are locked down to the following by default:

AutoRotate
BackgroundColor
Crop
Format
Quality
Resize

Early in the request we have the ValidatingRequest event handler we can tap into to add custom restrictions and sanitize the request. That can cancel the request.

If the request passes, anything that makes an image request containing a querystring that does not match the above processors will throw a 400 exception. InterceptAll mode will still let requests without querystring parameters through.

Installing the config package will not enable additional processors. You have to manually enable each one by adding the enable="true" attribute to the processor configuration.

If you think that is ok then I am ready to ship once I finish writing documentation. @Shazwazza @Jeavon I want to check that my 400 exception is not going to cause trouble in Umbraco, are there image requests made that contain a single querystring parameter that is not part of my list? A timestamp perhaps?

JimBobSquarePants on 5 Jun 2016

Not sure off the top of my head, @Jeavon would certainly know better. I can have a look in our code this week if he's not sure

Shazwazza on 6 Jun 2016

Thanks!