Filing this as a bug for the escapeshellcmd part. Paraphrasing a discussion that was cleared to be public by the Security Team:
You can see this issue by:
1. Enabling the module
2. As a user with Administrative permission set the Executable Path to a value similar to:
/bin/bash -c "echo any command can go here"The issue stems from the _clamav_get_version_via_exec function's mis-use of the escapeshellcmd function.
The escapeshellcmd function escapes any characters in a string that might be used to trick a shell command into executing arbitrary commands but does not stop an attacker from passing an arbitrary number of arguments to the executable. This allows an attacker to specify an executable path that will execute arbitrary commands for example:
/bin/bash -c ‘execute these commands’
And another comment:
There are really two issues here:
1. the misuse of the escapeshellcmd function (i'm talking about this line of code but it may appear in other locations https://cgit.drupalcode.org/clamav/tree/clamav.inc#n372)
2. Being able to specify the path of the executableThe first issue could be solved by checking if the the "Executable path" is actually a file (using file_exists for example, which i assume would return false if the path contains arbitrary arguments).
For the second issue, can you make it so that the "Executable path" path can be hard coded into a settings file? if the setting is hard coded then the web interface doesn't allow the admin user to change it. of course, if the admin user can change the settings file in the web interface then that wouldn't work. I assume that this solution would be backwards compatible as existing users could continue to use the web interface value.
This can be fixed in public because achieving command injection requires a trusted permission.
One other idea from private discussion: The module could remove the UI for inputting the path and instead just require site admins to hard-code it in $conf in settings.php.
It seems the 8.x code detects if the path is a file before executing, which helps mitigate the risk.
The issue is originally reported by https://www.drupal.org/u/jpginc
Comments
Comment #2
mcdruid commentedThanks greggles, I'll work on a patch for the D7 branch.
Comment #4
mcdruid commentedI considered using
is_executablerather thanis_filein the check, but decided it would not enhance security, and to be consistent with the D8 branch.