Advanced search  

News:

cpg1.5.48 Security release - upgrade mandatory!
The Coppermine development team is releasing a security update for Coppermine in order to counter a recently discovered vulnerability. It is important that all users who run version cpg1.5.46 or older update to this latest version as soon as possible.
[more]

Pages: [1]   Go Down

Author Topic: [Fixed]: Potential XSS issue reported by Ilja van Sprundel  (Read 34140 times)

0 Members and 1 Guest are viewing this topic.

Joachim Müller

  • Dev Team member
  • Coppermine addict
  • ****
  • Offline Offline
  • Gender: Male
  • Posts: 47843
  • aka "GauGau"
    • gaugau.de
[Fixed]: Potential XSS issue reported by Ilja van Sprundel
« on: April 25, 2010, 09:55:37 pm »

Ilja van Sprundel (ivansprundel [AT] ioactive [DOT] com) by email:
Quote
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]
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
Quote
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]
...
        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]
    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.
« Last Edit: May 25, 2010, 04:40:38 pm by Joachim Müller »
Logged

iljavs

  • Coppermine newbie
  • Offline Offline
  • Posts: 1
Re: Potential XSS issue reported by Ilja van Sprundel
« Reply #1 on: April 26, 2010, 05:21:22 pm »

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.
Logged

Αndré

  • Administrator
  • Coppermine addict
  • *****
  • Country: de
  • Offline Offline
  • Gender: Male
  • Posts: 15764
Re: Potential XSS issue reported by Ilja van Sprundel
« Reply #2 on: April 26, 2010, 05:56:42 pm »

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?
Logged

Joachim Müller

  • Dev Team member
  • Coppermine addict
  • ****
  • Offline Offline
  • Gender: Male
  • Posts: 47843
  • aka "GauGau"
    • gaugau.de
Re: Potential XSS issue reported by Ilja van Sprundel
« Reply #3 on: April 26, 2010, 05:57:45 pm »

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

Joachim
Logged

Αndré

  • Administrator
  • Coppermine addict
  • *****
  • Country: de
  • Offline Offline
  • Gender: Male
  • Posts: 15764
Re: Potential XSS issue reported by Ilja van Sprundel
« Reply #4 on: April 26, 2010, 08:24:00 pm »

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 ::)
Logged

Nibbler

  • Guest
Re: Potential XSS issue reported by Ilja van Sprundel
« Reply #5 on: April 26, 2010, 08:58:15 pm »

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

Αndré

  • Administrator
  • Coppermine addict
  • *****
  • Country: de
  • Offline Offline
  • Gender: Male
  • Posts: 15764
Re: Potential XSS issue reported by Ilja van Sprundel
« Reply #6 on: April 27, 2010, 07:50:58 am »

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.
Logged

Joachim Müller

  • Dev Team member
  • Coppermine addict
  • ****
  • Offline Offline
  • Gender: Male
  • Posts: 47843
  • aka "GauGau"
    • gaugau.de
Re: Potential XSS issue reported by Ilja van Sprundel
« Reply #7 on: May 19, 2010, 04:51:16 pm »

bump
Logged

Αndré

  • Administrator
  • Coppermine addict
  • *****
  • Country: de
  • Offline Offline
  • Gender: Male
  • Posts: 15764
Re: Potential XSS issue reported by Ilja van Sprundel
« Reply #8 on: May 19, 2010, 05:41:06 pm »

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

If so, we can apply int:
Code: [Select]
            $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]
            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];
Logged

Αndré

  • Administrator
  • Coppermine addict
  • *****
  • Country: de
  • Offline Offline
  • Gender: Male
  • Posts: 15764
Re: Potential XSS issue reported by Ilja van Sprundel
« Reply #9 on: May 19, 2010, 05:52:52 pm »

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?
Logged

Joachim Müller

  • Dev Team member
  • Coppermine addict
  • ****
  • Offline Offline
  • Gender: Male
  • Posts: 47843
  • aka "GauGau"
    • gaugau.de
Re: Potential XSS issue reported by Ilja van Sprundel
« Reply #10 on: May 20, 2010, 07:11:30 am »

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

Αndré

  • Administrator
  • Coppermine addict
  • *****
  • Country: de
  • Offline Offline
  • Gender: Male
  • Posts: 15764
Re: Potential XSS issue reported by Ilja van Sprundel
« Reply #11 on: May 20, 2010, 08:58:13 am »

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.
Logged

Joachim Müller

  • Dev Team member
  • Coppermine addict
  • ****
  • Offline Offline
  • Gender: Male
  • Posts: 47843
  • aka "GauGau"
    • gaugau.de
Re: Potential XSS issue reported by Ilja van Sprundel
« Reply #12 on: May 20, 2010, 10:36:41 am »

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

Αndré

  • Administrator
  • Coppermine addict
  • *****
  • Country: de
  • Offline Offline
  • Gender: Male
  • Posts: 15764
Re: Potential XSS issue reported by Ilja van Sprundel
« Reply #13 on: May 20, 2010, 11:17:59 am »

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

phill104

  • Administrator
  • Coppermine addict
  • *****
  • Country: gb
  • Offline Offline
  • Gender: Male
  • Posts: 4885
    • Windsurf.me
Re: Potential XSS issue reported by Ilja van Sprundel
« Reply #14 on: May 24, 2010, 09:53:14 pm »

Does the update script delete image_processor.php? I cannot see where it does.
Logged
It is a mistake to think you can solve any major problems just with potatoes.

Αndré

  • Administrator
  • Coppermine addict
  • *****
  • Country: de
  • Offline Offline
  • Gender: Male
  • Posts: 15764
Re: Potential XSS issue reported by Ilja van Sprundel
« Reply #15 on: May 24, 2010, 09:55:34 pm »

File will be overwritten with an 'empty' file.
Logged

Joachim Müller

  • Dev Team member
  • Coppermine addict
  • ****
  • Offline Offline
  • Gender: Male
  • Posts: 47843
  • aka "GauGau"
    • gaugau.de
Re: Potential XSS issue reported by Ilja van Sprundel
« Reply #16 on: May 25, 2010, 08:27:00 am »

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

Logged

phill104

  • Administrator
  • Coppermine addict
  • *****
  • Country: gb
  • Offline Offline
  • Gender: Male
  • Posts: 4885
    • Windsurf.me
Re: Potential XSS issue reported by Ilja van Sprundel
« Reply #17 on: May 25, 2010, 08:45:40 am »

Thanks for the verification.
Logged
It is a mistake to think you can solve any major problems just with potatoes.

Joachim Müller

  • Dev Team member
  • Coppermine addict
  • ****
  • Offline Offline
  • Gender: Male
  • Posts: 47843
  • aka "GauGau"
    • gaugau.de
Re: Potential XSS issue reported by Ilja van Sprundel
« Reply #18 on: May 25, 2010, 04:38:42 pm »

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.
Logged
Pages: [1]   Go Up
 

Page created in 0.021 seconds with 19 queries.