Closed st3iny closed 3 years ago
Investigated a case yesterday when files_antivirus (clamav via socket) slowdown the uploading of new files via web.
AvirWrapper.shouldWrap
return true for uploads/web-file-upload-a37afee81a3d38104ab8808b454e5590-1629448320149/0.ocTransferId741408323.part
.
strpos($path, '/files/')
returns false the above path and is loosely compared to 0 => evaluating to true. I wonder if that might be related to your case. The user certificates are not stored in the users home directory afaik :shrug: :confused:
Investigated a case yesterday when files_antivirus (clamav via socket) slowdown the uploading of new files via web.
AvirWrapper.shouldWrap
return true foruploads/web-file-upload-a37afee81a3d38104ab8808b454e5590-1629448320149/0.ocTransferId741408323.part
.
strpos($path, '/files/')
returns false the above path and is loosely compared to 0 => evaluating to true. I wonder if that might be related to your case. The user certificates are not stored in the users home directory afaik shrug confused
Yeah we should fix that too. Replacing it with a strict comparison (===
) should do it but is out of scope of this PR.
I fixed the tests and removed redundant parentheses around the strpos(...)
call.
Yeah we should fix that too. Replacing it with a strict comparison (
===
) should do it but is out of scope of this PR.
If you change strpos($path, '/files/') == 0
to strpos($path, '/files/') === 0
it will not scan the certificate bundle to create because it's outside the users home directory. Sorry for not making that clear in my comment :see_no_evil:
Yeah we should fix that too. Replacing it with a strict comparison (
===
) should do it but is out of scope of this PR.If you change
strpos($path, '/files/') == 0
tostrpos($path, '/files/') === 0
it will not scan the certificate bundle to create because it's outside the users home directory. Sorry for not making that clear in my comment see_no_evil
Right! One single equals can have such consequences. :exploding_head:
Thanks :+1:
I wrote down my findings about this problem and added some additional fixes: https://github.com/nextcloud/files_antivirus/pull/205
Second iteration (fix check if a file is inside the users home storage)
https://github.com/nextcloud/files_antivirus/blob/e9955478525509569c1a8810024a70af1193ff4b/lib/AvirWrapper.php#L77-L83
https://github.com/nextcloud/files_antivirus/blob/e888f598f93f9864c509f19e6f1a32d79186b033/lib/AvirWrapper.php#L97-L103
When
AvirWrapper.shouldWrap
returns true the storage wrapper for scanning a file is attached => the file will be scanned.I assume the method should return true when a file is in the users home directory or moved into it. A chunk has a path like
uploads/web-file-upload-a37afee81a3d38104ab8808b454e5590-1629448320149/0.ocTransferId741408323.part
.AvirWrapper.shouldWrap
return true for this path becausestrpos($path, '/files/')
returns false and is loosely compared to 0 => evaluating to true.Note: It sounds good to me to already scan chunks during upload and it also seems to work. Work means that the chunks are sent to clamav. However clamav does not detect a virus for chunks of an uploaded zip with a movie and eicar but in the assembled file. Also the behavior when one chunk is rejected is undefined.
First iteration (exclude certificate bundle from scanner)
Currently, the certificate bundles are scanned which can lead to an endless recursion when Kaspersky is used. I solved this by ignoring certificate bundles entirely.
Feedback is highly appreciated as I'm not familiar with the code. Feel free to suggest a better solution.
Excerpt of the recursion on a live system: