Moodle
  1. Moodle
  2. MDL-19380

Reimplement support for antivirus software in file uploads and repo pickers

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0, 2.1.1, 2.2
    • Fix Version/s: 2.1.2
    • Component/s: Files API
    • Labels:
    • Rank:
      235
    1. patch_commit_35a49d94579c.patch
      6 kB
      Petr Škoda
    2. patch2.txt
      9 kB
      Craig B

      Activity

      Hide
      Martin Dougiamas added a comment -

      We talked about this last week ... do you want to make some notes here?

      Show
      Martin Dougiamas added a comment - We talked about this last week ... do you want to make some notes here?
      Hide
      Matteo Scaramuccia added a comment -

      Look at http://moodle.org/mod/forum/discuss.php?d=178175 for a possible patch proposal: the user declares it as a working proof of concept i.e. he have decided to propose it to the community before proposing it to the board.

      Show
      Matteo Scaramuccia added a comment - Look at http://moodle.org/mod/forum/discuss.php?d=178175 for a possible patch proposal: the user declares it as a working proof of concept i.e. he have decided to propose it to the community before proposing it to the board.
      Hide
      Craig B added a comment -

      Hi, could anyone please let me know what needs to be done to my patch to have it accepted? Hopefully I'll have the ability to apply these. Thanks.

      Show
      Craig B added a comment - Hi, could anyone please let me know what needs to be done to my patch to have it accepted? Hopefully I'll have the ability to apply these. Thanks.
      Hide
      Petr Škoda added a comment -

      Hello,
      thanks for the patch! I am going to work on this issue soon. I noticed some issues:
      1/ there is a missing shell escaping on the temp file name
      2/ it is not compatible with Windows
      3/ maybe it would not be necessary to email support and simply reject the file from user and log the failure
      4/ I suppose files from all repositories should be verified
      5/ it would be nice to have cron scanner too
      6/ we could add support for more scanners - maybe a new plugin type

      Show
      Petr Škoda added a comment - Hello, thanks for the patch! I am going to work on this issue soon. I noticed some issues: 1/ there is a missing shell escaping on the temp file name 2/ it is not compatible with Windows 3/ maybe it would not be necessary to email support and simply reject the file from user and log the failure 4/ I suppose files from all repositories should be verified 5/ it would be nice to have cron scanner too 6/ we could add support for more scanners - maybe a new plugin type
      Hide
      Craig B added a comment -

      Thanks, I'll have a look at these issues and hopefully help out.

      Show
      Craig B added a comment - Thanks, I'll have a look at these issues and hopefully help out.
      Hide
      Matteo Scaramuccia added a comment -

      Hi all,
      about Petr's (3): 1.9 was used to do that, e.g.: lib/uploadlib::clam_scan_moodle_file() => lib/uploadlib.php::clam_mail_admins().

      HTH,
      Matteo

      Show
      Matteo Scaramuccia added a comment - Hi all, about Petr's (3): 1.9 was used to do that, e.g.: lib/uploadlib::clam_scan_moodle_file() => lib/uploadlib.php::clam_mail_admins() . HTH, Matteo
      Hide
      Petr Škoda added a comment -

      Yes, but the workflow was a bit different in 1.9 - previously we were testing files during the form submission, now we are doing it much earlier. I personally think the information is more important for person uploading the files, admin should be able to see it in logs in necessary. I would not personally want to have my mailbox flooded with these notifications. This could be configurable if more people require sending of these notifications to admin, in any case it should go through the new messaging subsystem.

      Show
      Petr Škoda added a comment - Yes, but the workflow was a bit different in 1.9 - previously we were testing files during the form submission, now we are doing it much earlier. I personally think the information is more important for person uploading the files, admin should be able to see it in logs in necessary. I would not personally want to have my mailbox flooded with these notifications. This could be configurable if more people require sending of these notifications to admin, in any case it should go through the new messaging subsystem.
      Hide
      Sam Marshall added a comment -

      Thanks to those working on the issue. I note it is marked as 'DEV backlog' but when there is a fix, if possible could it please be backported to 2.1.x as well?

      Show
      Sam Marshall added a comment - Thanks to those working on the issue. I note it is marked as 'DEV backlog' but when there is a fix, if possible could it please be backported to 2.1.x as well?
      Hide
      Petr Škoda added a comment -

      Let's get some code first, test it for some time and then think about backporting.

      Show
      Petr Škoda added a comment - Let's get some code first, test it for some time and then think about backporting.
      Hide
      Craig B added a comment -

      Patch for MDL-19380

      Show
      Craig B added a comment - Patch for MDL-19380
      Hide
      Craig B added a comment -

      I've uploaded a patch that attempts to fix issues 1, 2, and 3. I wasn't sure how to log detection so I added an option to email support in the settings page. I hope escapeshellargs() is appropriate for escaping the temp file name. Also I've only tested on 2.1 in Windows Server 2008 and Debian 6, but if this is OK I'll test on 2.0 and generate a new patch if needed.

      Please excuse my ignorance with this; I've not worked with others before so am new to all this patching stuff. Hopefully I can be of use to this project once I've learned the ropes.

      Show
      Craig B added a comment - I've uploaded a patch that attempts to fix issues 1, 2, and 3. I wasn't sure how to log detection so I added an option to email support in the settings page. I hope escapeshellargs() is appropriate for escaping the temp file name. Also I've only tested on 2.1 in Windows Server 2008 and Debian 6, but if this is OK I'll test on 2.0 and generate a new patch if needed. Please excuse my ignorance with this; I've not worked with others before so am new to all this patching stuff. Hopefully I can be of use to this project once I've learned the ropes.
      Hide
      Petr Škoda added a comment -

      Thanks for the patch, hopefully I will find more time later this week to review it.

      Show
      Petr Škoda added a comment - Thanks for the patch, hopefully I will find more time later this week to review it.
      Hide
      Rod Norfor added a comment -

      I am a bit confused, why is the function added to repository/lib.php rather than lib/uploadlib.php where the other clam functions are?

      If repository/lib.php is the correct place the clam functions in lib/uploadlib.php should probably end up in deprecatedlib.php.

      Not really sure where lib/uploadlib.php is used in moodle 2, and this fix doesn't seem to change admin/handlevirus.php which does not do anything due to the TODO.

      I also think the patch was better emailing support than the list of admins.... we have a lot of admins...

      Thanks.

      Rod

      Show
      Rod Norfor added a comment - I am a bit confused, why is the function added to repository/lib.php rather than lib/uploadlib.php where the other clam functions are? If repository/lib.php is the correct place the clam functions in lib/uploadlib.php should probably end up in deprecatedlib.php. Not really sure where lib/uploadlib.php is used in moodle 2, and this fix doesn't seem to change admin/handlevirus.php which does not do anything due to the TODO. I also think the patch was better emailing support than the list of admins.... we have a lot of admins... Thanks. Rod
      Hide
      Petr Škoda added a comment -

      My patch only fixes the upload via repositories and one regression in uploadlib, the uploadlib is deprecated since Moodle 2.0, but it should still work fine.

      This is not a final solution of course, somebody will have to spend a lot more time on cleanup of upload lib and virus handling, I did as few changes as possible to get something ready for integration into 2.1.x.

      This patch does not address the compatibility with on-access virus scanner, in fact I had to turn it off in my computer completely when writing this.

      Show
      Petr Škoda added a comment - My patch only fixes the upload via repositories and one regression in uploadlib, the uploadlib is deprecated since Moodle 2.0, but it should still work fine. This is not a final solution of course, somebody will have to spend a lot more time on cleanup of upload lib and virus handling, I did as few changes as possible to get something ready for integration into 2.1.x. This patch does not address the compatibility with on-access virus scanner, in fact I had to turn it off in my computer completely when writing this.
      Hide
      Eloy Lafuente (stronk7) added a comment - - edited

      The patch seems correct, but I've some doubts before integrating it (surely due to my lack of knowledge of the repos stuff):

      1. Looking for uses of "move_to_filepool" - the place where you've added the call to antivir_scan_file() - I've found these:
        1. filepicker.php (#256): within the "download" action. I guess that "download" (from repo) is, really, one upload into Moodle, yes?
        2. repository_ajax.php (#243): Same than above, it's in one "download" action. Also correct?
      2. Based in the previous point it surprised me a lot to see that, in "repository_ajax.php" you've also hacked the "upload" action by:
        1. Undoing MDL-23407 (the exception catching). So the default exception handler will be invoked on virus (and other exceptions)... perhaps borking the json/ajax request/response?
        2. Modifying the "upload" repo, by adding also there a custom call to antivir_scan_file(), why do we need it there, if we know it's prone to cause the json problems and, also, those files are being processed in the "download" section?
      3. Finally, I think that testing instructions should be a bit more specific, telling the ways (js and non-js) and the origins (repository types) where we should be trying with eicar and without it. And checking that the detection is notified as expected, I'm not sure what the user is supposed to get).

      So, perhaps, we should delay this one week to see if everything is in place? 100% FYC.

      Ciao

      Show
      Eloy Lafuente (stronk7) added a comment - - edited The patch seems correct, but I've some doubts before integrating it (surely due to my lack of knowledge of the repos stuff): Looking for uses of "move_to_filepool" - the place where you've added the call to antivir_scan_file() - I've found these: filepicker.php (#256): within the "download" action. I guess that "download" (from repo) is, really, one upload into Moodle, yes? repository_ajax.php (#243): Same than above, it's in one "download" action. Also correct? Based in the previous point it surprised me a lot to see that, in "repository_ajax.php" you've also hacked the "upload" action by: Undoing MDL-23407 (the exception catching). So the default exception handler will be invoked on virus (and other exceptions)... perhaps borking the json/ajax request/response? Modifying the "upload" repo, by adding also there a custom call to antivir_scan_file(), why do we need it there, if we know it's prone to cause the json problems and, also, those files are being processed in the "download" section? Finally, I think that testing instructions should be a bit more specific, telling the ways (js and non-js) and the origins (repository types) where we should be trying with eicar and without it. And checking that the detection is notified as expected, I'm not sure what the user is supposed to get). So, perhaps, we should delay this one week to see if everything is in place? 100% FYC. Ciao
      Hide
      Petr Škoda added a comment -

      failing, yes I agree, this needs more work

      Show
      Petr Škoda added a comment - failing, yes I agree, this needs more work
      Hide
      Petr Škoda added a comment - - edited

      1. correct
      2. no exception patching is necessary, the ajax script does it automatically, no json problems there
      3. user gets standard JS error message from the file picker

      Show
      Petr Škoda added a comment - - edited 1. correct 2. no exception patching is necessary, the ajax script does it automatically, no json problems there 3. user gets standard JS error message from the file picker
      Hide
      Petr Škoda added a comment -

      Attaching patch here so that I do not have to maintain it in my repo. It works for me and I am not planning any more changes there, I think it is suitable for stable, more changes should be done in master only:

      • win support
      • support for full scan and quarantine of files
      • support of on-access scanners
      Show
      Petr Škoda added a comment - Attaching patch here so that I do not have to maintain it in my repo. It works for me and I am not planning any more changes there, I think it is suitable for stable, more changes should be done in master only: win support support for full scan and quarantine of files support of on-access scanners
      Hide
      Petr Škoda added a comment -

      The last missing bit is correct redirect in non-JS UI version, I am going to fix it tomorrow and update the pull request.

      Show
      Petr Škoda added a comment - The last missing bit is correct redirect in non-JS UI version, I am going to fix it tomorrow and update the pull request.
      Hide
      Eloy Lafuente (stronk7) added a comment -

      Assuming the last version is github's one, lol. Could you, plz, stop those games?

      Show
      Eloy Lafuente (stronk7) added a comment - Assuming the last version is github's one, lol. Could you, plz, stop those games?
      Hide
      Eloy Lafuente (stronk7) added a comment -

      Blindy trusting (and not loving) that the hack is necessary, this has been integrated and backported to 21_STABLE, thanks!

      Show
      Eloy Lafuente (stronk7) added a comment - Blindy trusting (and not loving) that the hack is necessary, this has been integrated and backported to 21_STABLE, thanks!
      Hide
      Aparup Banerjee added a comment -

      i've tested uploading some eicar files from dropbox (with a temporary fix to the file picker's upload problem in integration master)

      This seems to work for me.

      some initial attempts of eicar files were scanned (based on apache error log clamav update warning) but not reported as viruses.. but then everything started to work, all viruses were caught from uploads and from dropbox and some other repositories.

      The messages via messaging on a mis-configuration of the AV setup does come via email. (not via popup or anything else)

      Show
      Aparup Banerjee added a comment - i've tested uploading some eicar files from dropbox (with a temporary fix to the file picker's upload problem in integration master) This seems to work for me. some initial attempts of eicar files were scanned (based on apache error log clamav update warning) but not reported as viruses.. but then everything started to work, all viruses were caught from uploads and from dropbox and some other repositories. The messages via messaging on a mis-configuration of the AV setup does come via email. (not via popup or anything else)
      Hide
      Eloy Lafuente (stronk7) added a comment -

      Many thanks for the hard work, this has been sent upstream and is available in all the git and cvs repositories.

      Show
      Eloy Lafuente (stronk7) added a comment - Many thanks for the hard work, this has been sent upstream and is available in all the git and cvs repositories.
      Hide
      John White added a comment -

      Hi all, many thanks for this work. It was a surprise to us to find clam didn't work in 2.0!
      Is it possible to add this patch to the latest 2.0.x or do we have to patch that ourselves.
      Again many thanks.

      Show
      John White added a comment - Hi all, many thanks for this work. It was a surprise to us to find clam didn't work in 2.0! Is it possible to add this patch to the latest 2.0.x or do we have to patch that ourselves. Again many thanks.
      Hide
      Aparup Banerjee added a comment -

      Hi John,
      i would suggest at this stage that it might be better to consider upgrading to 2.1 (if not 2.2 which has just been released)

      Show
      Aparup Banerjee added a comment - Hi John, i would suggest at this stage that it might be better to consider upgrading to 2.1 (if not 2.2 which has just been released)

        People

        • Votes:
          9 Vote for this issue
          Watchers:
          12 Start watching this issue

          Dates

          • Created:
            Updated:
            Resolved: