From 23bfe0a4485257d58929dce304c210ff324cb28f Mon Sep 17 00:00:00 2001 From: Petr Skoda Date: Sun, 7 Aug 2011 11:26:03 +0200 Subject: [PATCH 1/2] MDL-19380 reimplement antivir scanning in repositories --- lib/uploadlib.php | 3 +- repository/lib.php | 74 ++++++++++++++++++++++++++++++++++ repository/repository_ajax.php | 13 +----- repository/upload/lib.php | 7 ++++ 4 files changed, 85 insertions(+), 12 deletions(-) diff --git a/lib/uploadlib.php b/lib/uploadlib.php index 5129c811ae8..f81efbaaa5f 100644 --- a/lib/uploadlib.php +++ b/lib/uploadlib.php @@ -697,7 +697,8 @@ function clam_message_admins($notice) { $admins = get_admins(); foreach ($admins as $admin) { $eventdata = new stdClass(); - $eventdata->modulename = 'moodle'; + $eventdata->component = 'moodle'; + $eventdata->name = 'errors'; $eventdata->userfrom = get_admin(); $eventdata->userto = $admin; $eventdata->subject = $subject; diff --git a/repository/lib.php b/repository/lib.php index 5216e1fb9c6..132d5c74f61 100644 --- a/repository/lib.php +++ b/repository/lib.php @@ -946,6 +946,76 @@ abstract class repository { return call_user_func_array(array('repository_' . $plugin, $function), $args); } + /** + * Scan file, throws exception in case of infected file. + * + * Please note that the scanning engine must be able to access the file, + * permissions of the file are not modified here! + * + * @static + * @param string $thefile + * @param string $filename name of the file + * @param bool $deleteinfected + * @return void + */ + public static function antivir_scan_file($thefile, $filename, $deleteinfected) { + global $CFG; + + if (!is_readable($thefile)) { + // this should not happen + return; + } + + if (empty($CFG->runclamonupload) or empty($CFG->pathtoclam)) { + // clam not enabled + return; + } + + $CFG->pathtoclam = trim($CFG->pathtoclam); + + if (!file_exists($CFG->pathtoclam) or !is_executable($CFG->pathtoclam)) { + // misconfigured clam - use the old notification for now + require("$CFG->libdir/uploadlib.php"); + $notice = get_string('clamlost', 'moodle', $CFG->pathtoclam); + clam_message_admins($notice); + return; + } + + // do NOT mess with permissions here, the calling party is responsible for making + // sure the scanner engine can access the files! + + // execute test + $cmd = escapeshellcmd($CFG->pathtoclam).' --stdout '.escapeshellarg($thefile); + exec($cmd, $output, $return); + + if ($return == 0) { + // perfect, no problem found + return; + + } else if ($return == 1) { + // infection found + if ($deleteinfected) { + unlink($thefile); + } + throw new moodle_exception('virusfounduser', 'moodle', '', array('filename'=>$filename)); + + } else { + //unknown problem + require("$CFG->libdir/uploadlib.php"); + $notice = get_string('clamfailed', 'moodle', get_clam_error_code($return)); + $notice .= "\n\n". implode("\n", $output); + clam_message_admins($notice); + if ($CFG->clamfailureonupload === 'actlikevirus') { + if ($deleteinfected) { + unlink($thefile); + } + throw new moodle_exception('virusfounduser', 'moodle', '', array('filename'=>$filename)); + } else { + return; + } + } + } + /** * Move file from download folder to file pool using FILE API * @global object $DB @@ -962,6 +1032,10 @@ abstract class repository { */ public static function move_to_filepool($thefile, $record) { global $DB, $CFG, $USER, $OUTPUT; + + // scan for viruses if possible, throws exception if problem found + self::antivir_scan_file($thefile, $record->filename, empty($CFG->repository_no_delete)); //TODO: MDL-28637 this repository_no_delete is a bloody hack! + if ($record->filepath !== '/') { $record->filepath = trim($record->filepath, '/'); $record->filepath = '/'.$record->filepath.'/'; diff --git a/repository/repository_ajax.php b/repository/repository_ajax.php index 7f0e1276539..08839e7d5c3 100644 --- a/repository/repository_ajax.php +++ b/repository/repository_ajax.php @@ -249,17 +249,8 @@ switch ($action) { } break; case 'upload': - // handle exception here instead moodle default exception handler - // see MDL-23407 - try { - // TODO: add file scanning MDL-19380 into each plugin - $result = $repo->upload($saveas_filename, $maxbytes); - echo json_encode($result); - } catch (Exception $e) { - $err->error = $e->getMessage(); - echo json_encode($err); - die; - } + $result = $repo->upload($saveas_filename, $maxbytes); + echo json_encode($result); break; case 'overwrite': diff --git a/repository/upload/lib.php b/repository/upload/lib.php index 82850423a95..9d93e4c3832 100644 --- a/repository/upload/lib.php +++ b/repository/upload/lib.php @@ -102,6 +102,13 @@ class repository_upload extends repository { } } + // scan the files, throws exception and deletes if virus found + // this is tricky because clamdscan daemon might not be able to access the files + $permissions = fileperms($_FILES[$elname]['tmp_name']); + @chmod($_FILES[$elname]['tmp_name'], $CFG->filepermissions); + self::antivir_scan_file($_FILES[$elname]['tmp_name'], $_FILES[$elname]['name'], true); + @chmod($_FILES[$elname]['tmp_name'], $permissions); + if (empty($saveas_filename)) { $record->filename = clean_param($_FILES[$elname]['name'], PARAM_FILE); } else { From 84641adc3b395182dd14a3a7958d02092388368f Mon Sep 17 00:00:00 2001 From: Petr Skoda Date: Thu, 18 Aug 2011 08:57:42 +0200 Subject: [PATCH 2/2] MDL-19380 fix redirect when virus found in JS-less mode --- repository/filepicker.php | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/repository/filepicker.php b/repository/filepicker.php index e35039fa387..b7b1a65f3f3 100644 --- a/repository/filepicker.php +++ b/repository/filepicker.php @@ -113,8 +113,15 @@ switch ($action) { case 'upload': // The uploaded file has been processed in plugin construct function // redirect to default page - $repo->upload('', $maxbytes); - redirect($home_url, get_string('uploadsucc','repository')); + try { + $repo->upload('', $maxbytes); + redirect($home_url, get_string('uploadsucc','repository')); + } catch (moodle_exception $e) { + // inject target URL + $e->link = $PAGE->url->out(); + echo $OUTPUT->header(); // hack: we need the embedded header here, standard error printing would not use it + throw $e; + } break; case 'search': @@ -262,8 +269,15 @@ case 'download': $record->license = ''; $record->author = ''; $record->source = $thefile['url']; - $info = repository::move_to_filepool($thefile['path'], $record); - redirect($home_url, get_string('downloadsucc', 'repository')); + try { + $info = repository::move_to_filepool($thefile['path'], $record); + redirect($home_url, get_string('downloadsucc', 'repository')); + } catch (moodle_exception $e) { + // inject target URL + $e->link = $PAGE->url->out(); + echo $OUTPUT->header(); // hack: we need the embedded header here, standard error printing would not use it + throw $e; + } } else { print_error('cannotdownload', 'repository'); }