Moodle

user may not have access to quiz backup course files area when exporting questions

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Fixed
  • Affects Version/s: 1.9
  • Fix Version/s: 1.9.2
  • Component/s: Questions, Quiz
  • Labels:
    None
  • Affected Branches:
    MOODLE_19_STABLE
  • Fixed Branches:
    MOODLE_19_STABLE

Description

When a user has the moodle/question:view capability for a question, they can export it. However, unless they also have have moodle/site:backup, they will not be able to download the exported file which is saved in the backupdata area of course files. (this is because normally, you don't want students to be able to download questions.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Tim Hunt added a comment -

My suggested fix for this is as follows:

Around line 77 of file.php, where it says:

// security: only editing teachers can access backups
if (!has_capability('moodle/site:backup', ...

And an "or has_capability('moodle/question:view', ...". This will let people download question export files they have created. However, we will not change file/index.php, so that still only people with moodle/site:backup will be able to browse that folder.

This is not properly secure because it will let people with moodle/question:view download course backups, if they can guess the file name. However, those file names are hard to guess.

I was going to say that I did not think this was a very big security risk, because file accesses are logged, so if someone is naughty, we can catch them. However, now I look, I find that they are not logged. Weird. Sam wrote some code to log file accesses in our code. I told him to put a patch in the tracker.

Of course, in Moodle 2.0, we will be able to fix this securely with the repository API, using the access control lists it provides, but I think it is important to get this fixed in 1.9.x

Petr, please can you review this suggestion with your security hat on. Thanks.

Show
Tim Hunt added a comment - My suggested fix for this is as follows: Around line 77 of file.php, where it says: // security: only editing teachers can access backups if (!has_capability('moodle/site:backup', ... And an "or has_capability('moodle/question:view', ...". This will let people download question export files they have created. However, we will not change file/index.php, so that still only people with moodle/site:backup will be able to browse that folder. This is not properly secure because it will let people with moodle/question:view download course backups, if they can guess the file name. However, those file names are hard to guess. I was going to say that I did not think this was a very big security risk, because file accesses are logged, so if someone is naughty, we can catch them. However, now I look, I find that they are not logged. Weird. Sam wrote some code to log file accesses in our code. I told him to put a patch in the tracker. Of course, in Moodle 2.0, we will be able to fix this securely with the repository API, using the access control lists it provides, but I think it is important to get this fixed in 1.9.x Petr, please can you review this suggestion with your security hat on. Thanks.
Hide
Permalink
Tim Hunt added a comment -

Actually, we can do a better job than this. Quiz exports are actually stored in /backupdata/quiz, so we just need to allow download from there if the user has moodle/question:view, and security is much less of a problem.

Show
Tim Hunt added a comment - Actually, we can do a better job than this. Quiz exports are actually stored in /backupdata/quiz, so we just need to allow download from there if the user has moodle/question:view, and security is much less of a problem.
Hide
Permalink
Eloy Lafuente (stronk7) added a comment -

Yup. I think separate dirs is definitively better! In fact, if they are disjoint dirs... BETTER. FYC.

Ciao

Show
Eloy Lafuente (stronk7) added a comment - Yup. I think separate dirs is definitively better! In fact, if they are disjoint dirs... BETTER. FYC. Ciao
Hide
Permalink
Tim Hunt added a comment -

Well the directory names are just what the code currently uses, and I don't really want to change it - at least until we move to the repository API. Changing would probably just confuse users on a stable branch.

Show
Tim Hunt added a comment - Well the directory names are just what the code currently uses, and I don't really want to change it - at least until we move to the repository API. Changing would probably just confuse users on a stable branch.
Hide
Permalink
Petr Škoda (skodak) added a comment -

my 10 for this it is IMO a major security problem, backup contains very sensitive info including password hashes, preventing browsing is definitely not enough because the filenames are in expected format

I think there should be a different folder for this

Show
Petr Škoda (skodak) added a comment - my 10 for this it is IMO a major security problem, backup contains very sensitive info including password hashes, preventing browsing is definitely not enough because the filenames are in expected format I think there should be a different folder for this
Hide
Permalink
Petr Škoda (skodak) added a comment -

sending a patch with send_temp_file() that we discussed today..

Show
Petr Škoda (skodak) added a comment - sending a patch with send_temp_file() that we discussed today..
Hide
Permalink
Petr Škoda (skodak) added a comment -

the idea is to create the export file in moodledata/temp/export/quiz/userid/something and use send_temp_file() which deletes the file after sending to user,
benefits are:

  • we no not have to care about security much
  • no wasting of space in moodledata
  • simple use in various export scripts
Show
Petr Škoda (skodak) added a comment - the idea is to create the export file in moodledata/temp/export/quiz/userid/something and use send_temp_file() which deletes the file after sending to user, benefits are:
  • we no not have to care about security much
  • no wasting of space in moodledata
  • simple use in various export scripts
Hide
Permalink
Tim Hunt added a comment -

Looks good to me. Please can we put this in 1.9 and 2.0, then I will do the rest.

Show
Tim Hunt added a comment - Looks good to me. Please can we put this in 1.9 and 2.0, then I will do the rest.
Hide
Permalink
Tim Hunt added a comment -

Oh, just one other thought. To make it even easier for developers, should we make another function something like

get_temp_file_name($mod, $userid = 0) {\
global $CFG, $USER;
if (!$userid) {
$userid = $USER->id;
}
$folder = "$CFG->dataroot/temp/export/$mod/$userid";
ensure_directory_exists($folder);
return tempnam($folder, '');

(Sorry can't remember if our function is actually called ensure_directory_exists.)

This whole scheme will leave lots of empty directories lying around?

Show
Tim Hunt added a comment - Oh, just one other thought. To make it even easier for developers, should we make another function something like get_temp_file_name($mod, $userid = 0) {\ global $CFG, $USER; if (!$userid) { $userid = $USER->id; } $folder = "$CFG->dataroot/temp/export/$mod/$userid"; ensure_directory_exists($folder); return tempnam($folder, ''); (Sorry can't remember if our function is actually called ensure_directory_exists.)
This whole scheme will leave lots of empty directories lying around?
Hide
Permalink
Tim Hunt added a comment -

Petr, where have we got to with this? What is preventing us from committing your patch? Thanks.

Show
Tim Hunt added a comment - Petr, where have we got to with this? What is preventing us from committing your patch? Thanks.
Hide
Permalink
Petr Škoda (skodak) added a comment -

send temp file support in cvs

Show
Petr Škoda (skodak) added a comment - send temp file support in cvs
Hide
Permalink
Tim Hunt added a comment -

OK, the attached patch should finish off this bug. The patch is against 1.9 stable. Please can you review it, and in particular say whether you think this should go in before or after 1.9.2.

It does not change behaviour for people with 'moodle/site:backup' capability. For people without it, it saves the export file in a temporary area, and automatically starts the download using JavaScript with a fallback link, via a new question/exportfile.php stript.

Some security is provided because the temp file name contains the $USER->id, but the question/exportfile.php URL does not.

If this does go in to 1.9.2, I will merge it to head, and file a new track issue to re-write this bit of code using the file API, once that is done.

Show
Tim Hunt added a comment - OK, the attached patch should finish off this bug. The patch is against 1.9 stable. Please can you review it, and in particular say whether you think this should go in before or after 1.9.2. It does not change behaviour for people with 'moodle/site:backup' capability. For people without it, it saves the export file in a temporary area, and automatically starts the download using JavaScript with a fallback link, via a new question/exportfile.php stript. Some security is provided because the temp file name contains the $USER->id, but the question/exportfile.php URL does not. If this does go in to 1.9.2, I will merge it to head, and file a new track issue to re-write this bit of code using the file API, once that is done.
Hide
Permalink
Petr Škoda (skodak) added a comment -

seems ok, +1 for commit,
I am really looking forward to the new file API that should solve all these problems nicely

thanks

Show
Petr Škoda (skodak) added a comment - seems ok, +1 for commit, I am really looking forward to the new file API that should solve all these problems nicely thanks
Hide
Permalink
Tim Hunt added a comment -

Note that once the Files API is finished, this fix will need to be redone. I have created MDL-15573 to track that.

Show
Tim Hunt added a comment - Note that once the Files API is finished, this fix will need to be redone. I have created MDL-15573 to track that.

People

Vote (0)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4.3#663-r165197) | Report a problem
Powered by a free Atlassian JIRA open source license for Moodle. Try JIRA - bug tracking software for your team.