Skip to content

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

Open
vagiguere wants to merge 2 commits into
appwrite:masterfrom
vagiguere:fix-26-submit-a-pull-request-to-fix-fileScanInStream
Open

Add a check for eof to avoid adding empty string to the socket data.#27
vagiguere wants to merge 2 commits into
appwrite:masterfrom
vagiguere:fix-26-submit-a-pull-request-to-fix-fileScanInStream

Conversation

@vagiguere

Copy link
Copy Markdown

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 : #26

Have you read the Contributing Guidelines on issues?

(Write your answer here.)

@adsjim

adsjim commented Sep 22, 2023

Copy link
Copy Markdown

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

@rang501

rang501 commented Oct 31, 2023

Copy link
Copy Markdown

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.

@adsjim

adsjim commented Oct 31, 2023

Copy link
Copy Markdown

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

@rang501

rang501 commented Oct 31, 2023

Copy link
Copy Markdown

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

@adsjim

adsjim commented Oct 31, 2023

Copy link
Copy Markdown

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

@adsjim

adsjim commented Mar 13, 2024

Copy link
Copy Markdown

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);
    }`

@pehbehbeh

Copy link
Copy Markdown

Also using the version from #27 (comment) - works like a charm. Would be great to see this merged.

@vagiguere

vagiguere commented Jul 25, 2024

Copy link
Copy Markdown
Author

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

@xrow

xrow commented Sep 25, 2024

Copy link
Copy Markdown

This PR works for me. Please merge.

@ktallafus

Copy link
Copy Markdown

Working perfectly. Would be great to see this merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

🐛 Bug Report: ClamAv::fileScanInStream does not detect eicar.com file

6 participants