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.
sineflow · Jan 18, 2022 · 5601b75 · 5601b75
1 parent 5fba062
commit 5601b75
Show file tree

Hide file tree

Showing 8 changed files with 99 additions and 18 deletions.
diff --git 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
@@ -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
@@ -0,0 +1,45 @@
+<?php
+
+namespace Sineflow\ClamAV\Exception;
+
+class FileScanException extends \RuntimeException
+{
+    /**
+     * @var string
+     */
+    private $fileName;
+
+    /**
+     * @var string
+     */
+    private $errorMessage;
+
+    /**
+     * @param string $fileName
+     * @param string $errorMessage
+     */
+    public function __construct(string $fileName, string $errorMessage)
+    {
+        $this->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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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.'],
         ];
     }
 }