U4-9959 - SPIKE: ImageProcessor DOS - Update GetCropUrl

Created by Jeffrey Schoemaker 26 May 2017, 14:08:37 Updated by David Peck 06 Jul 2018, 08:35:50

Subtask of: U4-9609

Since a long time the possibility to DOS via ImageProcessor a website exists in every Umbraco instance. By simpling iterating over each possible querystring-parameter of an image (for example; width, height, but also watermark, alpha, etcetera) it's possible to let the CPU of an instance spike and flood the disk with cached images.

This is related to the discussion we've had over here: https://github.com/JimBobSquarePants/ImageProcessor/issues/49. In Umbraco 7.3 or 7.4 the configfiles for ImageProcessor got locked down a bit, so that by default you cannot use some advanced parameters like alpha, blur, etcetera, but the still the vulnerability of course exists. A short demo that I did during the German UmbracoFestival can be found here (https://slideslive.com/38900783/secure-your-umbraco; around minute 33:00)

How we've prevented this attack in our code is the following:

  • Add a security token to each image that uses ImageProcessor-parameters
  • Check every imagerequest that has parameters if the generated token is valid. If not it throws an error, if no parameters are specified (thus a request to the original image) it just returns the original image

We use our own function to create image output, based on a CropDataSet or a MediaId and in that function we add the generated token (based on a hash of the image, the query strings, and a salt specified in the web.config). But I think most of the developers use the default function GetCropUrl() that comes with Umbraco and I think that function should have this logic implemented as well.

I've attached some files that we're using, but you cannot use them probably 1-on-1 because we've added some of our own logic ourselves that you will have to strip. I've attached:

  • ImageProcessorSecurityTokenCheck.cs; An event that checks whether the token is there and if it needs to be checked. When you’re in /umbraco/ (editing in the backend, and maybe resizing pictures in the richtext editor or grid) it doesn’t trigger. ** It uses a function PerplexLib.Security.Hash(pathAndQuerystring, PictureHelper.Salt).Hash that hashes the querystring together with a hash that is specified in the web.config. The hash-function is encapsulated in another dll, but it simple hashes in based on SHA-512 I guess.
  • PictureHelper.cs: A function that creates our images based on an ImageCrop or an ImageId. It’s a really big function with a lot of custom logic that applies to our own picture-rendering, but the important stuff happens in the functions: ** AddSecurityToken ** GetSecurityToken ** AddTokenToAllImages

And of course there is an event registered OnApplicationStarting:

void IApplicationEventHandler.OnApplicationStarting(UmbracoApplicationBase umbracoApplication, ApplicationContext applicationContext) { ImageProcessingModule.ValidatingRequest += ImageProcessorSecurityTokenCheck.ValidatingRequest; }

Finally we’ve added some code that is implemented for the PropertyValueConverters for the grid and the rte:

namespace PerplexBasis.Code.Umbraco.PropertyValueConverters { public class PerplexGridValueConverter : GridValueConverter { public override object ConvertSourceToObject(PublishedPropertyType propertyType, object source, bool preview) { // Call the method form the base class JToken json = base.ConvertSourceToObject(propertyType, source, preview) as JToken; if (json == null) return json;

        // Add security token to all <img> tags to block DOS attack on ImageProcessor
        return JToken.Parse(PictureHelper.AddTokenToAllImages(json.ToString()));
    }
}

}

I know it's not the nicest issue you will receive this week, but I think we should implement this due security issues.

Regards, Jeffrey

3 Attachments

Download ImageProcessorSecurityTokenCheck.cs

Download PictureHelper.cs

Comments

Sebastiaan Janssen 26 May 2017, 14:53:49

Cool! Makes sense, so it's an antiforgery token for ImageProcessor?

Haven't looked into the code yet and won't have time until after codegarden but it sounds like a good fix!


Jeffrey Schoemaker 29 May 2017, 06:52:04

You can look at it like it's a AntiForgerytoken, but the token will be the same for each request.

For an example implementation you can look at https://www.perplex.nl and for example the image on the homepage; https://www.perplex.nl/media/2096/perplex-pand.jpg?width=480&quality=80&token=QzGMQl06Mu. With every request it regenerates the token based on the querystrings and if it matches it returns the image, and if not (if you change the width to 490 or add another querystring ) it returns an empty response.


Sebastiaan Janssen 31 May 2017, 09:16:01

@jeffrey.schoemaker@perplex.nl Maybe I'm missing the point here but how is this different from the built-in functionality where you define presets to limit the actions you can perform with imageprocessor to pre-defined ones?


David Peck 31 May 2018, 12:48:30

