From 7be0d4292a78394139f1d34f8ec77ff529586e8c Mon Sep 17 00:00:00 2001 From: Ruslan Kabalin Date: Thu, 9 Jul 2015 12:03:55 +0100 Subject: [PATCH 1/4] MDL-50888 antivirus_clamav: Implement scan using Unix domain sockets. This is a faster way of scanning files than using command line exec call, but only available on unix-like systems. --- lib/antivirus/clamav/adminlib.php | 105 ++++++++++++++++++ lib/antivirus/clamav/classes/scanner.php | 59 +++++++++- lib/antivirus/clamav/db/upgrade.php | 10 ++ .../clamav/lang/en/antivirus_clamav.php | 13 ++- lib/antivirus/clamav/settings.php | 25 ++++- lib/antivirus/clamav/version.php | 2 +- 6 files changed, 208 insertions(+), 6 deletions(-) create mode 100644 lib/antivirus/clamav/adminlib.php diff --git a/lib/antivirus/clamav/adminlib.php b/lib/antivirus/clamav/adminlib.php new file mode 100644 index 00000000000..f473cea3198 --- /dev/null +++ b/lib/antivirus/clamav/adminlib.php @@ -0,0 +1,105 @@ +. + +/** + * ClamAV antivirus adminlib. + * + * @package antivirus_clamav + * @copyright 2015 Ruslan Kabalin, Lancaster University. + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +defined('MOODLE_INTERNAL') || die(); + +/** + * Admin setting for running, adds verification. + * + * @package antivirus_clamav + * @copyright 2015 Ruslan Kabalin, Lancaster University. + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class antivirus_clamav_runningmethod_setting extends admin_setting_configselect { + /** + * Save a setting + * + * @param string $data + * @return string empty or error string + */ + public function write_setting($data) { + $validated = $this->validate($data); + if ($validated !== true) { + return $validated; + } + return parent::write_setting($data); + } + + /** + * Validate data. + * + * This ensures that unix socket transport is supported by this system. + * + * @param string $data + * @return mixed True on success, else error message. + */ + public function validate($data) { + if ($data === 'unixsocket') { + $supportedtransports = stream_get_transports(); + if (!array_search('unix', $supportedtransports)) { + return get_string('errornounixsocketssupported', 'antivirus_clamav'); + } + } + return true; + } +} +/** + * Admin setting for unix socket path, adds verification. + * + * @package antivirus_clamav + * @copyright 2015 Ruslan Kabalin, Lancaster University. + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class antivirus_clamav_pathtounixsocket_setting extends admin_setting_configtext { + /** + * Validate data. + * + * This ensures that unix socket setting is correct and ClamAV is running. + * + * @param string $data + * @return mixed True on success, else error message. + */ + public function validate($data) { + $result = parent::validate($data); + if ($result !== true) { + return $result; + } + $runningmethod = get_config('antivirus_clamav', 'runningmethod'); + if ($runningmethod === 'unixsocket') { + $socket = stream_socket_client('unix://' . $data, $errno, $errstr, ANTIVIRUS_CLAMAV_SOCKET_TIMEOUT); + if (!$socket) { + return get_string('errorcantopensocket', 'antivirus_clamav', "$errstr ($errno)"); + } else { + // Send PING query to ClamAV socket to check its running state. + fwrite($socket, "nPING\n"); + $response = stream_get_line($socket, 4); + fclose($socket); + if ($response !== 'PONG') { + return get_string('errorclamavnoresponse', 'antivirus_clamav'); + } + } + } + return true; + } +} diff --git a/lib/antivirus/clamav/classes/scanner.php b/lib/antivirus/clamav/classes/scanner.php index e8e58e36dab..d5112d1307d 100644 --- a/lib/antivirus/clamav/classes/scanner.php +++ b/lib/antivirus/clamav/classes/scanner.php @@ -26,6 +26,9 @@ namespace antivirus_clamav; defined('MOODLE_INTERNAL') || die(); +/** Default socket timeout */ +define('ANTIVIRUS_CLAMAV_SOCKET_TIMEOUT', 10); + /** * Class implemeting ClamAV antivirus. * @copyright 2015 Ruslan Kabalin, Lancaster University. @@ -38,8 +41,14 @@ class scanner extends \core\antivirus\scanner { * @return bool True if all necessary config settings been entered */ public function is_configured() { - return (bool)$this->get_config('pathtoclam'); + if ($this->get_config('runningmethod') === 'commandline') { + return (bool)$this->get_config('pathtoclam'); + } else if ($this->get_config('runningmethod') === 'unixsocket') { + return (bool)$this->get_config('pathtounixsocket'); + } + return false; } + /** * Scan file, throws exception in case of infected file. * @@ -59,7 +68,8 @@ class scanner extends \core\antivirus\scanner { } // Execute the scan using preferable method. - list($return, $notice) = $this->scan_file_execute_commandline($file); + $method = 'scan_file_execute_' . $this->get_config('runningmethod'); + list($return, $notice) = $this->$method($file); if ($return == 0) { // Perfect, no problem found, file is clean. @@ -153,4 +163,49 @@ class scanner extends \core\antivirus\scanner { return array($return, $notice); } + + /** + * Scan file using unix socket. + * + * @param string $file Full path to the file. + * @return array ($return, $notice) Execution return code and notification text. + */ + public function scan_file_execute_unixsocket($file) { + $socket = stream_socket_client('unix://' . $this->get_config('pathtounixsocket'), $errno, $errstr, ANTIVIRUS_CLAMAV_SOCKET_TIMEOUT); + if (!$socket) { + // Can't open socket for some reason, notify admins. + $notice = get_string('errorcantopensocket', 'antivirus_clamav', "$errstr ($errno)"); + return array(-1, $notice); + } else { + // Execute scanning. We are running SCAN command and passing file as an argument, + // it is the fastest option, but clamav user need to be able to access it, so + // we give group read permissions first and assume 'clamav' user is in web server + // group (in Debian the default webserver group is 'www-data'). + // Using 'n' as command prefix is forcing clamav to only treat \n as newline delimeter, + // this is to avoid unexpected newline characters on different systems. + $perms = fileperms($file); + chmod($file, 0640); + fwrite($socket, "nSCAN ".$file."\n"); + $output = stream_get_line($socket, 4096); + fclose($socket); + // After scanning we revert permissions to initial ones. + chmod($file, $perms); + // Parse the output. + $splitoutput = explode(': ', $output); + $message = trim($splitoutput[1]); + if ($message === 'OK') { + return array(0, ''); + } else { + $parts = explode(' ', $message); + $status = array_pop($parts); + if ($status === 'FOUND') { + return array(1, ''); + } else { + $notice = get_string('clamfailed', 'antivirus_clamav', $this->get_clam_error_code(2)); + $notice .= "\n\n" . $output; + return array(2, $notice); + } + } + } + } } diff --git a/lib/antivirus/clamav/db/upgrade.php b/lib/antivirus/clamav/db/upgrade.php index 45eb5c104ed..7b870956e05 100644 --- a/lib/antivirus/clamav/db/upgrade.php +++ b/lib/antivirus/clamav/db/upgrade.php @@ -34,5 +34,15 @@ function xmldb_antivirus_clamav_upgrade($oldversion) { // Moodle v3.1.0 release upgrade line. // Put any upgrade step following this. + if ($oldversion < 2016072100) { + // Make command line a default running method for now. We depend on this + // config variable in antivirus scan running, it should be defined. + if (!get_config('antivirus_clamav', 'runningmethod')) { + set_config('runningmethod', 'commandline', 'antivirus_clamav'); + } + + upgrade_plugin_savepoint(true, 2016072100, 'antivirus', 'clamav'); + } + return true; } diff --git a/lib/antivirus/clamav/lang/en/antivirus_clamav.php b/lib/antivirus/clamav/lang/en/antivirus_clamav.php index d83a608caa1..543dd026be5 100644 --- a/lib/antivirus/clamav/lang/en/antivirus_clamav.php +++ b/lib/antivirus/clamav/lang/en/antivirus_clamav.php @@ -25,12 +25,21 @@ $string['configclamactlikevirus'] = 'Treat files like viruses'; $string['configclamdonothing'] = 'Treat files as OK'; $string['configclamfailureonupload'] = 'If you have configured clam to scan uploaded files, but it is configured incorrectly or fails to run for some unknown reason, how should it behave? If you choose \'Treat files like viruses\', they\'ll be moved into the quarantine area, or deleted. If you choose \'Treat files as OK\', the files will be moved to the destination directory like normal. Either way, admins will be alerted that clam has failed. If you choose \'Treat files like viruses\' and for some reason clam fails to run (usually because you have entered an invalid pathtoclam), ALL files that are uploaded will be moved to the given quarantine area, or deleted. Be careful with this setting.'; -$string['configpathtoclam'] = 'Path to ClamAV. Probably something like /usr/bin/clamscan or /usr/bin/clamdscan. You need this in order for ClamAV to run.'; $string['configquarantinedir'] = 'If you want ClamAV to move infected files to a quarantine directory, enter it here. It must be writable by the webserver. If you leave this blank, or if you enter a directory that doesn\'t exist or isn\'t writable, infected files will be deleted. Do not include a trailing slash.'; $string['clamfailed'] = 'ClamAV has failed to run. The return error message was "{$a}". Here is the output from ClamAV:'; $string['clamfailureonupload'] = 'On ClamAV failure'; +$string['errorcantopensocket'] = 'Connecting to Unix domain socket resulted in error {$a}'; +$string['errorclamavnoresponse'] = 'ClamAV does not respond; check daemon running state.'; +$string['errornounixsocketssupported'] = 'Unix domain socket transport is not supported on this system. Please use the command line option instead.'; $string['invalidpathtoclam'] = 'Path to ClamAV, {$a}, is invalid.'; -$string['pathtoclam'] = 'ClamAV path'; +$string['pathtoclam'] = 'Command line'; +$string['pathtoclamdesc'] = 'If the running method is set to "command line", enter the path to ClamAV here. On Linux this will be /usr/bin/clamscan or /usr/bin/clamdscan.'; +$string['pathtounixsocket'] = 'Unix domain socket'; +$string['pathtounixsocketdesc'] = 'If the running method is set to "Unix domain socket", enter the path to ClamAV Unix socket here. On Debian Linux this will be /var/run/clamav/clamd.ctl. Please make sure that clamav daemon has read access to uploaded files, the easiest way to ensure that is to add \'clamav\' user to your webserver group (\'www-data\' on Debian Linux).'; $string['pluginname'] = 'ClamAV antivirus'; $string['quarantinedir'] = 'Quarantine directory'; +$string['runningmethod'] = 'Running method'; +$string['runningmethoddesc'] = 'Method of running ClamAV. Command line is used by default, however on Unix systems better performance can be obtained by using system sockets.'; +$string['runningmethodcommandline'] = 'Command line'; +$string['runningmethodunixsocket'] = 'Unix domain socket'; $string['unknownerror'] = 'There was an unknown error with ClamAV.'; diff --git a/lib/antivirus/clamav/settings.php b/lib/antivirus/clamav/settings.php index 6e41ec761f1..cdba4445516 100644 --- a/lib/antivirus/clamav/settings.php +++ b/lib/antivirus/clamav/settings.php @@ -25,10 +25,33 @@ defined('MOODLE_INTERNAL') || die(); if ($ADMIN->fulltree) { + require_once(__DIR__ . '/adminlib.php'); + require_once(__DIR__ . '/classes/scanner.php'); + + // Running method. + $runningmethodchoice = array( + 'commandline' => get_string('runningmethodcommandline', 'antivirus_clamav'), + 'unixsocket' => get_string('runningmethodunixsocket', 'antivirus_clamav'), + ); + $settings->add(new antivirus_clamav_runningmethod_setting('antivirus_clamav/runningmethod', + get_string('runningmethod', 'antivirus_clamav'), + get_string('runningmethoddesc', 'antivirus_clamav'), + 'commandline', $runningmethodchoice)); + + // Path to ClamAV scanning utility (used in command line running method). $settings->add(new admin_setting_configexecutable('antivirus_clamav/pathtoclam', - new lang_string('pathtoclam', 'antivirus_clamav'), new lang_string('configpathtoclam', 'antivirus_clamav'), '')); + new lang_string('pathtoclam', 'antivirus_clamav'), new lang_string('pathtoclamdesc', 'antivirus_clamav'), '')); + + // Path to ClamAV unix socket (used in unix socket running method). + $settings->add(new antivirus_clamav_pathtounixsocket_setting('antivirus_clamav/pathtounixsocket', + new lang_string('pathtounixsocket', 'antivirus_clamav'), + new lang_string('pathtounixsocketdesc', 'antivirus_clamav'), '', PARAM_PATH)); + + // Quarantine directory path. $settings->add(new admin_setting_configdirectory('antivirus_clamav/quarantinedir', new lang_string('quarantinedir', 'antivirus_clamav'), new lang_string('configquarantinedir', 'antivirus_clamav'), '')); + + // How to act on ClamAV failure. $options = array( 'donothing' => new lang_string('configclamdonothing', 'antivirus_clamav'), 'actlikevirus' => new lang_string('configclamactlikevirus', 'antivirus_clamav'), diff --git a/lib/antivirus/clamav/version.php b/lib/antivirus/clamav/version.php index 80dd77710fc..05b3d6823eb 100644 --- a/lib/antivirus/clamav/version.php +++ b/lib/antivirus/clamav/version.php @@ -24,6 +24,6 @@ defined('MOODLE_INTERNAL') || die(); -$plugin->version = 2016052300; // The current plugin version (Date: YYYYMMDDXX). +$plugin->version = 2016072100; // The current plugin version (Date: YYYYMMDDXX). $plugin->requires = 2016051900; // Requires this Moodle version. $plugin->component = 'antivirus_clamav'; // Full name of the plugin (used for diagnostics). From 83a43b88fea12af5b840e1f676e3b3d0c9aaf4b6 Mon Sep 17 00:00:00 2001 From: Ruslan Kabalin Date: Fri, 4 Mar 2016 14:32:09 +0000 Subject: [PATCH 2/4] MDL-50888 antivirus_clamav: Unit tests refactoring. Due to configurable nature of scanning method selection, unit test needs to be updated to always point to commadline method. There is no need to write separate tests for socket scanning method, as the scanning method needs to be mocked anyway (i.e. will be identical to commandline scanning from test perspective). --- lib/antivirus/clamav/tests/scanner_test.php | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/lib/antivirus/clamav/tests/scanner_test.php b/lib/antivirus/clamav/tests/scanner_test.php index 0a81d42efbb..32f3f562810 100644 --- a/lib/antivirus/clamav/tests/scanner_test.php +++ b/lib/antivirus/clamav/tests/scanner_test.php @@ -56,8 +56,11 @@ class antivirus_clamav_scanner_testcase extends advanced_testcase { public function test_scan_file_no_virus() { $antivirus = $this->getMockBuilder('\antivirus_clamav\scanner') - ->setMethods(array('scan_file_execute_commandline', 'message_admins')) + ->setMethods(array('scan_file_execute_commandline', 'message_admins', 'get_config')) ->getMock(); + // Initiate mock scanning with configuration setting to use commandline. + $configmap = array(array('runningmethod', 'commandline')); + $antivirus->method('get_config')->will($this->returnValueMap($configmap)); // Configure scan_file_execute_commandline method stub to behave // as if no virus has been found. @@ -79,8 +82,11 @@ class antivirus_clamav_scanner_testcase extends advanced_testcase { public function test_scan_file_virus() { $antivirus = $this->getMockBuilder('\antivirus_clamav\scanner') - ->setMethods(array('scan_file_execute_commandline', 'message_admins')) + ->setMethods(array('scan_file_execute_commandline', 'message_admins', 'get_config')) ->getMock(); + // Initiate mock scanning with configuration setting to use commandline. + $configmap = array(array('runningmethod', 'commandline')); + $antivirus->method('get_config')->will($this->returnValueMap($configmap)); // Configure scan_file_execute_commandline method stub to behave // as if virus has been found. @@ -121,8 +127,9 @@ class antivirus_clamav_scanner_testcase extends advanced_testcase { // Set expectation that message_admins is called. $antivirus->expects($this->atLeastOnce())->method('message_admins')->with($this->equalTo('someerror')); - // Initiate mock scanning with configuration setting to do nothing on scanning error. - $configmap = array(array('clamfailureonupload', 'donothing')); + // Initiate mock scanning with configuration setting to do nothing on + // scanning error and using commandline. + $configmap = array(array('clamfailureonupload', 'donothing'), array('runningmethod', 'commandline')); $antivirus->method('get_config')->will($this->returnValueMap($configmap)); // Run mock scanning with deleting infected file. @@ -148,8 +155,9 @@ class antivirus_clamav_scanner_testcase extends advanced_testcase { // Set expectation that message_admins is called. $antivirus->expects($this->atLeastOnce())->method('message_admins')->with($this->equalTo('someerror')); - // Initiate mock scanning with configuration setting to act like virus on scanning error. - $configmap = array(array('clamfailureonupload', 'actlikevirus')); + // Initiate mock scanning with configuration setting to act like virus on + // scanning error and using commandline. + $configmap = array(array('clamfailureonupload', 'actlikevirus'), array('runningmethod', 'commandline')); $antivirus->method('get_config')->will($this->returnValueMap($configmap)); // Run mock scanning without deleting infected file. From 4cc1b5bc0c3c237ad3b6d419b21fe7354a87c4a9 Mon Sep 17 00:00:00 2001 From: Ruslan Kabalin Date: Wed, 13 Apr 2016 15:56:28 +0100 Subject: [PATCH 3/4] MDL-50888 antivirus: Move post-scan logic to manager. In the initial implementation, infected file cleanup and exception throwing was done at the plugin level. This is somewhat incorrect from plugin responsibility perspective. The patch moves cleanup and exception logic to the manager. Antivirus plugin responsibility is limited to perform scanning and respond with one of the result statuses defined in the \core\antivirus\scanner. --- lib/antivirus/clamav/classes/scanner.php | 76 +++++++++++------------- lib/classes/antivirus/manager.php | 9 ++- lib/classes/antivirus/scanner.php | 37 ++++++++++-- 3 files changed, 75 insertions(+), 47 deletions(-) diff --git a/lib/antivirus/clamav/classes/scanner.php b/lib/antivirus/clamav/classes/scanner.php index d5112d1307d..c4c10704b87 100644 --- a/lib/antivirus/clamav/classes/scanner.php +++ b/lib/antivirus/clamav/classes/scanner.php @@ -30,7 +30,7 @@ defined('MOODLE_INTERNAL') || die(); define('ANTIVIRUS_CLAMAV_SOCKET_TIMEOUT', 10); /** - * Class implemeting ClamAV antivirus. + * Class implementing ClamAV antivirus. * @copyright 2015 Ruslan Kabalin, Lancaster University. * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ @@ -50,48 +50,34 @@ class scanner extends \core\antivirus\scanner { } /** - * Scan file, throws exception in case of infected file. + * Scan file. + * + * This method is normally called from antivirus manager (\core\antivirus\manager::scan_file). * * @param string $file Full path to the file. * @param string $filename Name of the file (could be different from physical file if temp file is used). - * @param bool $deleteinfected whether infected file needs to be deleted. - * @throws moodle_exception If file is infected. - * @return void + * @return int Scanning result constant. */ - public function scan_file($file, $filename, $deleteinfected) { - global $CFG; - + public function scan_file($file, $filename) { if (!is_readable($file)) { // This should not happen. debugging('File is not readable.'); - return; + return self::SCAN_RESULT_ERROR; } // Execute the scan using preferable method. $method = 'scan_file_execute_' . $this->get_config('runningmethod'); - list($return, $notice) = $this->$method($file); + $return = $this->$method($file); - if ($return == 0) { - // Perfect, no problem found, file is clean. - return; - } else if ($return == 1) { - // Infection found. - if ($deleteinfected) { - unlink($file); - } - throw new \core\antivirus\scanner_exception('virusfounduser', '', array('filename' => $filename)); - } else { - // Unknown problem. - $this->message_admins($notice); + if ($return === self::SCAN_RESULT_ERROR) { + $this->message_admins($this->get_scanning_notice()); + // If plugin settings require us to act like virus on any error, + // return SCAN_RESULT_FOUND result. if ($this->get_config('clamfailureonupload') === 'actlikevirus') { - if ($deleteinfected) { - unlink($file); - } - throw new \core\antivirus\scanner_exception('virusfounduser', '', array('filename' => $filename)); - } else { - return; + return self::SCAN_RESULT_FOUND; } } + return $return; } /** @@ -132,7 +118,7 @@ class scanner extends \core\antivirus\scanner { * Scan file using command line utility. * * @param string $file Full path to the file. - * @return array ($return, $notice) Execution return code and notification text. + * @return int Scanning result constant. */ public function scan_file_execute_commandline($file) { $pathtoclam = trim($this->get_config('pathtoclam')); @@ -140,7 +126,8 @@ class scanner extends \core\antivirus\scanner { if (!file_exists($pathtoclam) or !is_executable($pathtoclam)) { // Misconfigured clam, notify admins. $notice = get_string('invalidpathtoclam', 'antivirus_clamav', $pathtoclam); - return array(-1, $notice); + $this->set_scanning_notice($notice); + return self::SCAN_RESULT_ERROR; } $clamparam = ' --stdout '; @@ -155,27 +142,35 @@ class scanner extends \core\antivirus\scanner { // Execute scan. $cmd = escapeshellcmd($pathtoclam).$clamparam.escapeshellarg($file); exec($cmd, $output, $return); - $notice = ''; - if ($return > 1) { + // Return variable will contain execution return code. It will be 0 if no virus is found, + // 1 if virus is found, and 2 or above for the error. Return codes 0 and 1 correspond to + // SCAN_RESULT_OK and SCAN_RESULT_FOUND constants, so we return them as it is. + // If there is an error, it gets stored as scanning notice and function + // returns SCAN_RESULT_ERROR. + if ($return > self::SCAN_RESULT_FOUND) { $notice = get_string('clamfailed', 'antivirus_clamav', $this->get_clam_error_code($return)); $notice .= "\n\n". implode("\n", $output); + $this->set_scanning_notice($notice); + return self::SCAN_RESULT_ERROR; } - return array($return, $notice); + return (int)$return; } /** - * Scan file using unix socket. + * Scan file using Unix domain sockets. * * @param string $file Full path to the file. - * @return array ($return, $notice) Execution return code and notification text. + * @return int Scanning result constant. */ public function scan_file_execute_unixsocket($file) { - $socket = stream_socket_client('unix://' . $this->get_config('pathtounixsocket'), $errno, $errstr, ANTIVIRUS_CLAMAV_SOCKET_TIMEOUT); + $socket = stream_socket_client('unix://' . $this->get_config('pathtounixsocket'), + $errno, $errstr, ANTIVIRUS_CLAMAV_SOCKET_TIMEOUT); if (!$socket) { // Can't open socket for some reason, notify admins. $notice = get_string('errorcantopensocket', 'antivirus_clamav', "$errstr ($errno)"); - return array(-1, $notice); + $this->set_scanning_notice($notice); + return self::SCAN_RESULT_ERROR; } else { // Execute scanning. We are running SCAN command and passing file as an argument, // it is the fastest option, but clamav user need to be able to access it, so @@ -194,16 +189,17 @@ class scanner extends \core\antivirus\scanner { $splitoutput = explode(': ', $output); $message = trim($splitoutput[1]); if ($message === 'OK') { - return array(0, ''); + return self::SCAN_RESULT_OK; } else { $parts = explode(' ', $message); $status = array_pop($parts); if ($status === 'FOUND') { - return array(1, ''); + return self::SCAN_RESULT_FOUND; } else { $notice = get_string('clamfailed', 'antivirus_clamav', $this->get_clam_error_code(2)); $notice .= "\n\n" . $output; - return array(2, $notice); + $this->set_scanning_notice($notice); + return self::SCAN_RESULT_ERROR; } } } diff --git a/lib/classes/antivirus/manager.php b/lib/classes/antivirus/manager.php index 0ea9847f913..784ce9719f7 100644 --- a/lib/classes/antivirus/manager.php +++ b/lib/classes/antivirus/manager.php @@ -69,7 +69,14 @@ class manager { public static function scan_file($file, $filename, $deleteinfected) { $antiviruses = self::get_enabled(); foreach ($antiviruses as $antivirus) { - $antivirus->scan_file($file, $filename, $deleteinfected); + $result = $antivirus->scan_file($file, $filename); + if ($result === $antivirus::SCAN_RESULT_FOUND) { + // Infection found. + if ($deleteinfected) { + unlink($file); + } + throw new \core\antivirus\scanner_exception('virusfounduser', '', array('filename' => $filename)); + } } } diff --git a/lib/classes/antivirus/scanner.php b/lib/classes/antivirus/scanner.php index 369c48d2db3..5f4186c5813 100644 --- a/lib/classes/antivirus/scanner.php +++ b/lib/classes/antivirus/scanner.php @@ -35,8 +35,17 @@ defined('MOODLE_INTERNAL') || die(); * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ abstract class scanner { + /** Scanning result indicating no virus found. */ + const SCAN_RESULT_OK = 0; + /** Scanning result indicating that virus is found. */ + const SCAN_RESULT_FOUND = 1; + /** Scanning result indicating the error. */ + const SCAN_RESULT_ERROR = 2; + /** @var stdClass the config for antivirus */ protected $config; + /** @var string scanning notice */ + protected $scanningnotice = ''; /** * Class constructor. @@ -55,7 +64,6 @@ abstract class scanner { * Config get method. * * @param string $property config property to get. - * * @return mixed * @throws \coding_exception */ @@ -66,6 +74,25 @@ abstract class scanner { throw new \coding_exception('Config property "' . $property . '" doesn\'t exist'); } + /** + * Get scanning notice. + * + * @return string + */ + public function get_scanning_notice() { + return $this->scanningnotice; + } + + /** + * Set scanning notice. + * + * @param string $notice notice to set. + * @return void + */ + protected function set_scanning_notice($notice) { + $this->scanningnotice = $notice; + } + /** * Are the antivirus settings configured? * @@ -74,15 +101,13 @@ abstract class scanner { public abstract function is_configured(); /** - * Scan file, throws exception in case of infected file. + * Scan file. * * @param string $file Full path to the file. * @param string $filename Name of the file (could be different from physical file if temp file is used). - * @param bool $deleteinfected whether infected file needs to be deleted. - * @throws \core\antivirus\scanner_exception If file is infected. - * @return void + * @return int Scanning result constants. */ - public abstract function scan_file($file, $filename, $deleteinfected); + public abstract function scan_file($file, $filename); /** * Email admins about antivirus scan outcomes. From f8927a7cde6a0e47689976f5d600b7ec31940517 Mon Sep 17 00:00:00 2001 From: Ruslan Kabalin Date: Thu, 14 Apr 2016 10:37:44 +0100 Subject: [PATCH 4/4] MDL-50888 antivirus: Unit test refactoring. Remove all cleanup and exception test references. We expect plugin just to respond with scanning result constant and notice where applicable. Add tests for \core\antivirus\manager. --- lib/antivirus/clamav/tests/scanner_test.php | 155 +++++++++++--------- lib/tests/antivirus_test.php | 64 ++++++++ lib/tests/fixtures/testable_antivirus.php | 69 +++++++++ 3 files changed, 222 insertions(+), 66 deletions(-) create mode 100644 lib/tests/fixtures/testable_antivirus.php diff --git a/lib/antivirus/clamav/tests/scanner_test.php b/lib/antivirus/clamav/tests/scanner_test.php index 32f3f562810..4aa45db06c6 100644 --- a/lib/antivirus/clamav/tests/scanner_test.php +++ b/lib/antivirus/clamav/tests/scanner_test.php @@ -26,6 +26,7 @@ defined('MOODLE_INTERNAL') || die(); class antivirus_clamav_scanner_testcase extends advanced_testcase { + /** @var string temporary file used in testing */ protected $tempfile; protected function setUp() { @@ -49,80 +50,96 @@ class antivirus_clamav_scanner_testcase extends advanced_testcase { // Test specifying file that does not exist. $nonexistingfile = $this->tempfile . '_'; $this->assertFileNotExists($nonexistingfile); - // Run mock scanning with deleting infected file. - $antivirus->scan_file($nonexistingfile, '', true); + // Run mock scanning, we expect SCAN_RESULT_ERROR. + $this->assertEquals(2, $antivirus->scan_file($nonexistingfile, '')); $this->assertDebuggingCalled(); } public function test_scan_file_no_virus() { + $methods = array( + 'scan_file_execute_commandline', + 'scan_file_execute_unixsocket', + 'message_admins', + 'get_config', + ); $antivirus = $this->getMockBuilder('\antivirus_clamav\scanner') - ->setMethods(array('scan_file_execute_commandline', 'message_admins', 'get_config')) + ->setMethods($methods) ->getMock(); // Initiate mock scanning with configuration setting to use commandline. $configmap = array(array('runningmethod', 'commandline')); $antivirus->method('get_config')->will($this->returnValueMap($configmap)); - // Configure scan_file_execute_commandline method stub to behave - // as if no virus has been found. - $antivirus->method('scan_file_execute_commandline')->willReturn(array(0, '')); + // Configure scan_file_execute_commandline and scan_file_execute_unixsocket + // method stubs to behave as if no virus has been found (SCAN_RESULT_OK). + $antivirus->method('scan_file_execute_commandline')->willReturn(0); + $antivirus->method('scan_file_execute_unixsocket')->willReturn(0); // Set expectation that message_admins is NOT called. $antivirus->expects($this->never())->method('message_admins'); - // Run mock scanning with deleting infected file. - $this->assertFileExists($this->tempfile); - try { - $antivirus->scan_file($this->tempfile, '', true); - } catch (\core\antivirus\scanner_exception $e) { - $this->fail('Exception scanner_exception is not expected in clean file scanning.'); - } - // File expected to remain in place. + // Run mock scanning. $this->assertFileExists($this->tempfile); + $this->assertEquals(0, $antivirus->scan_file($this->tempfile, '')); + + // Initiate mock scanning with configuration setting to use unixsocket. + $configmap = array(array('runningmethod', 'unixsocket')); + $antivirus->method('get_config')->will($this->returnValueMap($configmap)); + + // Run mock scanning. + $this->assertEquals(0, $antivirus->scan_file($this->tempfile, '')); } public function test_scan_file_virus() { + $methods = array( + 'scan_file_execute_commandline', + 'scan_file_execute_unixsocket', + 'message_admins', + 'get_config', + ); $antivirus = $this->getMockBuilder('\antivirus_clamav\scanner') - ->setMethods(array('scan_file_execute_commandline', 'message_admins', 'get_config')) + ->setMethods($methods) ->getMock(); // Initiate mock scanning with configuration setting to use commandline. $configmap = array(array('runningmethod', 'commandline')); $antivirus->method('get_config')->will($this->returnValueMap($configmap)); - // Configure scan_file_execute_commandline method stub to behave - // as if virus has been found. - $antivirus->method('scan_file_execute_commandline')->willReturn(array(1, '')); + // Configure scan_file_execute_commandline and scan_file_execute_unixsocket + // method stubs to behave as if virus has been found (SCAN_RESULT_FOUND). + $antivirus->method('scan_file_execute_commandline')->willReturn(1); + $antivirus->method('scan_file_execute_unixsocket')->willReturn(1); // Set expectation that message_admins is NOT called. $antivirus->expects($this->never())->method('message_admins'); - // Run mock scanning without deleting infected file. - $this->assertFileExists($this->tempfile); - try { - $antivirus->scan_file($this->tempfile, '', false); - } catch (\moodle_exception $e) { - $this->assertInstanceOf('\core\antivirus\scanner_exception', $e); - } - // File expected to remain in place. + // Run mock scanning. $this->assertFileExists($this->tempfile); + $this->assertEquals(1, $antivirus->scan_file($this->tempfile, '')); - // Run mock scanning with deleting infected file. - try { - $antivirus->scan_file($this->tempfile, '', true); - } catch (\moodle_exception $e) { - $this->assertInstanceOf('\core\antivirus\scanner_exception', $e); - } - // File expected to be deleted. - $this->assertFileNotExists($this->tempfile); + // Initiate mock scanning with configuration setting to use unixsocket. + $configmap = array(array('runningmethod', 'unixsocket')); + $antivirus->method('get_config')->will($this->returnValueMap($configmap)); + + // Run mock scanning. + $this->assertEquals(1, $antivirus->scan_file($this->tempfile, '')); } public function test_scan_file_error_donothing() { + $methods = array( + 'scan_file_execute_commandline', + 'scan_file_execute_unixsocket', + 'message_admins', + 'get_config', + 'get_scanning_notice', + ); $antivirus = $this->getMockBuilder('\antivirus_clamav\scanner') - ->setMethods(array('scan_file_execute_commandline', 'message_admins', 'get_config')) + ->setMethods($methods) ->getMock(); - // Configure scan_file_execute_commandline method stub to behave - // as if there is a scanning error. - $antivirus->method('scan_file_execute_commandline')->willReturn(array(2, 'someerror')); + // Configure scan_file_execute_commandline and scan_file_execute_unixsocket + // method stubs to behave as if there is a scanning error (SCAN_RESULT_ERROR). + $antivirus->method('scan_file_execute_commandline')->willReturn(2); + $antivirus->method('scan_file_execute_unixsocket')->willReturn(2); + $antivirus->method('get_scanning_notice')->willReturn('someerror'); // Set expectation that message_admins is called. $antivirus->expects($this->atLeastOnce())->method('message_admins')->with($this->equalTo('someerror')); @@ -132,25 +149,36 @@ class antivirus_clamav_scanner_testcase extends advanced_testcase { $configmap = array(array('clamfailureonupload', 'donothing'), array('runningmethod', 'commandline')); $antivirus->method('get_config')->will($this->returnValueMap($configmap)); - // Run mock scanning with deleting infected file. - $this->assertFileExists($this->tempfile); - try { - $antivirus->scan_file($this->tempfile, '', true); - } catch (\core\antivirus\scanner_exception $e) { - $this->fail('Exception scanner_exception is not expected with config setting to do nothing on error.'); - } - // File expected to remain in place. + // Run mock scanning. $this->assertFileExists($this->tempfile); + $this->assertEquals(2, $antivirus->scan_file($this->tempfile, '')); + + // Initiate mock scanning with configuration setting to do nothing on + // scanning error and using unixsocket. + $configmap = array(array('clamfailureonupload', 'donothing'), array('runningmethod', 'unixsocket')); + $antivirus->method('get_config')->will($this->returnValueMap($configmap)); + + // Run mock scanning. + $this->assertEquals(2, $antivirus->scan_file($this->tempfile, '')); } public function test_scan_file_error_actlikevirus() { + $methods = array( + 'scan_file_execute_commandline', + 'scan_file_execute_unixsocket', + 'message_admins', + 'get_config', + 'get_scanning_notice', + ); $antivirus = $this->getMockBuilder('\antivirus_clamav\scanner') - ->setMethods(array('scan_file_execute_commandline', 'message_admins', 'get_config')) + ->setMethods($methods) ->getMock(); - // Configure scan_file_execute_commandline method stub to behave - // as if there is a scanning error. - $antivirus->method('scan_file_execute_commandline')->willReturn(array(2, 'someerror')); + // Configure scan_file_execute_commandline and scan_file_execute_unixsocket + // method stubs to behave as if there is a scanning error (SCAN_RESULT_ERROR). + $antivirus->method('scan_file_execute_commandline')->willReturn(2); + $antivirus->method('scan_file_execute_unixsocket')->willReturn(2); + $antivirus->method('get_scanning_notice')->willReturn('someerror'); // Set expectation that message_admins is called. $antivirus->expects($this->atLeastOnce())->method('message_admins')->with($this->equalTo('someerror')); @@ -160,23 +188,18 @@ class antivirus_clamav_scanner_testcase extends advanced_testcase { $configmap = array(array('clamfailureonupload', 'actlikevirus'), array('runningmethod', 'commandline')); $antivirus->method('get_config')->will($this->returnValueMap($configmap)); - // Run mock scanning without deleting infected file. - $this->assertFileExists($this->tempfile); - try { - $antivirus->scan_file($this->tempfile, '', false); - } catch (\moodle_exception $e) { - $this->assertInstanceOf('\core\antivirus\scanner_exception', $e); - } - // File expected to remain in place. + // Run mock scanning, we expect SCAN_RESULT_FOUND since configuration + // require us to act like virus. $this->assertFileExists($this->tempfile); + $this->assertEquals(1, $antivirus->scan_file($this->tempfile, '')); - // Run mock scanning with deleting infected file. - try { - $antivirus->scan_file($this->tempfile, '', true); - } catch (\moodle_exception $e) { - $this->assertInstanceOf('\core\antivirus\scanner_exception', $e); - } - // File expected to be deleted. - $this->assertFileNotExists($this->tempfile); + // Initiate mock scanning with configuration setting to act like virus on + // scanning error and using unixsocket. + $configmap = array(array('clamfailureonupload', 'actlikevirus'), array('runningmethod', 'unixsocket')); + $antivirus->method('get_config')->will($this->returnValueMap($configmap)); + + // Run mock scanning, we expect SCAN_RESULT_FOUND since configuration + // require us to act like virus. + $this->assertEquals(1, $antivirus->scan_file($this->tempfile, '')); } } diff --git a/lib/tests/antivirus_test.php b/lib/tests/antivirus_test.php index 13320e9cfc3..895df2c1a7b 100644 --- a/lib/tests/antivirus_test.php +++ b/lib/tests/antivirus_test.php @@ -24,8 +24,27 @@ */ defined('MOODLE_INTERNAL') || die(); +require_once(__DIR__ . '/fixtures/testable_antivirus.php'); class core_antivirus_testcase extends advanced_testcase { + protected $tempfile; + + protected function setUp() { + global $CFG; + // Use our special testable fixture plugin. + $CFG->antiviruses = 'testable'; + + $this->resetAfterTest(); + + // Create tempfile. + $tempfolder = make_request_directory(false); + $this->tempfile = $tempfolder . '/' . rand(); + touch($this->tempfile); + } + + protected function tearDown() { + @unlink($this->tempfile); + } public function test_manager_get_antivirus() { // We are using clamav plugin in the test, @@ -34,4 +53,49 @@ class core_antivirus_testcase extends advanced_testcase { $antivirusdirect = new \antivirus_clamav\scanner(); $this->assertEquals($antivirusdirect, $antivirusviaget); } + + public function test_manager_scan_file_no_virus() { + // Run mock scanning. + $this->assertFileExists($this->tempfile); + try { + \core\antivirus\manager::scan_file($this->tempfile, 'OK', true); + } catch (\moodle_exception $e) { + $this->fail('Exception scanner_exception is not expected in clean file scanning.'); + } + // File expected to remain in place. + $this->assertFileExists($this->tempfile); + } + + public function test_manager_scan_file_error() { + // Run mock scanning. + $this->assertFileExists($this->tempfile); + try { + \core\antivirus\manager::scan_file($this->tempfile, 'ERROR', true); + } catch (\moodle_exception $e) { + $this->fail('Exception scanner_exception is not expected in error file scanning.'); + } + // File expected to remain in place. + $this->assertFileExists($this->tempfile); + } + + public function test_manager_scan_file_virus() { + // Run mock scanning without deleting infected file. + $this->assertFileExists($this->tempfile); + try { + \core\antivirus\manager::scan_file($this->tempfile, 'FOUND', false); + } catch (\moodle_exception $e) { + $this->assertInstanceOf('\core\antivirus\scanner_exception', $e); + } + // File expected to remain in place. + $this->assertFileExists($this->tempfile); + + // Run mock scanning with deleting infected file. + try { + \core\antivirus\manager::scan_file($this->tempfile, 'FOUND', true); + } catch (\moodle_exception $e) { + $this->assertInstanceOf('\core\antivirus\scanner_exception', $e); + } + // File expected to be deleted. + $this->assertFileNotExists($this->tempfile); + } } diff --git a/lib/tests/fixtures/testable_antivirus.php b/lib/tests/fixtures/testable_antivirus.php new file mode 100644 index 00000000000..c25f8229da3 --- /dev/null +++ b/lib/tests/fixtures/testable_antivirus.php @@ -0,0 +1,69 @@ +. + +/** + * Provides \antivirus_testable class. + * + * @package core + * @subpackage fixtures + * @category test + * @copyright 2016 Ruslan Kabalin, Lancaster University. + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +namespace antivirus_testable; + +defined('MOODLE_INTERNAL') || die(); + +/** + * Testable antivirus plugin. + * + * @copyright 2016 Ruslan Kabalin, Lancaster University. + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class scanner extends \core\antivirus\scanner { + /** + * Are the necessary antivirus settings configured? + * + * @return bool True if all necessary config settings been entered + */ + public function is_configured() { + return true; + } + + /** + * Scan file. + * + * Provides fake responses for testing \core\antivirus\manager. + * + * @param string $file Full path to the file. + * @param string $filename For mocking purposes, filename defines expected response. + * @return int Scanning result constant. + */ + public function scan_file($file, $filename) { + switch ($filename) { + case 'OK': + return self::SCAN_RESULT_OK; + case 'FOUND': + return self::SCAN_RESULT_FOUND; + case 'ERROR': + return self::SCAN_RESULT_ERROR; + default: + debugging('$filename should be either OK, FOUND or ERROR.'); + break; + } + } +}