Allowing modules to restrict file access

Allowing modules to restrict file access

by sam marshall -
Number of replies: 15
Picture of Core developers Picture of Peer reviewers Picture of Plugin developers
Hi,

I want to check in a patch to file.php (and filelib) which allows modules to control file requests (so that a file request goes 'via' the module). The main goal of this patch is so that modules can prevent certain files within their moddata area from being accessible to anyone on that course, but it achieves a few other things too and I think it's a decent change that fits within the current way Moodle works and makes a genuine improvement.

It should have absolutely no impact on any existing code, core or addon, except:
  • Any code that previously referred to URLs like the following: file.php/3/!via/... (where 3 is a course code) will stop working. I am pretty sure there won't be any such code. smile
  • Requests for file.php/3/moddata/somemodule/whatever will take fractionally longer as it requires somemodule/lib.php for that now. (It only needs this for a function_exists check; could be cached if it ever becomes a performance problem.)
If there are no major objections I'll check it in tomorrow. The patch is attached.

The change was previously discussed here - http://moodle.org/mod/forum/discuss.php?d=37878 - but I have modified it to take note of Eloy's security concerns. Eloy's other issue was that really this whole area of the system sucks and should be redesigned properly - he's right, but given the principles of incremental change that we were told were so important, and also the practicality that we'd like to have this in now, I feel this is a good change in the meantime.

Basically what this change achieves is the following:
  • If a frog_process_file_request function exists, then trying to access file.php/13/moddata/frog/anything will give an error (in other words, once a module uses this mechanism, its files are prevented from direct access).
  • Accessing file.php/13/!via/frog/anything will result in a call to the frog_process_file_request function, which will be passed the value of 'anything' and the course number, and can return the full path of a file which the user should be allowed to download, or false if they shouldn't.
  • The return from this function is further checked and modules are only allowed to return:
    • Files that are within the requested course, but are not within moddata (thus preventing access to data of other modules).
    • Files that are within moddata/thismodule on any course.
    • Files that are within dataroot/moddata/thismodule.
The 'via' handling happens after checking that the valid course was provided and require_login etc., but before other security checks, which still apply to the newly-resolved real filename.

By the way, this code is required by the resourcepage module which we'll be using here, as it provides security on file downloads (e.g. if a file is set to only display on a certain date, you can't DL it before, even if you guess the filename). We hope to release the resourcepage module code for public use if anyone else wants it - so I'm trying to review my core changes and get them in now, in order that installing resourcepage doesn't involve running a whole bunch of patches first... This is the largest, I think.

--sam
Average of ratings: -
In reply to sam marshall

Re: Allowing modules to restrict file access

by Matt Oquist -
We might actually be trying to get a real repository into 1.6, but it's definitely a tight schedule.  In any case, we're discussing it over here and I thought you might be interested.
Average of ratings: -
In reply to Matt Oquist

Re: Allowing modules to restrict file access

by sam marshall -
Picture of Core developers Picture of Peer reviewers Picture of Plugin developers
Thanks for the pointer. Yep, I saw the repository stuff but I skimmed it since we aren't - yet - doing integration with our enterprise content management system. I suspect our plan is to see what comes out of that fuss, then judge what if any use we can make of it. I know what's been suggested so far wasn't particularly interesting for us. (E.g. being able to browse a repository and select a file, as an alternative to upload, is OK but doesn't solve any of our near-term integration issues.)

And more importantly :>, I'm not the lead in charge of that project, that's Rod I think... *checks* yes it is! THANK THE GODS etc. smile

Joking aside, passing off access control to a remote repository would achieve some of the same things. But I think there is a sort of fundamental point in a modular 'architecture' that modules should be able to have control over their file downloads, which this rather simple patch gives them.

--sam
Average of ratings: -
In reply to sam marshall

Re: Allowing modules to restrict file access

by Martin Dougiamas -
Picture of Core developers Picture of Documentation writers Picture of Moodle HQ Picture of Particularly helpful Moodlers Picture of Plugin developers Picture of Testers
> But I think there is a sort of fundamental point in a modular 'architecture' that modules should be able to have control over their file downloads

They can run their own file.php if they want  wink
Average of ratings: -
In reply to Martin Dougiamas

Re: Allowing modules to restrict file access

by ellen friedlander -
Martin, is there an easy way to prevent students/customers from downloading files to their systems?  And can you give an answer that an idiot (myself) could understand?
Average of ratings: -
In reply to sam marshall

Re: Allowing modules to restrict file access

by Martin Dougiamas -
Picture of Core developers Picture of Documentation writers Picture of Moodle HQ Picture of Particularly helpful Moodlers Picture of Plugin developers Picture of Testers
Hi, Sam.

I had to check that ! was a valid character in a URL, and it is, so I can add that to the list of things I learned today. smile

All the file handling is currently being examined for a complete revamp in Moodle 2.0, as I explained at the dev meeting. Matt's pointed out the discussion on Repository APIs which is what that is about. The default repository is actually the local filesystem, so it's very relevant.

I totally like the idea that modules should be able to handle their own files, and I like the way your approach extends security without breaking existing modules, but I'm worried about this hack suddenly introducing a whole lot of new URLs that we then have to back-support in future in the new repository API and in backups. It's probably not a huge issue if only one or two modules are using it in 1.6.

I guess I'll go with the majority opinion on this one. if anyone has a real problem with this extension, please post now.
Average of ratings: -
In reply to Martin Dougiamas

Re: Allowing modules to restrict file access

by sam marshall -
Picture of Core developers Picture of Peer reviewers Picture of Plugin developers
Moodle lost my post (I accidentally clicked something and when I clicked Back it didn't have my text) so I'm doing a briefer version this time...

1) For repositories and back-porting - since this is actually only a translation that maps onto existing paths (course/moddata/whatever), presuming that there is going to be support for those existing paths, it should be OK.

2) Backup is only an issue if the global shared folder is used, I think it should be semantically defined that (course) backup doesn't include such shared data. There are definitely issues about that which this patch doesn't claim to solve. But those apply however you implement this, i.e. whether or not this change happens. Maybe it's more like a health warning about using the feature.

3) I doubt anyone else will use it soon. smile

I'm going to check this in tomorrow if there aren't any major objections.
Average of ratings: -
In reply to sam marshall

Re: Allowing modules to restrict file access

by Michael Penney -
What we were planning for backup on MyFiles is to keep the same links, but rewriting the course id# to the new course id#.

These would not be shared in the restored course (if restored to a new course), unless the teacher goes in and re-shares those links to the new course, in which case they start working again.

Of course this is for a personal file system rather than a central repository, and the idea was for the original owner to keep control of where their files are used.
Average of ratings: -
In reply to Martin Dougiamas

Re: Allowing modules to restrict file access

by Petr Skoda -
Picture of Core developers Picture of Documentation writers Picture of Peer reviewers Picture of Plugin developers
I do not like this approach. file.php is already too hackish, it is not good to add more tweaks into it. I have told it here several times before that IMO there is only one way out of it: move out module related functions from file.php, modules would have full cotrol over modfile.php. At the same time we should move moddata folders out of course files to separate course and module files.
Lots of present problems are related to storage of module files in course files area (== out of moddata folder) - for example resources, quiz images, ... Modules do not know which files belong to them.

file.php would be serving files from local disk or any other DMS with it's own much simpler security implementation (only people, groups, dates, teacher roles, etc). File.php would serve only trusted content, ordinary students or guests must not be allowed to upload files there. We could allow student uploads only if the files would be served from different ww address - unfortunately not everybody can afford it sad
At the top level would be courses plus teacher folders. Automatic relinking of course files would be preserved, relinking to teacher folders might be solved by aliases (something like fileserver shares). Teacher folders would have to be backuped separately.

modfile.php would either serve files from disk (the same as curent file.php) or rely on modules to supply file content as file handle, in variable or url redirection (from db, hive, local file system, etc, ...). Modfile would also serve untrusted files, I have already described some mechanism at Security Center.

I have already written modfile.php; file.php would be cleaned a bit; backups would need minor changes too; we need some simple file browser for moddata area (and editor integration) and maybe some userfile.php - that's all.

If there is anybody willing to fund this, please let me know wink

skodak
Average of ratings: -
In reply to Petr Skoda

Re: Allowing modules to restrict file access

