Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-33927

It's possible to bypass the maximum number of attachements by overloading the drag and drop option of the filepicker.

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3.2
    • Component/s: Filepicker
    • Labels:
    • Testing Instructions:
      Hide
      1. Create a forum and set maximum number of attachments to 3
      2. Create a discussion and drag in 3 files to the attachment
      3. Quickly drag in another file, whilst the original file upload is still processing
      4. Post to the forum

      Expected result - an error message, just above the attachments area, reading 'You must not attach more than 3 files here.'

      Note: you can make testing a lot easier by opening up repository/upload/lib.php, finding the 'process_upload' function and adding the line 'sleep(5);' at the start of it, to get the upload script to wait 5 seconds before responding.

      Show
      Create a forum and set maximum number of attachments to 3 Create a discussion and drag in 3 files to the attachment Quickly drag in another file, whilst the original file upload is still processing Post to the forum Expected result - an error message, just above the attachments area, reading 'You must not attach more than 3 files here.' Note: you can make testing a lot easier by opening up repository/upload/lib.php, finding the 'process_upload' function and adding the line 'sleep(5);' at the start of it, to get the upload script to wait 5 seconds before responding.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
      MDL-33927_enforce_max_attachments

      Description

      Note: This works best when anti-virus software is enabled and scans uploaded files. This give you more time to drop in more files while it's loading the first batch.
      Replication steps

      1. Create a forum and set maximum number of attachments to 1 or more.
      2. create a discussion and drag in a number of files below the attachment limit. While it's processing those files, drag in another batch below the limit. This can be repeated multiple times. (I managed to drag in eight files in a two attachment limit forum).
      3. Post to the forum.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            salvetore Michael de Raadt added a comment -

            While this is a bug, I don't think it can be exploited by a malicious user as the file-size limits are still being applied.

            Perhaps there needs to be a validation of the number of files when the form reaches the server.

            Show
            salvetore Michael de Raadt added a comment - While this is a bug, I don't think it can be exploited by a malicious user as the file-size limits are still being applied. Perhaps there needs to be a validation of the number of files when the form reaches the server.
            Hide
            marina Marina Glancy added a comment -

            Davo, can you look at it please

            Show
            marina Marina Glancy added a comment - Davo, can you look at it please
            Hide
            davosmith Davo Smith added a comment -

            I'm not sure there is a quick and easy solution to this.

            The upload repository (which receives the file data uploaded by d&d upload), does not know how many files are allowed to be attached (nor is there a sensible way for it to find out).

            We could, as with the 'accepted_types' param, pass on the maxfiles limit, but (aside from any insecurity in trusting parameters passed from the browser), I'm not quite sure if this would actually work. If the server is still processing the previous upload request, will the correct final figure be there already or not?

            One thing we certainly need to look into is checking the file limit when finally accepting the whole form, but I'll have to start tracing through the form element validation to track down how to implement that.

            Show
            davosmith Davo Smith added a comment - I'm not sure there is a quick and easy solution to this. The upload repository (which receives the file data uploaded by d&d upload), does not know how many files are allowed to be attached (nor is there a sensible way for it to find out). We could, as with the 'accepted_types' param, pass on the maxfiles limit, but (aside from any insecurity in trusting parameters passed from the browser), I'm not quite sure if this would actually work. If the server is still processing the previous upload request, will the correct final figure be there already or not? One thing we certainly need to look into is checking the file limit when finally accepting the whole form, but I'll have to start tracing through the form element validation to track down how to implement that.
            Hide
            davosmith Davo Smith added a comment -

            I've added a check on form submission that the maximum number of files has not been exceeded (this would also stop someone from uploading too many files by hacking the javascript locally).

            Show
            davosmith Davo Smith added a comment - I've added a check on form submission that the maximum number of files has not been exceeded (this would also stop someone from uploading too many files by hacking the javascript locally).
            Hide
            fred Frédéric Massart added a comment - - edited

            Hi Davo,

            code looks good and works well. Before pushing it for integration, you might want to fix the typo on getMaxBytes which is getMaxbytes, I know, that's trivial .

            Cheers!

            Show
            fred Frédéric Massart added a comment - - edited Hi Davo, code looks good and works well. Before pushing it for integration, you might want to fix the typo on getMaxBytes which is getMaxbytes , I know, that's trivial . Cheers!
            Hide
            davosmith Davo Smith added a comment -

            Not sure how the getMaxfiles / getMaxFiles typo had worked in my testing, but I've corrected it now and pushing through for integration.

            Show
            davosmith Davo Smith added a comment - Not sure how the getMaxfiles / getMaxFiles typo had worked in my testing, but I've corrected it now and pushing through for integration.
            Hide
            poltawski Dan Poltawski added a comment -

            The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

            TIA and ciao

            Show
            poltawski Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thanks Davo, this has been integrated now

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks Davo, this has been integrated now
            Hide
            andyjdavis Andrew Davis added a comment - - edited

            This doesn't seem to be working how it should. As per the testing instructions I set maximum attachments to 3. I dragged in 3 txt files all together then quickly dragged in a fourth txt file. It was accepted. Once they were done processing if I try to drag in any more files I get an error about too many attachments displayed in an overlay.

            Note: I do get an error when i actually try to submit my new discussion. Its just the ajax error that isn't happening.

            Firefox 14.01 on Ubuntu 11.04

            Show
            andyjdavis Andrew Davis added a comment - - edited This doesn't seem to be working how it should. As per the testing instructions I set maximum attachments to 3. I dragged in 3 txt files all together then quickly dragged in a fourth txt file. It was accepted. Once they were done processing if I try to drag in any more files I get an error about too many attachments displayed in an overlay. Note: I do get an error when i actually try to submit my new discussion. Its just the ajax error that isn't happening. Firefox 14.01 on Ubuntu 11.04
            Hide
            andyjdavis Andrew Davis added a comment -

            Uploading a screenshot.

            Show
            andyjdavis Andrew Davis added a comment - Uploading a screenshot.
            Hide
            davosmith Davo Smith added a comment -

            Due to the asynchronous nature of the uploading and the fact that the upload repository does not have any knowledge of how many files are allowed, it would be difficult to track the current file count whilst uploads are still processing.

            This is why I have implemented a solution which checks the file upload total at the point where the form is submitted (as per the testing instructions and my comment on 10th July).

            Given that this issue can only occur with slow uploads and where the user is deliberately trying to upload more files than are allowed (ignoring the text displayed immediately above the file area), the fix seems reasonable to me.

            Show
            davosmith Davo Smith added a comment - Due to the asynchronous nature of the uploading and the fact that the upload repository does not have any knowledge of how many files are allowed, it would be difficult to track the current file count whilst uploads are still processing. This is why I have implemented a solution which checks the file upload total at the point where the form is submitted (as per the testing instructions and my comment on 10th July). Given that this issue can only occur with slow uploads and where the user is deliberately trying to upload more files than are allowed (ignoring the text displayed immediately above the file area), the fix seems reasonable to me.
            Hide
            andyjdavis Andrew Davis added a comment -

            Ok. Passing

            Show
            andyjdavis Andrew Davis added a comment - Ok. Passing
            Hide
            nebgor Aparup Banerjee added a comment -

            yay, it works!

            This issue has been put through rigorous processes and finally swam upstream along with some 65 others this week.

            Thank you all for taking the time to get us here.

            cheers!

            Show
            nebgor Aparup Banerjee added a comment - yay, it works! This issue has been put through rigorous processes and finally swam upstream along with some 65 others this week. Thank you all for taking the time to get us here. cheers!

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  10/Sep/12