From d753351edc04a45c61411ee09607fb932d314617 Mon Sep 17 00:00:00 2001 From: Derky Date: Tue, 23 Oct 2018 23:51:55 +0200 Subject: [ticket/security/227] Replace ImageMagick support with thumbnail event SECURITY-227 --- .../container/services_install_obtain_data.yml | 7 -- phpBB/docs/INSTALL.html | 1 - phpBB/includes/acp/acp_attachments.php | 68 +---------------- phpBB/includes/functions_acp.php | 7 +- phpBB/includes/functions_posting.php | 42 +++++----- phpBB/includes/questionnaire/questionnaire.php | 1 - phpBB/install/convertors/functions_phpbb20.php | 1 - phpBB/install/schemas/schema_data.sql | 1 - phpBB/language/en/acp/attachments.php | 5 +- .../db/migration/data/v32x/remove_imagick.php | 31 ++++++++ .../install_database/task/add_config_settings.php | 4 - .../obtain_data/task/obtain_imagick_path.php | 89 ---------------------- tests/console/thumbnail_test.php | 1 - tests/functional/acp_attachments_test.php | 78 ------------------- tests/functional/fileupload_form_test.php | 1 - 15 files changed, 60 insertions(+), 277 deletions(-) create mode 100644 phpBB/phpbb/db/migration/data/v32x/remove_imagick.php delete mode 100644 phpBB/phpbb/install/module/obtain_data/task/obtain_imagick_path.php delete mode 100644 tests/functional/acp_attachments_test.php diff --git a/phpBB/config/installer/container/services_install_obtain_data.yml b/phpBB/config/installer/container/services_install_obtain_data.yml index cd8d0c8072..010aba829d 100644 --- a/phpBB/config/installer/container/services_install_obtain_data.yml +++ b/phpBB/config/installer/container/services_install_obtain_data.yml @@ -33,13 +33,6 @@ services: tags: - { name: install_obtain_data, order: 40 } - installer.obtain_data.obtain_imagick_path: - class: phpbb\install\module\obtain_data\task\obtain_imagick_path - arguments: - - '@installer.helper.config' - tags: - - { name: install_obtain_data, order: 60 } - installer.obtain_data.obtain_server_data: class: phpbb\install\module\obtain_data\task\obtain_server_data arguments: diff --git a/phpBB/docs/INSTALL.html b/phpBB/docs/INSTALL.html index 9c2671e085..853607886b 100644 --- a/phpBB/docs/INSTALL.html +++ b/phpBB/docs/INSTALL.html @@ -159,7 +159,6 @@
  • zlib Compression support
  • Remote FTP support
  • XML support
    • -
  • ImageMagick support
  • GD Support
    • diff --git a/phpBB/includes/acp/acp_attachments.php b/phpBB/includes/acp/acp_attachments.php index 6c2df8d999..e8e0cd5cbb 100644 --- a/phpBB/includes/acp/acp_attachments.php +++ b/phpBB/includes/acp/acp_attachments.php @@ -164,7 +164,6 @@ class acp_attachments 'img_create_thumbnail' => array('lang' => 'CREATE_THUMBNAIL', 'validate' => 'bool', 'type' => 'radio:yes_no', 'explain' => true), 'img_max_thumb_width' => array('lang' => 'MAX_THUMB_WIDTH', 'validate' => 'int:0:999999999999999', 'type' => 'number:0:999999999999999', 'explain' => true, 'append' => ' ' . $user->lang['PIXEL']), 'img_min_thumb_filesize' => array('lang' => 'MIN_THUMB_FILESIZE', 'validate' => 'int:0:999999999999999', 'type' => 'number:0:999999999999999', 'explain' => true, 'append' => ' ' . $user->lang['BYTES']), - 'img_imagick' => array('lang' => 'IMAGICK_PATH', 'validate' => 'absolute_path', 'type' => 'text:20:200', 'explain' => true, 'append' => '  [ ' . $user->lang['SEARCH_IMAGICK'] . ' ]'), 'img_max' => array('lang' => 'MAX_IMAGE_SIZE', 'validate' => 'int:0:9999', 'type' => 'dimension:0:9999', 'explain' => true, 'append' => ' ' . $user->lang['PIXEL']), 'img_link' => array('lang' => 'IMAGE_LINK_SIZE', 'validate' => 'int:0:9999', 'type' => 'dimension:0:9999', 'explain' => true, 'append' => ' ' . $user->lang['PIXEL']), ) @@ -234,37 +233,15 @@ class acp_attachments $template->assign_var('S_ATTACHMENT_SETTINGS', true); - if ($action == 'imgmagick') - { - $this->new_config['img_imagick'] = $this->search_imagemagick(); - } - - // We strip eventually manual added convert program, we only want the patch - if ($this->new_config['img_imagick']) - { - // Change path separator - $this->new_config['img_imagick'] = str_replace('\\', '/', $this->new_config['img_imagick']); - $this->new_config['img_imagick'] = str_replace(array('convert', '.exe'), array('', ''), $this->new_config['img_imagick']); - - // Check for trailing slash - if (substr($this->new_config['img_imagick'], -1) !== '/') - { - $this->new_config['img_imagick'] .= '/'; - } - } - $supported_types = get_supported_image_types(); // Check Thumbnail Support - if (!$this->new_config['img_imagick'] && (!isset($supported_types['format']) || !count($supported_types['format']))) + if (!isset($supported_types['format']) || !count($supported_types['format'])) { $this->new_config['img_create_thumbnail'] = 0; } - $template->assign_vars(array( - 'U_SEARCH_IMAGICK' => $this->u_action . '&action=imgmagick', - 'S_THUMBNAIL_SUPPORT' => (!$this->new_config['img_imagick'] && (!isset($supported_types['format']) || !count($supported_types['format']))) ? false : true) - ); + $template->assign_var('S_THUMBNAIL_SUPPORT', (!isset($supported_types['format']) || !count($supported_types['format'])) ? false : true); // Secure Download Options - Same procedure as with banning $allow_deny = ($this->new_config['secure_allow_deny']) ? 'ALLOWED' : 'DISALLOWED'; @@ -1495,47 +1472,6 @@ class acp_attachments return $group_select; } - /** - * Search Imagick - */ - function search_imagemagick() - { - $imagick = ''; - - $exe = ((defined('PHP_OS')) && (preg_match('#^win#i', PHP_OS))) ? '.exe' : ''; - - $magic_home = getenv('MAGICK_HOME'); - - if (empty($magic_home)) - { - $locations = array('C:/WINDOWS/', 'C:/WINNT/', 'C:/WINDOWS/SYSTEM/', 'C:/WINNT/SYSTEM/', 'C:/WINDOWS/SYSTEM32/', 'C:/WINNT/SYSTEM32/', '/usr/bin/', '/usr/sbin/', '/usr/local/bin/', '/usr/local/sbin/', '/opt/', '/usr/imagemagick/', '/usr/bin/imagemagick/'); - $path_locations = str_replace('\\', '/', (explode(($exe) ? ';' : ':', getenv('PATH')))); - - $locations = array_merge($path_locations, $locations); - - foreach ($locations as $location) - { - // The path might not end properly, fudge it - if (substr($location, -1) !== '/') - { - $location .= '/'; - } - - if (@file_exists($location) && @is_readable($location . 'mogrify' . $exe) && @filesize($location . 'mogrify' . $exe) > 3000) - { - $imagick = str_replace('\\', '/', $location); - continue; - } - } - } - else - { - $imagick = str_replace('\\', '/', $magic_home); - } - - return $imagick; - } - /** * Test Settings */ diff --git a/phpBB/includes/functions_acp.php b/phpBB/includes/functions_acp.php index 8bf42aa36e..9b7491305c 100644 --- a/phpBB/includes/functions_acp.php +++ b/phpBB/includes/functions_acp.php @@ -564,9 +564,6 @@ function validate_config_vars($config_vars, &$cfg_array, &$error) $cfg_array[$config_name] = trim($destination); - // Absolute file path - case 'absolute_path': - case 'absolute_path_writable': // Path being relative (still prefixed by phpbb_root_path), but with the ability to escape the root dir... case 'path': case 'wpath': @@ -585,7 +582,7 @@ function validate_config_vars($config_vars, &$cfg_array, &$error) break; } - $path = in_array($config_definition['validate'], array('wpath', 'path', 'rpath', 'rwpath')) ? $phpbb_root_path . $cfg_array[$config_name] : $cfg_array[$config_name]; + $path = $phpbb_root_path . $cfg_array[$config_name]; if (!file_exists($path)) { @@ -598,7 +595,7 @@ function validate_config_vars($config_vars, &$cfg_array, &$error) } // Check if the path is writable - if ($config_definition['validate'] == 'wpath' || $config_definition['validate'] == 'rwpath' || $config_definition['validate'] === 'absolute_path_writable') + if ($config_definition['validate'] == 'wpath' || $config_definition['validate'] == 'rwpath') { if (file_exists($path) && !$phpbb_filesystem->is_writable($path)) { diff --git a/phpBB/includes/functions_posting.php b/phpBB/includes/functions_posting.php index 1e8f1ad00d..2d713481a5 100644 --- a/phpBB/includes/functions_posting.php +++ b/phpBB/includes/functions_posting.php @@ -519,7 +519,7 @@ function get_supported_image_types($type = false) */ function create_thumbnail($source, $destination, $mimetype) { - global $config, $phpbb_filesystem; + global $config, $phpbb_filesystem, $phpbb_dispatcher; $min_filesize = (int) $config['img_min_thumb_filesize']; $img_filesize = (file_exists($source)) ? @filesize($source) : false; @@ -551,25 +551,31 @@ function create_thumbnail($source, $destination, $mimetype) return false; } - $used_imagick = false; + $thumbnail_created = false; - // Only use ImageMagick if defined and the passthru function not disabled - if ($config['img_imagick'] && function_exists('passthru')) - { - if (substr($config['img_imagick'], -1) !== '/') - { - $config['img_imagick'] .= '/'; - } - - @passthru(escapeshellcmd($config['img_imagick']) . 'convert' . ((defined('PHP_OS') && preg_match('#^win#i', PHP_OS)) ? '.exe' : '') . ' -quality 85 -geometry ' . $new_width . 'x' . $new_height . ' "' . str_replace('\\', '/', $source) . '" "' . str_replace('\\', '/', $destination) . '"'); - - if (file_exists($destination)) - { - $used_imagick = true; - } - } + /** + * Create thumbnail event to replace GD thumbnail creation with for example ImageMagick + * + * @event core.thumbnail_create_before + * @var string source Image source path + * @var string destination Thumbnail destination path + * @var string mimetype Image mime type + * @var float new_width Calculated thumbnail width + * @var float new_height Calculated thumbnail height + * @var bool thumbnail_created Set to true to skip default GD thumbnail creation + * @since 3.2.4-RC1 + */ + $vars = array( + 'source', + 'destination', + 'mimetype', + 'new_width', + 'new_height', + 'thumbnail_created', + ); + extract($phpbb_dispatcher->trigger_event('core.thumbnail_create_before', compact($vars))); - if (!$used_imagick) + if (!$thumbnail_created) { $type = get_supported_image_types($type); diff --git a/phpBB/includes/questionnaire/questionnaire.php b/phpBB/includes/questionnaire/questionnaire.php index ee6d0ee2a6..5e7aca6a55 100644 --- a/phpBB/includes/questionnaire/questionnaire.php +++ b/phpBB/includes/questionnaire/questionnaire.php @@ -369,7 +369,6 @@ class phpbb_questionnaire_phpbb_data_provider 'hot_threshold' => true, 'img_create_thumbnail' => true, 'img_display_inlined' => true, - 'img_imagick' => true, 'img_link_height' => true, 'img_link_width' => true, 'img_max_height' => true, diff --git a/phpBB/install/convertors/functions_phpbb20.php b/phpBB/install/convertors/functions_phpbb20.php index 8b80eec4c8..2da901d3de 100644 --- a/phpBB/install/convertors/functions_phpbb20.php +++ b/phpBB/install/convertors/functions_phpbb20.php @@ -1694,7 +1694,6 @@ function phpbb_import_attach_config() $config->set('img_create_thumbnail', $attach_config['img_create_thumbnail']); $config->set('img_max_thumb_width', 400); $config->set('img_min_thumb_filesize', $attach_config['img_min_thumb_filesize']); - $config->set('img_imagick', $attach_config['img_imagick']); } /** diff --git a/phpBB/install/schemas/schema_data.sql b/phpBB/install/schemas/schema_data.sql index 21f26b5f08..0027119bfe 100644 --- a/phpBB/install/schemas/schema_data.sql +++ b/phpBB/install/schemas/schema_data.sql @@ -149,7 +149,6 @@ INSERT INTO phpbb_config (config_name, config_value) VALUES ('hot_threshold', '2 INSERT INTO phpbb_config (config_name, config_value) VALUES ('icons_path', 'images/icons'); INSERT INTO phpbb_config (config_name, config_value) VALUES ('img_create_thumbnail', '0'); INSERT INTO phpbb_config (config_name, config_value) VALUES ('img_display_inlined', '1'); -INSERT INTO phpbb_config (config_name, config_value) VALUES ('img_imagick', ''); INSERT INTO phpbb_config (config_name, config_value) VALUES ('img_link_height', '0'); INSERT INTO phpbb_config (config_name, config_value) VALUES ('img_link_width', '0'); INSERT INTO phpbb_config (config_name, config_value) VALUES ('img_max_height', '0'); diff --git a/phpBB/language/en/acp/attachments.php b/phpBB/language/en/acp/attachments.php index 123c35a502..994d792bf9 100644 --- a/phpBB/language/en/acp/attachments.php +++ b/phpBB/language/en/acp/attachments.php @@ -111,8 +111,6 @@ $lang = array_merge($lang, array( 'IMAGE_LINK_SIZE' => 'Image link dimensions', 'IMAGE_LINK_SIZE_EXPLAIN' => 'Display image attachment as an inline text link if image is larger than this. To disable this behaviour, set the values to 0px by 0px.', - 'IMAGICK_PATH' => 'ImageMagick path', - 'IMAGICK_PATH_EXPLAIN' => 'Full path to the imagemagick convert application, e.g. /usr/bin/.', 'MAX_ATTACHMENTS' => 'Maximum number of attachments per post', 'MAX_ATTACHMENTS_PM' => 'Maximum number of attachments per private message', @@ -135,7 +133,7 @@ $lang = array_merge($lang, array( 'NO_EXT_GROUP_SPECIFIED' => 'No extension group specified.', 'NO_FILE_CAT' => 'None', 'NO_IMAGE' => 'No image', - 'NO_THUMBNAIL_SUPPORT' => 'Thumbnail support has been disabled. For proper functionality either the GD extension need to be available or imagemagick being installed. Both were not found.', + 'NO_THUMBNAIL_SUPPORT' => 'Thumbnail support has been disabled. For proper functionality the GD extension needs to be available.', 'NO_UPLOAD_DIR' => 'The upload directory you specified does not exist.', 'NO_WRITE_UPLOAD' => 'The upload directory you specified cannot be written to. Please alter the permissions to allow the webserver to write to it.', @@ -147,7 +145,6 @@ $lang = array_merge($lang, array( 'REMOVE_DISALLOWED_IPS' => 'Remove or un-exclude disallowed IPs/hostnames', 'RESYNC_FILES_STATS_CONFIRM' => 'Are you sure you wish to resynchronise file statistics?', - 'SEARCH_IMAGICK' => 'Search for ImageMagick', 'SECURE_ALLOW_DENY' => 'Allow/Deny list', 'SECURE_ALLOW_DENY_EXPLAIN' => 'Change the default behaviour when secure downloads are enabled of the Allow/Deny list to that of a whitelist (Allow) or a blacklist (Deny).', 'SECURE_DOWNLOADS' => 'Enable secure downloads', diff --git a/phpBB/phpbb/db/migration/data/v32x/remove_imagick.php b/phpBB/phpbb/db/migration/data/v32x/remove_imagick.php new file mode 100644 index 0000000000..a257511856 --- /dev/null +++ b/phpBB/phpbb/db/migration/data/v32x/remove_imagick.php @@ -0,0 +1,31 @@ + +* @license GNU General Public License, version 2 (GPL-2.0) +* +* For full copyright and license information, please see +* the docs/CREDITS.txt file. +* +*/ + +namespace phpbb\db\migration\data\v32x; + +class remove_imagick extends \phpbb\db\migration\migration +{ + static public function depends_on() + { + return array( + '\phpbb\db\migration\data\v32x\v323rc2', + ); + } + + public function update_data() + { + return array( + array('config.remove', array('img_imagick')), + ); + } +} diff --git a/phpBB/phpbb/install/module/install_database/task/add_config_settings.php b/phpBB/phpbb/install/module/install_database/task/add_config_settings.php index 54114e3f9c..ba439609ff 100644 --- a/phpBB/phpbb/install/module/install_database/task/add_config_settings.php +++ b/phpBB/phpbb/install/module/install_database/task/add_config_settings.php @@ -150,10 +150,6 @@ class add_config_settings extends \phpbb\install\task_base 'INSERT INTO ' . $this->config_table . " (config_name, config_value) VALUES ('default_lang', '" . $this->db->sql_escape($this->install_config->get('default_lang')) . "')", - 'UPDATE ' . $this->config_table . " - SET config_value = '" . $this->db->sql_escape($this->install_config->get('img_imagick')) . "' - WHERE config_name = 'img_imagick'", - 'UPDATE ' . $this->config_table . " SET config_value = '" . $this->db->sql_escape($this->install_config->get('server_name')) . "' WHERE config_name = 'server_name'", diff --git a/phpBB/phpbb/install/module/obtain_data/task/obtain_imagick_path.php b/phpBB/phpbb/install/module/obtain_data/task/obtain_imagick_path.php deleted file mode 100644 index 377d96ed1a..0000000000 --- a/phpBB/phpbb/install/module/obtain_data/task/obtain_imagick_path.php +++ /dev/null @@ -1,89 +0,0 @@ - - * @license GNU General Public License, version 2 (GPL-2.0) - * - * For full copyright and license information, please see - * the docs/CREDITS.txt file. - * - */ - -namespace phpbb\install\module\obtain_data\task; - -class obtain_imagick_path extends \phpbb\install\task_base implements \phpbb\install\task_interface -{ - /** - * @var \phpbb\install\helper\config - */ - protected $config; - - /** - * Constructor - * - * @param \phpbb\install\helper\config $config Installer's config - */ - public function __construct(\phpbb\install\helper\config $config) - { - $this->config = $config; - - parent::__construct(true); - } - - /** - * {@inheritdoc} - */ - public function run() - { - // Can we find ImageMagick anywhere on the system? - $exe = (DIRECTORY_SEPARATOR == '\\') ? '.exe' : ''; - - $magic_home = getenv('MAGICK_HOME'); - $img_imagick = ''; - if (empty($magic_home)) - { - $locations = array('C:/WINDOWS/', 'C:/WINNT/', 'C:/WINDOWS/SYSTEM/', 'C:/WINNT/SYSTEM/', 'C:/WINDOWS/SYSTEM32/', 'C:/WINNT/SYSTEM32/', '/usr/bin/', '/usr/sbin/', '/usr/local/bin/', '/usr/local/sbin/', '/opt/', '/usr/imagemagick/', '/usr/bin/imagemagick/'); - $path_locations = str_replace('\\', '/', (explode(($exe) ? ';' : ':', getenv('PATH')))); - - $locations = array_merge($path_locations, $locations); - foreach ($locations as $location) - { - // The path might not end properly, fudge it - if (substr($location, -1, 1) !== '/') - { - $location .= '/'; - } - - if (@file_exists($location) && @is_readable($location . 'mogrify' . $exe) && @filesize($location . 'mogrify' . $exe) > 3000) - { - $img_imagick = str_replace('\\', '/', $location); - continue; - } - } - } - else - { - $img_imagick = str_replace('\\', '/', $magic_home); - } - - $this->config->set('img_imagick', $img_imagick); - } - - /** - * {@inheritdoc} - */ - static public function get_step_count() - { - return 0; - } - - /** - * {@inheritdoc} - */ - public function get_task_lang_name() - { - return ''; - } -} diff --git a/tests/console/thumbnail_test.php b/tests/console/thumbnail_test.php index 439e3623fa..d5fbfa0fed 100644 --- a/tests/console/thumbnail_test.php +++ b/tests/console/thumbnail_test.php @@ -46,7 +46,6 @@ class phpbb_console_command_thumbnail_test extends phpbb_database_test_case $config = $this->config = new \phpbb\config\config(array( 'img_min_thumb_filesize' => 2, 'img_max_thumb_width' => 2, - 'img_imagick' => '', )); $this->db = $this->db = $this->new_dbal(); diff --git a/tests/functional/acp_attachments_test.php b/tests/functional/acp_attachments_test.php deleted file mode 100644 index 8e810a508a..0000000000 --- a/tests/functional/acp_attachments_test.php +++ /dev/null @@ -1,78 +0,0 @@ - - * @license GNU General Public License, version 2 (GPL-2.0) - * - * For full copyright and license information, please see - * the docs/CREDITS.txt file. - * - */ - -/** - * @group functional - */ -class phpbb_functional_acp_attachments_test extends phpbb_functional_test_case -{ - public function data_imagick_path_linux() - { - return array( - array('/usr/bin', 'Configuration updated successfully'), - array('/usr/foobar', 'The entered path “/usr/foobar” does not exist.'), - array('/usr/bin/which', 'The entered path “/usr/bin/which” is not a directory.'), - ); - } - - /** - * @dataProvider data_imagick_path_linux - */ - public function test_imagick_path_linux($imagick_path, $expected) - { - if (strtolower(substr(PHP_OS, 0, 5)) !== 'linux') - { - $this->markTestSkipped('Unable to test linux specific paths on other OS.'); - } - - $this->login(); - $this->admin_login(); - - $crawler = self::request('GET', 'adm/index.php?i=attachments&mode=attach&sid=' . $this->sid); - - $form = $crawler->selectButton('Submit')->form(array('config[img_imagick]' => $imagick_path)); - - $crawler = self::submit($form); - $this->assertContains($expected, $crawler->filter('#main')->text()); - } - - public function data_imagick_path_windows() - { - return array( - array('C:\Windows', 'Configuration updated successfully'), - array('C:\Windows\foobar1', 'The entered path “C:\Windows\foobar1” does not exist.'), - array('C:\Windows\explorer.exe', 'The entered path “C:\Windows\explorer.exe” is not a directory.'), - ); - } - - /** - * @dataProvider data_imagick_path_windows - */ - public function test_imagick_path_windows($imagick_path, $expected) - { - if (strtolower(substr(PHP_OS, 0, 3)) !== 'win') - { - $this->markTestSkipped('Unable to test windows specific paths on other OS.'); - } - - $this->login(); - $this->admin_login(); - - $crawler = self::request('GET', 'adm/index.php?i=attachments&mode=attach&sid=' . $this->sid); - - $form = $crawler->selectButton('Submit')->form(array('config[img_imagick]' => $imagick_path)); - - $crawler = self::submit($form); - $this->assertContains($expected, $crawler->filter('#main')->text()); - } -} diff --git a/tests/functional/fileupload_form_test.php b/tests/functional/fileupload_form_test.php index d381fa1ae2..b0780172ff 100644 --- a/tests/functional/fileupload_form_test.php +++ b/tests/functional/fileupload_form_test.php @@ -99,7 +99,6 @@ class phpbb_functional_fileupload_form_test extends phpbb_functional_test_case $form = $crawler->selectButton('Submit')->form(array( 'config[check_attachment_content]' => 0, - 'config[img_imagick]' => '', )); self::submit($form); -- cgit v1.2.1