Image Handler 2 Security issue?

[Shane78]

i'll PM you

[DivaVocals]

Thanks for that DrByte.. In the case of IH2, it turns out that Tim's code was written to make sure that a wider variety of hosting configurations could use it. At that time there were many hosting services that required "777" in order to make the file/folder "writable by Apache/PHP". Though some of these hosts are still out there, things have changed a bit since Tim's codebase was released..

On some hosts, the folders INSIDE the bmz_cache folder will be created with 777 permissions. While on some hosts the folders inside the bmz_cache will inherit permissions from the parent (which should hopefully be set at 755).

I've been informed that this difference likely has something to do with how PHP is implemented some hosts server.

that is, whether it is running as an apache module

or

whether is run as CGI

After doing a little more digging, I discovered that this was the code which drives the directory creation:

/**
 * Creates a directory hierachy.
 *
 * @link    http://www.php.net/manual/en/function.mkdir.php
 * @author  <[email protected]>
 * @author  Andreas Gohr <[email protected]>
 * @author  Tim Kroeger <[email protected]>
 */
function io_mkdir_p($target){
  global $bmzConf;

  if (is_dir($target) || empty($target)) return 1; // best case check first
  if (@file_exists($target) && !is_dir($target)) return 0;
  //recursion
  if (io_mkdir_p(substr($target, 0, strrpos($target, '/')))){
    if($bmzConf['safemodehack']){
      $dir = preg_replace('/^' . preg_quote(realpath($bmzConf['ftp']['root']), '/') . '/', '', $target);
      return io_mkdir_ftp($dir);
    }else{
      return @mkdir($target, 0755); // crawl back up & create dir tree
    }
  }
  return 0;
}

Given that the majority or webhosts are now configured differently than when Tim wrote IH2 in 2006.. So it's possible that some of this code may be obsolete for current hosting configurations.

The changes that I suggested are as follows:

/**
 * Creates a directory hierachy.
 *
 * @link    http://www.php.net/manual/en/function.mkdir.php
 * @author  <[email protected]>
 * @author  Andreas Gohr <[email protected]>
 * @author  Tim Kroeger <[email protected]>
 */
function io_mkdir_p($target){
  global $bmzConf;

  if (is_dir($target) || empty($target)) return 1; // best case check first
  if (@file_exists($target) && !is_dir($target)) return 0;
  //recursion
  if (io_mkdir_p(substr($target, 0, strrpos($target, '/')))){
/*    if($bmzConf['safemodehack']){
      $dir = preg_replace('/^' . preg_quote(realpath($bmzConf['ftp']['root']), '/') . '/', '', $target);
      return io_mkdir_ftp($dir);
    }else{*/
      return @mkdir($target, 0755); // crawl back up & create dir tree
 //   }
  }
  return 0;
}

Still testing, but I think this ought to resolve the issue and allow IH2 to work across most hosting configurations.

Would love your insight sir..

No, I didn't say that.

Some mods, including some of the most popular ones, require "writable" folders. That's not necessarily bad. Even Zen Cart itself requires some writable files/folders for normal operation such as the admin uploading product images, and so on. There's nothing wrong with a mod requiring writable files/folders if there's good reason for it. Granted, where possible that should be avoided, but it's not a reason to call it naughty.

In the case of your fears, what's bad is using a hosting service that requires "777" in order to make the file/folder "writable by Apache/PHP" instead of being able to use a lower permission level to accomplish the same thing.

I'm not sure why you're bent on declaring all mods bad for some reason.

[DrByte]

It's worth a try.

Maybe McShane will volunteer to test it to prove that it's working satisfactorily?

[Shane78]

It's worth a try.

Maybe McShane will volunteer to test it to prove that it's working satisfactorily?

i would be more than happy to test. In fact, if you wish i can create you a webspace on one of my servers, give you FTP access and you can play yourself?

just let me know, it will take 10 mins to set up.
(but please note, its almost 7pm in newcastle, fast approching beer o clock... try an let me know before 7 otherwise if you want to take me up on the offer it wil be 2moro)

[DivaVocals]

It's worth a try.

Maybe McShane will volunteer to test it to prove that it's working satisfactorily?

Thanks.. I also asked ckosloff and a couple of other programmers to take a look at my proposed code changes.. I also have a few folks testing it as well..

The downside is that it's likely that older hosting configurations will not be able to use IH2 which is why we will likely just comment the code out (versus removing it altogether) This way those who need the original code can still use it (with a little instruction of course..)

[Shane78]

Thanks.. I also asked ckosloff and a couple of other programmers to take a look at my proposed code changes.. I also have a few folks testing it as well..

The downside is that it's likely that older hosting configurations will not be able to use IH2 which is why we will likely just comment the code out (versus removing it altogether) This way those who need the original code can still use it (with a little instruction of course..)

the offer is there if you want it

[DrByte]

Instead of commenting out the code, you could find out where the bmz safemode flag is set, and change the rules for that to be most compatible across hosting environments.

[DivaVocals]

Instead of commenting out the code, you could find out where the bmz safemode flag is set, and change the rules for that to be most compatible across hosting environments.

Actually that safemode function is turned off by default and is never executed at all, and unless one is rooting around in the IH2 code, I don't think many know it's even there.. Which is why I commented it out.. It's a long forgotten buried feature Tim included for some hosting configurations:

From Tim's site:

The safemodehack option tries to solve problems with creating and deleting server-created directories on servers having safe_mode or safe_mode_gid enabled. It is not thoroughly tested yet so any feedback is welcome.

There is NO reference to it in the IH2 readme file (not even the one Tim created that I started out with when I updated the IH2 readme file). I wouldn't have known about it except I was digging through the IH2 code.

To use it it has to be turned on, and the settings are in a configuration file:

/* Safemode Hack */
/*$bmzConf['safemodehack'] = 0;               //read http://wiki.breakmyzencart.com/zen-cart:safemodehack !
$bmzConf['ftp']['host'] = 'localhost';
$bmzConf['ftp']['port'] = '21';
$bmzConf['ftp']['user'] = 'user';
$bmzConf['ftp']['pass'] = 'password';
$bmzConf['ftp']['root'] = DIR_FS_CATALOG;*/

and the safemode function code is here:

/**
 * Creates a directory using FTP
 * This is used when the safemode workaround is enabled
 * @author <[email protected]>
 */
function io_mkdir_ftp($dir){
  global $messageStack;
  global $bmzConf;

  if(!function_exists('ftp_connect')){
    $messageStack->add("FTP support not found - safemode workaround not usable", "error");
    return false;
  }
  
  $conn = @ftp_connect($bmzConf['ftp']['host'], $bmzConf['ftp']['port'], 10);
  if(!$conn){
    $messageStack->add("FTP connection failed", "error");
    return false;
  }

  if(!@ftp_login($conn, $bmzConf['ftp']['user'], $bmzConf['ftp']['pass'])){
    $messageStack->add("FTP login failed", "error");
    return false;
  }

  //create directory
  $ok = @ftp_mkdir($conn, $dir);
  //set permissions (using the directory umask)
  @ftp_site($conn, sprintf("CHMOD %04o %s", (0777 - $bmzConf['dmask']), $dir));

  @ftp_close($conn);
  return $ok;
}

We're toying with the idea of simply commenting out all of this code because it applies to hosting configurations that are in the serious minority.. However for the FEW hosts that still need this safemode hack the code will still be available..

Your thoughts.. Should we approach this differently??

[DrByte]

Safe Mode is highly discouraged and will no longer even exist in PHP 6.
I see no reason not to drop support for it altogether. Anyone using such old hosting that requires safe mode shouldn't be running an ecommerce site on that hosting service anyway.

[DivaVocals]

My thoughts exactly.. ckosloff seems to be erring on the side of caution.. I want to disable the safemode function, by commenting it out. Though I am not opposed to simply deleting the code a together and simply adding a line to the readme that indicates hosting setups that will NOT be able to use IH2..

So I'm am waiting for feedback from the code gurus who I have asked to look at this.. Plus I am waiting for feedback from my testers.. (My testing on my own servers is all good..) Then I can wrap this all up and get it resubmitted..

Thanks again for your input!

Safe Mode is highly discouraged and will no longer even exist in PHP 6.
I see no reason not to drop support for it altogether. Anyone using such old hosting that requires safe mode shouldn't be running an ecommerce site on that hosting service anyway.