Add AVMaxFileSize config option #133

Merged
merged 9 commits into from Nov 21, 2016

Projects

None yet

7 participants

@VicDeo
Member
VicDeo commented Nov 2, 2016 edited

Description

  • Adds File size limit
  • Fixes #124 and #125

Changes in Docs:

Stream Length (bytes) 26214400 (25Mb) by default. Should be <= than StreamMaxLength ClamAV value
File size limit, -1 means no limit (bytes) -1 by default - prevent background scanner from checking large files (Has no effect while scanning newly uploaded files)

To test:

  1. set StreamMaxLength in /etc/clamd.conf to e.g. 10M
  2. Configure the app in daemon or daemon(socket) mode
  3. cat several 100Mb files with EICAR signature
  4. Try upload or background scan
@VicDeo VicDeo Add AVMaxFileSize config option
2c3e6ec
@VicDeo VicDeo added this to the 9.2 milestone Nov 2, 2016
@mention-bot

@VicDeo, thanks for your PR! By analyzing the history of the files in this pull request, we identified @fcturner, @DeepDiver1975 and @vgezer to be potential reviewers.

@VicDeo VicDeo added 3 - To review and removed 2 - Developing labels Nov 3, 2016
@VicDeo VicDeo changed the title from [WIP] Add AVMaxFileSize config option to Add AVMaxFileSize config option Nov 4, 2016
@VicDeo
Member
VicDeo commented Nov 4, 2016

@PVince81 please have a look

