Moodle

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

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.

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.

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

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

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

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.

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?

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

Patch for MDL-19380

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.

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

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

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.

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

failing, yes I agree, this needs more work

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

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

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

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

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

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)

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

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.

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)