by sam marshall -
Picture of Core developers Picture of Peer reviewers Picture of Plugin developers
Well, I don't really have a specific criticism of your idea, but I don't believe my changes are hacky (any more than the rest of Moodle's module interface). Instead they provide a clear, single control point that allows modules access to restrict/control the serving of files either from their global or course-based moddata folder.

In fact, these changes could allow removal of some of the existing cruft in file.php (I don't plan to do that, but somebody could), and should stop the need to include any more. (Basically at present if any module needs to stop people downloading its files, it needs to edit file.php. That's why there is so much crap in there. Now if you want to stop your files being downloaded all you need to do is make sure they're in your moddata folder and create a module_process_file_request function.)

Not a hack: a clean incremental improvement.

(Oh, by the way - the code was reviewed by others here, mostly Tim, so it should be pretty good quality. He did make me fix one inelegant part. smile

--sam
Average of ratings: -
In reply to sam marshall

file.php in Moodle 2.0

by Petr Skoda -
Picture of Core developers Picture of Documentation writers Picture of Peer reviewers Picture of Plugin developers
file.php is one big hack - I know it because I have written most of the code wink
Your modification is a nice hack, but it is a hack that tries to work around design limitations of file.php itself. It solves only one small problem and leaves all the great ones. Would it not be easier to fix the design of file.php in 2.0?

Please do not put your code above into main CVS.


Some more info about my proposal:

file.php

usage:
  • file.php/#courseid/some/uploaded/course/file/image.jpg - normal relinking during backup/restore
  • file.php/staff/#userid/their/own/data/file.txt - not relinked at all
  • file.php/shares/#sharename/data/file.txt - the same file as above, only site name relinked
file.php would only care about users and courses - external plugins would only need to have access to user and course information; there would be zero interaction with modules.

You could mark any folder as shared. Course backup/restore would not handle the share content at all, it would only care for content under #courseid. Imagine you could upload those big juicy videos and share them in many courses. If you copy them to another site into share with the same name, links would not be broken after restore :-O

Basic access right for course folders would be public/hidden. In teacher folders we could have "public/all courses I am teaching in/in courses of given teachers/hidden". We do not need to handle file specific rights, but some plugins can.

We could store the files by default in $CFG->dataroot/files, but each DMS would be able to use anything else.

modfile.php

usage:
  • modfile.php/#moduleinstanceid/resource.pdf - file from local filesystem served by not yet written shared resource module
  • modfile.php/#moduleinstanceid/all_assignments.zip - cached copy of all submitted assignments from given instance, generated only when needed
  • modfile.php/#moduleinstanceid/#postid/forum_attachment.jpg?filesessionkey=1dsimf45su7
  • modfile.php/#moduleinstanceid/export/myglossary.xml - glossary export file generated from db data
modfile.php can not be linked directly from html text stored in db, every time the link is constructed on the fly. All student submitted files must be "unlocked" with file session key before viewing (more info at Security Center).

There are various xxxfile.php files around, each doing something a bit different, this way you could only add one function into mod/xxx/lib.php and then link to modfile.php from module ui. Easy, simple, few lines of code.

Security is harcoded into modules, modfile.php does no checks at all. You can find some outdated modfile here.

Each module decides where to store data, the usual place might be:
$CFG->moddataroot = $CFG->dataroot.'/modfiles';

Of course those changes are not compatible with current code, but the already created content is compatible with new file.php.

skodak
Average of ratings: -
In reply to Petr Skoda

Re: file.php in Moodle 2.0

by sam marshall -
Picture of Core developers Picture of Peer reviewers Picture of Plugin developers
OK, I won't check it in. Yay increasing differences... the chances of anyone outside the OU being able to use our module are looking remoter by the day. But whatever.

Your proposal sounds sensible and addresses a few more issues. As long as the modules use the existing send_file (or some improvement of it that handles repositories) then they shouldn't screw up file serving, which was my initial concern on reading it.

--sam
Average of ratings: -
In reply to sam marshall

Re: file.php in Moodle 2.0

by Martin Dougiamas -
Picture of Core developers Picture of Documentation writers Picture of Moodle HQ Picture of Particularly helpful Moodlers Picture of Plugin developers Picture of Testers
Yeah, I think if this was something that wasn't going to be long-term and would probably only benefit a few modules or less in the period between 1.6 and 2.0 (or 1.7) then it makes sense for your module to handle its files using a private script for now.
Average of ratings: -
In reply to Martin Dougiamas

Re: file.php in Moodle 2.0

by sam marshall -
Picture of Core developers Picture of Peer reviewers Picture of Plugin developers
A private script is not a solution (unless we store files outside the course folders) because a key requirement is to *stop* people downloading files... you can't do that without changing file.php.

It's not a particularly big deal, just one more difference in our version. (Should anybody else want to use the module, they'll have to apply some patches, including this one.)

Once there's an 'official' way to do this we might switch the module to use it.
Average of ratings: -
In reply to sam marshall

Re: file.php in Moodle 2.0

by Petr Skoda -
Picture of Core developers Picture of Documentation writers Picture of Peer reviewers Picture of Plugin developers
Why not add modfile.php into 1.6.x CVS, new modules could store its files in dataroot/modfiles/#cmid/ and we could leave the old file.php and modules unchanged. The only problem might be in backup of user data, but I am sure we can solve it too. Later we could convert old modules one by one...
Average of ratings: -
In reply to Petr Skoda

Re: file.php in Moodle 2.0

by sam marshall -
Picture of Core developers Picture of Peer reviewers Picture of Plugin developers
Sounds like a good idea for somebody to do. I don't have time to do my work twice though! So I'll leave it to others.

--sam
Average of ratings: -