magento1-Ho_Import icon indicating copy to clipboard operation
magento1-Ho_Import copied to clipboard

Should changes from SUPEE-11086 be applied to Ho_Import_Model_Template_Filter::_getVariable ?

Open hostep opened this issue 6 years ago • 1 comments

Hi guys

I'm currently patching some M1 shops with the security patch SUPEE-11086. I always write some custom script to see if any of the fixes done in a security patch should be applied to the custom code we added to a shop, and my script detected this module.

SUPEE-11086 changes the following lines in the file lib/Varien/Filter/Template.php:

diff --git a/lib/Varien/Filter/Template.php b/lib/Varien/Filter/Template.php
index e56960f5..7af4f2a7 100644
--- a/lib/Varien/Filter/Template.php
+++ b/lib/Varien/Filter/Template.php
@@ -289,6 +289,8 @@ class Varien_Filter_Template implements Zend_Filter_Interface
         $stackVars = $tokenizer->tokenize();
         $result = $default;
         $last = 0;
+        /** @var $emailPathValidator Mage_Adminhtml_Model_Email_PathValidator */
+        $emailPathValidator = $this->getEmailPathValidator();
         for($i = 0; $i < count($stackVars); $i ++) {
             if ($i == 0 && isset($this->_templateVars[$stackVars[$i]['name']])) {
                 // Getting of template value
@@ -305,9 +307,13 @@ class Varien_Filter_Template implements Zend_Filter_Interface
                     if (method_exists($stackVars[$i-1]['variable'], $stackVars[$i]['name'])
                         || substr($stackVars[$i]['name'], 0, 3) == 'get'
                     ) {
+                        $isEncrypted = false;
+                        if ($stackVars[$i]['name'] == 'getConfig') {
+                            $isEncrypted = $emailPathValidator->isValid($stackVars[$i]['args']);
+                        }
                         $stackVars[$i]['variable'] = call_user_func_array(
                             array($stackVars[$i-1]['variable'], $stackVars[$i]['name']),
-                            $stackVars[$i]['args']
+                            !$isEncrypted ? $stackVars[$i]['args'] : array(null)
                         );
                     }
                 }
@@ -322,4 +328,14 @@ class Varien_Filter_Template implements Zend_Filter_Interface
         Varien_Profiler::stop("email_template_proccessing_variables");
         return $result;
     }
+
+    /**
+     * Retrieve model object
+     *
+     * @return Mage_Core_Model_Abstract
+     */
+    protected function getEmailPathValidator()
+    {
+        return Mage::getModel('adminhtml/email_pathValidator');
+    }
 }

And since this module inherits from that class and rewrites the _getVariable method, the same changes might be needed here as well?

I don't really understand yet what security issue this is fixing and if this can be exploited using an import, but who knows ...

Thanks!

hostep avatar Apr 29 '19 11:04 hostep

This might not be very important, since I don't think this method is called anywhere? It was added in https://github.com/ho-nl/magento1-Ho_Import/commit/bb705804e348173e5ea1c7b0e225bf3cdd274835, but I don't see where that templateEngine method gets called? It doesn't seem to be called in this module and also nowhere in core Magento as far as I can tell...

hostep avatar Apr 29 '19 12:04 hostep