When applying this fix I get a considerable loss of image quality and my quality settings applied either globaly or on specific files seems to be ignored. I have verified and confirm what makes the difference in the quality is precisely the changes advised here. I am running latest evo. Any clues what might be?
-
- 24,544 Posts
Are we talking about Evo or Revo, or both?
Is PhpThumb installed by default in the Evolution install?
I’ve verified that the fix posted here does not affect the q parameter, so something else must be causing it to fail. Perhaps the host has configured the GD or ImageMagik library incorrectly, or perhaps the phpThumb configuration is different on your install. I was able to correctly manipulate an image using multiple parameters, including the ’q’ parameter with or without the patch installed.
-
- 3 Posts
Hey all,
I just want to point out, that the patch suggested is not correct in 100% of the use cases. We’ve had multiple requests which lead to generating valid convert commands having some of these characters: [, ], #
All of those got escaped by the escapeshellcmd() invocation, which effectively prevented ImageMagick from performing the expected transformation.
Removing the escapeshellcmd() call leaves us with only $command = preg_replace(’/;.*$/’,’’,$command); which only detects ’;’ as the command terminating sequence. And yet, there are more sequences, which could be used as command terminators: &&, |, >, <, &
So, we’re left with the two possibilities: either implement the escapeshellcmd() call and have some of our commands broken, or leave out escapeshellcmd() and still have vulnerable phpthumb by means of the attacker using alternative command terminating sequences.
In addition, in most of the articles I’ve come across that describe this vulnerability, it’s said that the command injection happens by appending an arbitrary string to one of the fltr[] parameters. In fact there are other $_GET parameters, which are included in the final command string without any sanitization: dpi being one of them.
That said, the only possible solution for us was to track down all parameters which were directly included in the final command without any sanitization and possibly wrap their values with escapeshellarg() calls. Unfortunately you can’t parse the final command in a central location like SafeExec() and wrap the parameters’ values with escapeshellarg() because the ImageMagick commands could be so complex that you can’t possibly parse them and isolate only the parameter values.
That’s why we had to go through the phpThumb.class.php code and wrap the params’ values inline. Ugly solution, but since we hadn’t seen any other patch apart from the one described in this thread, we didn’t have any other options.
I’m curious as to whether someone else thinks escapeshellcmd() does not suffice, or if I was completely wrong about this.