Subscribe: RSS
  • 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?
    • I’d try commenting out the preg_replace line and leave only this one:
      $command = escapeshellcmd($command);


      It’s possible that you’re doing things that require more than one argument being passed to the command line. If that’s the case, then you might be out of luck: the hack succeeds by sending multiple arguments to the command line, first the legitimate one, then the malicious arguments. The fix I posted assumes that only the first argument is legitimate, but since I don’t know the whole scope of the code, I can’t say whether or not there are any legitimate cases where multiple "normal" commands are piped to the command line.

      If you want to send me a private message with your code zipped up and the exact steps to replicate the issue I can take a look, but no promises.
      • Quote from: Everett at Dec 10, 2010, 12:45 AM

        I’d try commenting out the preg_replace line and leave only this one:
        $command = escapeshellcmd($command);


        It’s possible that you’re doing things that require more than one argument being passed to the command line. If that’s the case, then you might be out of luck: the hack succeeds by sending multiple arguments to the command line, first the legitimate one, then the malicious arguments. The fix I posted assumes that only the first argument is legitimate, but since I don’t know the whole scope of the code, I can’t say whether or not there are any legitimate cases where multiple "normal" commands are piped to the command line.

        If you want to send me a private message with your code zipped up and the exact steps to replicate the issue I can take a look, but no promises.

        That is correct, I am using several params and options in the URL. I will PM you a sample of one of my images call so you can have a look.

        Thanks for taking time to see this.

        Jose R. Lopez
        • Are we talking about Evo or Revo, or both?

          Is PhpThumb installed by default in the Evolution install?
            Get my Book: MODX:The Official Guide
            MODX info for everyone: http://bobsguides.com/MODx.html
            My MODX Extras
            Bob's Guides is now hosted at A2 MODX Hosting
          • Quote from: BobRay at Dec 10, 2010, 10:10 PM

            Are we talking about Evo or Revo, or both?

            Is PhpThumb installed by default in the Evolution install?

            If it is my case, I am on Evo. PHPThumb does not come as a default in Evo release.

            Jose R. Lopez
            • 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.
              • Thanks Everett.

                On my shared hosting all the php functions that run shell commands are disabled. I think this removes the vulnerability, but as escapeshellcmd is also disabled, scripts containing the above fix fail. My solution, so I only need keep one working copy of phpthumb to deploy both on this and other sites is below.

                All it does is check for escapeshellcmd being disabled. If it is, it doesn’t call it, and just in case this is disabled but the other shell commands are not (admittedly an odd situation), it won’t continue to try to run the command in question at all.

                Just replace the old SafeExec function with the one below.

                -- Tim.


                	function SafeExec($command) {
                
                		if (!phpthumb_functions::FunctionIsDisabled('escapeshellcmd')) {
                
                			// --------- SECURITY FIX ----------	
                			// Strip off any commands after the first semi-colon
                			// and prepare the data to be sent to the command line.
                			// EVERETT @ www.fireproofsocks.com 9/26/2010
                			$command = preg_replace('/;.*$/','',$command); // <-- *NIX only 
                			$command = escapeshellcmd($command);
                			// ---------------------------------
                	
                			static $AllowedExecFunctions = array();
                			if (empty($AllowedExecFunctions)) {
                				$AllowedExecFunctions = array('shell_exec'=>true, 'passthru'=>true, 'system'=>true, 'exec'=>true);
                				foreach ($AllowedExecFunctions as $key => $value) {
                					$AllowedExecFunctions[$key] = !phpthumb_functions::FunctionIsDisabled($key);
                				}
                			}
                			$command .= ' 2>&1'; // force redirect stderr to stdout
                			foreach ($AllowedExecFunctions as $execfunction => $is_allowed) {
                				if (!$is_allowed) {
                					continue;
                				}
                				$returnvalue = false;
                				switch ($execfunction) {
                					case 'passthru':
                					case 'system':
                						ob_start();
                						$execfunction($command);
                						$returnvalue = ob_get_contents();
                						ob_end_clean();
                						break;
                
                					case 'exec':
                						$output = array();
                						$lastline = $execfunction($command, $output);
                						$returnvalue = implode("\n", $output);
                						break;
                
                					case 'shell_exec':
                						ob_start();
                						$returnvalue = $execfunction($command);
                						ob_end_clean();
                						break;
                				}
                				return $returnvalue;
                			}
                			return false;
                		} else {
                			return false;
                		}
                	}
                
                • Thanks, Tim.
                  • 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.
                    • Strictly speaking, the line to strip out semi-colons is optional because the escapeshellcmd function should make safe any extra arguments. The 2nd line of the patch should sanitize the other sequences you pointed out:

                      $command = escapeshellcmd($command);


                      Do you have a test case where this patch fails?