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

ImageProcessor.Web and DoS #49

Closed
meixger opened this Issue May 23, 2014 · 55 comments

Comments

Projects
None yet
8 participants
@meixger

meixger commented May 23, 2014

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?

@gfyans

This comment has been minimized.

Show comment
Hide comment
@gfyans

gfyans May 23, 2014

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

gfyans commented May 23, 2014

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

@JimBobSquarePants

This comment has been minimized.

Show comment
Hide comment
@JimBobSquarePants

JimBobSquarePants May 23, 2014

Owner

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.

Owner

JimBobSquarePants commented May 23, 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.

@kgiszewski

This comment has been minimized.

Show comment
Hide comment
@kgiszewski

kgiszewski Aug 10, 2015

@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 commented Aug 10, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@kgiszewski

kgiszewski Aug 10, 2015

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

kgiszewski commented Aug 10, 2015

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

@JimBobSquarePants

This comment has been minimized.

Show comment
Hide comment
@JimBobSquarePants

JimBobSquarePants Aug 11, 2015

Owner

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.

this.Processor.Settings.TryGetValue("RestrictTo", out restrictions);

Owner

JimBobSquarePants commented Aug 11, 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.

this.Processor.Settings.TryGetValue("RestrictTo", out restrictions);

@kgiszewski

This comment has been minimized.

Show comment
Hide comment
@kgiszewski

kgiszewski Aug 11, 2015

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

kgiszewski commented Aug 11, 2015

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

@kgiszewski

This comment has been minimized.

Show comment
Hide comment
@kgiszewski

kgiszewski Aug 11, 2015

Confirmed it works. Thank you.

kgiszewski commented Aug 11, 2015

Confirmed it works. Thank you.

@kgiszewski

This comment has been minimized.

Show comment
Hide comment
@kgiszewski

kgiszewski Aug 11, 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 commented Aug 11, 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

This comment has been minimized.

Show comment
Hide comment
@kgiszewski

kgiszewski Aug 11, 2015

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

kgiszewski commented Aug 11, 2015

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

@kgiszewski

This comment has been minimized.

Show comment
Hide comment
@kgiszewski

kgiszewski Aug 11, 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 commented Aug 11, 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?

@JimBobSquarePants

This comment has been minimized.

Show comment
Hide comment
@JimBobSquarePants

JimBobSquarePants Aug 11, 2015

Owner

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.

public static ImageFactory AutoProcess(this ImageFactory factory, string queryString)

Owner

JimBobSquarePants commented Aug 11, 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.

public static ImageFactory AutoProcess(this ImageFactory factory, string queryString)

JimBobSquarePants added a commit that referenced this issue Aug 21, 2015

Throw 403 When incorrect resize is attempted #49
This ensures that there are no cached files generated when attempted
resizing events are out with restricted sizes.
@JimBobSquarePants

This comment has been minimized.

Show comment
Hide comment
@JimBobSquarePants

JimBobSquarePants Aug 21, 2015

Owner

Will throw 403 in next release.

Owner

JimBobSquarePants commented Aug 21, 2015

Will throw 403 in next release.

@kgiszewski

This comment has been minimized.

Show comment
Hide comment
@kgiszewski

kgiszewski Aug 21, 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 commented Aug 21, 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 :)

@JimBobSquarePants

This comment has been minimized.

Show comment
Hide comment
@JimBobSquarePants

JimBobSquarePants Aug 21, 2015

Owner

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. :)

Owner

JimBobSquarePants commented Aug 21, 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. :)

@kgiszewski

This comment has been minimized.

Show comment
Hide comment
@kgiszewski

kgiszewski Aug 21, 2015

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

kgiszewski commented Aug 21, 2015

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

@JimBobSquarePants

This comment has been minimized.

Show comment
Hide comment
@JimBobSquarePants

JimBobSquarePants Aug 21, 2015

Owner

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

Owner

JimBobSquarePants commented Aug 21, 2015

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

@kgiszewski

This comment has been minimized.

Show comment
Hide comment
@kgiszewski

kgiszewski Aug 21, 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 commented Aug 21, 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 :)

@JimBobSquarePants

This comment has been minimized.

Show comment
Hide comment
@JimBobSquarePants

JimBobSquarePants Aug 21, 2015

Owner

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

using (MemoryStream inStream = new MemoryStream(imageBuffer))

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

Owner

JimBobSquarePants commented Aug 21, 2015

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

using (MemoryStream inStream = new MemoryStream(imageBuffer))

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

@kgiszewski

This comment has been minimized.

Show comment
Hide comment
@kgiszewski

kgiszewski Aug 21, 2015

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

kgiszewski commented Aug 21, 2015

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

