Image security issue, all images are public and ignore capability checks

[brendanheywood]

This filter is really cool and I really want to use - but I can't consider production ready because of the way the images are served.

The way it broadly works is:

1. filter rewrites the img src which points back to this plugins file path
2. browser asks this plugin for the new sized image
3. image is resized on demand and served publicly with no capability checks

The way I think it should work is:

1. filter detects unoptimized image with no alternate sizes, creates an ad hoc task, and then serves original image as-is un optimized. Cache this 'miss' in MUC for a very short time like 2 minutes (or don't cache at all). Some optimizations like the deferred scroll loading could still be performed on the original file.

2. ad hoc tasks processes the image and resizes it and saves it back into the original file area and stores some metadata somewhere that the alternate sizes are available

3. on a later page view, the filter detects the alternate sizes are now available and rewrites to refer to them instead. The urls point to the original file area. Cache this filter metadata lookup 'hit' in MUC for as long as possible.

4. The original plugin (eg mod forum) serves the file and does all it's own checks to see if the student can actually see the file and then serves it.

[gthomas2]

Just letting you know, I've started looking at this issue.

[brendanheywood]

hi @gthomas2 yeah we may have some capacity to help but probably not til well into Jan. We are also potentially coming at this from a video as well as well as image perspective, ie also transcoding several video files in the background in a similar way to the srcset variants.

Option 1

There is still some potential downsides so as always needs a bunch of testing. One thing I have thought of is that if we save the original files back into the filearea then if you come back later to edit, then you will see all the various sizes of files in that area which could be confusing.

Option 2:

The file api callbacks are really clunky as instead of returning a static value like 'denied' or 'server' they actually just serve the file directly, so we can't call the callback on behalf of this plugin to confirm the security checks and then serve it ourselves.

A crazy thought: possibly we could do some php hoop jumping, like point the urls at this plugin, but then dynamically redefine the 'send_stored_file' function via runkit or reflection, then call the original plugins callback eg forum_pluginfile() which does the check based on the original file and arguments, and then calls our new version of 'send_stored_file' which we then internally remap to serve the resized version. This also means we can resize on the fly instead of ad hoc tasks (but would still use tasks for heavy stuff like the video transcoding).

Option 3:

Ultimately it's probably better if this is not done as a filter but moved into moodle core as a specific media resizing api. To me that's the best architecture.

There is a loose connection here with the existing file conversion api in moodle, but it needs to be extended to not only allow conversion to other file types (eg word -> pdf for the assignment marking) but also to different versions of the save file type (eg resize image, transcode video, compress audio etc)

https://docs.moodle.org/dev/File_Converters

[gthomas2]

Option 1 - I don't think it's the end of the world if people go to the file system and see multiple image files, especially if they are all prefixed "resized-" or something.

Option 2 - I've had similar thoughts about how to make this work with some terrible hacky PHP. Sadly, as the old code in lib.php is anything but OOP, we can't use reflection to make it work differently. I tried using ob_start to figure out if access should be given but the problem is that it still sends headers - it basically does not work. I have tried a different approach - aping what core moodle does for checking file access before it calls the module specific plugin file function. It has the disadvantage that we have to whitelist what the plugin is likely to work with and also that the security might not be as granular as it is with the unoptimised image - e.g. for Forums, it won't bother checking hidden posts, etc.. Here's a diff of the branch :

https://github.com/gthomas2/moodle-filter_imageopt/compare/issue7-first-fix-attempt?expand=1

If you could take a look at this and give me your thoughts that would be great.
If this is not a suitable fix then I think option 1 is probably the way to go.

Option 3 - This is giving me some ideas because there are quite a few things that can be done with the file system by using core functionality. There are new call backs for when files are created so we could trigger adhoc resizing tasks at the creation point. Also I think that the file system is plugable now so we could write or own file system plugin which intelligently selects the best image to send. Or we could use reflection to hack the existing file system class.
But yes, overall if we could just do this in core it would be a much more optimal solution.

[brendanheywood]

1. Yeah it's not completely the end of the world if they are visible, but for some use cases it will be quite ugly - like if you are transcoding to a matrix of different sizes and codecs you might get 10 variants to the whole filesystem will be quite cluttered. Responsive images will at most only produce 2 or 3 versions.

2. I think this is worth more investigation:

http://php.net/manual/en/function.runkit-function-redefine.php

If we can just bootstrap moodle, then swap out the definition of send_stored_file and then your plugin calls back to the original one, then I think this will fly and be much simpler and more robust than whitelists.

3. Yes - part of the reason this is on my radar is that we have invested a lot of work into writing an object storage alternate plugin for the filesystem which massively reduces the storage costs, which in turn makes turning moodle into the video repo itself much more appealing instead of relying on 3rd party embeds like youtube. Moodle already serves video quite well (and also offline in the app), it's just the transcoding is a bit of a pain and the manual setup of the video tag with all the various tracks is a minefield for course creators.

https://github.com/catalyst/moodle-tool_objectfs

I don't think the file api is the right place to serve alternate versions of files, it's just not designed that way. The file converter api is much closer to what we need, but would need extending:

https://docs.moodle.org/dev/File_Converters

p.s. no rush, I'm on leave as of tomorrow so will be out of comms until 2nd week jan - have a good break :)

[gthomas2]

Thanks for your thoughts on this Branden. May I wish you happy holidays and a prosperous new year. ;-)

[gthomas2]

@brendanheywood - I've had a go at option 1:

https://github.com/gthomas2/moodle-filter_imageopt/compare/issue7-fix-with-subdir?expand=1

I'm putting the resized images in sub folders. In this way, even if you had 100s of different resized files it wouldn't confuse the user.

[brendanheywood]

happy 2018 @gthomas2 :)

I've only looked at the code so far, but not run it. Looks pretty good but one little thing so far: in filter_imageopt_pluginfile you are redirecting to the real resized image. From a perf point of view we want to avoid redirects especially on high latency networks / mobile. I think it would be better to instead just call the relevant plugins file_pluginfile() with the corrected arguments and remove the redirect. We also need to test the http headers to make sure this is getting reasonable browser caching too.

[gthomas2]

Hi Brendan. I've uploaded some improvements. The redirects should only happen the first time a non-resized image is viewed. However, I think your suggestion of calling the relevant plugins file_pluginfile() function would be better - I'll do that as a separate issue though if that's OK.

[gthomas2]

Hi Brendan. Actually it was easier than I thought to use the core file_pluginfile function instead of a redirect, so I went ahead and did that. The url that uses the filters pluginfile.php should only ever be shown once. As soon as an optimised version of the image is available it will instead just serve images from the plugins url but with /imageopt/[maxwidth] in the url. If you could test this out for me at some point it would massively help me. I really appreciate all your help and comments on this so far by the way. Happy 2018 to you too!

[brendanheywood]

hi @gthomas2 yeah that looks pretty solid :)

Now if it was doing the srcset multiple urls it would be pretty well perfect

[gthomas2]

@brendanheywood - Thanks so much for your help with this, greatly appreciated. I'll add the srcset as a separate issue.