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

News:

CPG Release 1.6.26
Correct PHP8.2 issues with user and language managers.
Additional fixes for PHP 8.2
Correct PHP8 error with SMF 2.0 bridge.
Correct IPTC supplimental category parsing.
Download and info HERE

Main Menu

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

Started by Joachim Müller, April 25, 2010, 09:55:37 PM

Previous topic - Next topic

0 Members and 1 Guest are viewing this topic.

Joachim Müller

Ilja van Sprundel (ivansprundel [AT] ioactive [DOT] com) by email:
QuoteHey 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:

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
Quotefound another one. this one looks quite severe. it looks like a command injection bug when image magick is used:
imageObjectIM.class.php:
...
       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:
    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


Αndré

Hm. Never worked with IM. Am I right that all these values should be numerical?
            $clip_top = $cliparray[0];
            $clip_right = $cliparray[1];
            $clip_bottom = $cliparray[2];
            $clip_left = $cliparray[3];


If so, we can apply int:
            $clip_top = (int)$cliparray[0];
            $clip_right = (int)$cliparray[1];
            $clip_bottom = (int)$cliparray[2];
            $clip_left = (int)$cliparray[3];

or is_numeric
            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:
Quote from: http://www.imagemagick.org/Usage/crop/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.
It is a mistake to think you can solve any major problems just with potatoes.

Α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

It is a mistake to think you can solve any major problems just with potatoes.

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.