Sanitize skin urls that could be used for XSS (#4654)

Co-authored-by: kiatng <[email protected]>
OpenMage · Feb 26, 2025 · d307e5b · d307e5b
1 parent a1dbce2
commit d307e5b
Show file tree

Hide file tree

Showing 4 changed files with 11 additions and 5 deletions.
diff --git a/app/code/core/Mage/Core/Model/Design/Package.php b/app/code/core/Mage/Core/Model/Design/Package.php
@@ -359,8 +359,10 @@ public function getSkinBaseUrl(array $params = [])
     {
         $params['_type'] = 'skin';
         $this->updateParamDefaults($params);
-        return Mage::getBaseUrl('skin', isset($params['_secure']) ? (bool) $params['_secure'] : null)
-            . $params['_area'] . '/' . $params['_package'] . '/' . $params['_theme'] . '/';
+        $urlPath = $params['_area'] . '/' . $params['_package'] . '/' . $params['_theme'] . '/';
+        // Prevent XSS through malformed configuration
+        $urlPath = htmlspecialchars($urlPath, ENT_HTML5 | ENT_QUOTES, 'UTF-8');
+        return Mage::getBaseUrl('skin', isset($params['_secure']) ? (bool) $params['_secure'] : null) . $urlPath;
     }
 
     /**
@@ -524,7 +526,8 @@ public function getSkinUrl($file = null, array $params = [])
         }
         $this->updateParamDefaults($params);
         if (!empty($file)) {
-            $result = $this->_fallback(
+            // This updates $params with the base package and default theme if the file is not found
+            $this->_fallback(
                 $file,
                 $params,
                 $this->_fallback->getFallbackScheme(

diff --git a/app/code/core/Mage/Core/etc/system.xml b/app/code/core/Mage/Core/etc/system.xml
@@ -332,6 +332,7 @@
                         </template_ua_regexp>
                         <skin translate="label">
                             <label>Skin (Images / CSS)</label>
+                            <validate>validate-no-html-tags</validate>
                             <sort_order>30</sort_order>
                             <show_in_default>1</show_in_default>
                             <show_in_website>1</show_in_website>

diff --git a/app/design/adminhtml/default/default/template/oauth/authorize/head-simple.phtml b/app/design/adminhtml/default/default/template/oauth/authorize/head-simple.phtml
@@ -30,7 +30,7 @@
     var BLANK_URL = '<?php echo $this->getJsUrl() ?>blank.html';
     var BLANK_IMG = '<?php echo $this->getJsUrl() ?>spacer.gif';
     var BASE_URL = '<?php echo $this->getUrl('*') ?>';
-    var SKIN_URL = '<?php echo $this->jsQuoteEscape($this->getSkinUrl()) ?>';
+    var SKIN_URL = '<?php echo $this->getSkinUrl() ?>';
     var FORM_KEY = '<?php echo $this->getFormKey() ?>';
 //]]>
 </script>

diff --git a/app/design/adminhtml/default/default/template/page/head.phtml b/app/design/adminhtml/default/default/template/page/head.phtml
@@ -21,13 +21,15 @@
 <link rel="icon" href="<?php echo $this->getSkinUrl('favicon.ico') ?>" type="image/x-icon"/>
 
 <script type="text/javascript">
+//<![CDATA[
     var BLANK_URL = '<?php echo $this->getJsUrl() ?>blank.html';
     var BLANK_IMG = '<?php echo $this->getJsUrl() ?>spacer.gif';
     var BASE_URL = '<?php echo $this->getUrl('*') ?>';
-    var SKIN_URL = '<?php echo $this->jsQuoteEscape($this->getSkinUrl()) ?>';
+    var SKIN_URL = '<?php echo $this->getSkinUrl() ?>';
     var FORM_KEY = '<?php echo $this->getFormKey() ?>';
     <?php # BC: cast to INT in case of non-existing method getLoadingTimeout() in 3rd-party code?>
     var LOADING_TIMEOUT = <?php echo (int)$this->getLoadingTimeout() ?>;
+//]]>
 </script>
 
 <?php echo $this->getCssJsHtml() ?>