From 5601b7524032babbc50a694ec756f33449642a2a Mon Sep 17 00:00:00 2001 From: pmishev Date: Tue, 18 Jan 2022 15:03:57 +0000 Subject: [PATCH] Errors from clamav when scanning files now throw FileScanException or \RuntimeException if the response could not be parsed. Fixed reading response from socket so any errors are caught. --- composer.json | 3 +- src/DTO/ScannedFile.php | 14 +++++- src/Exception/FileScanException.php | 45 +++++++++++++++++++ src/Exception/SocketException.php | 8 ++-- .../AbstractScanStrategyClamdSocket.php | 4 +- src/Socket/Socket.php | 12 +++-- tests/Files/inaccessible.txt | 1 + tests/Functional/ScannerTest.php | 30 ++++++++++--- 8 files changed, 99 insertions(+), 18 deletions(-) create mode 100644 src/Exception/FileScanException.php create mode 100644 tests/Files/inaccessible.txt diff --git a/composer.json b/composer.json index 2b7e95e..88d00e1 100644 --- a/composer.json +++ b/composer.json @@ -16,7 +16,8 @@ "symfony/http-kernel": "~4.0 || ~5.0", "symfony/config": "~4.0 || ~5.0", "symfony/dependency-injection": "~4.0 || ~5.0", - "symfony/yaml": "~4.0 || ~5.0" + "symfony/yaml": "~4.0 || ~5.0", + "ext-sockets": "*" }, "require-dev": { "phpunit/phpunit": "^9.0" diff --git a/src/DTO/ScannedFile.php b/src/DTO/ScannedFile.php index bf46d1d..9d413dd 100644 --- a/src/DTO/ScannedFile.php +++ b/src/DTO/ScannedFile.php @@ -2,6 +2,8 @@ namespace Sineflow\ClamAV\DTO; +use Sineflow\ClamAV\Exception\FileScanException; + class ScannedFile { /** @@ -29,9 +31,17 @@ class ScannedFile */ public function __construct(string $rawResponse) { - $this->rawResponse = $rawResponse; + $isParsed = preg_match('/(.*):(.*)(FOUND|OK|ERROR)/i', $rawResponse, $matches); + + if (!$isParsed) { + throw new \RuntimeException(sprintf('Failed to parse clamav response: %s', $rawResponse)); + } - preg_match('/(.*):(.*)(FOUND|OK)/i', $rawResponse, $matches); + if ($matches[3] === 'ERROR') { + throw new FileScanException($matches[1], trim($matches[2])); + } + + $this->rawResponse = $rawResponse; $this->fileName = $matches[1]; $this->virusName = trim($matches[2]); $this->isClean = ($matches[3] === 'OK'); diff --git a/src/Exception/FileScanException.php b/src/Exception/FileScanException.php new file mode 100644 index 0000000..e584f8c --- /dev/null +++ b/src/Exception/FileScanException.php @@ -0,0 +1,45 @@ +fileName = $fileName; + $this->errorMessage = $errorMessage; + $message = sprintf('Error scanning "%s": %s', $fileName, $errorMessage); + + parent::__construct($message); + } + + /** + * @return string + */ + public function getFileName(): string + { + return $this->fileName; + } + + /** + * @return string + */ + public function getErrorMessage(): string + { + return $this->errorMessage; + } +} diff --git a/src/Exception/SocketException.php b/src/Exception/SocketException.php index 8de0c4b..a24b806 100644 --- a/src/Exception/SocketException.php +++ b/src/Exception/SocketException.php @@ -10,10 +10,10 @@ class SocketException extends \RuntimeException protected $errorCode; /** - * @param string $message - * @param int $socketErrorCode + * @param string $message + * @param int|null $socketErrorCode */ - public function __construct($message, int $socketErrorCode = null) + public function __construct(string $message, int $socketErrorCode = null) { $this->errorCode = $socketErrorCode; if ($socketErrorCode) { @@ -27,7 +27,7 @@ public function __construct($message, int $socketErrorCode = null) * Get socket error (returned from 'socket_last_error') * @return int */ - public function getErrorCode() + public function getErrorCode(): ?int { return $this->errorCode; } diff --git a/src/ScanStrategy/AbstractScanStrategyClamdSocket.php b/src/ScanStrategy/AbstractScanStrategyClamdSocket.php index 084e117..2acb142 100644 --- a/src/ScanStrategy/AbstractScanStrategyClamdSocket.php +++ b/src/ScanStrategy/AbstractScanStrategyClamdSocket.php @@ -3,6 +3,7 @@ namespace Sineflow\ClamAV\ScanStrategy; use Sineflow\ClamAV\DTO\ScannedFile; +use Sineflow\ClamAV\Exception\FileScanException; use Sineflow\ClamAV\Socket\Socket; abstract class AbstractScanStrategyClamdSocket @@ -19,8 +20,9 @@ abstract class AbstractScanStrategyClamdSocket */ public function scan(string $filePath): ScannedFile { + // clamav can scan a directory, but we don't support this at the moment if (!is_file($filePath)) { - throw new \RuntimeException(sprintf('%s is not a file', $filePath)); + throw new FileScanException($filePath, 'Not a file.'); } $response = $this->socket->sendCommand('SCAN ' . $filePath); diff --git a/src/Socket/Socket.php b/src/Socket/Socket.php index e729f39..816d54b 100644 --- a/src/Socket/Socket.php +++ b/src/Socket/Socket.php @@ -48,15 +48,19 @@ public function sendCommand($dataIn, $flagsSend = 0, $flagsReceive = MSG_WAITALL $this->connect(); if (false === socket_send($this->socket, $dataIn, strlen($dataIn), $flagsSend)) { - throw new SocketException('Writing to socket failed', socket_last_error()); + throw new SocketException('Writing to socket failed', socket_last_error($this->socket)); } $dataOut = ''; - while ($bytes = socket_recv($this->socket, $chunk, self::MAX_READ_BYTES, $flagsReceive)) { + + do { + $bytes = socket_recv($this->socket, $chunk, self::MAX_READ_BYTES, $flagsReceive); if (false === $bytes) { - throw new SocketException('Reading from socket failed', socket_last_error()); + $socketError = socket_last_error($this->socket); + socket_close($this->socket); + throw new SocketException('Reading from socket failed', $socketError); } $dataOut .= $chunk; - } + } while ($bytes); socket_close($this->socket); return $dataOut; diff --git a/tests/Files/inaccessible.txt b/tests/Files/inaccessible.txt new file mode 100644 index 0000000..73934d7 --- /dev/null +++ b/tests/Files/inaccessible.txt @@ -0,0 +1 @@ +Permissions for this file will be set to 000 in the beginning of the test, to make sure it's inaccessible diff --git a/tests/Functional/ScannerTest.php b/tests/Functional/ScannerTest.php index b5394ef..e048655 100644 --- a/tests/Functional/ScannerTest.php +++ b/tests/Functional/ScannerTest.php @@ -3,12 +3,26 @@ namespace Sineflow\ClamAV\ScanStrategy\Tests; use PHPUnit\Framework\TestCase; +use Sineflow\ClamAV\Exception\FileScanException; use Sineflow\ClamAV\Scanner; use Sineflow\ClamAV\ScanStrategy\ScanStrategyClamdNetwork; use Sineflow\ClamAV\ScanStrategy\ScanStrategyClamdUnix; class ScannerTest extends TestCase { + private static $originalFilePermissionsOfInaccessibleFile; + + public static function setUpBeforeClass(): void + { + self::$originalFilePermissionsOfInaccessibleFile = fileperms(__DIR__.'/../Files/inaccessible.txt'); + chmod(__DIR__.'/../Files/inaccessible.txt', 0000); + } + + public static function tearDownAfterClass(): void + { + chmod(__DIR__.'/../Files/inaccessible.txt', self::$originalFilePermissionsOfInaccessibleFile); + } + public function testPingWithClamdUnix() { $scanner = new Scanner(new ScanStrategyClamdUnix()); @@ -50,13 +64,15 @@ public function testScanValidFilesWithClamdUnix(string $filePath, bool $expected /** * @dataProvider invalidFilesToCheckProvider */ - public function testScanInvalidFilesWithClamdUnix(string $filePath) + public function testScanInvalidFilesWithClamdUnix(string $filePath, string $expectedErrorMessage) { $scanner = new Scanner(new ScanStrategyClamdUnix()); - $this->expectException(\RuntimeException::class); + $this->expectException(FileScanException::class); + $this->expectExceptionMessage($expectedErrorMessage); $scanner->scan($filePath); } + /** * @dataProvider validFilesToCheckProvider */ @@ -73,11 +89,12 @@ public function testScanValidFilesWithClamdNetwork(string $filePath, bool $expec /** * @dataProvider invalidFilesToCheckProvider */ - public function testScanInvalidFilesWithClamdNetwork(string $filePath) + public function testScanInvalidFilesWithClamdNetwork(string $filePath, string $expectedErrorMessage) { $scanner = new Scanner(new ScanStrategyClamdNetwork()); - $this->expectException(\RuntimeException::class); + $this->expectException(FileScanException::class); + $this->expectExceptionMessage($expectedErrorMessage); $scanner->scan($filePath); } @@ -94,8 +111,9 @@ public function validFilesToCheckProvider() public function invalidFilesToCheckProvider() { return [ - [__DIR__.'/../Files/'], - ['file_does_not_exist'], + [__DIR__.'/../Files/', 'Error scanning "'.__DIR__.'/../Files/'.'": Not a file.'], + ['file_does_not_exist', 'Error scanning "file_does_not_exist": Not a file.'], + [__DIR__.'/../Files/inaccessible.txt', 'Error scanning "'.__DIR__.'/../Files/inaccessible.txt'.'": Access denied.'], ]; } }