From 2926ceba6a06a2f0f95452ae838a89247c493c93 Mon Sep 17 00:00:00 2001 From: JoshyPHP Date: Fri, 13 Dec 2019 01:46:09 +0100 Subject: [PATCH 1/2] [ticket/16250] Add a service to check BBCodes safeness PHPBB3-16250 --- .../container/services_text_formatter.yml | 5 ++ phpBB/includes/acp/acp_bbcodes.php | 19 ++++- phpBB/language/en/acp/posting.php | 3 +- .../textformatter/acp_utils_interface.php | 38 +++++++++ phpBB/phpbb/textformatter/s9e/acp_utils.php | 67 ++++++++++++++++ tests/functional/acp_bbcodes_test.php | 40 ++++++++++ tests/text_formatter/s9e/acp_utils_test.php | 79 +++++++++++++++++++ 7 files changed, 246 insertions(+), 5 deletions(-) create mode 100644 phpBB/phpbb/textformatter/acp_utils_interface.php create mode 100644 phpBB/phpbb/textformatter/s9e/acp_utils.php create mode 100644 tests/text_formatter/s9e/acp_utils_test.php diff --git a/phpBB/config/default/container/services_text_formatter.yml b/phpBB/config/default/container/services_text_formatter.yml index 07087cd4a9..4e4abf6564 100644 --- a/phpBB/config/default/container/services_text_formatter.yml +++ b/phpBB/config/default/container/services_text_formatter.yml @@ -4,6 +4,11 @@ parameters: text_formatter.cache.renderer.key: _text_formatter_renderer services: + text_formatter.acp_utils: + class: phpbb\textformatter\s9e\acp_utils + arguments: + - '@text_formatter.s9e.factory' + text_formatter.cache: alias: text_formatter.s9e.factory diff --git a/phpBB/includes/acp/acp_bbcodes.php b/phpBB/includes/acp/acp_bbcodes.php index a67f3c54f9..9583f9a869 100644 --- a/phpBB/includes/acp/acp_bbcodes.php +++ b/phpBB/includes/acp/acp_bbcodes.php @@ -157,7 +157,7 @@ class acp_bbcodes * @var string bbcode_tpl The bbcode HTML replacement string * @var string bbcode_helpline The bbcode help line string * @var array hidden_fields Array of hidden fields for use when - * submitting form when $warn_text is true + * submitting form when $warn_unsafe is true * @since 3.1.0-a3 */ $vars = array( @@ -172,14 +172,25 @@ class acp_bbcodes ); extract($phpbb_dispatcher->trigger_event('core.acp_bbcodes_modify_create', compact($vars))); - $warn_text = preg_match('%<[^>]*\{text[\d]*\}[^>]*>%i', $bbcode_tpl); + $acp_utils = $phpbb_container->get('text_formatter.acp_utils'); + $bbcode_info = $acp_utils->analyse_bbcode($bbcode_match, $bbcode_tpl); + $warn_unsafe = ($bbcode_info['status'] === 'unsafe'); - if (!$warn_text && !check_form_key($form_key)) + if ($bbcode_info['status'] === 'invalid_definition') + { + trigger_error($user->lang['BBCODE_INVALID'] . adm_back_link($this->u_action), E_USER_WARNING); + } + if ($bbcode_info['status'] === 'invalid_template') + { + trigger_error($user->lang['BBCODE_INVALID_TEMPLATE'] . adm_back_link($this->u_action), E_USER_WARNING); + } + + if (!$warn_unsafe && !check_form_key($form_key)) { trigger_error($user->lang['FORM_INVALID'] . adm_back_link($this->u_action), E_USER_WARNING); } - if (!$warn_text || confirm_box(true)) + if (!$warn_unsafe || confirm_box(true)) { $data = $this->build_regexp($bbcode_match, $bbcode_tpl); diff --git a/phpBB/language/en/acp/posting.php b/phpBB/language/en/acp/posting.php index 1667aa6011..3bff6b9185 100644 --- a/phpBB/language/en/acp/posting.php +++ b/phpBB/language/en/acp/posting.php @@ -42,7 +42,7 @@ $lang = array_merge($lang, array( 'ACP_BBCODES_EXPLAIN' => 'BBCode is a special implementation of HTML offering greater control over what and how something is displayed. From this page you can add, remove and edit custom BBCodes.', 'ADD_BBCODE' => 'Add a new BBCode', - 'BBCODE_DANGER' => 'The BBCode you are trying to add seems to use a {TEXT} token inside a HTML attribute. This is a possible XSS security issue. Try using the more restrictive {SIMPLETEXT} or {INTTEXT} types instead. Only proceed if you understand the risks involved and you consider the use of {TEXT} absolutely unavoidable.', + 'BBCODE_DANGER' => 'The BBCode you are trying to add seems unsafe. If the BBCode uses a {TEXT} token in a sensitive context, try using a more restrictive type instead. Only proceed if you understand the risks involved.', 'BBCODE_DANGER_PROCEED' => 'Proceed', //'I understand the risk', 'BBCODE_ADDED' => 'BBCode added successfully.', @@ -56,6 +56,7 @@ $lang = array_merge($lang, array( 'BBCODE_INVALID_TAG_NAME' => 'The BBCode tag name that you selected already exists.', 'BBCODE_INVALID' => 'Your BBCode is constructed in an invalid form.', + 'BBCODE_INVALID_TEMPLATE' => 'Your BBCode’s template is invalid.', 'BBCODE_TAG' => 'Tag', 'BBCODE_TAG_TOO_LONG' => 'The tag name you selected is too long.', 'BBCODE_TAG_DEF_TOO_LONG' => 'The tag definition that you have entered is too long, please shorten your tag definition.', diff --git a/phpBB/phpbb/textformatter/acp_utils_interface.php b/phpBB/phpbb/textformatter/acp_utils_interface.php new file mode 100644 index 0000000000..d1e3de9989 --- /dev/null +++ b/phpBB/phpbb/textformatter/acp_utils_interface.php @@ -0,0 +1,38 @@ + +* @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\textformatter; + +interface acp_utils_interface +{ + /** + * Analyse given BBCode definition for issues and safeness + * + * Required elements in the return array: + * - status: + * - "safe" The BBCode is valid and can be safely used by anyone. + * - "unsafe" The BBCode is valid but may be unsafe to use. + * - "invalid_definition" There is an issue with the definition. + * - "invalid_template" There is an issue with the template. + * + * Optional elements in the return array: + * - name: Name of the BBCode based on the definition. Required if status is "safe". + * - error_text: Textual description of the issue in plain text or as a L_* string. + * - error_html: Visual description of the issue in HTML. + * + * @param string $definition BBCode definition, e.g. [b]{TEXT}[/b] + * @param string $template BBCode template, e.g. {TEXT} + * @return array + */ + public function analyse_bbcode(string $definition, string $template): array; +} diff --git a/phpBB/phpbb/textformatter/s9e/acp_utils.php b/phpBB/phpbb/textformatter/s9e/acp_utils.php new file mode 100644 index 0000000000..981fa60813 --- /dev/null +++ b/phpBB/phpbb/textformatter/s9e/acp_utils.php @@ -0,0 +1,67 @@ + +* @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\textformatter\s9e; + +use phpbb\textformatter\acp_utils_interface; +use s9e\TextFormatter\Configurator\Exceptions\UnsafeTemplateException; + +class acp_utils implements acp_utils_interface +{ + /** + * @var factory $factory + */ + protected $factory; + + /** + * @param factory $factory + */ + public function __construct(factory $factory) + { + $this->factory = $factory; + } + + /** + * {@inheritdoc} + */ + public function analyse_bbcode(string $definition, string $template): array + { + $configurator = $this->factory->get_configurator(); + $return = ['status' => 'safe']; + + // Capture and normalize the BBCode name manually because there's no easy way to retrieve + // it in TextFormatter <= 2.x + if (preg_match('(\\[([-\\w]++))', $definition, $m)) + { + $return['name'] = strtoupper($m[1]); + } + + try + { + $configurator->BBCodes->addCustom($definition, $template); + } + catch (UnsafeTemplateException $e) + { + $return['status'] = 'unsafe'; + $return['error_text'] = $e->getMessage(); + $return['error_html'] = $e->highlightNode(''); + } + catch (\Exception $e) + { + $return['status'] = (preg_match('(xml|xpath|xsl)i', $e->getMessage())) ? 'invalid_template' : 'invalid_definition'; + $return['error_text'] = $e->getMessage(); + } + + return $return; + } +} diff --git a/tests/functional/acp_bbcodes_test.php b/tests/functional/acp_bbcodes_test.php index 58681dfa07..cc6397fdfd 100644 --- a/tests/functional/acp_bbcodes_test.php +++ b/tests/functional/acp_bbcodes_test.php @@ -43,4 +43,44 @@ class phpbb_functional_acp_bbcodes_test extends phpbb_functional_test_case $this->assertContains('
c
', $html); $this->assertContains('
d
', $html); } + + /** + * @dataProvider get_bbcode_error_tests + */ + public function test_bbcode_error($match, $tpl, $error) + { + $this->login(); + $this->admin_login(); + + $crawler = self::request('GET', 'adm/index.php?i=acp_bbcodes&sid=' . $this->sid . '&mode=bbcodes&action=add'); + $form = $crawler->selectButton('Submit')->form([ + 'bbcode_match' => $match, + 'bbcode_tpl' => $tpl + ]); + $crawler = self::submit($form); + + $text = $crawler->filter('.errorbox')->text(); + $this->assertStringContainsString($error, $text); + } + + public function get_bbcode_error_tests() + { + return [ + [ + 'XXX', + '', + 'BBCode is constructed in an invalid form' + ], + [ + '[x]{TEXT}[/x]', + '{TEXT}', + 'unsafe' + ], + ]; + } } diff --git a/tests/text_formatter/s9e/acp_utils_test.php b/tests/text_formatter/s9e/acp_utils_test.php new file mode 100644 index 0000000000..9d84924042 --- /dev/null +++ b/tests/text_formatter/s9e/acp_utils_test.php @@ -0,0 +1,79 @@ + +* @license GNU General Public License, version 2 (GPL-2.0) +* +* For full copyright and license information, please see +* the docs/CREDITS.txt file. +* +*/ + +class phpbb_textformatter_s9e_acp_utils_test extends phpbb_test_case +{ + /** + * @dataProvider get_analyse_bbcode_tests + */ + public function test_analyse_bbcode($definition, $template, $expected) + { + $container = $this->get_test_case_helpers()->set_s9e_services(); + $factory = $container->get('text_formatter.s9e.factory'); + $acp_utils = new \phpbb\textformatter\s9e\acp_utils($factory); + $actual = $acp_utils->analyse_bbcode($definition, $template); + + $this->assertEquals($expected, $actual); + } + + public function get_analyse_bbcode_tests() + { + return [ + [ + '[x]{TEXT}[/x]', + '{TEXT}', + [ + 'status' => 'safe', + 'name' => 'X' + ] + ], + [ + '[hr]', + '
', + [ + 'status' => 'safe', + 'name' => 'HR' + ] + ], + [ + '[x]{TEXT}[/x]', + '', + [ + 'status' => 'unsafe', + 'name' => 'X', + 'error_text' => 'Cannot allow unfiltered data in this context', + 'error_html' => '<script> + <xsl:apply-templates/> +</script>' + ] + ], + [ + '???', + '
', + [ + 'status' => 'invalid_definition', + 'error_text' => 'Cannot interpret the BBCode definition' + ] + ], + [ + '[x]{TEXT}[/x]', + ' 'invalid_template', + 'name' => 'X', + 'error_text' => "Invalid XSL: Couldn't find end of Start Tag invalid line 1\n" + ] + ], + ]; + } +} From 2733ce07129dceb5b60acdceba1689fa5339a523 Mon Sep 17 00:00:00 2001 From: JoshyPHP Date: Mon, 16 Dec 2019 01:34:26 +0100 Subject: [PATCH 2/2] [ticket/16250] Reworked status as constants PHPBB3-16250 --- phpBB/includes/acp/acp_bbcodes.php | 12 ++++----- .../textformatter/acp_utils_interface.php | 26 +++++++++++++++---- phpBB/phpbb/textformatter/s9e/acp_utils.php | 6 ++--- 3 files changed, 30 insertions(+), 14 deletions(-) diff --git a/phpBB/includes/acp/acp_bbcodes.php b/phpBB/includes/acp/acp_bbcodes.php index 9583f9a869..84dbbf02ba 100644 --- a/phpBB/includes/acp/acp_bbcodes.php +++ b/phpBB/includes/acp/acp_bbcodes.php @@ -174,16 +174,16 @@ class acp_bbcodes $acp_utils = $phpbb_container->get('text_formatter.acp_utils'); $bbcode_info = $acp_utils->analyse_bbcode($bbcode_match, $bbcode_tpl); - $warn_unsafe = ($bbcode_info['status'] === 'unsafe'); + $warn_unsafe = ($bbcode_info['status'] === $acp_utils::BBCODE_STATUS_UNSAFE); - if ($bbcode_info['status'] === 'invalid_definition') - { - trigger_error($user->lang['BBCODE_INVALID'] . adm_back_link($this->u_action), E_USER_WARNING); - } - if ($bbcode_info['status'] === 'invalid_template') + if ($bbcode_info['status'] === $acp_utils::BBCODE_STATUS_INVALID_TEMPLATE) { trigger_error($user->lang['BBCODE_INVALID_TEMPLATE'] . adm_back_link($this->u_action), E_USER_WARNING); } + if ($bbcode_info['status'] === $acp_utils::BBCODE_STATUS_INVALID_DEFINITION) + { + trigger_error($user->lang['BBCODE_INVALID'] . adm_back_link($this->u_action), E_USER_WARNING); + } if (!$warn_unsafe && !check_form_key($form_key)) { diff --git a/phpBB/phpbb/textformatter/acp_utils_interface.php b/phpBB/phpbb/textformatter/acp_utils_interface.php index d1e3de9989..cdee56f19d 100644 --- a/phpBB/phpbb/textformatter/acp_utils_interface.php +++ b/phpBB/phpbb/textformatter/acp_utils_interface.php @@ -15,15 +15,31 @@ namespace phpbb\textformatter; interface acp_utils_interface { + /** + * There is an issue with the definition + */ + const BBCODE_STATUS_INVALID_DEFINITION = 'invalid_definition'; + + /** + * There is an issue with the template + */ + const BBCODE_STATUS_INVALID_TEMPLATE = 'invalid_template'; + + /** + * The BBCode is valid and can be safely used by anyone + */ + const BBCODE_STATUS_SAFE = 'safe'; + + /** + * The BBCode is valid but may be unsafe to use + */ + const BBCODE_STATUS_UNSAFE = 'unsafe'; + /** * Analyse given BBCode definition for issues and safeness * * Required elements in the return array: - * - status: - * - "safe" The BBCode is valid and can be safely used by anyone. - * - "unsafe" The BBCode is valid but may be unsafe to use. - * - "invalid_definition" There is an issue with the definition. - * - "invalid_template" There is an issue with the template. + * - status: see BBCODE_STATUS_* constants * * Optional elements in the return array: * - name: Name of the BBCode based on the definition. Required if status is "safe". diff --git a/phpBB/phpbb/textformatter/s9e/acp_utils.php b/phpBB/phpbb/textformatter/s9e/acp_utils.php index 981fa60813..c4a668020e 100644 --- a/phpBB/phpbb/textformatter/s9e/acp_utils.php +++ b/phpBB/phpbb/textformatter/s9e/acp_utils.php @@ -37,7 +37,7 @@ class acp_utils implements acp_utils_interface public function analyse_bbcode(string $definition, string $template): array { $configurator = $this->factory->get_configurator(); - $return = ['status' => 'safe']; + $return = ['status' => self::BBCODE_STATUS_SAFE]; // Capture and normalize the BBCode name manually because there's no easy way to retrieve // it in TextFormatter <= 2.x @@ -52,13 +52,13 @@ class acp_utils implements acp_utils_interface } catch (UnsafeTemplateException $e) { - $return['status'] = 'unsafe'; + $return['status'] = self::BBCODE_STATUS_UNSAFE; $return['error_text'] = $e->getMessage(); $return['error_html'] = $e->highlightNode(''); } catch (\Exception $e) { - $return['status'] = (preg_match('(xml|xpath|xsl)i', $e->getMessage())) ? 'invalid_template' : 'invalid_definition'; + $return['status'] = (preg_match('(xml|xpath|xsl)i', $e->getMessage())) ? self::BBCODE_STATUS_INVALID_TEMPLATE : self::BBCODE_STATUS_INVALID_DEFINITION; $return['error_text'] = $e->getMessage(); }