@JimBobSquarePants

This comment has been minimized.

Show comment
Hide comment
@JimBobSquarePants

JimBobSquarePants Aug 22, 2015

Owner

Ace thanks!

Owner

JimBobSquarePants commented Aug 22, 2015

Ace thanks!

nul800sebastiaan pushed a commit to nul800sebastiaan/ImageProcessor that referenced this issue Feb 3, 2016

Throw 403 When incorrect resize is attempted JimBobSquarePants#49
This ensures that there are no cached files generated when attempted
resizing events are out with restricted sizes.
@Shazwazza

This comment has been minimized.

Show comment
Hide comment
@Shazwazza

Shazwazza Mar 21, 2016

Contributor

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!

Contributor

Shazwazza commented Mar 21, 2016

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!

@JimBobSquarePants

This comment has been minimized.

Show comment
Hide comment
@JimBobSquarePants

JimBobSquarePants Mar 21, 2016

Owner

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

Owner

JimBobSquarePants commented Mar 21, 2016

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

@Shazwazza

This comment has been minimized.

Show comment
Hide comment
@Shazwazza

Shazwazza Mar 22, 2016

Contributor

@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 ?

Contributor

Shazwazza commented Mar 22, 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 ?

@JimBobSquarePants

This comment has been minimized.

Show comment
Hide comment
@JimBobSquarePants

JimBobSquarePants Mar 22, 2016

Owner

@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.

Owner

JimBobSquarePants commented Mar 22, 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.

@Jeavon

This comment has been minimized.

Show comment
Hide comment
@Jeavon

Jeavon Mar 22, 2016

Collaborator

@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!

Collaborator

Jeavon commented Mar 22, 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!

@JeffreyPerplex

This comment has been minimized.

Show comment
Hide comment
@JeffreyPerplex

JeffreyPerplex May 20, 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 commented May 20, 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 :)!

@JimBobSquarePants

This comment has been minimized.

Show comment
Hide comment
@JimBobSquarePants

JimBobSquarePants May 24, 2016

Owner

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?

Owner

JimBobSquarePants commented May 24, 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?

@Shazwazza

This comment has been minimized.

Show comment
Hide comment
@Shazwazza

Shazwazza May 24, 2016

Contributor

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

Contributor

Shazwazza commented May 24, 2016

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

@Jeavon

This comment has been minimized.

Show comment
Hide comment
@Jeavon

Jeavon May 24, 2016

Collaborator

Yes we do use the crop processor in Umbraco

Collaborator

Jeavon commented May 24, 2016

Yes we do use the crop processor in Umbraco

@JimBobSquarePants

This comment has been minimized.

Show comment
Hide comment
@JimBobSquarePants

JimBobSquarePants May 24, 2016

Owner

Cheers. That stays then...

Owner

JimBobSquarePants commented May 24, 2016

Cheers. That stays then...

@JimBobSquarePants

This comment has been minimized.

Show comment
Hide comment
@JimBobSquarePants

JimBobSquarePants Jun 5, 2016

Owner

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?

Owner

JimBobSquarePants commented Jun 5, 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?

@Shazwazza

This comment has been minimized.

Show comment
Hide comment
@Shazwazza

Shazwazza Jun 6, 2016

Contributor

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

Contributor

Shazwazza commented Jun 6, 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

@JimBobSquarePants

This comment has been minimized.

Show comment
Hide comment
@JimBobSquarePants
Owner

JimBobSquarePants commented Jun 6, 2016

Thanks!

@Jeavon

This comment has been minimized.

Show comment
Hide comment
@Jeavon

Jeavon Jun 6, 2016

Collaborator

@JimBobSquarePants so something like /images/mybg.jpg?v=0.0.1?

Collaborator

Jeavon commented Jun 6, 2016

@JimBobSquarePants so something like /images/mybg.jpg?v=0.0.1?

@JimBobSquarePants

This comment has been minimized.

Show comment
Hide comment
@JimBobSquarePants

JimBobSquarePants Jun 6, 2016

Owner

@Jeavon yeah, something like that. That would trigger the processor and cache previously. It would be possible to create a noop processor to allow it per server if necessary.

Owner

JimBobSquarePants commented Jun 6, 2016

@Jeavon yeah, something like that. That would trigger the processor and cache previously. It would be possible to create a noop processor to allow it per server if necessary.

@Jeavon

This comment has been minimized.

Show comment
Hide comment
@Jeavon

Jeavon Jun 6, 2016

Collaborator

We don't have anything which does that specifically in Umbraco but cache busting querystrings are pretty common practice. I would certainly allow "v" and "rnd", perhaps a noop processor where variables can be easily added? IP caching with only cache busting variables is good and is very useful when using the Azure Blob cache.

