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 Bug
    • Status: Closed
    • Priority: Major 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
    • Rank:
      42031

      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.

        Issue Links

          Activity

          Hide
          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
          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 Glancy added a comment -

          Davo, can you look at it please

          Show
          Marina Glancy added a comment - Davo, can you look at it please
          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          Sam Hemelryk added a comment -

          Thanks Davo, this has been integrated now

          Show
          Sam Hemelryk added a comment - Thanks Davo, this has been integrated now
          Hide
          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
          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
          Andrew Davis added a comment -

          Uploading a screenshot.

          Show
          Andrew Davis added a comment - Uploading a screenshot.
          Hide
          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
          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
          Andrew Davis added a comment -

          Ok. Passing

          Show
          Andrew Davis added a comment - Ok. Passing
          Hide
          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
          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: