php-clamav icon indicating copy to clipboard operation
php-clamav copied to clipboard

Add a check for eof to avoid adding empty string to the socket data.

Open vagiguere opened this issue 2 years ago • 10 comments

What does this PR do?

Do not send the last empty string through the socket for files of less than 8192 bytes. This avoid detection errors for eicar files.

Test Plan

Tested the function with multiple eicar files.

Related PRs and Issues

Resolve : https://github.com/appwrite/php-clamav/issues/26

Have you read the Contributing Guidelines on issues?

(Write your answer here.)

vagiguere avatar May 24 '23 13:05 vagiguere

Hi - just to say I forked the repo and applied this fix and it works great.

adsjim avatar Sep 22 '23 10:09 adsjim

This fix still fails with file https://github.com/fire1ce/eicar-standard-antivirus-test-files/blob/master/eicar-adobe-acrobat-javascript-alert.pdf I removed chunk logic and then that one was detected as well.

rang501 avatar Oct 31 '23 12:10 rang501

@rang501 will removing the chunking cause issues for large files though?

adsjim avatar Oct 31 '23 13:10 adsjim

Not sure, we have file size limits (under 100MB), so this should not be a problem. I checked how the Drupal module does this and I didn't see any chunking, it was fully sent to Clamav.

Anyway, the current fix did improve detection but did not fix it fully. I see it as a security hole to bypass detection and made a decision to skip chunking which worked and the file was detected.

The failed files were https://github.com/fire1ce/eicar-standard-antivirus-test-files: eicar-adobe-acrobat-javascript-alert eicar-powerpoint-action-macro-msgbox.ppt

rang501 avatar Oct 31 '23 13:10 rang501

Drupal source code here, for anyone working on this project https://github.com/stemount/clamav-drupal-rest/blob/master/src/Scanner/DaemonUnixSocket.php

adsjim avatar Oct 31 '23 13:10 adsjim

Non-chunking version of the method is below. I don't think the chunking is necessary actually because stream_copy_to_stream does this internally anyway.

   public function fileScanInStream(string $file): bool
    {
        $file_handler = fopen($file, 'r');
        $scanner_handler = socket_export_stream($this->getSocket());

        // Push to the ClamAV socket.
        $bytes = filesize($file);
        fwrite($scanner_handler, "zINSTREAM\0");
        fwrite($scanner_handler, pack("N", $bytes));
        stream_copy_to_stream($file_handler, $scanner_handler);
    
        // Send a zero-length block to indicate that we're done sending file data.
        fwrite($scanner_handler, pack("N", 0));
    
        // Request a response from the service.
        $response = trim(fgets($scanner_handler));
    
        fclose($scanner_handler);

        return preg_match('/^stream: OK$/', $response);
    }`

adsjim avatar Mar 13 '24 11:03 adsjim

Also using the version from https://github.com/appwrite/php-clamav/pull/27#issuecomment-1994168446 - works like a charm. Would be great to see this merged.

pehbehbeh avatar Jul 25 '24 07:07 pehbehbeh

@adsjim I added your change to the PR in ef971c2e6b98098a54e8ad84411c5a7525be743d. Thank you. Hope someone accept the PR now to stop working with a fork.

vagiguere avatar Jul 25 '24 14:07 vagiguere

This PR works for me. Please merge.

xrow avatar Sep 25 '24 07:09 xrow