Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Won't Fix
    • Affects Version/s: 1.9.12, 2.0.3
    • Fix Version/s: None
    • Component/s: Backup
    • Labels:
    • Database:
      Any
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE
    • Rank:
      17646

      Description

      We had in incident occur, where somebody did a backup and restore, and files were included in their destination course that were not in their original source, and were not their files at all.

      We tracked it down, and they came from another course which created a backup at the same time on the system. Doing some research, I can only find that backup_unique_code is set based on time(), and never appears to be validated as unique. Relying on the probability of two backups not happening at the same time seems to be very risky, especially on large/busy Moodle installs.

      If they conflict occurs, it would seem to mean unpredictable backups, and the possibility of file leakage from one party to the other. While very hard to actively exploit, the risk of data leakage seems moderate to high.

        Activity

        Hide
        Michael de Raadt added a comment -

        Thanks for reporting this.

        I assume you are talking about course files, not submission data. I'm not sure that, even if there is file leakage, that this would pose a security threat.

        I've put this on our backlog and we'll try to get to it as soon as we can.

        In the meantime adding more information, such as screenshots, replication instructions, fix test instructions, a workaround or even a code solution, will help us and other users.

        Show
        Michael de Raadt added a comment - Thanks for reporting this. I assume you are talking about course files, not submission data. I'm not sure that, even if there is file leakage, that this would pose a security threat. I've put this on our backlog and we'll try to get to it as soon as we can. In the meantime adding more information, such as screenshots, replication instructions, fix test instructions, a workaround or even a code solution, will help us and other users.
        Hide
        Eric Merrill added a comment - - edited

        Basically if two backups are started within the same second, they will use the same temp/backup directory. I can't give idea of what problems it will cause because it is not predictable. Possible problems are:

        Leaking of course files
        Leaking of student submitted data (moddata)
        Leaking of course content (posts, grades, etc).
        Incomplete or corrupt XML

        It can be kinda replicated by starting to backups at nearly the same time - but due to the nature of the problem, it is hard to say way exactly will happen on each attempt.

        At the most basic level, the problem that backup_unique_code isn't actually guaranteed to be unique, even within the same install. The solution would be to make it unique. One possibility is in the function that creates the backup directory, check and see if it already exists, if it does, increment backup_unique_code until you find an unused ones. Other option would be to put a random directory, so you would have something like temp/backup/asdf83nmafud/12859921043, or you could just replace backup_unique_code with some other entity, like a uniqid(). My problem is, I don't know enough about the backup system to know if backup_unique_code needs to be the time the transaction started.

        Show
        Eric Merrill added a comment - - edited Basically if two backups are started within the same second, they will use the same temp/backup directory. I can't give idea of what problems it will cause because it is not predictable. Possible problems are: Leaking of course files Leaking of student submitted data (moddata) Leaking of course content (posts, grades, etc). Incomplete or corrupt XML It can be kinda replicated by starting to backups at nearly the same time - but due to the nature of the problem, it is hard to say way exactly will happen on each attempt. At the most basic level, the problem that backup_unique_code isn't actually guaranteed to be unique, even within the same install. The solution would be to make it unique. One possibility is in the function that creates the backup directory, check and see if it already exists, if it does, increment backup_unique_code until you find an unused ones. Other option would be to put a random directory, so you would have something like temp/backup/asdf83nmafud/12859921043, or you could just replace backup_unique_code with some other entity, like a uniqid(). My problem is, I don't know enough about the backup system to know if backup_unique_code needs to be the time the transaction started.
        Hide
        David Mudrak added a comment -

        Basically if two backups are started within the same second, they will use the same temp/backup directory.

        That is not true. See the method

        protected function calculate_backupid() {
            // Current epoch time + type + id + format + interactive + mode + userid + operation
            // should be unique enough. Add one random part at the end
            $this->backupid = md5(time() . '-' . $this->type . '-' . $this->id . '-' . $this->format . '-' .
                                  $this->interactive . '-' . $this->mode . '-' . $this->userid . '-' .
                                  $this->operation . '-' . random_string(20));
        }
        

        the returned backupid is then used as a name of the temporary directory in temp/backup/

        Show
        David Mudrak added a comment - Basically if two backups are started within the same second, they will use the same temp/backup directory. That is not true. See the method protected function calculate_backupid() { // Current epoch time + type + id + format + interactive + mode + userid + operation // should be unique enough. Add one random part at the end $ this ->backupid = md5(time() . '-' . $ this ->type . '-' . $ this ->id . '-' . $ this ->format . '-' . $ this ->interactive . '-' . $ this ->mode . '-' . $ this ->userid . '-' . $ this ->operation . '-' . random_string(20)); } the returned backupid is then used as a name of the temporary directory in temp/backup/
        Hide
        Eric Merrill added a comment -

        Ok - turns out to be true and not true

        Moodle 1.9.x does not have this, it uses time(). The thing that threw me off in Moodle 2, is that backup/backup_scheduled.php still have the line $backup_unique_code = time();

        Theoretically it could still be a problem in Moodle 2 if a scheduled backup takes less then a second, or if somehow there are 2 scheduled passes running at the same time. It seems odd they wouldn't use the new code, but that seems remote enough to me that it wouldn't be a problem to leave.

        I'll look at backporting calculate_backupid to Moodle 1.9.x

        Show
        Eric Merrill added a comment - Ok - turns out to be true and not true Moodle 1.9.x does not have this, it uses time(). The thing that threw me off in Moodle 2, is that backup/backup_scheduled.php still have the line $backup_unique_code = time(); Theoretically it could still be a problem in Moodle 2 if a scheduled backup takes less then a second, or if somehow there are 2 scheduled passes running at the same time. It seems odd they wouldn't use the new code, but that seems remote enough to me that it wouldn't be a problem to leave. I'll look at backporting calculate_backupid to Moodle 1.9.x
        Hide
        Eric Merrill added a comment -

        Woops, didn't mean to click close, thought I hit cancel, but still:
        Because 1.9.x is nearly EOL, and this doesn't seem to affect MOODLE_22 and head, it seems like we can close this issue.

        Show
        Eric Merrill added a comment - Woops, didn't mean to click close, thought I hit cancel, but still: Because 1.9.x is nearly EOL, and this doesn't seem to affect MOODLE_22 and head, it seems like we can close this issue.
        Hide
        Robert Puffer added a comment -

        Just for the sake of a nebulous argument I'd assert that a lot of schools I know are at least a year away from going to 2.x so the rumors of 1.9's death are greatly exagerated.

        Show
        Robert Puffer added a comment - Just for the sake of a nebulous argument I'd assert that a lot of schools I know are at least a year away from going to 2.x so the rumors of 1.9's death are greatly exagerated.
        Hide
        Eric Merrill added a comment -

        I actually agree with that, but Moodle's official stance is that dev for 1.9.x is ending in June ( http://docs.moodle.org/dev/Releases#Moodle_1.9 ).

        Like I said, I didn't mean to close this, but I can't seem to re-open it, so if someone wants to do that, I'm all for it.

        Show
        Eric Merrill added a comment - I actually agree with that, but Moodle's official stance is that dev for 1.9.x is ending in June ( http://docs.moodle.org/dev/Releases#Moodle_1.9 ). Like I said, I didn't mean to close this, but I can't seem to re-open it, so if someone wants to do that, I'm all for it.
        Hide
        Robert Puffer added a comment -

        In point of fact, Catalyst has picked up support through end-of-2012

        Show
        Robert Puffer added a comment - In point of fact, Catalyst has picked up support through end-of-2012
        Hide
        Dan Marsden added a comment -

        support for bug fixing on 1.9 ended a long time ago - Only fixes for Serious security issues are being supported long term - so I think although accidental this should stay closed

        Show
        Dan Marsden added a comment - support for bug fixing on 1.9 ended a long time ago - Only fixes for Serious security issues are being supported long term - so I think although accidental this should stay closed

          People

          • Votes:
            22 Vote for this issue
            Watchers:
            8 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: