[Fixed]: Potential XSS issue reported by Ilja van Sprundel

[Joachim Müller]

Ilja van Sprundel (ivansprundel [AT] ioactive [DOT] com) by email:

Hey Joachim,
I'm emailing this to you, since I cannot find any security contact anywhere on the coppermine website, and from what I can read in the comments the bug is in code you maintain.
I recently (about 2 days ago) downloaded the coppermine source (cpg14x) and read the image_processor.php code. it seems to contain several cross site scripting bugs:

Code Select Expand

function make_form($next_form_action, $path_to_preview_image, $path_to_primary_image, $file_name) {

global $event;
global $album;
global $title;
global $caption;
global $keywords;
global $user1;
global $user2;
global $user3;
global $user4;
global $lang_image_processor_php;
...
print "<form action=\"$next_form_action\" method=\"post\">";
print "<input type=\"hidden\" name=\"album\" value=\"$album\" />";
print "<input type=\"hidden\" name=\"title\" value=\"$title\" />"; <-- potential xss
print "<input type=\"hidden\" name=\"caption\" value=\"$caption\" />"; <-- potential xss
print "<input type=\"hidden\" name=\"keywords\" value=\"$keywords\" />"; <-- potential xss
print "<input type=\"hidden\" name=\"user1\" value=\"$user1\" />"; <-- potential xss
print "<input type=\"hidden\" name=\"user2\" value=\"$user2\" />"; <-- potential xss
print "<input type=\"hidden\" name=\"user3\" value=\"$user3\" />"; <-- potential xss
print "<input type=\"hidden\" name=\"user4\" value=\"$user4\" />"; <-- potential xss
print "<input type=\"hidden\" name=\"event\" value=\"$event\" />"; <-- potential xss
print "<input type=\"hidden\" name=\"file_name\" value=\"$file_name\" />"; <-- potential xss ??
print "<input type=\"hidden\" name=\"transitory_image_path\" value=\"$path_to_primary_image\" />"; <-- potential xss ??
print "<input type=\"hidden\" name=\"preview_image_path\" value=\"$path_to_preview_image\" />";  <-- potential xss ??
...
}

