Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[QA] cannot upload files larger than ca. 20 MB #443

Open
jnweiger opened this issue May 21, 2021 · 10 comments · Fixed by #445
Open

[QA] cannot upload files larger than ca. 20 MB #443

jnweiger opened this issue May 21, 2021 · 10 comments · Fixed by #445
Assignees

Comments

@jnweiger
Copy link
Contributor

jnweiger commented May 21, 2021

Seen with files_antivirus 1.0.0-rc2 running on https://oc1070-macafee-20210519.jw-qa.owncloud.works/owncloud connected to
Ralf's McAfee ICAP instance. Both ownCloud and McAfee are on separate hetzner machines. Thus file transfer times matter.

  • 1.0.0Beta1 had no read timeout (only ClamAV and Kaspersky were supported)
  • 1.0.0-beta2 introduced a 5 sec timeout. (McAfee support was added)
  • 1.0.0-rc1 and 1.0.0-rc2 have a 10 sec timeout.

When the timeout occurs ownCloud reports Unknown ICAP response: "" and blocks the upload.
An exe file of ca 18 MB sporadically hits the limit. Bigger files (33, 41, 81, 125 MB as e.g. listed below) hit the limit reliably.
https://download.owncloud.com/desktop/ownCloud/stable/latest/win/ (ca 18 MB)
https://inkscape.org/gallery/item/8396/Inkscape-0.48.5-1-win32.exe (33 MB)
https://inkscape.org/gallery/item/3944/Inkscape-0.91-1.exe (41 MB)
https://inkscape.org/gallery/item/18066/inkscape-0.92.5-x64.msi (82 MB)
https://media.inkscape.org/dl/resources/file/inkscape-1.0.2-2-x64.msi (125 MB)

The timeout was introduced for McAfee, but also affects ClamAV and Kasperky.

Bad workaround:

  • increase the timeout to XXX sec. (150 sec is sufficient for up to 82 MB in the above setup)
  • this makes us wait XXX sec on each(!) upload (even very small files)

Acceptable hotfix:

  • No timeout on the very first read, and a small 2 sec timeout on all subsequent reads. This reliably
    processes all these files, and takes 5 times faster on small files, than the current implementation.

ICAP compliant implementation:

  • Parse the Content-Length: header in the response envelope, read exactly this payload length. No timeouts needed.
@jnweiger jnweiger mentioned this issue May 21, 2021
32 tasks
jnweiger added a commit that referenced this issue May 21, 2021
This implements the hotfix suggsted in #443
@micbar micbar assigned VicDeo and unassigned micbar May 25, 2021
@VicDeo
Copy link
Member

VicDeo commented May 25, 2021

@jnweiger here is the C-ICAP response line by line... There is no Content-Length there:

0 = "ICAP/1.0 200 OK\r\n"
1 = "Server: C-ICAP/0.4.4\r\n"
2 = "Connection: close\r\n"
3 = "ISTag: CI0001-VdGmOb1D29kBIlH75+mvLAAA\r\n"
4 = "X-Infection-Found: Type=0; Resolution=2; Threat=Win.Test.EICAR_HDB-1;\r\n"
5 = "X-Violations-Found: 1\r\n"
6 = "\t-\r\n"
7 = "\tWin.Test.EICAR_HDB-1\r\n"
8 = "\t0\r\n"
9 = "\t0\r\n"
10 = "Encapsulated: res-hdr=0, res-body=108\r\n"
11 = "\r\n"
12 = "HTTP/1.0 403 Forbidden\r\n"
13 = "Server: C-ICAP\r\n"
14 = "Connection: close\r\n"
15 = "Content-Type: text/html\r\n"
16 = "Content-Language: en\r\n"
17 = "\r\n"
18 = "1d7\r\n"
19 = "<html>\n"
20 = " <head>\n"
21 = "   <title>VIRUS FOUND</title>\n"
22 = "</head>\n"
23 = "\n"
24 = "<body>\n"
25 = "<h1>VIRUS FOUND</h1>\n"
26 = "\n"
27 = "\n"
28 = "You tried to upload/download a file that contains the virus: \n"
29 = "   <b> Win.Test.EICAR_HDB-1 </b>\n"
30 = "<br>\n"
31 = "The Http location is: \n"
32 = "<b>  127.0.0.1/ </b>\n"
33 = "\n"
34 = "<p>\n"
35 = "  For more information contact your system administrator\n"
36 = "\n"
37 = "<hr>\n"
38 = "<p>\n"
39 = "This message generated by C-ICAP service: <b> avscan?allow204=on&sizelimit=off&mode=simple </b>\n"
40 = "<br>Antivirus engine: <b> clamav-01024/25976 </b>\n"
41 = "\n"
42 = "</p>\n"
43 = "\n"
44 = "</body>\n"
45 = "</html>\n"
46 = "  \r\n"
47 = "0\r\n"
48 = "\r\n"

Is it possible to have a similar response from mcAfee?

@VicDeo
Copy link
Member

VicDeo commented May 25, 2021

In fact we don't need the whole response - we can read it line by line until the target header is found or EOF marker.
Whatever comes first.

@jnweiger
Copy link
Contributor Author

What I saw from McAfee had a Content-Length. Seems that ClamAv does not do that.

But in any case, we
a) have the length. In your example line 18 is the length (encoded in hex). and
b) have an eof marker. (lines 46, 47, 48...)

@jnweiger
Copy link
Contributor Author

jnweiger commented May 25, 2021

Here is a response I saw from McAfee (\r and \n added to make the line endings clear)
The content length (2718 bytes) is specified as a Content-Length: header, and also (in hex) as A9E\r\n at the start of the body.

ICAP/1.0 200 OK\r\n
ISTag: "00007400-18.32.12-00009990"\r\n
Encapsulated: res-hdr=0, res-body=122\r\n
\r\n
HTTP/1.1 403 VirusFound\r\n
Content-Type: text/html\r\n
Cache-Control: no-cache\r\n
Content-Length: 2718\r\n
X-Frame-Options: deny\r\n
\r\n
A9E\r\n
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">\n
<html>\n
<!-- FileName: index.html\n
     Language: [en]\n
-->\n
<!--Head-->\n
<head>\n
  <meta content="text/html; charset=UTF-8" http-equiv="Content-Type">\n
  <meta http-equiv="X-UA-Compatible" content="IE=7" />\n
  <title>McAfee Web Gateway - Notification</title>\n
  <script src="/mwg-internal/de5fs23hu73ds/files/javascript/sw.js" type="text/javascript" ></script>\n
  <link rel="stylesheet" href="/mwg-internal/de5fs23hu73ds/files/default/stylesheet.css" />\n
</head>\n
<!--/Head-->\n
<!--Body-->\n
<body onload="swOnLoad();">\n
  <table class='bodyTable'>\n
    <tr>\n
      <td class='bodyData' background='/mwg-internal/de5fs23hu73ds/files/default/img/bg_body.gif'>\n
<!--Logo-->\n
<table class='logoTable'>\n
  <tr>\n
    <td class='logoData'>\n
      <a href='http://www.mcafee.com'>\n
        <img src='/mwg-internal/de5fs23hu73ds/files/default/img/logo_mwg.png'></a>\n
    </td>\n
  </tr>\n
</table>\n
<!--/Logo-->\n
<!--Contents-->\n
<!-- FileName: VirusFound.html\n
     Language: [en]\n
-->\n
<!--Title-->\n
<table class='titleTable' background='/mwg-internal/de5fs23hu73ds/files/default/img/bg_navbar.jpg'>\n
  <tr>\n
    <td class='titleData'>\n
      Malware Detected\n
    </td>\n
  </tr>\n
</table>\n
<!--/Title-->\n
\n
<!--Content-->\n
<table class="contentTable">\n
  <tr>\n
    <td class="contentData">\n
      The transferred file contained a virus and was therefore blocked.\n
    </td>\n
  </tr>\n
</table>\n
<!--/Content-->\n
\n
<!--Info-->\n
<table class="infoTable">\n
  <tr>\n
    <td class="infoData">\n
      <b>URL: </b><script type="text/javascript">break_line("http://www.origin-server.example.com/origin-resource/form.pl");</script><br />\n
      <b>Media Type: </b>text/plain<br />\n
      <b>Virus Name: </b>EICAR test file<br />\n
      <b>Full Filename: </b>form.pl<br />\n
    </td>\n
  </tr>\n
</table>\n
<!--/Info-->\n
\n
<!--/Contents-->\n
<!--Policy-->\n
<table class='policyTable'>\n
  <tr>\n
    <td class='policyHeading'>\n
      <hr>\n
      Company Acceptable Use Policy\n
    </td>\n
  </tr>\n
  <tr>\n
    <td class='policyData'>\n
      This is an optional acceptable use disclaimer that appears on every page. You may change the wording or remove this section entirely in index.html.\n
    </td>\n
  </tr>\n
</table>\n
<!--/Policy-->\n
<!--Foot-->\n
<table class='footTable'>\n
  <tr>\n
    <td class='helpDeskData' background='/mwg-internal/de5fs23hu73ds/files/default/img/bg_navbar.jpg'>\n
      For assistance, please contact your system administrator.\n
    </td>\n
  </tr>\n
  <tr>\n
    <td class='footData'>\n
      generated <span id="time">2021-05-21 11:59:45</span> by McAfee Web Gateway\n
      <br />\n
      \n
    </td>\n
  </tr>\n
</table>\n
<!--/Foot-->\n
      </td>\n
    </tr>\n
  </table>\n
</body>\n
<!--/Body-->\n
</html>\n
\r\n
0\r\n
\r\n

@VicDeo
Copy link
Member

VicDeo commented May 26, 2021

@jnweiger I checked #445 against the content you posted above.
Should work as expected now.

@VicDeo
Copy link
Member

VicDeo commented May 27, 2021

@jnweiger

here is the response of McAfee on a clean file:

ICAP/1.0 204 No modification needed\r\n
ISTag: "00007405-8.32.140-00009997"\r\n
\r\n
---McAfee hangs here and finally returns boolean false---

Neither Content-Length nor anything meaningful is passed along in this case.

@VicDeo
Copy link
Member

VicDeo commented May 27, 2021

to overcome this I'd suggest to set timeout to 5 seconds in case ICAP status equals to 204. Not that clean but should help.

Implemented in #445 solution: on ICAP status 204 we stop further reading and consider the file to be clean.

@jnweiger
Copy link
Contributor Author

jnweiger commented May 31, 2021

Confirmed "improved" in files_anntivirus-1.0.0-rc3 (including #445).

  • Short files (both infected and clean) are now proccessed without any noticable delays in clamAV, Kasperksy and McAfee
  • But with McAfee, some longer files run into a default PHP timeout:
    • inkscape-0.45.1.exe (43 MB) uploads in ca 40 seconds.
    • inkscape-0.91.msi (85 MB) fails consistently after 64-68 seconds.
    • UniversalGcodeSender.zip (28 MB) fails consistently after 64-68 seconds.

The failures show this message:
image

Workaround: Patch the timeout to be 5 minutes. -> #446
In my setup with the McAfee scanner at vmlab.owncloud.works, the above files then succeed:

  • UniversalGcodeSender.zip 28 MB takes 3:25 time in "processing files"
  • inkscape-0.91.msi (85 MB) takes 78 sec in "processing files"

Ubuntu 20.04 ships with /etc/php/7.4/apache2/php.ini:default_socket_timeout = 60
@VicDeo should owncloud production systems have a longer timeout?

@VicDeo
Copy link
Member

VicDeo commented Jun 10, 2021

I can say only that timeout should be set at the sane value that considers:

  • desired file size limit
  • network speed
  • performance of the system that does the scan.

I see no universal value except -1 (infinite). It could be set at the runtime just before sending the data.

@jnweiger
Copy link
Contributor Author

@GeraldLeikam #445 might improve the situation here. Please retest with
1.1.0-rc.2 to see what the current limits are.

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 a pull request may close this issue.

3 participants