Moodle
  1. Moodle
  2. MDL-33950

Make sure that repository_ajax.php correctly validates access control when creating references

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3.2
    • Component/s: Files API
    • Labels:
    • Testing Instructions:
      Hide

      TEST 1

      1. work in one browser as admin and in another as teacher
      2. as admin create two courses and enroll teacher in both of them
      3. as admin create a resource in course A and add files to it
      4. as teacher create a resource in course B and add file from #3 as a copy/reference, open dialogue window "Select file" and wait
      5. as admin unenroll teacher from course A (teacher can no longer access the file he is trying to insert)
      6. as a teacher click 'select' and make sure you can not select a file

      TEST 2
      1. as admin create two courses and enroll teacher only in course B
      2. as admin create a resource in course A and add files to it
      3. as admin create a resource in course B and add file as a reference (alias) to the file from #2.
      4. as teacher create a resource in course B and try to pick a file from #3 (both as copy and a reference)
      5. make sure teacher is able to pick the file. In case of reference he will see '(Undisclosed)' as the source because he can not access course A.

      Repeat both tests with Javascript disabled for teacher (note that user can not create aliases without JS)

      Show
      TEST 1 1. work in one browser as admin and in another as teacher 2. as admin create two courses and enroll teacher in both of them 3. as admin create a resource in course A and add files to it 4. as teacher create a resource in course B and add file from #3 as a copy/reference, open dialogue window "Select file" and wait 5. as admin unenroll teacher from course A (teacher can no longer access the file he is trying to insert) 6. as a teacher click 'select' and make sure you can not select a file TEST 2 1. as admin create two courses and enroll teacher only in course B 2. as admin create a resource in course A and add files to it 3. as admin create a resource in course B and add file as a reference (alias) to the file from #2. 4. as teacher create a resource in course B and try to pick a file from #3 (both as copy and a reference) 5. make sure teacher is able to pick the file. In case of reference he will see '(Undisclosed)' as the source because he can not access course A. Repeat both tests with Javascript disabled for teacher (note that user can not create aliases without JS)
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Rank:
      42057

      Description

      If I read the code right there is no access control validation in repository_ajax.php, see block starting with "if ($usefilereference) {",
      somehow the $source contents should be validated for moodle files and maybe also for external repos.

      This could probably allow stealing of any moodle file.

        Activity

        Hide
        Petr Škoda added a comment -

        problem here there is nothing designed for this kind of access control, file_browser tells deals with virtual addresses (better than nothing), but it does not deal with future dates, it also tells you only if the file can be read or written, linking would have to be added as new feature I am afraid.

        Show
        Petr Škoda added a comment - problem here there is nothing designed for this kind of access control, file_browser tells deals with virtual addresses (better than nothing), but it does not deal with future dates, it also tells you only if the file can be read or written, linking would have to be added as new feature I am afraid.
        Hide
        Petr Škoda added a comment -

        Also there should be extra validation that $source is describing file coming from the give n repository.

        Show
        Petr Škoda added a comment - Also there should be extra validation that $source is describing file coming from the give n repository.
        Hide
        Martin Dougiamas added a comment -

        Can we not check that the source file (if it's in Moodle) is readable by the current user AT THE EXACT TIME we create any reference?

        Show
        Martin Dougiamas added a comment - Can we not check that the source file (if it's in Moodle) is readable by the current user AT THE EXACT TIME we create any reference?
        Hide
        Martin Dougiamas added a comment -

        If this doesn't make 2.3 it's not the end of the world, but we should fix it for 2.3.1 at the latest

        Show
        Martin Dougiamas added a comment - If this doesn't make 2.3 it's not the end of the world, but we should fix it for 2.3.1 at the latest
        Hide
        Marina Glancy added a comment -

        I started to look at it close and found that validation was in fact too strict

        repository::copy_to_area() checked if the file is accessible (by using file_browser/file_info API). But in case when teacher tries to insert a file A that is an alias to file B, it checked the access to file B.

        So I had to re-write copy_to_area() so it does not check the access any more and add function repository::file_is_accessible()

        And I removed repository_recent::copy_to_area() because it is now no different from parent.

        Show
        Marina Glancy added a comment - I started to look at it close and found that validation was in fact too strict repository::copy_to_area() checked if the file is accessible (by using file_browser/file_info API). But in case when teacher tries to insert a file A that is an alias to file B, it checked the access to file B. So I had to re-write copy_to_area() so it does not check the access any more and add function repository::file_is_accessible() And I removed repository_recent::copy_to_area() because it is now no different from parent.
        Hide
        Dan Poltawski added a comment -

        Hi Marina,

        Unless i'm missing something this is wrong:

        $repo = repository::get_repository_by_id($repo_id, $contextid, $repooptions);
        

        $repooptions isn't defined and there isn't a third argument? So seemingly this code path hasn't been executed and needs testing.

        Also:

        • The filesize checks looking for -1 should be changed to use USER_CAN_IGNORE_FILE_SIZE_LIMITS I think (see MDL-27156)
        • get_context_instance_by_id should be context::instance_by_id()
        • Question: Does the default implementation file_is_accessible() (checking against userid) match the existing behaviour?

        So, sending this back. The first point in particular.

        Show
        Dan Poltawski added a comment - Hi Marina, Unless i'm missing something this is wrong: $repo = repository::get_repository_by_id($repo_id, $contextid, $repooptions); $repooptions isn't defined and there isn't a third argument? So seemingly this code path hasn't been executed and needs testing. Also: The filesize checks looking for -1 should be changed to use USER_CAN_IGNORE_FILE_SIZE_LIMITS I think (see MDL-27156 ) get_context_instance_by_id should be context::instance_by_id() Question: Does the default implementation file_is_accessible() (checking against userid) match the existing behaviour? So, sending this back. The first point in particular.
        Hide
        CiBoT added a comment -

        Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

        Show
        CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
        Hide
        Marina Glancy added a comment -

        I added a new commit that creates an instance of repository correctly in JS and non-JS filepickers.

        Default implementation of file_is_accessible() is checking through file_info. But for recent files (at the moment) the files with userid are retrieived (see repository_recent::get_recent_files), so the file_is_accessible() also checks userid.

        Show
        Marina Glancy added a comment - I added a new commit that creates an instance of repository correctly in JS and non-JS filepickers. Default implementation of file_is_accessible() is checking through file_info. But for recent files (at the moment) the files with userid are retrieived (see repository_recent::get_recent_files), so the file_is_accessible() also checks userid.
        Hide
        Dan Poltawski added a comment -

        I've integrated this now.

        Marina - would've been nice to have branches for this, cherry-picking 3 commits is a pain

        Show
        Dan Poltawski added a comment - I've integrated this now. Marina - would've been nice to have branches for this, cherry-picking 3 commits is a pain
        Hide
        Tim Barker added a comment -

        Congrats this has passed testing as per the testing instructions (and a bit more).

        One unrelated thing that I noticed is that when javascript is turned off, Server files was not an available option in the non-js file manager form element. I will check for duplicates and raise this elsewhere.

        Show
        Tim Barker added a comment - Congrats this has passed testing as per the testing instructions (and a bit more). One unrelated thing that I noticed is that when javascript is turned off, Server files was not an available option in the non-js file manager form element. I will check for duplicates and raise this elsewhere.
        Hide
        Dan Poltawski added a comment -

        Congratulations!

        You've made it into the weekly release!

        Thanks for your contribution - here are some random drummers to keep you inspired for the next week!
        http://www.youtube.com/watch?v=_QhpHUmVCmY

        Show
        Dan Poltawski added a comment - Congratulations! You've made it into the weekly release! Thanks for your contribution - here are some random drummers to keep you inspired for the next week! http://www.youtube.com/watch?v=_QhpHUmVCmY
        Hide
        Michael de Raadt added a comment -

        So was this, in fact, a security issue? I need to know for security reporting.

        The issue was reported as a security issue, but then according to Marina's comments, our validation was in fact too strict.

        If the issue was resolved as something different to what it was reported as, it would be good to see its security level changed and its summary corrected.

        Show
        Michael de Raadt added a comment - So was this, in fact, a security issue? I need to know for security reporting. The issue was reported as a security issue, but then according to Marina's comments, our validation was in fact too strict. If the issue was resolved as something different to what it was reported as, it would be good to see its security level changed and its summary corrected.
        Hide
        Marina Glancy added a comment -

        I don't think it is a security issue

        Show
        Marina Glancy added a comment - I don't think it is a security issue
        Hide
        Michael de Raadt added a comment -

        Thanks, Marina.

        Show
        Michael de Raadt added a comment - Thanks, Marina.

          People

          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: