ClamAV doesn't support scanning multifile uploads

[Hugh Davenport]

When a user uploads using the new HTML multifile upload support, the file is scanned by clamav. The code for this scanning assumes the single upload still.

[Hugh Davenport (hugh-davenport)]

https://reviews.mahara.org/1717

[Hugh Davenport (hugh-davenport)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

 status fixreleased
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iJwEAQECAAYFAlCbHO8ACgkQuMoJ2LQ3zxH8TAP/YN4BiCJZsn5a899/0UzV31Qg
lM8LXAwZWa6zFv6t0BQUHCqe6eFK9wPp51qgCWWXjUZ3vvvVcsyeWp6626aBFKSU
pCQXI9E7huPw802nJQ9WcZXRBUmgw87ww72Tx4mybnu7SPSrkZgXdnPGSMwDs89N
oWvTpl7Xuac48e6p0lU=
=ouU+
-----END PGP SIGNATURE-----

[Yaju Mahida (yvm)]

We have investigated a serious security bug in Mahara 1.5.x Series which allows to upload virus infected file. This loophole/bug allows to upload the infected file without scanning it and from Mahara user interface this gives an impression that the file was scanned successfully.

The issue is happening in the function mahara_clam_scan_file defined in /htdocs/lib/uploadmanager.php.

This ClamAV Bug #1055239 fix this issue which is only applied to Mahara 1.6.0. According to my investigation this should be applied to all existing supported releases of Mahara including 1.5.x.

Exactly this happens at /htdocs/lib/uploadmanager.php Line 309 $cmd = $pathtoclam .' '. escapeshellarg($fullpath) ." 2>&1"; where escapeshellarg($fullpath) returns NULL value and as a result the ClamAV is scanning existing directory and gives OK status for virus infected file. We used the ClamAV test file to replicate this issue.

The PHP escapeshellarg function expects a string and an array is passed to it which gives NULL as a result and due to same reason chmod also fails at line 313.

Following is the error log entry,

Wed Dec 12 09:55:07 2012] [error] [client 192.168.0.110] [WAR] 60 (lib/uploadmanager.php:313) chmod() expects parameter 1 to be string, array given, referer: https://localhost/view/blocks.php?id=10
[Wed Dec 12 09:55:07 2012] [error] [client 192.168.0.110] Call stack (most recent first):, referer: https://localhost/view/blocks.php?id=10
[Wed Dec 12 09:55:07 2012] [error] [client 192.168.0.110] * log_message("chmod() expects parameter 1 to be string, array gi...", 8, true, true, "/htdocs/lib/uploadmanager.php", 313) at /htdocs/lib/errors.php:446, referer: https://$
[Wed Dec 12 09:55:07 2012] [error] [client 192.168.0.110] * error(2, "chmod() expects parameter 1 to be string, array gi...", "/htdocs/lib/uploadmanager.php", 313, array(size 6)) at Unknown:0, referer: https://localhost/v$
[Wed Dec 12 09:55:07 2012] [error] [client 192.168.0.110] * chmod(array(size 1), 420) at /htdocs/lib/uploadmanager.php:313, referer: https://localhost/view/blocks.php?id=10
[Wed Dec 12 09:55:07 2012] [error] [client 192.168.0.110] * mahara_clam_scan_file(array(size 5), 0) at /htdocs/lib/uploadmanager.php:114, referer: https://localhost/view/blocks.php?id=10
[Wed Dec 12 09:55:07 2012] [error] [client 192.168.0.110] * upload_manager->preprocess_file() at /htdocs/artefact/file/lib.php:950, referer: https://localhost/view/blocks.php?id=10
[Wed Dec 12 09:55:07 2012] [error] [client 192.168.0.110] * ArtefactTypeFile::save_uploaded_file("userfile", object(stdClass), 0) at /htdocs/artefact/file/form/elements/filebrowser.php:756, referer: https://localhost/view$
[Wed Dec 12 09:55:07 2012] [error] [client 192.168.0.110] * pieform_element_filebrowser_upload(object(Pieform), array(size 13), array(size 5)) at /htdocs/artefact/file/form/elements/filebrowser.php:587, referer: https://eportfolio.us$
[Wed Dec 12 09:55:07 2012] [error] [client 192.168.0.110] * pieform_element_filebrowser_doupdate(object(Pieform), array(size 13)) at /htdocs/artefact/file/form/elements/filebrowser.php:356, referer: https://localhost/view$
[Wed Dec 12 09:55:07 2012] [error] [client 192.168.0.110]...

[Yaju Mahida (yvm)]

Attached the uploadmanager.php which we used for debugging.

PHP debuggging without fix 2 files uploaded

In the Loop isset($inputindex) :
$tmpfile: NULL

In the Loop is_array($file) && is_uploaded_file($tmpname)
$fullpath: array(2) { [0]=> string(14) "/tmp/phpDdqaTc" [1]=> string(14) "/tmp/phpM4aRv1" }

$escapeshellarg($fullpath): NULL
$escapeshellcmd($fullpath): NULL

$cmd: string(24) "/usr/bin/clamdscan 2>&1"

$output: array(5) { [0]=> string(28) "/htdocs/artefact/file: OK" [1]=> string(0) "" [2]=> string(36) "----------- SCAN SUMMARY -----------" [3]=> string(17) "Infected files: 0" [4]=> string(25) "Time: 0.124 sec (0 m 0 s)" }

$return): int(0) In the Loop isset($inputindex) : $tmpfile: NULL In the Loop is_array($file) && is_uploaded_file($tmpname) $fullpath: array(2) { [0]=> string(14) "/tmp/phpDdqaTc" [1]=> string(14) "/tmp/phpM4aRv1" } $escapeshellarg($fullpath): NULL $escapeshellcmd($fullpath): NULL $cmd: string(24) "/usr/bin/clamdscan 2>&1" $output: array(5) { [0]=> string(28) "/htdocs/artefact/file: OK" [1]=> string(0) "" [2]=> string(36) "----------- SCAN SUMMARY -----------" [3]=> string(17) "Infected files: 0" [4]=> string(25) "Time: 0.112 sec (0 m 0 s)" }

$return): int(0)

PHP debuggging without fix 1 file uploaded

In the Loop isset($inputindex) :
$tmpfile: NULL

In the Loop is_array($file) && is_uploaded_file($tmpname)
$fullpath: array(1) { [0]=> string(14) "/tmp/php3gkh1m" }

$escapeshellarg($fullpath): NULL
$escapeshellcmd($fullpath): NULL

$cmd: string(24) "/usr/bin/clamdscan 2>&1"

$output: array(5) { [0]=> string(28) "/htdocs/artefact/file: OK" [1]=> string(0) "" [2]=> string(36) "----------- SCAN SUMMARY -----------" [3]=> string(17) "Infected files: 0" [4]=> string(25) "Time: 0.124 sec (0 m 0 s)" }

$return: int(0)

PHP debuggging with fix 2 files uploaded

In the Loop isset($inputindex) :
$tmpfile: NULL

In the Loop is_array($file) && is_uploaded_file($tmpname)
$fullpath: string(14) "/tmp/phpvnyvSe"

$escapeshellarg($fullpath): string(16) "'/tmp/phpvnyvSe'"
$escapeshellcmd($fullpath): string(14) "/tmp/phpvnyvSe"

$cmd: string(38) "/usr/bin/clamdscan /tmp/phpvnyvSe 2>&1"

$output: array(5) { [0]=> string(38) "/tmp/phpvnyvSe: ClamAV-Test-File FOUND" [1]=> string(0) "" [2]=> string(36) "----------- SCAN SUMMARY -----------" [3]=> string(17) "Infected files: 1" [4]=> string(25) "Time: 0.258 sec (0 m 0 s)" }

$return): int(1)

In the Loop isset($inputindex) :
$tmpfile: NULL

In the Loop is_array($file) && is_uploaded_file($tmpname)
$fullpath: string(14) "/tmp/php0nP112"

$escapeshellarg($fullpath): string(16) "'/tmp/php0nP112'"
$escapeshellcmd($fullpath): string(14) "/tmp/php0nP112"

$cmd: string(38) "/usr/bin/clamdscan /tmp/php0nP112 2>&1"

$output: array(5) { [0]=> string(38) "/tmp/php0nP112: ClamAV-Test-File FOUND" [1]=> string(0) "" [2]=> string(36) "----------- SCAN SUMMARY -----------" [3]=> string(17) "Infected files: 1" [4]=> string(25) "Time: 0.237 sec (0 m 0 s)" }

$return): int(1)

PHP debuggging with fix 1 file uploaded

In the Loop isset($inputindex) :
$tmpfile: NULL

In the Loop is_array($file) && is_uploaded_file($tmpname)
$fullpath: string(14) "/tmp/php8pJlgm"

$escapeshellarg($fullpath): string(16) "'/tm...

[Aaron Wells (u-aaronw)]

As Yaju points out, we should push this fix to 1.5 as well. I've pushed a security patch for it to gerrit. We can include it in the 1.5.11 release.

https://reviews.mahara.org/2219

[Aaron Wells (u-aaronw)]

Hi Yaju,

Subscribing you to this bug since you had commented on it earlier. We're going to backport this fix into the 1.5.11 release.

Cheers,
Aaron

[Aaron Wells (u-aaronw)]

Well, I accidentally already pushed a non-secret patch for 1.5 to https://reviews.mahara.org/2218. Let's just go with that.