//------------------------------MAIN CODE BLOCK---------------------------
...
if (!isset($_POST['degrees'])) {
...
      $event    = $_POST['event'];
      $album    = (int)$_POST['album'];
      $title    = $_POST['title'];
      $caption  = $_POST['caption'];
      $keywords = $_POST['keywords'];
      $user1    = $_POST['user1'];
      $user2    = $_POST['user2'];
      $user3    = $_POST['user3'];
      $user4    = $_POST['user4'];
...
      make_form($_SERVER[PHP_SELF], $path_to_preview_image, $path_to_primary_image, $file_name);
} else {
...
      $degrees  = $_POST['degrees'];
      $event    = $_POST['event'];
      $path_to_primary_image = $_POST['transitory_image_path'];
      $path_to_preview_image = $_POST['preview_image_path'];
      $transitory_file_name = $_POST['file_name'];
      $album    = (int)$_POST['album'];
      $title    = $_POST['title'];
      $caption  = $_POST['caption'];
      $keywords = $_POST['keywords'];
      $user1    = $_POST['user1'];
      $user2    = $_POST['user2'];
      $user3    = $_POST['user3'];
      $user4    = $_POST['user4'];
...
              print "<form action=\"db_input.php\" method=\"post\">";
              print "<input type=\"hidden\" name=\"album\" value=\"$album\" />"; <-- potential xss
              print "<input type=\"hidden\" name=\"title\" value=\"$title\" />"; <-- potential xss
              print "<input type=\"hidden\" name=\"caption\" value=\"$caption\" />"; <-- potential xss
              print "<input type=\"hidden\" name=\"keywords\" value=\"$keywords\" />"; <-- potential xss
              print "<input type=\"hidden\" name=\"user1\" value=\"$user1\" />"; <-- potential xss
              print "<input type=\"hidden\" name=\"user2\" value=\"$user2\" />"; <-- potential xss
              print "<input type=\"hidden\" name=\"user3\" value=\"$user3\" />"; <-- potential xss
              print "<input type=\"hidden\" name=\"user4\" value=\"$user4\" />"; <-- potential xss
              print "<input type=\"hidden\" name=\"event\" value=\"$event\" />"; <-- potential xss
              print "<input type=\"hidden\" name=\"transitory_image_path\" value=\"$path_to_primary_image\" />"; <-- potential xss ??
              print "<input type=\"hidden\" name=\"file_name\" value=\"$transitory_file_name\" />"; <-- potential xss ??
...

Regards,
Ilja van Sprundel.

and then

found another one. this one looks quite severe. it looks like a command injection bug when image magick is used:
imageObjectIM.class.php:

Code Select Expand

...
       function cropImage(&$clipval)
       {
...
                               $cliparray = split(",",$clipval);
           $clip_top = $cliparray[0]; <-- gets used in exec() no filtering done whatso ever (no escapeshellarg or escapeshellcmd())
           $clip_right = $cliparray[1]; <-- gets used in exec() no filtering done whatso ever (no escapeshellarg or escapeshellcmd())
           $clip_bottom = $cliparray[2]; <-- gets used in exec() no filtering done whatso ever (no escapeshellarg or escapeshellcmd())
           $clip_left = $cliparray[3]; <-- gets used in exec() no filtering done whatso ever (no escapeshellarg or escapeshellcmd())
...
          if (eregi("win",getenv('OS'))) {
              $imgFile = str_replace("'","\"" ,$imgFile );
              $cmd = "\"".str_replace("\\","/", $CONFIG['impath'])."convert\" -quality {$this->quality} {$CONFIG['im_options']} -crop {$new_w}x{$new_h}+{$clip_left}+{$clip_top} ".str_replace("\\","/" ,$imgFile )." ".str_replace("\\","/" ,$imgFile );
              exec ("\"$cmd\"", $output, $retval); <-- remote command execution
          } else {
              $cmd = "{$CONFIG['impath']}convert -quality {$this->quality} {$CONFIG['im_options']} -crop {$new_w}x{$new_h}+{$clip_left}+{$clip_top} $imgFile $imgFile";
              exec ($cmd, $output, $retval); <-- remote command execution
          }


I found one place where this is called directly with user input:
picEditor.php:

Code Select Expand


    if ($imgObj->imgRes){
        if ($_POST['clipval'] && $_POST['cropping']==true){
                $imgObj = $imgObj->cropImage($_POST['clipval']); <-- attacker can supply command exec stuff here !!
        }



Regards,
Ilja van Sprundel.

[iljavs]

Hi,
I did not intent for this to become public before it was fixed, and I find this to be a grave error on the developers part. There is a remote command execution bug in there and you're putting your users at risk. I've been told that the usual way to resolve bugs is to do it publicly and openly on the forum, which I think is great, but honestly, there should be exceptions made for security bugs, as such I'd like to request that this be removed until the bugs are fixed, and that for this one time you guys deviate from the normal policy and do this privately, over email, at least until the bug is fixed.

Regards,
Ilja van Sprundel.

[Αndré]

I don't know which boards can be accessed for regular users, but maybe we can move it to 'cpg 1.4 coding' or another board where regular users don't have access?

[Joachim Müller]

Did so. Means that Ilja can't read here neither.

Joachim

[Αndré]

I sent him a PM that we moved the topic to a dev only board.

Edit: maybe that was a little bit senseless, as we have the 'moved' message

[Nibbler]

File is not used for anything. May as well be deleted.

[Αndré]

I agree that image_processor.php can be deleted. But picEditor.php is used for 'Crop and Rotate'. We need to validate $_POST['clipval'] somehow.

[Joachim Müller]

bump

[Αndré]

Hm. Never worked with IM. Am I right that all these values should be numerical?

Code Select Expand

            $clip_top = $cliparray[0];
            $clip_right = $cliparray[1];
            $clip_bottom = $cliparray[2];
            $clip_left = $cliparray[3];

If so, we can apply int:

Code Select Expand

            $clip_top = (int)$cliparray[0];
            $clip_right = (int)$cliparray[1];
            $clip_bottom = (int)$cliparray[2];
            $clip_left = (int)$cliparray[3];
or is_numeric

Code Select Expand

            foreach($cliparray as $value) {
               if (!is_numeric($value)) die();
            }
            $clip_top = $cliparray[0];
            $clip_right = $cliparray[1];
            $clip_bottom = $cliparray[2];
            $clip_left = $cliparray[3];


[Αndré]

As far I can see, IM needs that syntax for cropping:

convert rose: -crop 40x30+10+10  crop.gif
  convert rose: -crop 40x30+40+30  crop_br.gif
  convert rose: -crop 40x30-10-10  crop_tl.gif
  convert rose: -crop 90x60-10-10  crop_all.gif
  convert rose: -crop 40x30+90+60  crop_miss.gif

So all of our passed values should be only numerical values. Now we have to decide which fix we want to apply. I suggest the !is_numeric -> die approach. Thoughts?

[Joachim Müller]

I suggest not to die if a parameter is wrong, but just to ignore the parameter silently.

[Αndré]

Committed fix in r7551.

Now we return the object as it is if any parameter isn't numeric.

Announcement thread dummy has been created in advance.

Can someone with a IM environment confirm that cropping still works with this fix? Thanks.

[Joachim Müller]

Confirmed - rotate and crop works for me on my testbed with ImageMagick

[Αndré]

I think we can release a new package. Applied same fix to cpg1.5.x in r7554.

[phill104]

Does the update script delete image_processor.php? I cannot see where it does.

[Αndré]

File will be overwritten with an 'empty' file.

[Joachim Müller]

As Αndré suggested, the dangerous file will be overwritten with a harmless one. That's the way we agreed to deal with outdated files for cpg1.4.x. This has changed in cpg1.5.x, where we actually delete those files. I have applied a corresponding delete command to cpg1.5.x in case someone upgrades from cpg1.4.26 or older to cpg1.5.x

[phill104]

Thanks for the verification.

[Joachim Müller]

Thanks to all (especially to André) who fixed this issue. Hopefully, this release will be the last in the cpg1.4.x series.
Moving this thread back to a board where all interessted parties can read it as well.