VicDeo added some commits Nov 3, 2016
@VicDeo VicDeo Reset socket when config limit is reached. Log only once per file 72a308a
@VicDeo VicDeo Lets have avStreamMaxLength option too 0bfc37d
@VicDeo VicDeo Mock interface, not private class
c6c0f38
@VicDeo VicDeo Repeat last successful chunk while reopening connection
106c285
+ $qb->expr()->eq('fc.storage', 'ss.numeric_id'),
+ $qb->expr()->orX(
+ $qb->expr()->like('ss.id', $qb->expr()->literal('local::%')),
+ $qb->expr()->like('ss.id', $qb->expr()->literal('home::%'))
@PVince81
PVince81 Nov 10, 2016 Contributor

unrelated, but reminded me of a potential issue, raised as #137

@PVince81
Contributor

Code looks good.

@owncloud/qa can you help testing ?

@VicDeo Any chance to add unit tests of some sorts ? That quite some complex logic there.
Even a test that only covers the stream stuff using a dummy stream would be nice.

For the docs change, please raise a ticket in the documentation repo.

@PVince81
Contributor

@VicDeo question: what if the virus signature overlaps with the retry-boundary, can it still be detected properly ?

@PVince81
Contributor

maybe the unit test could have its own dummy signature and have the mock stream receiver detect it (which simulates whatever clamd does)

@VicDeo
Member
VicDeo commented Nov 10, 2016

@PVince81

what if the virus signature overlaps with the retry-boundary, can it still be detected properly ?

E.g. When Stream Length contains 3 chunks. And file has 7.
The scanning will be done as
1st scan request: chunks 1-2-3
2nd scan request: chunks 3-4-5
3rd scan request: chunks 5-6-7

if a signature is spread across chunks 2-3-4 - the answer is No
In general ClamAV doesn't support large files at all. So it is still an improvement :)

Hm. I've just came upon an idea to hide chunk size from UI and make it completely internal.

VicDeo added some commits Nov 11, 2016
@VicDeo VicDeo Remove chunkSize from UI. Put shutdownScanner back
7dff334
@VicDeo VicDeo Add Dummy Clam
b3480ae
@VicDeo
Member
VicDeo commented Nov 15, 2016

@PVince81 I added a dumb avir server but it fails to scan more than one file unfortunately

@sidey79
sidey79 commented Nov 15, 2016

With Last PR, Chunk Size can be removed from the docs i think.

@VicDeo
Member
VicDeo commented Nov 15, 2016

@sidey79 thanks, updated

@ms270169

(Conversation before done in #125)
@VicDeo I have tested branch fix-125 (b3480ae).

The byteCount should be cleared to zero, if the stream is closed (scanner.php#L143). Test with EICAR Virus Signature file results in Unsupported Media Type. File not uploaded.

@VicDeo
Member
VicDeo commented Nov 16, 2016

@ms270169 byteCount is reset to 0 in Scanner::initScanner that is called just one line below

scanner.php#L144

@VicDeo VicDeo closed this Nov 16, 2016
@VicDeo VicDeo reopened this Nov 16, 2016
@ms270169

@VicDeo, you are right (I was wrong), byteCount ist cleared to zero in initScanner(). Virus scanning works now as desired.

I tested it with the following configuration:

  • testfile: 30000 bytes length, 68 byte EICAR virus signature on position 16360-17027
  • config-value Chunk Size: 8192 (config not used, because I add the file with plus button)
  • used chunk-size: 8192
  • config-value Stream Length: 20000
  • config-value File size limit: -1

Virus signature overlaps second and third chunk. Virus is only detected if second chunk is repeated after reinitialisation of stream (if block overlapping is implemented).

Result:
Virus detection successful, File not accepted by the cloud-system.

The user gets the info "Unsupported media Type", which is a confusing message. If the other bug, leading to this wrong message could not be fixed (see #125 (comment)), I would recommend to add a log entry in case of virus detection (for example before avirwrapper.php#L99).

@VicDeo
Member
VicDeo commented Nov 16, 2016 edited

@ms270169
Filesystem InvalidContentExeption is mapped to HTTP status 415.
"Unsupported media Type" is a text representation for HTTP status 415.
There is no special HTTP status "User just uploaded a virus" unfortunately.

I can't do anything better on the antivirus side. 403 with message "Forbidden" is neither too much descriptive. :/

@VicDeo
Member
VicDeo commented Nov 17, 2016

grr. It drive me nuts. All methods of Config mock return null on the second test execution.

@ms270169

@VicDeo , maybe you have an idea how to better visualize the message. Because the information about a detected virus is inside the HTTP 415 response. If the message of the response would be shown, everything would be clear for the user...

<d:error xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns">
  <s:exception>OCA\DAV\Connector\Sabre\Exception\UnsupportedMediaType</s:exception>
  <s:message>Virus Eicar-Test-Signature is detected in the file. Upload cannot be completed.</s:message>
</d:error>
@VicDeo
Member
VicDeo commented Nov 21, 2016

I still have no idea why PHPUnit mocks return null for the second testcase.
I created own mocks instead of mocking with PHPUnit API.
It works this way.

@VicDeo
Member
VicDeo commented Nov 21, 2016

@PVince81 ready from my POV

@PVince81
Contributor

@VicDeo from what I see this was tested and only needs a code review, correct ?

@VicDeo VicDeo Fix ItemTest
eb7dfec
@VicDeo
Member
VicDeo commented Nov 21, 2016

@PVince81 yes
@ms270169 helped me a lot with a design overview.

@PVince81
Contributor

The code looks good, great to have tests!

@VicDeo I didn't find where "AVMaxFileSize" is covered / passed in the tests, mind pointing me there ? I guess it probably has another name

@VicDeo
Member
VicDeo commented Nov 21, 2016

@PVince81 Not covered.
It is used in background scanner only, because file size is unpredictable while using a stream wrapper.
BackgroundScanner::getFilesForScan is the only place in code where is it used.

@PVince81
Contributor

Ok, fine 👍

@PVince81 PVince81 merged commit 23c1c03 into master Nov 21, 2016

3 checks passed

Scrutinizer 5 new issues, 8 updated code elements
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@PVince81 PVince81 deleted the fix-125 branch Nov 21, 2016
@VicDeo VicDeo referenced this pull request Nov 22, 2016
Merged

[Stable9.1] fix 125 #140

@VicDeo
Member
VicDeo commented Nov 25, 2016 edited

Stable9.1: #140
Stable9: #144
Stable8.2: #145

@zubanst
zubanst commented Dec 14, 2016

Any back-port plan for 9.0.x? In 9.0.6 I have the same issue described in #125 .

@VicDeo VicDeo referenced this pull request Dec 15, 2016
Merged

Stable9 fix 125 #144

@VicDeo
Member
VicDeo commented Dec 15, 2016

@zubanst would you like to test #144 ?

@zubanst
zubanst commented Dec 15, 2016 edited

@VicDeo I tested it on 9.0.6 and it works. I don't get anymore the log file filled. I tested also the reaction online trying to upload an EICAR file and it detects it and stops the upload. For me the fix can be applied in 9.0.6

@VicDeo VicDeo referenced this pull request Dec 19, 2016
Merged

Stable8.2: Fix 125 #145

@SergioBertolinSG
Member

Updated steps:

To reproduce:

1. set StreamMaxLength in /etc/clamd.conf to e.g. 10M
2. Configure the app in daemon or daemon(socket) mode
3. In files_antivirus config in administration change the values:
   Stream Length   to 10000000 bytes
   File size limit, -1 means no limit  to 10000000 bytes
4. cat several 100Mb files with EICAR signature
5. Try upload or background scan

Expected: It detects the virus.
Not expected: It uploads the file.

@VicDeo
Member
VicDeo commented Feb 1, 2017

@SergioBertolinSG
Indeed Stream Length in files_antivirus configuration should match Clamav StreamMaxLength option.
File size limit has no effect on upload.

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