Collaborator

Jeavon commented Jun 6, 2016

We don't have anything which does that specifically in Umbraco but cache busting querystrings are pretty common practice. I would certainly allow "v" and "rnd", perhaps a noop processor where variables can be easily added? IP caching with only cache busting variables is good and is very useful when using the Azure Blob cache.

@JimBobSquarePants

This comment has been minimized.

Show comment
Hide comment
@JimBobSquarePants

JimBobSquarePants Jun 6, 2016

Owner

How to do that without making it an attack vector though?

Owner

JimBobSquarePants commented Jun 6, 2016

How to do that without making it an attack vector though?

@Jeavon

This comment has been minimized.

Show comment
Hide comment
@Jeavon

Jeavon Jun 6, 2016

Collaborator

True, caching of these is not a massive advantage. My Azure CDN Toolkit will resolve asset paths directly that are not being cached by ImageProcessor anyway but the requests with only "v" or "rnd" querystring variables will need to be allowed and ignored by Image Processor.

Collaborator

Jeavon commented Jun 6, 2016

True, caching of these is not a massive advantage. My Azure CDN Toolkit will resolve asset paths directly that are not being cached by ImageProcessor anyway but the requests with only "v" or "rnd" querystring variables will need to be allowed and ignored by Image Processor.

@JimBobSquarePants

This comment has been minimized.

Show comment
Hide comment
@JimBobSquarePants

JimBobSquarePants Jun 6, 2016

Owner

Hows about that?

Owner

JimBobSquarePants commented Jun 6, 2016

Hows about that?

@Jeavon

This comment has been minimized.

Show comment
Hide comment
@Jeavon

Jeavon Jun 6, 2016

Collaborator

Looks fab!

Collaborator

Jeavon commented Jun 6, 2016

Looks fab!

@JimBobSquarePants

This comment has been minimized.

Show comment
Hide comment
@JimBobSquarePants

JimBobSquarePants Jun 6, 2016

Owner

Ace.... Just docs to write now.... groan 😢

Owner

JimBobSquarePants commented Jun 6, 2016

Ace.... Just docs to write now.... groan 😢

@Jeavon

This comment has been minimized.

Show comment
Hide comment
@Jeavon

Jeavon Jun 6, 2016

Collaborator

👍 This is going to be quite breaking for developers using processors in Umbraco which will now be disabled by default (.e.g. RoundedCorners), I'm guessing this new ImageProcessor version will at minimum be a dependency on Umbraco v7.5? /cc @Shazwazza @nul800sebastiaan

Collaborator

Jeavon commented Jun 6, 2016

👍 This is going to be quite breaking for developers using processors in Umbraco which will now be disabled by default (.e.g. RoundedCorners), I'm guessing this new ImageProcessor version will at minimum be a dependency on Umbraco v7.5? /cc @Shazwazza @nul800sebastiaan

@JimBobSquarePants

This comment has been minimized.

Show comment
Hide comment
@JimBobSquarePants

JimBobSquarePants Jun 7, 2016

Owner

New version now released 💥

Thanks everyone for their input. These kinds of conversations are awesome!

Owner

JimBobSquarePants commented Jun 7, 2016

New version now released 💥

Thanks everyone for their input. These kinds of conversations are awesome!

JimBobSquarePants added a commit that referenced this issue Oct 27, 2016

Throw 403 When incorrect resize is attempted #49
This ensures that there are no cached files generated when attempted
resizing events are out with restricted sizes.


Former-commit-id: fdb4223
Former-commit-id: 92f7279a2cee740fbbc4fcfb9ee6353d2b592c18

JimBobSquarePants added a commit that referenced this issue Oct 27, 2016

Throw 403 When incorrect resize is attempted #49
This ensures that there are no cached files generated when attempted
resizing events are out with restricted sizes.


Former-commit-id: 587af11
Former-commit-id: 780fdd132e5fa8a752de49bb6ea12f5bfcbd2769

JimBobSquarePants added a commit that referenced this issue Oct 27, 2016

Disable most web processors by default
Fix #49


Former-commit-id: e6ffe71
Former-commit-id: 30b9cc4cc05b4be574d047da8305345b753b3d14

JimBobSquarePants added a commit that referenced this issue Oct 27, 2016

Allow optional cachebuster touch #49
Former-commit-id: 877cc95
Former-commit-id: f2c82c9ff40a391766c0a8b94d384fb10fa9d639
@blowsie

This comment has been minimized.

Show comment
Hide comment
@blowsie

blowsie Jan 17, 2018

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 commented Jan 17, 2018

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?

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