summaryrefslogtreecommitdiff
path: root/modules
diff options
context:
space:
mode:
Diffstat (limited to 'modules')
-rw-r--r--modules/gallery/helpers/gallery_graphics.php5
-rw-r--r--modules/gallery/helpers/graphics.php11
-rw-r--r--modules/gallery/helpers/legal_file.php12
-rw-r--r--modules/gallery/helpers/movie.php10
-rw-r--r--modules/gallery/helpers/photo.php9
-rw-r--r--modules/gallery/models/item.php71
-rw-r--r--modules/gallery/tests/Item_Model_Test.php16
-rw-r--r--modules/gallery/tests/Movie_Helper_Test.php36
-rw-r--r--modules/gallery/tests/Photo_Helper_Test.php18
-rw-r--r--modules/watermark/controllers/admin_watermarks.php13
10 files changed, 149 insertions, 52 deletions
diff --git a/modules/gallery/helpers/gallery_graphics.php b/modules/gallery/helpers/gallery_graphics.php
index b78bd9a7..eb76353f 100644
--- a/modules/gallery/helpers/gallery_graphics.php
+++ b/modules/gallery/helpers/gallery_graphics.php
@@ -172,6 +172,11 @@ class gallery_graphics_Core {
module::event("graphics_composite_completed", $input_file, $output_file, $options, $item);
} catch (ErrorException $e) {
+ // Unlike rotate and resize, composite catches its exceptions here. This is because
+ // composite is typically called for watermarks. If during thumb/resize generation
+ // the watermark fails, we'd still like the image resized, just without its watermark.
+ // If the exception isn't caught here, graphics::generate will replace it with a
+ // placeholder.
Kohana_Log::add("error", $e->getMessage());
}
}
diff --git a/modules/gallery/helpers/graphics.php b/modules/gallery/helpers/graphics.php
index e8df56ed..e66908c4 100644
--- a/modules/gallery/helpers/graphics.php
+++ b/modules/gallery/helpers/graphics.php
@@ -237,7 +237,16 @@ class graphics_Core {
graphics::_replace_image_with_placeholder($item, "resize");
}
graphics::_replace_image_with_placeholder($item, "thumb");
- graphics::_update_item_dimensions($item);
+ try {
+ graphics::_update_item_dimensions($item);
+ } catch (Exception $e) {
+ // Looks like get_file_metadata couldn't identify our placeholders. We should never get
+ // here, but in the odd case we do, we need to do something. Let's put in hardcoded values.
+ if ($item->is_photo()) {
+ list ($item->resize_width, $item->resize_height) = array(200, 200);
+ }
+ list ($item->thumb_width, $item->thumb_height) = array(200, 200);
+ }
$item->save();
throw $e;
}
diff --git a/modules/gallery/helpers/legal_file.php b/modules/gallery/helpers/legal_file.php
index ab9047c8..9ed564a1 100644
--- a/modules/gallery/helpers/legal_file.php
+++ b/modules/gallery/helpers/legal_file.php
@@ -24,6 +24,8 @@ class legal_file_Core {
private static $movie_extensions;
private static $photo_types;
private static $movie_types;
+ private static $blacklist = array("php", "php3", "php4", "php5", "phtml", "phtm", "shtml", "shtm",
+ "pl", "cgi", "asp", "sh", "py", "c", "js");
/**
* Create a default list of allowed photo MIME types paired with their extensions and then let
@@ -38,6 +40,9 @@ class legal_file_Core {
$types_by_extension_wrapper->types_by_extension = array(
"jpg" => "image/jpeg", "jpeg" => "image/jpeg", "gif" => "image/gif", "png" => "image/png");
module::event("photo_types_by_extension", $types_by_extension_wrapper);
+ foreach (self::$blacklist as $key) {
+ unset($types_by_extension_wrapper->types_by_extension[$key]);
+ }
self::$photo_types_by_extension = $types_by_extension_wrapper->types_by_extension;
}
if ($extension) {
@@ -67,6 +72,9 @@ class legal_file_Core {
$types_by_extension_wrapper->types_by_extension = array(
"flv" => "video/x-flv", "mp4" => "video/mp4", "m4v" => "video/x-m4v");
module::event("movie_types_by_extension", $types_by_extension_wrapper);
+ foreach (self::$blacklist as $key) {
+ unset($types_by_extension_wrapper->types_by_extension[$key]);
+ }
self::$movie_types_by_extension = $types_by_extension_wrapper->types_by_extension;
}
if ($extension) {
@@ -118,7 +126,7 @@ class legal_file_Core {
$extensions_wrapper = new stdClass();
$extensions_wrapper->extensions = array_keys(legal_file::get_photo_types_by_extension());
module::event("legal_photo_extensions", $extensions_wrapper);
- self::$photo_extensions = $extensions_wrapper->extensions;
+ self::$photo_extensions = array_diff($extensions_wrapper->extensions, self::$blacklist);
}
if ($extension) {
// return true if in array, false if not
@@ -139,7 +147,7 @@ class legal_file_Core {
$extensions_wrapper = new stdClass();
$extensions_wrapper->extensions = array_keys(legal_file::get_movie_types_by_extension());
module::event("legal_movie_extensions", $extensions_wrapper);
- self::$movie_extensions = $extensions_wrapper->extensions;
+ self::$movie_extensions = array_diff($extensions_wrapper->extensions, self::$blacklist);
}
if ($extension) {
// return true if in array, false if not
diff --git a/modules/gallery/helpers/movie.php b/modules/gallery/helpers/movie.php
index 6844771b..d4b907a2 100644
--- a/modules/gallery/helpers/movie.php
+++ b/modules/gallery/helpers/movie.php
@@ -192,8 +192,16 @@ class movie_Core {
$metadata->extension = strtolower($extension);
}
- // Run movie_get_file_metadata events which can modify the class, then return results.
+ // Run movie_get_file_metadata events which can modify the class.
module::event("movie_get_file_metadata", $file_path, $metadata);
+
+ // If the post-events results are invalid, throw an exception. Note that, unlike photos, having
+ // zero width and height isn't considered invalid (as is the case when FFmpeg isn't installed).
+ if (!$metadata->mime_type || !$metadata->extension ||
+ ($metadata->mime_type != legal_file::get_movie_types_by_extension($metadata->extension))) {
+ throw new Exception("@todo ILLEGAL_OR_UNINDENTIFIABLE_FILE");
+ }
+
return array($metadata->width, $metadata->height, $metadata->mime_type,
$metadata->extension, $metadata->duration);
}
diff --git a/modules/gallery/helpers/photo.php b/modules/gallery/helpers/photo.php
index 51e51507..2d32f0d3 100644
--- a/modules/gallery/helpers/photo.php
+++ b/modules/gallery/helpers/photo.php
@@ -133,8 +133,15 @@ class photo_Core {
$metadata->height = 0;
}
- // Run photo_get_file_metadata events which can modify the class, then return results.
+ // Run photo_get_file_metadata events which can modify the class.
module::event("photo_get_file_metadata", $file_path, $metadata);
+
+ // If the post-events results are invalid, throw an exception.
+ if (!$metadata->width || !$metadata->height || !$metadata->mime_type || !$metadata->extension ||
+ ($metadata->mime_type != legal_file::get_photo_types_by_extension($metadata->extension))) {
+ throw new Exception("@todo ILLEGAL_OR_UNINDENTIFIABLE_FILE");
+ }
+
return array($metadata->width, $metadata->height, $metadata->mime_type, $metadata->extension);
}
}
diff --git a/modules/gallery/models/item.php b/modules/gallery/models/item.php
index 197d3057..33b8a89d 100644
--- a/modules/gallery/models/item.php
+++ b/modules/gallery/models/item.php
@@ -21,6 +21,7 @@ class Item_Model_Core extends ORM_MPTT {
protected $children = "items";
protected $sorting = array();
public $data_file = null;
+ private $data_file_error = null;
public function __construct($id=null) {
parent::__construct($id);
@@ -378,18 +379,26 @@ class Item_Model_Core extends ORM_MPTT {
// Get the width, height and mime type from our data file for photos and movies.
if ($this->is_photo() || $this->is_movie()) {
- if ($this->is_photo()) {
- list ($this->width, $this->height, $this->mime_type, $extension) =
- photo::get_file_metadata($this->data_file);
- } else if ($this->is_movie()) {
- list ($this->width, $this->height, $this->mime_type, $extension) =
- movie::get_file_metadata($this->data_file);
- }
-
- // Force an extension onto the name if necessary
- $pi = pathinfo($this->data_file);
- if (empty($pi["extension"])) {
- $this->name = "{$this->name}.$extension";
+ try {
+ if ($this->is_photo()) {
+ list ($this->width, $this->height, $this->mime_type, $extension) =
+ photo::get_file_metadata($this->data_file);
+ } else if ($this->is_movie()) {
+ list ($this->width, $this->height, $this->mime_type, $extension) =
+ movie::get_file_metadata($this->data_file);
+ }
+
+ // Force an extension onto the name if necessary
+ $pi = pathinfo($this->data_file);
+ if (empty($pi["extension"])) {
+ $this->name = "{$this->name}.$extension";
+ }
+
+ // Data file valid - make sure the flag is reset to false.
+ $this->data_file_error = false;
+ } catch (Exception $e) {
+ // Data file invalid - set the flag so it's reported during item validation.
+ $this->data_file_error = true;
}
}
@@ -436,17 +445,24 @@ class Item_Model_Core extends ORM_MPTT {
// appropriate for its data. We don't try to preserve the name of the data file, though,
// because the name is typically a temporary randomly-generated name.
if (isset($this->data_file)) {
- $extension = pathinfo($this->data_file, PATHINFO_EXTENSION);
- $new_name = pathinfo($this->name, PATHINFO_FILENAME) . ".$extension";
- if (!empty($extension) && strcmp($this->name, $new_name)) {
- $this->name = $new_name;
- }
- if ($this->is_photo()) {
- list ($this->width, $this->height, $this->mime_type, $extension) =
- photo::get_file_metadata($this->data_file);
- } else if ($this->is_movie()) {
- list ($this->width, $this->height, $this->mime_type, $extension) =
- movie::get_file_metadata($this->data_file);
+ try {
+ $extension = pathinfo($this->data_file, PATHINFO_EXTENSION);
+ $new_name = pathinfo($this->name, PATHINFO_FILENAME) . ".$extension";
+ if (!empty($extension) && strcmp($this->name, $new_name)) {
+ $this->name = $new_name;
+ }
+ if ($this->is_photo()) {
+ list ($this->width, $this->height, $this->mime_type, $extension) =
+ photo::get_file_metadata($this->data_file);
+ } else if ($this->is_movie()) {
+ list ($this->width, $this->height, $this->mime_type, $extension) =
+ movie::get_file_metadata($this->data_file);
+ }
+ // Data file valid - make sure the flag is reset to false.
+ $this->data_file_error = false;
+ } catch (Exception $e) {
+ // Data file invalid - set the flag so it's reported during item validation.
+ $this->data_file_error = true;
}
}
@@ -524,13 +540,6 @@ class Item_Model_Core extends ORM_MPTT {
// Replace the data file, if requested.
if ($this->data_file && ($this->is_photo() || $this->is_movie())) {
copy($this->data_file, $this->file_path());
-
- // Get the width, height and mime type from our data file for photos and movies.
- if ($this->is_photo()) {
- list ($this->width, $this->height) = photo::get_file_metadata($this->file_path());
- } else if ($this->is_movie()) {
- list ($this->width, $this->height) = movie::get_file_metadata($this->file_path());
- }
$this->thumb_dirty = 1;
$this->resize_dirty = 1;
}
@@ -966,6 +975,8 @@ class Item_Model_Core extends ORM_MPTT {
$v->add_error("name", "bad_data_file_path");
} else if (filesize($this->data_file) == 0) {
$v->add_error("name", "empty_data_file");
+ } else if ($this->data_file_error) {
+ $v->add_error("name", "invalid_data_file");
}
}
diff --git a/modules/gallery/tests/Item_Model_Test.php b/modules/gallery/tests/Item_Model_Test.php
index a1c5bce6..a93498dd 100644
--- a/modules/gallery/tests/Item_Model_Test.php
+++ b/modules/gallery/tests/Item_Model_Test.php
@@ -445,13 +445,25 @@ class Item_Model_Test extends Gallery_Unit_Test_Case {
$photo->set_data_file(MODPATH . "gallery/tests/Item_Model_Test.php");
$photo->save();
} catch (ORM_Validation_Exception $e) {
- $this->assert_same(array("mime_type" => "invalid", "name" => "illegal_data_file_extension"),
- $e->validation->errors());
+ $this->assert_same(array("name" => "illegal_data_file_extension"), $e->validation->errors());
return; // pass
}
$this->assert_true(false, "Shouldn't get here");
}
+ public function unsafe_data_file_replacement_with_valid_extension_test() {
+ $temp_file = TMPPATH . "masquerading_php.jpg";
+ copy(MODPATH . "gallery/tests/Item_Model_Test.php", $temp_file);
+ try {
+ $photo = test::random_photo();
+ $photo->set_data_file($temp_file);
+ $photo->save();
+ } catch (ORM_Validation_Exception $e) {
+ $this->assert_same(array("name" => "invalid_data_file"), $e->validation->errors());
+ return; // pass
+ }
+ }
+
public function urls_test() {
$photo = test::random_photo();
$this->assert_true(
diff --git a/modules/gallery/tests/Movie_Helper_Test.php b/modules/gallery/tests/Movie_Helper_Test.php
index 0c262620..03fa2da9 100644
--- a/modules/gallery/tests/Movie_Helper_Test.php
+++ b/modules/gallery/tests/Movie_Helper_Test.php
@@ -64,18 +64,42 @@ class Movie_Helper_Test extends Gallery_Unit_Test_Case {
public function get_file_metadata_with_no_extension_test() {
copy(MODPATH . "gallery/tests/test.flv", TMPPATH . "test_flv_with_no_extension");
- $this->assert_equal(array(360, 288, null, null, 6.00),
- movie::get_file_metadata(TMPPATH . "test_flv_with_no_extension"));
+ // Since mime type and extension are based solely on the filename, this is considered invalid.
+ try {
+ $metadata = movie::get_file_metadata(TMPPATH . "test_flv_with_no_extension");
+ $this->assert_true(false, "Shouldn't get here");
+ } catch (Exception $e) {
+ // pass
+ }
}
public function get_file_metadata_with_illegal_extension_test() {
- $this->assert_equal(array(0, 0, null, null, 0),
- movie::get_file_metadata(MODPATH . "gallery/tests/Movie_Helper_Test.php"));
+ try {
+ $metadata = movie::get_file_metadata(MODPATH . "gallery/tests/Movie_Helper_Test.php");
+ $this->assert_true(false, "Shouldn't get here");
+ } catch (Exception $e) {
+ // pass
+ }
}
public function get_file_metadata_with_illegal_extension_but_valid_file_contents_test() {
copy(MODPATH . "gallery/tests/test.flv", TMPPATH . "test_flv_with_php_extension.php");
- $this->assert_equal(array(360, 288, null, null, 6.00),
- movie::get_file_metadata(TMPPATH . "test_flv_with_php_extension.php"));
+ // Since mime type and extension are based solely on the filename, this is considered invalid.
+ try {
+ $metadata = movie::get_file_metadata(TMPPATH . "test_flv_with_php_extension.php");
+ $this->assert_true(false, "Shouldn't get here");
+ } catch (Exception $e) {
+ // pass
+ }
+ }
+
+ public function get_file_metadata_with_valid_extension_but_illegal_file_contents_test() {
+ copy(MODPATH . "gallery/tests/Photo_Helper_Test.php", TMPPATH . "test_php_with_flv_extension.flv");
+ // Since mime type and extension are based solely on the filename, this is considered valid.
+ // Of course, FFmpeg cannot extract width, height, or duration from the file. Note that this
+ // isn't a really a security problem, since the filename doesn't have a php extension and
+ // therefore will never be executed.
+ $this->assert_equal(array(0, 0, "video/x-flv", "flv", 0),
+ movie::get_file_metadata(TMPPATH . "test_php_with_flv_extension.flv"));
}
}
diff --git a/modules/gallery/tests/Photo_Helper_Test.php b/modules/gallery/tests/Photo_Helper_Test.php
index 5207a6db..79b5ccfd 100644
--- a/modules/gallery/tests/Photo_Helper_Test.php
+++ b/modules/gallery/tests/Photo_Helper_Test.php
@@ -40,8 +40,12 @@ class Photo_Helper_Test extends Gallery_Unit_Test_Case {
}
public function get_file_metadata_with_illegal_extension_test() {
- $this->assert_equal(array(0, 0, null, null),
- photo::get_file_metadata(MODPATH . "gallery/tests/Photo_Helper_Test.php"));
+ try {
+ $metadata = photo::get_file_metadata(MODPATH . "gallery/tests/Photo_Helper_Test.php");
+ $this->assert_true(false, "Shouldn't get here");
+ } catch (Exception $e) {
+ // pass
+ }
}
public function get_file_metadata_with_illegal_extension_but_valid_file_contents_test() {
@@ -53,4 +57,14 @@ class Photo_Helper_Test extends Gallery_Unit_Test_Case {
$this->assert_equal(array(1024, 768, "image/jpeg", "jpg"),
photo::get_file_metadata(TMPPATH . "test_jpg_with_php_extension.php"));
}
+
+ public function get_file_metadata_with_valid_extension_but_illegal_file_contents_test() {
+ copy(MODPATH . "gallery/tests/Photo_Helper_Test.php", TMPPATH . "test_php_with_jpg_extension.jpg");
+ try {
+ $metadata = photo::get_file_metadata(TMPPATH . "test_php_with_jpg_extension.jpg");
+ $this->assert_true(false, "Shouldn't get here");
+ } catch (Exception $e) {
+ // pass
+ }
+ }
}
diff --git a/modules/watermark/controllers/admin_watermarks.php b/modules/watermark/controllers/admin_watermarks.php
index 27c2efc9..59bb7fa9 100644
--- a/modules/watermark/controllers/admin_watermarks.php
+++ b/modules/watermark/controllers/admin_watermarks.php
@@ -102,18 +102,17 @@ class Admin_Watermarks_Controller extends Admin_Controller {
$name = preg_replace("/uploadfile-[^-]+-(.*)/", '$1', $pathinfo["basename"]);
$name = legal_file::smash_extensions($name);
- list ($width, $height, $mime_type, $extension) = photo::get_file_metadata($file);
- if (!$width || !$height || !$mime_type || !$extension ||
- !legal_file::get_photo_extensions($extension)) {
- message::error(t("Invalid or unidentifiable image file"));
- @unlink($file);
- return;
- } else {
+ try {
+ list ($width, $height, $mime_type, $extension) = photo::get_file_metadata($file);
// Force correct, legal extension type on file, which will be of our canonical type
// (i.e. all lowercase, jpg instead of jpeg, etc.). This renaming prevents the issues
// addressed in ticket #1855, where an image that looked valid (header said jpg) with a
// php extension was previously accepted without changing its extension.
$name = legal_file::change_extension($name, $extension);
+ } catch (Exception $e) {
+ message::error(t("Invalid or unidentifiable image file"));
+ @unlink($file);
+ return;
}
rename($file, VARPATH . "modules/watermark/$name");
generated by cgit v1.2.3 (git 2.46.0) at 2025-10-13 10:10:42 +0000