Merge pull request #189 from magento-ogre/MAGETWO-45593-xss-vuln

[Ogres] Security Bug Fix

Author: mazhalai

app/code/Magento/Sales/Helper/Admin.php

@@ -149,10 +149,29 @@ public function escapeHtmlWithLinks($data, $allowedTags = null)
             $links = [];
             $i = 1;
             $data = str_replace('%', '%%', $data);
-            $regexp = '@(<a[^>]*>(?:[^<]|<[^/]|</[^a]|</a[^>])*</a>)@';
+            $regexp = "/<a\s[^>]*href\s*?=\s*?([\"\']??)([^\" >]*?)\\1[^>]*>(.*)<\/a>/siU";
             while (preg_match($regexp, $data, $matches)) {
-                $links[] = $matches[1];
-                $data = str_replace($matches[1], '%' . $i . '$s', $data);
+                //Revert the sprintf escaping
+                $url = str_replace('%%', '%', $matches[2]);
+                $text = str_replace('%%', '%', $matches[3]);
+                //Check for an valid url
+                if ($url) {
+                    $urlScheme = strtolower(parse_url($url, PHP_URL_SCHEME));
+                    if ($urlScheme !== 'http' && $urlScheme !== 'https') {
+                        $url = null;
+                    }
+                }
+                //Use hash tag as fallback
+                if (!$url) {
+                    $url = '#';
+                }
+                //Recreate a minimalistic secure a tag
+                $links[] = sprintf(
+                    '<a href="%s">%s</a>',
+                    htmlspecialchars($url, ENT_QUOTES, 'UTF-8', false),
+                    $this->escaper->escapeHtml($text)
+                );
+                $data = str_replace($matches[0], '%' . $i . '$s', $data);
                 ++$i;
             }
             $data = $this->escaper->escapeHtml($data, $allowedTags);

app/code/Magento/Sales/Test/Unit/Helper/AdminTest.php

@@ -354,7 +354,37 @@ public function escapeHtmlWithLinksDataProvider()
                 '<a>some text in tags</a>',
                 '<a>some text in tags</a>',
                 'allowedTags' => ['a']
-            ]
+            ],
+            'Not replacement with placeholders' => [
+                "<a><script>alert(1)</script></a>",
+                '<a>&lt;script&gt;alert(1)&lt;/script&gt;</a>',
+                'allowedTags' => ['a']
+            ],
+            'Normal usage, url escaped' => [
+                '<a href=\"#\">Foo</a>',
+                '<a href="#">Foo</a>',
+                'allowedTags' => ['a']
+            ],
+            'Normal usage, url not escaped' => [
+                "<a href=http://example.com?foo=1&bar=2&baz[name]=BAZ>Foo</a>",
+                '<a href="http://example.com?foo=1&amp;bar=2&amp;baz[name]=BAZ">Foo</a>',
+                'allowedTags' => ['a']
+            ],
+            'XSS test' => [
+                "<a href=\"javascript&colon;alert(59)\">Foo</a>",
+                '<a href="#">Foo</a>',
+                'allowedTags' => ['a']
+            ],
+            'Additional regex test' => [
+                "<a href=\"http://example1.com\" href=\"http://example2.com\">Foo</a>",
+                '<a href="http://example1.com">Foo</a>',
+                'allowedTags' => ['a']
+            ],
+            'Break of valid urls' => [
+                "<a href=\"http://example.com?foo=text with space\">Foo</a>",
+                '<a href="#">Foo</a>',
+                'allowedTags' => ['a']
+            ],
         ];
     }
 }