forum.coppermine-gallery.net
Dev Board => cpg1.4 Testing/Bugs => cpg1.4 Testing/Bugs: FIXED/CLOSED => Topic started by: Joachim Müller on April 25, 2010, 09:55:37 pm
-
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:
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 thenfound 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.
-
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.
-
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?
-
Did so. Means that Ilja can't read here neither.
Joachim
-
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 ::)
-
File is not used for anything. May as well be deleted.
-
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.
-
bump
-
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];
-
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?
-
I suggest not to die if a parameter is wrong, but just to ignore the parameter silently.
-
Committed fix in r7551 (http://coppermine.svn.sourceforge.net/viewvc/coppermine/trunk/cpg1.4.x/include/imageObjectIM.class.php?r1=7551&r2=7550&pathrev=7551).
Now we return the object as it is if any parameter isn't numeric.
Announcement thread dummy (http://forum.coppermine-gallery.net/index.php/topic,65023.0.html) has been created in advance.
Can someone with a IM environment confirm that cropping still works with this fix? Thanks.
-
Confirmed - rotate and crop works for me on my testbed with ImageMagick
-
I think we can release a new package. Applied same fix to cpg1.5.x in r7554.
-
Does the update script delete image_processor.php? I cannot see where it does.
-
File will be overwritten with an 'empty' file.
-
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
-
Thanks for the verification.
-
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.