I've implemented an almost identical similar system, which I was going to release as a package but if a core PR is an option I'd gladly provide one. The only real difference is I used MD5 hashing because it's super quick and I don't think collisions/security are a major concern.

@sebastiaan this is different from where you define presets because the presets require configuration. Not everyone will know how to do this, especially those more front-end minded. In our case we're using src-set which generates 4 different image sizes per image location, with loads of different versions of dimensions, so presets will be a right pain. Crucially though, I think we want to provide security to those that perhaps don't have it in mind.


Nicholas Westby 14 Jun 2018, 16:45:24

This would throw a wrench in some existing installs. What we do is generate an image using GetCropUrl (and some functionality on top of that to massage the query string a bit), then we pass that off to the frontend. The frontend then alters the query string for the width of the image depending on the current size of the container the image is in (which changes on the frontend depending on the browser's viewport size). It's one strategy we use to implement responsive images.

If this were implemented as I think it's been described, it would prevent those images in which the query string gets altered by JavaScript on the client side browsers from working.

I'm not sure of a good solve for that while being mindful of security, but I wanted to mention it as it's a very real use case.


David Peck 19 Jun 2018, 07:58:42

Other than the speed improvements offered by MD5, my only other suggested change to Jeffrey's code is that it would be good having to set a unique salt. I'd suggest instead as a default value (cached): var salt = MachineKey.Protect("Umbraco");

I think it is a reasonable assumption that a machine key is unique and shared between load balanced servers. Documentation would be needed however, and perhaps retain the AppSetting as an override.


Ronald Barendse 19 Jun 2018, 14:56:36

@david I would argue that MachineKey.Protect(byte[] userData, params string[] purposes) with the correct inputs alone would suffice... The user data would be a unique value for the image itself (e.g. file name, hash or last change date) and the purposes based on the URL or query string parameters. No need to generate and keep another seed value besides the machine key! See MSDN: https://msdn.microsoft.com/en-us/library/system.web.security.machinekey.protect(v=vs.110).aspx.

Performance should not be a problem, since this API is used in lots of other places (HTML anti-forgery tokens, Web Forms view state, etc.). The bottleneck would probably be in generating a unique value for the image, so you can't just re-use the same query strings on other images...

If the incoming request doesn't have a valid token, it should probably return a 400 Bad Request (instead of an empty response).

ImageProcessor already allows for validating the incoming requests, so for now, you would need to generate the URLs yourself (as shown above). If Umbraco could add an event or something like an ICropUrlProvider for the all crop URL generating methods, that would be awesome and provide additional extension points.


David Peck 05 Jul 2018, 09:23:31

I see your point about MachineKey.Protect but I've done a few tests and MachineKey.Protect is ''much'' slower than MD5.

Totally agree about the 400 HTTP error code.

I think an ICropUrlProvider would be awesome. The only problem I see with this is the weight of the PR required, and the resultant testing. If there is an argument that an ICropUrlProvider would be of a wider benefit then perhaps it would be worth the effort in initially creating a PR for this and then a follow-up PR with the validation check.

The major drawback that I see with all of this is the requirement to crop urls added to the RTE. This is going to require a AJAX request and potentially HTML parsing/DOM manipulation.


David Peck 05 Jul 2018, 09:25:43

Encryption vs Hashing speeds


Nicholas Westby 05 Jul 2018, 15:48:44

Doesn't seem all that slow. If I calculated it correctly, each MachineKey.Protect operation takes roughly one hundredth of a millisecond (in other words, can do 100,000 per second). Slower doesn't make it slow.


David Peck 06 Jul 2018, 08:35:50

That's true it isn't slow (but it is 15 times slower). I suppose it is a question of weighing up a simpler solution (not having to set a unique value per installation), against what is probably an inconsequential performance hit. Personally I hate accepting a performance hit, but I can absolutely see a pragmatic argument for doing so.

Potentially a separate issue is whether you'd need the ability to handle a load balanced setup where the servers have different machine keys. I've always set them up to use the same machine key, but would everyone? It might be a dangerous assumption. If we need to handle the ability for a user to use a configuration option to override the MachineKey usage, then I can't see an argument for using MachineKey.Protect.


Priority: Task - Pri 1

Type: Bug

State: Open

Assignee:

Difficulty: Normal

Category: Security

Backwards Compatible: True

Fix Submitted:

Affected versions: 7.3.8, 7.4.2, 7.4.3, 7.5.1, 7.5.2, 7.5.3, 7.5.4, 7.5.5, 7.5.6, 7.5.7, 7.5.8, 7.5.9, 7.5.10, 7.5.11, 7.5.12, 7.5.13, 7.6.1

Due in version:

Sprint:

Story Points: 3

Cycle: 5