Added HtmlPurifier to improve MaliciousCode filtering

[Judx]

Description (*)

Add HtmlPurifier for improved MaliciousCode filtering. Found when researching https://helpx.adobe.com/security/products/magento/apsb23-50.html

Magento Open Source > 2.4.5 added HtmlPurifier library to improve the MaliciousCode filter.
#3600

Related Pull Requests

GHSA-3p73-mm7v-4f6m

Fixed Issues (if relevant)

1. HTML Purifier #3600

Manual testing scenarios (*)

1. Call Mage_Core_Model_Input_Filter_MaliciousCode::filter() with dirty html
2. The value is filtered with HTMLPurifier.

Questions or comments

Could someone run n98-magerun.phar dev:ide:phpstorm:meta for the new helper meta?

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All automated tests passed successfully (all builds are green)
• Add yourself to contributors list

[Judx]

Thank you for your contribution. I have improved the PR as suggested.

[fballiano]

seems like this version of htmlpurify is php8.0+ so we can't use it in main, so:

• we could downgrade htmlpurify
• add this PR to next

[Judx]

Good point. Which one do you suggest? I am down to downgrading htmlpurify as its mostly syntax changes php7 -> php8

[fballiano]

well, I'm biased towards releasing the next branch so... if you ask me... but maybe v4.15 could be added to main (or the latest php7 compatible version, I couldn't really find that info) and the latest one to next

[addison74]

There are two errors reported by PHPStans. the latest changes are not compatible with lower versions of PHP8. I would leave it as it was initially so as not to create a BC of all the beauty.

It is a good an important addition that I would leave in the main branch.

[Judx]

Everything seems to be correct now? Is it possible to squash commits while merging or should I do that?

[fballiano]

Everything seems to be correct now?

yep

Is it possible to squash commits while merging or should I do that?

we always do that when merging, don't worry :-)

[fballiano]

do you know how to trigger the filter() method in order to test this?

[Judx]

do you know how to trigger the filter() method in order to test this?

If you have n98-magerun try this:

1. n98-magerun dev:console
2. Mage::getModel('core/Input_Filter_MaliciousCode')->filter('test') -> test
3. Mage::getModel('core/Input_Filter_MaliciousCode')->filter("'';!--\"<XSS>=&{()}") -> '';!--"=&{()}

Different attacks and correct results can be found here: http://htmlpurifier.org/live/smoketests/xssAttacks.php

[Judx]

We might want to publish security advisory for this too if we want people to update fast.

[fballiano]

thanks @Judx, I was wondering where (in the adminhtml) is called that filter method, but anyway I like this PR a lot

[Judx]

thanks @Judx, I was wondering where (in the adminhtml) is called that filter method, but anyway I like this PR a lot

Great! I can find total 7 usages, in adminhtml i.e. Mage_Adminhtml_Block_Template:

        magento-lts  (7 usages found)
            app/code/core/Mage/Adminhtml/Block  (1 usage found)
                Template.php  (1 usage found)
                    Mage_Adminhtml_Block_Template  (1 usage found)
                        maliciousCodeFilter  (1 usage found)
                            78 return Mage::getSingleton('core/input_filter_maliciousCode')->filter($html);
            app/code/core/Mage/Adminhtml/controllers  (1 usage found)
                AjaxController.php  (1 usage found)
                    Mage_Adminhtml_AjaxController  (1 usage found)
                        translateAction  (1 usage found)
                            37 $item['custom'] = $filter->filter($item['custom']);
            app/code/core/Mage/Cms/Block/Widget  (1 usage found)
                Block.php  (1 usage found)
                    Mage_Cms_Block_Widget_Block  (1 usage found)
                        _beforeToHtml  (1 usage found)
                            73 Mage::getSingleton('core/input_filter_maliciousCode')->filter($block->getContent())
            app/code/core/Mage/Core/controllers  (1 usage found)
                AjaxController.php  (1 usage found)
                    Mage_Core_AjaxController  (1 usage found)
                        translateAction  (1 usage found)
                            36 $item['custom'] = $filter->filter($item['custom']);
            app/code/core/Mage/Core/Model/Input  (1 usage found)
                Filter.php  (1 usage found)
                    Mage_Core_Model_Input_Filter  (1 usage found)
                        _filter  (1 usage found)
                            230 $value = $zendFilter->filter($value);
            app/code/core/Mage/Customer/Block/Address/Renderer  (1 usage found)
                Default.php  (1 usage found)
                    Mage_Customer_Block_Address_Renderer_Default  (1 usage found)
                        _prepareAddressTemplateData  (1 usage found)
                            149 $result = preg_replace($urlRegExp, ' ', $maliciousCodeFilter->filter($data));
            app/code/core/Mage/ProductAlert/Block/Email  (1 usage found)
                Abstract.php  (1 usage found)
                    Mage_ProductAlert_Block_Email_Abstract  (1 usage found)
                        _getFilteredProductShortDescription  (1 usage found)
                            136 $shortDescription = Mage::getSingleton('core/input_filter_maliciousCode')->filter($shortDescription);

[sreichel]

Think doc block array|string was better, or do i miss something?

[mattdavenport]

If there is any interest, we have some functionality added on top of HtmlPurifier internally which enhances security by preventing javascript from being inserted into areas such as system config, categories, CMS, etc. If there is any interest I could prepare a branched PR (or separate issue). I do assume there are some legitimate use cases for JS in these areas though, so if there is any interest we could put it behind a configuration flag (e.g. "Allow JS in text inputs").

[Judx]

Waiting for merge! And superthanks to @mattdavenport would be great if you could improve this too! :)

[sreichel]

lgtm

[mattdavenport]

@Judx Apologies for the delay here. After further inspection it appears that our improvements have been made redundant by later versions of HTMLPurifier. Nonetheless, thank you for the this PR!